permission: check symlink target in cpSync#63208
Conversation
b7a57bf to
76d3603
Compare
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>
76d3603 to
7a2f106
Compare
|
|
||
| #ifndef S_ISDIR | ||
| #define S_ISDIR(mode) (((mode)&S_IFMT) == S_IFDIR) | ||
| #define S_ISDIR(mode) (((mode) & S_IFMT) == S_IFDIR) |
| // Permission check for verbatimSymlinks path (incomplete | ||
| // CVE-2025-55130 fix) |
There was a problem hiding this comment.
| // 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.
| 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); |
There was a problem hiding this comment.
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)
|
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. |
We do have documentation for that, but perhaps that's not clear. Happy to review any updates on it :) |
What
Add permission checks for symlink target paths in
CpSyncCopyDir(src/node_file.cc).Why
fs.cpSyncwithrecursive: truecopies symlinks viastd::filesystem::create_symlink()andcopy_symlink()without validating the symlink target against the permission model. This allows reading and writing files outside permitted paths through copied symlinks.fs.symlinkSyncwas fixed for this in the CVE-2025-55130 patch (lines 1353-1357) but the same check was not added to cpSync.Reproduction
Output (before fix):
How
Added permission checks using
env->permission()->is_granted()for bothkFileSystemReadandkFileSystemWriteon the resolved symlink target before:create_symlink()call (line ~3819)create_directory_symlink()call (line ~3822)copy_symlink()call in theverbatimSymlinkspath (line ~3754)Test
Added
test/parallel/test-fs-cp-permission-symlink.jswhich verifies thatfs.cpSyncthrowsERR_ACCESS_DENIEDwhen copying a directory containing a symlink pointing outside the allowed permission paths.Fixes: #63179
Refs: CVE-2025-55130