Skip to content

permission: make inserting an already-granted path idempotent - #66929

Open
itxcrusher wants to merge 1 commit into
nodejs:mainfrom
itxcrusher:fix-permission-idempotent-insert
Open

itxcrusher wants to merge 1 commit into
nodejs:mainfrom
itxcrusher:fix-permission-idempotent-insert

Conversation

@itxcrusher

Copy link
Copy Markdown

Fixes: #66901

permission.drop() aborts the process when two --allow-fs-read grants are identical up to and including their first *.

mkdir -p /tmp/sig
printf 'process.permission.drop("fs.read", process.argv[1]);\nconsole.log("survived");\n' > /tmp/sig/app.js

node --permission --allow-fs-read='/tmp/sig/*' --allow-fs-read='/tmp/sig/*.js' /tmp/sig/app.js
# Assertion failed: !path_prefix.empty()  at src/permission/fs_permission.h:59
# exit 134

Reproduced on v26.12.0, v26.12.1 and a v27 nightly, on Windows and independently on macOS arm64. v26.11.1 and v24.22.1 are unaffected.

Cause

Insert calls CreateChild at each * and at the final character. Inserting /tmp/sig/*.js after /tmp/sig/* reaches the node whose prefix is already /tmp/sig/*; the prefix-swap loop runs to completion because the strings match, child_was_end_node sets is_leaf = true, and it recurses as CreateChild(""). The path_prefix.empty() && !is_leaf condition is then false, so control reaches CHECK(!path_prefix.empty()), which is unconditional in release builds.

That condition encodes an assumption that an empty remaining prefix cannot occur on a node that is already a leaf. It can, as soon as a grant list containing both paths is replayed.

Startup is unaffected because GrantAccess only inserts when !Lookup(path), and /tmp/sig/*.js is already covered by /tmp/sig/*. #66556 moved the granted_paths_ push_back out of that branch so every explicit grant is recorded even when covered, and RebuildTree replays the whole list, so the first permission.drop() performs the insert that startup skipped.

Change

Inserting a path that is already granted is a no-op, so return the existing node rather than asserting.

Tests

Adds test/parallel/test-permission-drop-colliding-wildcards.js: the colliding grants plus a drop() must exit 0 and not be killed by a signal, the single-grant case must keep working, and the colliding grants must still grant what they declare.

How the change was verified

The C++ change was validated by extracting RadixTree verbatim from main (fs_permission.h 28-183 and fs_permission.cc 268-332, diff-verified byte-identical, with only kPathSeparator, CHECK, the debug plumbing and PrintTree stubbed) and driving it directly under -fsanitize=address,undefined. The same expectation table runs against an unpatched and a patched extract: 7 mismatches before, 0 after, no sanitizer reports.

The table includes regression controls for both prior security fixes in this file, since a change to CreateChild could plausibly reopen either:

  • CVE-2026-58043: /tmp/secret1, /tmp/secret2, /tmp/secret3 inserted, the shared prefix /tmp/secret must stay denied and the siblings granted.
  • CVE-2026-106071: /home/test* must not grant /home/tes; /t* must not grant /.
  • Documented behaviour from doc/api/permissions.md: /home/test* grants /home/test2, /home/test/* grants the directory and its contents, a trailing-separator grant still matches.

All pass with the change.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. permission Issues and PRs related to the Permission Model. labels Oct 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Welcome to Node.js, and thank you for your first contribution!

Before review, please take a moment to read:

Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal.

Caution

AgentScan found account activity patterns that may be consistent with automation. This is a heuristic, not proof that this pull request was opened by an agent or violates policy. AI-assisted contributions are permitted, but automated tooling must not open pull requests without advance approval, and contributors must personally understand, test, verify, and take responsibility for every submitted change. See the AgentScan analysis, AI use policy, and automation policy for additional context.

@itxcrusher
itxcrusher force-pushed the fix-permission-idempotent-insert branch from 781e62d to 18c9de5 Compare October 22, 2026 15:19
`Node::CreateChild` treats an empty remaining prefix as reachable
only when the node is not yet a leaf. Inserting a second grant that
is identical up to and including its first `*` walks to the existing
node, marks it a leaf, and recurses with an empty prefix. The
`!is_leaf` condition is then false and the unconditional `CHECK`
aborts the process.

Startup is unaffected because `GrantAccess` only inserts when the
path is not already covered, so the colliding insert is skipped while
flags are applied. `RebuildTree` replays the recorded grant list
unconditionally, so the first `permission.drop()` performs the insert
that startup skipped.

Inserting a path that is already granted is a no-op, so return the
existing node instead of asserting.

Fixes: nodejs#66901
Assisted-by: a closed-source coding agent
Signed-off-by: Hassaan <muhammadhassaanjaved99@gmail.com>
@itxcrusher
itxcrusher force-pushed the fix-permission-idempotent-insert branch from 18c9de5 to 146b9d2 Compare October 22, 2026 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agentscan:mixed-signals c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. permission Issues and PRs related to the Permission Model.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

permission: process.permission.drop() aborts (SIGABRT) when two --allow-fs-read grants collide at their first wildcard

2 participants