Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a Threat Model #5526

Merged
merged 4 commits into from Apr 4, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
51 changes: 51 additions & 0 deletions Security.md
Expand Up @@ -40,6 +40,57 @@ involving the following steps:
* Prepare fixes for all releases still under maintenance. These fixes will be
released as fast as possible to npm.

## The Express Threat Model

The Express threat model delineates the trusted elements within the framework, such as the underlying operating system or the runtime. Vulnerabilities necessitating the compromise of these trusted elements lie outside the scope of the Node.js threat model.
UlisesGascon marked this conversation as resolved.
Show resolved Hide resolved

For a vulnerability to be considered, it must adhere to the context of the Express threat model. This implies that it cannot assume the compromise of a trusted element, such as the operating system.

**Elements Express Does NOT Trust**:

1. Data received from the remote end of inbound or sent to remote outbound network connections, which are accepted through the use of Express API and transformed/validated by Express before being passed to the application.
UlisesGascon marked this conversation as resolved.
Show resolved Hide resolved

In simpler terms, if the data passing through Express to/from the application can initiate actions beyond those documented for the API, it likely signifies a security vulnerability. Examples of unwanted actions include polluting globals, causing an unrecoverable crash, or any other unexpected side effects jeopardizing confidentiality, integrity, or availability.

**Elements Express Trusts**:

1. Developers and infrastructure responsible for running it.
2. The operating system and JavaScript runtime Express operates under, including its configuration and anything within the control of the operating system.
3. The code it executes, comprising JavaScript and native code, even if dynamically loaded, such as dependencies installed from npm or similar repositories.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems strange to trust arbitrary plugins - this can be defended against.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree here. What exactly do we mean by this in "real terms". Like what would be an outcome we expect to happen when triaging an issues where this would apply? Is it this "Malicious Third-Party Modules (CWE-1357)"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not true, express should trust the middleware not to be malicious, otherwise the plugin system would not work and express is not responsible of how a malicious plugin interacts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is why I am not sure I am clear on the distinction here. What you say @marco-ippolito makes sense, but this language is a bit "wide in scope" imo, just want to make sure I understand the goal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basically all of the express middleware I've written would be considered malicious from a security view 😃

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I too think its a good point.

4. The resources it accesses, such as files, network connections, and other system resources, as long as they are accessed through the documented API.
5. The privileges of the execution user are inherited by the code it runs.
6. Inputs provided by the code it runs, as it is the application's responsibility to conduct necessary input validations, e.g., input to `JSON.parse()`.

### Examples of Vulnerabilities

Examples of vulnerabilities can be found in the [Express Security Updates](https://expressjs.com/en/advanced/security-updates.html).

### Examples of Non-Vulnerabilities

#### Malicious Third-Party Modules (CWE-1357)

* Code trusted by Express implies that any scenario requiring a malicious third-party module does not result in a vulnerability in Express. For instance, if an application uses a vulnerable third-party module susceptible to a denial of service attack, it is not considered a vulnerability in Express.

#### Prototype Pollution Attacks (CWE-1321)

* Express trusts the inputs provided by application code. Hence, scenarios necessitating control over user input are not considered vulnerabilities unless Express itself fails to sanitize the input properly.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prototype pollution is not a sanitization issue.

In javascript, you can store arbitrary user-controlled data as a key of a javascript object in a safe way without any need for sanitization. If express is not doing that, it's worth asking why.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There have been PP attacks which relied on missing input sanitization in the past, not that I am saying you are wrong, just that this can be caused by a few types of things. AFAIK all of those were addressed and closed out. Was this just speculation or trying to help find better wording for this section?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this just speculation or trying to help find better wording for this section?

That was trying to clarify the intention behind this paragraph, and it was before I realized that it was copy-pasted from Node.js.

If express had PP issues, which were all addressed and fixed, then it shouldn't be in "not an issue" section, no?

But honestly, I think this document should be written from the ground up based on actual issues reported by people (in github and email). Threat models of a js runtime and js library are very different, and issues that node.js faced before aren't necessarily applicable here. (I also had "wtf" moment reading about uncontrolled paths trying to figure out how it's related to express - do people really report these?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was trying to clarify the intention behind this paragraph, and it was before I realized that it was copy-pasted from Node.js.

Cool thanks! Just making sure I understood the intent since I was not sure my reply really addressed it.

then it shouldn't be in "not an issue" section, no? ... Threat models of a js runtime and js library are very different

Yeah that seems like a very relevant point! Hm. @UlisesGascon thoughts on this?


#### Uncontrolled Search Path Element (CWE-427)

* Express trusts the runtime environment accessible to it. Therefore, accessing/loading files from any accessible path is not a vulnerability. For example, misconfigurations with `express.static()` are not considered vulnerabilities. [Additional information](https://expressjs.com/en/starter/static-files.html).

#### External Control of System or Configuration Setting (CWE-15)

* If Express automatically loads a configuration from the environment, any modification to that environment does not constitute a vulnerability. For example, tampering with the `NODE_ENV` environment variable, causing Express to behave in dev mode instead of production mode, is not considered a vulnerability. [Additional information](https://expressjs.com/en/advanced/best-practice-performance.html#set-node_env-to-production).

#### Uncontrolled Resource Consumption (CWE-400) on Outbound Connections

* Express being asked to perform an action impacting the runtime's performance or causing resource exhaustion due to user application code is not a vulnerability.

#### Vulnerabilities Affecting JavaScript Runtime

* Express provides a stable API for interacting with most JavaScript runtime versions. Vulnerabilities within the JavaScript runtime itself or those requiring execution on an end-of-life version are not considered vulnerabilities in Express.

## Comments on this Policy

If you have suggestions on how this process could be improved please submit a
Expand Down