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

src,lib: stabilize permission model #56201

Merged
merged 3 commits into from
Dec 12, 2024

Conversation

RafaelGSS
Copy link
Member

@RafaelGSS RafaelGSS commented Dec 10, 2024

This PR upgrades the Permission Model from 1.1 (Active Development) to 2.0 (Stable).

I’ve been diving deep into the Permission Model since its release in Node.js 20.0.0, looking at its limitations and what’s been fixed so far. Most of the technical challenges have been addressed, except for how symlinks are handled. After a lot of research, it turns out this isn’t fixable due to how the Permission Model relies on file paths, making TOCTOU issues theoretically possible. This isn’t unique to Node.js though—even Deno’s permission system has similar behaviour (see this article).

Since the feature’s release, there’s been a shift in how we think about security in Node.js. We’ve leaned into a "Defense in Depth" approach—recognizing that no single feature will let you run untrusted code safely. Instead, these features are like seatbelts: they reduce risk significantly (let’s say 90% of cases, though that’s not a hard number) but won’t stop everything. This aligns with our threat model, and the Permission Model reflects that philosophy.

The only remaining "limitation" is symlink behaviour. Fixing this would require changing how the Permission Model works at a fundamental level. It’s not feasible because TOCTOU issues are always a possibility when operating on file paths. Importantly, this isn’t just a Node.js thing—other runtimes face the same challenge.

That said, symlinks aren’t a dealbreaker:

  • The Permission Model blocks symlink creation by default.
  • An attacker would need an exact map of existing symlinks in your system to do something harmful. That’s a very unlikely scenario.
  • If you explicitly allow access to something like /proc/, you’re responsible for understanding what that includes. The docs already cover this.

I have been talking with @tniessen in private as he has been indirectly involved in this feature (by raising concerns or suggestions). Some questions that he raised, and that I expect some of you might raise here too, were:

  • "Most importantly, is anyone actually using the permission model? What is the use case and what security expectations do these users have?"

As with any non-popular feature, it's hard to assess its usage in the ecosystem, but we have received some issues in the security-wg repository that could mean people are evaluating its usage:

I was also approached by many people on social media saying "thanks" for the feature and that they are looking forward to having it established. I also understand that testing a feature is different from using this feature in production.

  • "Following up on that, what security guarantees does the permission model provide, if any? Do those security guarantees actually match the security expectations of the people that actively use the permission model today?"

The Permission Model is most useful in development environments or scenarios where you want extra guardrails, but it doesn’t replace the core rule: don’t run untrusted code in Node.js.

If you configure it correctly, it’ll block most unwanted filesystem access, but it’s not a magic bullet. It’s a tool that works well when used as intended, and it complements Node.js’ broader security posture.


cc: @nodejs/security-wg

@RafaelGSS RafaelGSS added semver-major PRs that contain breaking changes and should be released in the next major version. notable-change PRs with changes that should be highlighted in changelogs. labels Dec 10, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/performance
  • @nodejs/security-wg

Copy link
Contributor

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @RafaelGSS.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Dec 10, 2024
@RafaelGSS RafaelGSS added the permission Issues and PRs related to the Permission Model label Dec 10, 2024
@RafaelGSS RafaelGSS force-pushed the move-permission-model-stable branch from 1a47bad to e1d0505 Compare December 10, 2024 03:10
Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.53%. Comparing base (f184a0a) to head (ab96885).
Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/process/pre_execution.js 66.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #56201   +/-   ##
=======================================
  Coverage   88.53%   88.53%           
=======================================
  Files         657      657           
  Lines      189858   189899   +41     
  Branches    36450    36464   +14     
=======================================
+ Hits       168089   168130   +41     
- Misses      14973    14977    +4     
+ Partials     6796     6792    -4     
Files with missing lines Coverage Δ
lib/internal/process/permission.js 69.44% <100.00%> (ø)
src/env.cc 85.45% <100.00%> (ø)
src/node_options.cc 87.98% <100.00%> (+0.10%) ⬆️
src/node_options.h 98.29% <100.00%> (ø)
lib/internal/process/pre_execution.js 90.69% <66.66%> (+0.13%) ⬆️

... and 26 files with indirect coverage changes

Copy link
Member

@mertcanaltin mertcanaltin left a comment

Choose a reason for hiding this comment

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

lgtm

@targos
Copy link
Member

targos commented Dec 10, 2024

You could add a flag alias so this is not semver-major.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@RafaelGSS RafaelGSS added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Dec 10, 2024
Move permission model from 1.1 (Active Development)
to 2.0 (Stable).
@RafaelGSS RafaelGSS force-pushed the move-permission-model-stable branch from d45ac4b to b00914b Compare December 10, 2024 17:42
@RafaelGSS RafaelGSS added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 10, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 10, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/permissions.md Outdated Show resolved Hide resolved
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 11, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 11, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 12, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 12, 2024
@nodejs-github-bot nodejs-github-bot merged commit be04d06 into nodejs:main Dec 12, 2024
60 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in be04d06

RafaelGSS added a commit to RafaelGSS/node that referenced this pull request Dec 12, 2024
RafaelGSS added a commit to RafaelGSS/node that referenced this pull request Dec 12, 2024
targos pushed a commit that referenced this pull request Dec 13, 2024
Move permission model from 1.1 (Active Development)
to 2.0 (Stable).

PR-URL: #56201
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
nodejs-github-bot added a commit that referenced this pull request Dec 18, 2024
Notable changes:

crypto:
  * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142
doc:
  * stabilize util.styleText (Rafael Gonzaga) #56265
module:
  * (SEMVER-MINOR) add prefix-only modules to `module.builtinModules` (Jordan Harband) #56185
  * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194
  * (SEMVER-MINOR) use synchronous hooks for preparsing in import(cjs) (Joyee Cheung) #55698
  * (SEMVER-MINOR) implement module.registerHooks() (Joyee Cheung) #55698
report:
  * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068
sqlite:
  * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213
src,lib:
  * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201
stream:
  * (SEMVER-MINOR) handle generator destruction from Duplex.from() (Matthieu Sieben) #55096

PR-URL: #56310
aduh95 added a commit that referenced this pull request Dec 18, 2024
Notable changes:

crypto:
  * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142
doc:
  * stabilize util.styleText (Rafael Gonzaga) #56265
module:
  * (SEMVER-MINOR) add prefix-only modules to `module.builtinModules` (Jordan Harband) #56185
  * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194
  * (SEMVER-MINOR) use synchronous hooks for preparsing in import(cjs) (Joyee Cheung) #55698
  * (SEMVER-MINOR) implement module.registerHooks() (Joyee Cheung) #55698
report:
  * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068
sqlite:
  * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213
src,lib:
  * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201
stream:
  * (SEMVER-MINOR) handle generator destruction from Duplex.from() (Matthieu Sieben) #55096

PR-URL: TODO
aduh95 pushed a commit that referenced this pull request Dec 18, 2024
Notable changes:

crypto:
  * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142
doc:
  * stabilize util.styleText (Rafael Gonzaga) #56265
module:
  * (SEMVER-MINOR) add prefix-only modules to `module.builtinModules` (Jordan Harband) #56185
  * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194
  * (SEMVER-MINOR) use synchronous hooks for preparsing in import(cjs) (Joyee Cheung) #55698
  * (SEMVER-MINOR) implement module.registerHooks() (Joyee Cheung) #55698
report:
  * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068
sqlite:
  * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213
src,lib:
  * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201
stream:
  * (SEMVER-MINOR) handle generator destruction from Duplex.from() (Matthieu Sieben) #55096

PR-URL: #56310
aduh95 pushed a commit that referenced this pull request Dec 18, 2024
Move permission model from 1.1 (Active Development)
to 2.0 (Stable).

PR-URL: #56201
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
aduh95 added a commit that referenced this pull request Dec 18, 2024
Notable changes:

crypto:
  * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142
dgram:
  * (SEMVER-MINOR) support blocklist in udp (theanarkh) #56087
doc:
  * stabilize util.styleText (Rafael Gonzaga) #56265
module:
  * (SEMVER-MINOR) add prefix-only modules to `module.builtinModules` (Jordan Harband) #56185
  * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194
  * (SEMVER-MINOR) use synchronous hooks for preparsing in import(cjs) (Joyee Cheung) #55698
  * (SEMVER-MINOR) implement module.registerHooks() (Joyee Cheung) #55698
report:
  * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068
sqlite:
  * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213
src,lib:
  * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201

PR-URL: #56310
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. permission Issues and PRs related to the Permission Model semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.