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

Conversation

UlisesGascon
Copy link
Member

@UlisesGascon UlisesGascon commented Mar 10, 2024

This proposal is very open for debate and improvements, it is basically a fork from Node.js one (nodejs/node#45223).

I suggest you to review Node.js one first if you are not familiar with Threat models at all.

The main goal of this Threat Model is to provide a better support for Security Researches and for us on the triage. This Threat model can be considered as the "playbook rules" for reporting vulnerabilities into the project.

Note: Only the Threat Model is in scope of this PR. The security.md has already existing elements that are going to change soon (see: expressjs/security-wg#7), so please keep the focus on the target.

Context
Ref: expressjs/security-wg#3

Your feedback is more than appreciated: @bensternthal @ruddermann @ljharb @lirantal @mcollina @fraxken @expressjs/security-wg @expressjs/security-triage @expressjs/express-tc

@UlisesGascon UlisesGascon added top priority Issues which the TC deem our current highest priorities for the project security labels Mar 10, 2024
@UlisesGascon UlisesGascon requested a review from a team March 10, 2024 18:36
@mcollina
Copy link

I think this is a good starting point.

Copy link

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM overall!

Security.md Outdated

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

@wesleytodd wesleytodd Mar 11, 2024

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.

Copy link
Member

@wesleytodd wesleytodd left a comment

Choose a reason for hiding this comment

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

Seems like a great start! I think this doc needs to move officially into the security wg repo as that is who will maintain it going forward, but we can deal with that in the future.

Security.md Outdated Show resolved Hide resolved
Co-authored-by: Chris de Almeida <ctcpip@users.noreply.github.com>
Security.md Outdated Show resolved Hide resolved
@ctcpip
Copy link
Member

ctcpip commented Mar 11, 2024

this looks great, just some small edit suggestions. also, we should consider moving this to the .github repo, but this can be done later

Co-authored-by: Chris de Almeida <ctcpip@users.noreply.github.com>
Security.md Outdated

#### 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

@wesleytodd wesleytodd Mar 11, 2024

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?

@marco-ippolito
Copy link
Contributor

I guess this is ready to be merged

@UlisesGascon
Copy link
Member Author

I will merge this PR, the follow up can be found at expressjs/security-wg#3 (comment)
and the Threat Model content at https://github.com/expressjs/security-wg/blob/main/tools/docs/ThreatModel.md#the-express-threat-model.

As we plan to do more iterations soon, we just decided in a previous TC meeting to move the initiative to the security wg until is completed.

otc-zuul bot pushed a commit to opentelekomcloud-infra/backstage that referenced this pull request Nov 19, 2024
chore(deps): Bump the npm_and_yarn group with 9 updates

Bumps the npm_and_yarn group with 9 updates:



Package
From
To




@backstage/plugin-app-backend
0.3.74
0.3.75


cross-spawn
7.0.3
7.0.6


dset
3.1.3
3.1.4


express
4.19.2
4.21.1


http-proxy-middleware
2.0.6
2.0.7


path-to-regexp
0.1.7
0.1.10


rollup
4.21.2
4.27.3


send
0.18.0
0.19.0


serve-static
1.15.0
1.16.2



Updates @backstage/plugin-app-backend from 0.3.74 to 0.3.75

Changelog
Sourced from @​backstage/plugin-app-backend's changelog.

@​backstage/plugin-app-backend
0.4.0
Minor Changes

815b702: Configuration is no longer injected into static assets if a index.html.tmpl file is present.

Patch Changes

815b702: The index.html templating is now done and served from memory rather than written to the filesystem. This means that you can now use config injection with a read-only filesystem, and you no longer need to use the app.disableConfigInjection flag.
Updated dependencies

@​backstage/config@​1.3.0
@​backstage/types@​1.2.0
@​backstage/config-loader@​1.9.2
@​backstage/plugin-auth-node@​0.5.4
@​backstage/backend-plugin-api@​1.0.2
@​backstage/errors@​1.2.5
@​backstage/plugin-app-node@​0.1.27



0.3.77-next.2
Patch Changes

Updated dependencies

@​backstage/plugin-auth-node@​0.5.4-next.2
@​backstage/backend-plugin-api@​1.0.2-next.2
@​backstage/config@​1.2.0
@​backstage/config-loader@​1.9.2-next.0
@​backstage/errors@​1.2.4
@​backstage/types@​1.1.1
@​backstage/plugin-app-node@​0.1.27-next.2



0.3.77-next.1
Patch Changes

Updated dependencies

@​backstage/backend-plugin-api@​1.0.2-next.1
@​backstage/config@​1.2.0
@​backstage/config-loader@​1.9.2-next.0
@​backstage/errors@​1.2.4
@​backstage/types@​1.1.1
@​backstage/plugin-app-node@​0.1.27-next.1
@​backstage/plugin-auth-node@​0.5.4-next.1



0.3.77-next.0
Patch Changes


... (truncated)


Commits

See full diff in compare view



Updates cross-spawn from 7.0.3 to 7.0.6

Changelog
Sourced from cross-spawn's changelog.

7.0.6 (2024-11-18)
Bug Fixes

update cross-spawn version to 7.0.5 in package-lock.json (f700743)

7.0.5 (2024-11-07)
Bug Fixes

fix escaping bug introduced by backtracking (640d391)

7.0.4 (2024-11-07)
Bug Fixes

disable regexp backtracking (#160) (5ff3a07)




Commits

77cd97f chore(release): 7.0.6
6717de4 chore: upgrade standard-version
f700743 fix: update cross-spawn version to 7.0.5 in package-lock.json
9a7e3b2 chore: fix build status badge
0852683 chore(release): 7.0.5
640d391 fix: fix escaping bug introduced by backtracking
bff0c87 chore: remove codecov
a7c6abc chore: replace travis with github workflows
9b9246e chore(release): 7.0.4
5ff3a07 fix: disable regexp backtracking (#160)
Additional commits viewable in compare view



Updates dset from 3.1.3 to 3.1.4

Commits

05b1ec0 3.1.4
16d6154 fix: prevent proto assignment via implicit string
See full diff in compare view



Updates express from 4.19.2 to 4.21.1

Release notes
Sourced from express's releases.

4.21.1
What's Changed

Backport a fix for CVE-2024-47764 to the 4.x branch by @​joshbuker in expressjs/express#6029
Release: 4.21.1 by @​UlisesGascon in expressjs/express#6031

Full Changelog: expressjs/express@4.21.0...4.21.1
4.21.0
What's Changed

Deprecate "back" magic string in redirects by @​blakeembrey in expressjs/express#5935
finalhandler@1.3.1 by @​wesleytodd in expressjs/express#5954
fix(deps): serve-static@1.16.2 by @​wesleytodd in expressjs/express#5951
Upgraded dependency qs to 6.13.0 to match qs in body-parser by @​agadzinski93 in expressjs/express#5946

New Contributors

@​agadzinski93 made their first contribution in expressjs/express#5946

Full Changelog: expressjs/express@4.20.0...4.21.0
4.20.0
What's Changed
Important

IMPORTANT: The default depth level for parsing URL-encoded data is now 32 (previously was Infinity)
Remove link renderization in html while using res.redirect

Other Changes

4.19.2 Staging by @​wesleytodd in expressjs/express#5561
remove duplicate location test for data uri by @​wesleytodd in expressjs/express#5562
feat: document beta releases expectations by @​marco-ippolito in expressjs/express#5565
Cut down on duplicated CI runs by @​jonchurch in expressjs/express#5564
Add a Threat Model by @​UlisesGascon in expressjs/express#5526
Assign captain of encodeurl by @​blakeembrey in expressjs/express#5579
Nominate jonchurch as repo captain for http-errors, expressjs.com, morgan, cors, body-parser by @​jonchurch in expressjs/express#5587
docs: update Security.md by @​inigomarquinez in expressjs/express#5590
docs: update triage nomination policy by @​UlisesGascon in expressjs/express#5600
Add CodeQL (SAST) by @​UlisesGascon in expressjs/express#5433
docs: add UlisesGascon as triage initiative captain by @​UlisesGascon in expressjs/express#5605
deps: encodeurl@~2.0.0 by @​blakeembrey in expressjs/express#5569
skip QUERY method test by @​jonchurch in expressjs/express#5628
ignore ETAG query test on 21 and 22, reuse skip util by @​jonchurch in expressjs/express#5639
add support Node.js@22 in the CI by @​mertcanaltin in expressjs/express#5627
doc: add table of contents, tc/triager lists to readme by @​mertcanaltin in expressjs/express#5619
List and sort all projects, add captains by @​blakeembrey in expressjs/express#5653
docs: add @​UlisesGascon as captain for cookie-parser by @​UlisesGascon in expressjs/express#5666
✨ bring back query tests for node 21 by @​ctcpip in expressjs/express#5690
[v4] Deprecate res.clearCookie accepting options.maxAge and options.expires by @​jonchurch in expressjs/express#5672
skip QUERY tests for Node 21 only, still not supported by @​jonchurch in expressjs/express#5695



... (truncated)


Changelog
Sourced from express's changelog.

4.21.1 / 2024-10-08

Backported a fix for CVE-2024-47764

4.21.0 / 2024-09-11

Deprecate res.location("back") and res.redirect("back") magic string
deps: serve-static@1.16.2

includes send@0.19.0


deps: finalhandler@1.3.1
deps: qs@6.13.0

4.20.0 / 2024-09-10

deps: serve-static@0.16.0

Remove link renderization in html while redirecting


deps: send@0.19.0

Remove link renderization in html while redirecting


deps: body-parser@0.6.0

add depth option to customize the depth level in the parser
IMPORTANT: The default depth level for parsing URL-encoded data is now 32 (previously was Infinity)


Remove link renderization in html while using res.redirect
deps: path-to-regexp@0.1.10

Adds support for named matching groups in the routes using a regex
Adds backtracking protection to parameters without regexes defined


deps: encodeurl@~2.0.0

Removes encoding of \, |, and ^ to align better with URL spec


Deprecate passing options.maxAge and options.expires to res.clearCookie

Will be ignored in v5, clearCookie will set a cookie with an expires in the past to instruct clients to delete the cookie






Commits

8e229f9 4.21.1
a024c8a fix(deps): cookie@0.7.1
7e562c6 4.21.0
1bcde96 fix(deps): qs@6.13.0 (#5946)
7d36477 fix(deps): serve-static@1.16.2 (#5951)
40d2d8f fix(deps): finalhandler@1.3.1
77ada90 Deprecate "back" magic string in redirects (#5935)
21df421 4.20.0
4c9ddc1 feat: upgrade to serve-static@0.16.0
9ebe5d5 feat: upgrade to send@0.19.0 (#5928)
Additional commits viewable in compare view



Updates http-proxy-middleware from 2.0.6 to 2.0.7

Release notes
Sourced from http-proxy-middleware's releases.

v2.0.7
Full Changelog: chimurai/http-proxy-middleware@v2.0.6...v2.0.7
v2.0.7-beta.1
Full Changelog: chimurai/http-proxy-middleware@v2.0.7-beta.0...v2.0.7-beta.1
v2.0.7-beta.0
Full Changelog: chimurai/http-proxy-middleware@v2.0.6...v2.0.7-beta.0



Changelog
Sourced from http-proxy-middleware's changelog.

v2.0.7

ci(github actions): add publish.yml
fix(filter): handle errors




Commits

1e92339 ci(github-actions): fix npm tag
90afb7c chore(package): v2.0.7
0b4274e fix(filter): handle errors
1bd6dd5 ci(github actions): add publish.yml
See full diff in compare view



Updates path-to-regexp from 0.1.7 to 0.1.10

Release notes
Sourced from path-to-regexp's releases.

Backtrack protection
Fixed

Add backtrack protection to parameters  29b96b4

This will break some edge cases but should improve performance



pillarjs/path-to-regexp@v0.1.9...v0.1.10
Support non-lookahead regex output
Added

Allow a non-lookahead regex (#312)  c4272e4

component/path-to-regexp@v0.1.8...v0.1.9
Support named matching groups in RegExp
Added

Add support for named matching groups (#301)  114f62d

pillarjs/path-to-regexp@v0.1.7...v0.1.8



Commits

c827fce 0.1.10
29b96b4 Add backtrack protection to parameters
ac4c234 Update repo url (#314)
bdb6635 0.1.9
c4272e4 Allow a non-lookahead regex (#312)
51a1955 0.1.8
114f62d Add support for named matching groups (#301)
See full diff in compare view



Updates rollup from 4.21.2 to 4.27.3

Release notes
Sourced from rollup's releases.

v4.27.3
4.27.3
2024-11-18
Bug Fixes

Revert object property tree-shaking for now (#5736)

Pull Requests

#5736: Revert object tree-shaking until some issues have been resolved (@​lukastaegert)

v4.27.2
4.27.2
2024-11-15
Bug Fixes

Ensure unused variables in patterns are always deconflicted if rendered (#5728)

Pull Requests

#5728: Fix more variable deconflicting issues (@​lukastaegert)

v4.27.1
4.27.1
2024-11-15
Bug Fixes

Fix some situations where parameter declarations could put Rollup into an infinite loop (#5727)

Pull Requests

#5727: Debug out-of-memory issues with Rollup v4.27.0 (@​lukastaegert)

v4.27.0
4.27.0
2024-11-15
Features

Tree-shake unused properties in object literals (#5420)

Bug Fixes


... (truncated)


Changelog
Sourced from rollup's changelog.

4.27.3
2024-11-18
Bug Fixes

Revert object property tree-shaking for now (#5736)

Pull Requests

#5736: Revert object tree-shaking until some issues have been resolved (@​lukastaegert)

4.27.2
2024-11-15
Bug Fixes

Ensure unused variables in patterns are always deconflicted if rendered (#5728)

Pull Requests

#5728: Fix more variable deconflicting issues (@​lukastaegert)

4.27.1
2024-11-15
Bug Fixes

Fix some situations where parameter declarations could put Rollup into an infinite loop (#5727)

Pull Requests

#5727: Debug out-of-memory issues with Rollup v4.27.0 (@​lukastaegert)

4.27.0
2024-11-15
Features

Tree-shake unused properties in object literals (#5420)

Bug Fixes

Change hash length limit to 21 to avoid inconsistent hash length (#5423)

Pull Requests


... (truncated)


Commits

7c0b1f8 4.27.3
10bc150 Revert  object tree-shaking (#5420) until some issues have been resolved (#5736)
a503a4d 4.27.2
6c68455 Fix more variable deconflicting issues (#5728)
aaf38b7 4.27.1
faeb905 Debug out-of-memory issues with Rollup v4.27.0 (#5727)
c035068 4.27.0
b58e48b fix(deps): update swc monorepo (major) (#5724)
50697b8 Reduce max hash size to 21 (#5723)
a9acb57 feat: implement object tree-shaking (#5420)
Additional commits viewable in compare view



Updates send from 0.18.0 to 0.19.0

Release notes
Sourced from send's releases.

0.19.0
What's Changed

Remove link renderization in html while redirecting (pillarjs/send#235)

New Contributors

@​UlisesGascon made their first contribution in pillarjs/send#235

Full Changelog: pillarjs/send@0.18.0...0.19.0



Changelog
Sourced from send's changelog.

0.19.0 / 2024-09-10

Remove link renderization in html while redirecting




Commits

9d2db99 0.19.0
ae4f298 Merge commit from fork
See full diff in compare view



Maintainer changes
This version was pushed to npm by ulisesgascon, a new releaser for send since your current version.


Updates serve-static from 1.15.0 to 1.16.2

Release notes
Sourced from serve-static's releases.

1.16.0
What's Changed

Remove link renderization in html while redirecting (expressjs/serve-static#173)

New Contributors

@​UlisesGascon made their first contribution in expressjs/serve-static#173

Full Changelog: expressjs/serve-static@v1.15.0...1.16.0



Changelog
Sourced from serve-static's changelog.

1.16.2 / 2024-09-11

deps: encodeurl@~2.0.0

1.16.1 / 2024-09-11

deps: send@0.19.0

1.16.0 / 2024-09-10

Remove link renderization in html while redirecting




Commits

ec9c5ec 1.16.2
f454d37 fix(deps): encodeurl@~2.0.0
77a8255 1.16.1
4263f49 fix(deps): send@0.19.0
48c7397 1.16.0
0c11fad Merge commit from fork
See full diff in compare view



Maintainer changes
This version was pushed to npm by wesleytodd, a new releaser for serve-static since your current version.


Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

@dependabot rebase will rebase this PR
@dependabot recreate will recreate this PR, overwriting any edits that have been made to it
@dependabot merge will merge this PR after your CI passes on it
@dependabot squash and merge will squash and merge this PR after your CI passes on it
@dependabot cancel merge will cancel a previously requested merge and block automerging
@dependabot reopen will reopen this PR if it is closed
@dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
@dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
@dependabot ignore <dependency name> major version will close this group update PR and stop Dependabot creating any more for the specific dependency's major version (unless you unignore this specific dependency's major version or upgrade to it yourself)
@dependabot ignore <dependency name> minor version will close this group update PR and stop Dependabot creating any more for the specific dependency's minor version (unless you unignore this specific dependency's minor version or upgrade to it yourself)
@dependabot ignore <dependency name> will close this group update PR and stop Dependabot creating any more for the specific dependency (unless you unignore this specific dependency or upgrade to it yourself)
@dependabot unignore <dependency name> will remove all of the ignore conditions of the specified dependency
@dependabot unignore <dependency name> <ignore condition> will remove the ignore condition of the specified dependency and ignore conditions
You can disable automated security fix PRs for this repo from the Security Alerts page.

Reviewed-by: Vladimir Vshivkov
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs security top priority Issues which the TC deem our current highest priorities for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants