Skip to content

permission: check symlink target in cpSync#63208

Open
fg0x0 wants to merge 1 commit intonodejs:mainfrom
fg0x0:fix/cpsync-symlink-permission-check
Open

permission: check symlink target in cpSync#63208
fg0x0 wants to merge 1 commit intonodejs:mainfrom
fg0x0:fix/cpsync-symlink-permission-check

Conversation

@fg0x0
Copy link
Copy Markdown

@fg0x0 fg0x0 commented May 9, 2026

What

Add permission checks for symlink target paths in CpSyncCopyDir (src/node_file.cc).

Why

fs.cpSync with recursive: true copies symlinks via std::filesystem::create_symlink() and copy_symlink() without validating the symlink target against the permission model. This allows reading and writing files outside permitted paths through copied symlinks.

fs.symlinkSync was fixed for this in the CVE-2025-55130 patch (lines 1353-1357) but the same check was not added to cpSync.

Reproduction

mkdir -p /tmp/allowed/src /tmp/allowed/dest /tmp/denied
echo SECRET > /tmp/denied/secret.txt
ln -sf /tmp/denied/secret.txt /tmp/allowed/src/link

node --permission \
  --allow-fs-read=/tmp/allowed --allow-fs-write=/tmp/allowed \
  --allow-fs-read=/usr --allow-fs-read=/lib -e '
const fs = require("node:fs");
try { fs.readFileSync("/tmp/denied/secret.txt"); } catch(e) { console.log("blocked:", e.code); }
try { fs.symlinkSync("/tmp/denied/secret.txt", "/tmp/allowed/dest/link"); } catch(e) { console.log("blocked:", e.code); }
fs.cpSync("/tmp/allowed/src/", "/tmp/allowed/dest/", { recursive: true });
console.log("read:", fs.readFileSync("/tmp/allowed/dest/link", "utf8").trim());
'

Output (before fix):

blocked: ERR_ACCESS_DENIED
blocked: ERR_ACCESS_DENIED
read: SECRET

How

Added permission checks using env->permission()->is_granted() for both kFileSystemRead and kFileSystemWrite on the resolved symlink target before:

  • create_symlink() call (line ~3819)
  • create_directory_symlink() call (line ~3822)
  • copy_symlink() call in the verbatimSymlinks path (line ~3754)

Test

Added test/parallel/test-fs-cp-permission-symlink.js which verifies that fs.cpSync throws ERR_ACCESS_DENIED when copying a directory containing a symlink pointing outside the allowed permission paths.

Fixes: #63179
Refs: CVE-2025-55130

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels May 9, 2026
@fg0x0 fg0x0 force-pushed the fix/cpsync-symlink-permission-check branch from b7a57bf to 76d3603 Compare May 9, 2026 06:15
fs.cpSync with recursive:true calls create_symlink() and
copy_symlink() without checking if the symlink target is within
the allowed permission paths.

fs.symlinkSync already validates symlink targets against the
permission model (added as the fix for CVE-2025-55130 at
src/node_file.cc:1353-1357). The same check was missing in
CpSyncCopyDir for both the standard symlink copy path and the
verbatimSymlinks code path.

Add permission checks for both kFileSystemRead and
kFileSystemWrite on the resolved symlink target before
create_symlink, create_directory_symlink, and copy_symlink
calls in CpSyncCopyDir.

Fixes: nodejs#63179
Refs: CVE-2025-55130
Signed-off-by: fg0x0 <fg0x0@local>
@fg0x0 fg0x0 force-pushed the fix/cpsync-symlink-permission-check branch from 76d3603 to 7a2f106 Compare May 9, 2026 06:21
@RafaelGSS RafaelGSS changed the title permission: check symlink target in cpSync (incomplete CVE-2025-55130) permission: check symlink target in cpSync May 10, 2026
Comment thread src/node_file.cc

#ifndef S_ISDIR
#define S_ISDIR(mode) (((mode)&S_IFMT) == S_IFDIR)
#define S_ISDIR(mode) (((mode) & S_IFMT) == S_IFDIR)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this change is not required

Comment thread src/node_file.cc
Comment on lines +3755 to +3756
// Permission check for verbatimSymlinks path (incomplete
// CVE-2025-55130 fix)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
// Permission check for verbatimSymlinks path (incomplete
// CVE-2025-55130 fix)
// Permission check for verbatimSymlinks path

It's not an incomplete fix, it's not a vulnerability, I told you.

Comment thread src/node_file.cc
Comment on lines +3758 to +3763
auto verb_target = std::filesystem::read_symlink(src, error);
if (error) break;
auto verb_target_abs = std::filesystem::weakly_canonical(
std::filesystem::absolute(src.parent_path() / verb_target));
auto verb_str = verb_target_abs.string();
auto verb_view = std::string_view(verb_str);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't resolve the path and then perform the check, this goes against permission model behavior, we never call a fs syscall to check if we can in-fact read/write to a file, this is controversial (that's why symlinks aren't supported)

@RafaelGSS RafaelGSS added the permission Issues and PRs related to the Permission Model label May 10, 2026
@fg0x0
Copy link
Copy Markdown
Author

fg0x0 commented May 10, 2026

Thanks for the review. I'll drop the whitespace change and remove the CVE reference from the comments.

On the design point - understood. The permission model avoids fs syscalls for validation by design, and resolving the symlink target before checking breaks that principle. I see why symlinks are intentionally unsupported.

Would a documentation-only approach be more appropriate here? Something like a note in the permission model docs that symlinks in copied directories can reference paths outside the allowed scope, so users relying on --allow-fs-read/write boundaries should be aware of this behavior.

Or if you'd prefer I just close the PR, that's fine too.

@RafaelGSS
Copy link
Copy Markdown
Member

RafaelGSS commented May 10, 2026

Would a documentation-only approach be more appropriate here? Something like a note in the permission model docs that symlinks in copied directories can reference paths outside the allowed scope, so users relying on --allow-fs-read/write boundaries should be aware of this behavior.

Or if you'd prefer I just close the PR, that's fine too.

We do have documentation for that, but perhaps that's not clear. Happy to review any updates on it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. 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.

fs.cpSync copies symlinks without permission check on target path

3 participants