From 119c401f4f7efbeb112d28f9dfc9c489674c9a79 Mon Sep 17 00:00:00 2001 From: Joshua van Rijswijk Date: Mon, 23 Mar 2026 17:23:29 +0100 Subject: [PATCH] fix(extract): prevent raced symlink writes outside cwd Fix a race in file extraction where a destination path could be swapped to a symlink after the existing `lstat()` check but before the file was opened. This change uses `O_NOFOLLOW` for extracted file writes on non-Windows platforms so the final open does not follow a symlink at the destination path. Also adds a regression test that reproduces the race and checks that extraction does not overwrite a file outside the target directory. PR-URL: https://github.com/isaacs/node-tar/pull/456 Credit: @Jvr2022 Close: #456 Reviewed-by: @isaacs --- src/get-write-flag.ts | 10 +++-- test/get-write-flag.js | 11 ++++- test/symlink-race-extract.ts | 80 ++++++++++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 5 deletions(-) create mode 100644 test/symlink-race-extract.ts diff --git a/src/get-write-flag.ts b/src/get-write-flag.ts index aa3fd883..ed49d359 100644 --- a/src/get-write-flag.ts +++ b/src/get-write-flag.ts @@ -12,7 +12,7 @@ const platform = process.env.__FAKE_PLATFORM__ || process.platform const isWindows = platform === 'win32' /* c8 ignore start */ -const { O_CREAT, O_TRUNC, O_WRONLY } = fs.constants +const { O_CREAT, O_NOFOLLOW, O_TRUNC, O_WRONLY } = fs.constants const UV_FS_O_FILEMAP = Number(process.env.__FAKE_FS_O_FILENAME__) || fs.constants.UV_FS_O_FILEMAP || @@ -22,7 +22,11 @@ const UV_FS_O_FILEMAP = const fMapEnabled = isWindows && !!UV_FS_O_FILEMAP const fMapLimit = 512 * 1024 const fMapFlag = UV_FS_O_FILEMAP | O_TRUNC | O_CREAT | O_WRONLY +const noFollowFlag = + !isWindows && typeof O_NOFOLLOW === 'number' ? + O_NOFOLLOW | O_TRUNC | O_CREAT | O_WRONLY + : null export const getWriteFlag = - !fMapEnabled ? - () => 'w' + noFollowFlag !== null ? () => noFollowFlag + : !fMapEnabled ? () => 'w' : (size: number) => (size < fMapLimit ? fMapFlag : 'w') diff --git a/test/get-write-flag.js b/test/get-write-flag.js index caad94e3..e1de2bf4 100644 --- a/test/get-write-flag.js +++ b/test/get-write-flag.js @@ -13,6 +13,13 @@ const __filename = fileURLToPath(import.meta.url) const hasFmap = !!fs.constants.UV_FS_O_FILEMAP const { platform } = process const UV_FS_O_FILEMAP = 0x20000000 +const unixFlag = + typeof fs.constants.O_NOFOLLOW === 'number' ? + fs.constants.O_NOFOLLOW | + fs.constants.O_TRUNC | + fs.constants.O_CREAT | + fs.constants.O_WRONLY + : 'w' switch (process.argv[2]) { case 'win32-fmap': { @@ -32,8 +39,8 @@ switch (process.argv[2]) { } case 'unix': { - t.equal(getWriteFlag(1), 'w') - t.equal(getWriteFlag(512 * 1024 + 1), 'w') + t.equal(getWriteFlag(1), unixFlag) + t.equal(getWriteFlag(512 * 1024 + 1), unixFlag) break } diff --git a/test/symlink-race-extract.ts b/test/symlink-race-extract.ts new file mode 100644 index 00000000..a754715d --- /dev/null +++ b/test/symlink-race-extract.ts @@ -0,0 +1,80 @@ +import fs, { + lstatSync, + readFileSync, + symlinkSync, + writeFileSync, +} from 'node:fs' +import { resolve } from 'node:path' +import t from 'tap' +import { extract } from '../src/extract.js' +import { Header } from '../src/header.js' + +if (typeof fs.constants.O_NOFOLLOW !== 'number') { + t.plan(0, 'no O_NOFOLLOW flag') + process.exit(0) +} + +const makeTarball = () => { + const header = Buffer.alloc(512) + new Header({ + path: 'victim.txt', + type: 'File', + size: 6, + }).encode(header) + + return Buffer.concat([ + header, + Buffer.from('PWNED\n'.padEnd(512, '\0')), + Buffer.alloc(1024), + ]) +} + +t.test('extract does not follow a raced-in symlink', async t => { + const dir = t.testdir({ + cwd: {}, + 'target.txt': 'ORIGINAL\n', + }) + const cwd = resolve(dir, 'cwd') + const target = resolve(dir, 'target.txt') + const tarball = resolve(dir, 'poc.tar') + const victim = resolve(cwd, 'victim.txt') + writeFileSync(tarball, makeTarball()) + + const warnings: [code: string, message: string][] = [] + const lstat = fs.lstat + let raced = false + fs.lstat = ((path, options, cb) => { + const callback = + typeof options === 'function' ? options : ( + (cb as Parameters[1] & + ((err: NodeJS.ErrnoException | null, stats: fs.Stats) => void)) + ) + if (!raced && String(path) === victim) { + raced = true + symlinkSync(target, victim) + const er = Object.assign(new Error('raced symlink'), { + code: 'ENOENT', + }) + process.nextTick(() => callback(er, undefined as never)) + return + } + return typeof options === 'function' ? + lstat(path, options) + : lstat(path, options, cb as never) + }) as typeof fs.lstat + t.teardown(() => { + fs.lstat = lstat + }) + + await extract({ + cwd, + file: tarball, + onwarn: (code, message) => warnings.push([code, String(message)]), + }) + + t.equal(readFileSync(target, 'utf8'), 'ORIGINAL\n') + t.equal(lstatSync(victim).isSymbolicLink(), true) + t.match(warnings, [ + ['TAR_ENTRY_ERROR', /ELOOP|symbolic link|Too many symbolic links/], + ]) +})