From b06d1be187e4af88bbf9f68733356f9b69e0a6b3 Mon Sep 17 00:00:00 2001 From: MK Date: Wed, 5 Aug 2026 20:41:32 +0800 Subject: [PATCH] fix: resolve symlink chains fully when extracting Backport of #140 to 1.x. isRealPathSafe() stopped walking as soon as realpath() failed on a dangling link, checking only that link's immediate target. A destination reached through several hops, or through a linked directory, was only partially resolved, so an entry could land somewhere the check had not accounted for. Resolve the remaining hops by hand instead, bounded by MAX_SYMLINK_DEPTH, and walk from whichever extraction root actually contains the target so a link named in the real namespace is not rejected. File entries no longer write through a symlink sitting at the destination. The link is replaced by the entry, which is how tar(1), node-tar and libarchive all behave. Where the platform has it, the write also opens with O_NOFOLLOW. --- lib/utils.js | 76 +++++++- test/tar/symlink-resolution.test.js | 283 ++++++++++++++++++++++++++++ test/util.js | 28 +++ 3 files changed, 378 insertions(+), 9 deletions(-) create mode 100644 test/tar/symlink-resolution.test.js diff --git a/lib/utils.js b/lib/utils.js index 38974ae..9d5f74a 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -5,6 +5,35 @@ const path = require('path'); const mkdirp = require('mkdirp'); const pump = require('pump'); +// Matches the kernel's own symlink chain limit closely enough to reject loops +// that realpath() never sees, without rejecting any realistic layout. +const MAX_SYMLINK_DEPTH = 32; + +// Numeric flags are accepted here per the "File system flags" section of the fs +// docs. The flag makes open() fail with ELOOP when the final component is a +// symlink, so the write never resolves one. It is undefined on Windows, where +// unlinkSymlink() below does the work. +const NO_FOLLOW_WRITE_FLAGS = typeof fs.constants.O_NOFOLLOW === 'number' + ? fs.constants.O_NOFOLLOW | fs.constants.O_CREAT | fs.constants.O_TRUNC | fs.constants.O_WRONLY + : 'w'; + +/** + * Remove a symlink sitting at the exact path an entry is about to be written to. + * Extraction replaces such a link rather than writing through to whatever it + * points at, which is how tar(1), node-tar and libarchive all behave. + * @param {string} target - Absolute path of the entry destination + * @param {function} callback - callback(err) + */ +function unlinkSymlink(target, callback) { + fs.lstat(target, function(err, stat) { + if (err) return callback(err.code === 'ENOENT' ? null : err); + if (!stat.isSymbolicLink()) return callback(null); + fs.unlink(target, function(err) { + callback(err && err.code !== 'ENOENT' ? err : null); + }); + }); +} + /** * Check if childPath is within parentPath (prevents path traversal attacks) * @param {string} childPath - The path to check @@ -30,16 +59,34 @@ function isPathWithinParent(childPath, parentPath) { * @param {string} parentDir - Absolute path of the extraction root * @param {string} realParentDir - Pre-resolved real path of parentDir (handles OS-level symlinks like /var -> /private/var on macOS) * @param {function} callback - callback(err, safe) + * @param {number} [depth] - Recursion depth when re-walking a dangling symlink's target */ -function isRealPathSafe(targetPath, parentDir, realParentDir, callback) { +function isRealPathSafe(targetPath, parentDir, realParentDir, callback, depth) { + depth = depth || 0; + // realpath() rejects long chains with ELOOP, but the dangling branch below resolves + // hop by hop without the kernel's help, so it needs its own bound. + if (depth >= MAX_SYMLINK_DEPTH) return callback(null, false); + function isWithinParent(p) { return isPathWithinParent(p, parentDir) || isPathWithinParent(p, realParentDir); } - const relative = path.relative(parentDir, targetPath); + // A link target may be written in either namespace when the two differ, as with + // /var -> /private/var on macOS. Walk from whichever root actually contains it, + // or the relative path below would climb out through '..' and reject a safe link. + let baseDir; + if (isPathWithinParent(targetPath, parentDir)) { + baseDir = parentDir; + } else if (isPathWithinParent(targetPath, realParentDir)) { + baseDir = realParentDir; + } else { + return callback(null, false); + } + + const relative = path.relative(baseDir, targetPath); const segments = relative.split(path.sep); let i = 0; - let current = parentDir; + let current = baseDir; function checkNext() { if (i >= segments.length) return callback(null, true); @@ -58,11 +105,15 @@ function isRealPathSafe(targetPath, parentDir, realParentDir, callback) { fs.realpath(current, function(err, resolved) { if (err) { if (err.code === 'ENOENT') { - // Dangling symlink - check textual target + // Dangling symlink: realpath() gave up, so resolve the textual target + // ourselves. Checking the target string alone is not enough, because the + // target may itself be a symlink, or sit under a directory that is one, + // and both get resolved when the entry is actually written. return fs.readlink(current, function(err, linkTarget) { if (err) return callback(null, false); const absTarget = path.resolve(path.dirname(current), linkTarget); - callback(null, isWithinParent(absTarget)); + if (!isWithinParent(absTarget)) return callback(null, false); + isRealPathSafe(absTarget, parentDir, realParentDir, callback, depth + 1); }); } // Fail closed: unexpected errors during symlink resolution are unsafe @@ -214,11 +265,18 @@ exports.makeUncompressFn = StreamClass => { mkdirp(dir, err => { if (err) return reject(err); - entryCount++; - pump(stream, fs.createWriteStream(destFilePath, { mode: opts.mode || header.mode }), err => { + unlinkSymlink(destFilePath, err => { if (err) return reject(err); - successCount++; - done(); + + entryCount++; + pump(stream, fs.createWriteStream(destFilePath, { + flags: NO_FOLLOW_WRITE_FLAGS, + mode: opts.mode || header.mode, + }), err => { + if (err) return reject(err); + successCount++; + done(); + }); }); }); } else if (header.type === 'symlink') { diff --git a/test/tar/symlink-resolution.test.js b/test/tar/symlink-resolution.test.js new file mode 100644 index 0000000..4183d4d --- /dev/null +++ b/test/tar/symlink-resolution.test.js @@ -0,0 +1,283 @@ +'use strict'; + +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const zlib = require('zlib'); +const uuid = require('uuid'); +const assert = require('assert'); +const compressing = require('../..'); +const { createTarBuffer, createZipBuffer } = require('../util'); + +// Extraction resolves a symlink chain hop by hop when realpath() cannot, so an +// entry whose destination passes through several links still lands where the +// resolved chain actually points, and never outside the extraction directory. +describe('test/tar/symlink-resolution.test.js', () => { + let tempDir; + + beforeEach(() => { + tempDir = path.join(os.tmpdir(), uuid.v4()); + fs.mkdirSync(tempDir, { recursive: true }); + }); + + afterEach(() => { + fs.rmSync(tempDir, { recursive: true, force: true }); + }); + + function gzipBuffer(buf) { + return new Promise((resolve, reject) => { + zlib.gzip(buf, (err, result) => { + if (err) return reject(err); + resolve(result); + }); + }); + } + + // destDir/entry -> destDir/hop -> outsideDir/other.txt, which does not exist, + // so realpath() cannot resolve the chain and each hop is walked by hand. + function setupChain(destDir, outsideDir) { + fs.mkdirSync(outsideDir, { recursive: true }); + fs.mkdirSync(destDir, { recursive: true }); + fs.symlinkSync(path.join(destDir, 'hop'), path.join(destDir, 'entry')); + fs.symlinkSync(path.join(outsideDir, 'other.txt'), path.join(destDir, 'hop')); + } + + // destDir/entry -> linkedDir/other.txt, where destDir/linkedDir -> outsideDir + function setupLinkedDir(destDir, outsideDir) { + fs.mkdirSync(outsideDir, { recursive: true }); + fs.mkdirSync(destDir, { recursive: true }); + fs.symlinkSync(outsideDir, path.join(destDir, 'linkedDir')); + fs.symlinkSync(path.join('linkedDir', 'other.txt'), path.join(destDir, 'entry')); + } + + describe('a chain whose first hop stays inside destDir', () => { + it('should not write past the end of the chain', async () => { + const destDir = path.join(tempDir, 'dest'); + const outsideDir = path.join(tempDir, 'outside'); + setupChain(destDir, outsideDir); + + const tarBuffer = await createTarBuffer([ + { name: 'entry', type: 'file', content: 'content' }, + ]); + + await compressing.tar.uncompress(tarBuffer, destDir); + + assert.strictEqual( + fs.existsSync(path.join(outsideDir, 'other.txt')), + false, + 'The entry should not be written at the end of the chain' + ); + }); + + it('should handle a chain longer than two hops', async () => { + const destDir = path.join(tempDir, 'dest'); + const outsideDir = path.join(tempDir, 'outside'); + fs.mkdirSync(outsideDir, { recursive: true }); + fs.mkdirSync(destDir, { recursive: true }); + fs.symlinkSync(path.join(destDir, 'hop1'), path.join(destDir, 'entry')); + fs.symlinkSync(path.join(destDir, 'hop2'), path.join(destDir, 'hop1')); + fs.symlinkSync(path.join(outsideDir, 'other.txt'), path.join(destDir, 'hop2')); + + const tarBuffer = await createTarBuffer([ + { name: 'entry', type: 'file', content: 'content' }, + ]); + + await compressing.tar.uncompress(tarBuffer, destDir); + + assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false); + }); + + it('should behave the same in tgz extraction', async () => { + const destDir = path.join(tempDir, 'dest'); + const outsideDir = path.join(tempDir, 'outside'); + setupChain(destDir, outsideDir); + + const tarBuffer = await createTarBuffer([ + { name: 'entry', type: 'file', content: 'content' }, + ]); + await compressing.tgz.uncompress(await gzipBuffer(tarBuffer), destDir); + + assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false); + }); + + it('should behave the same in zip extraction', async () => { + const destDir = path.join(tempDir, 'dest'); + const outsideDir = path.join(tempDir, 'outside'); + setupChain(destDir, outsideDir); + + const zipBuffer = await createZipBuffer([ + { name: 'entry', content: 'content' }, + ]); + await compressing.zip.uncompress(zipBuffer, destDir); + + assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false); + }); + }); + + describe('a chain passing through a linked directory', () => { + it('should resolve the directory component of the link target', async () => { + const destDir = path.join(tempDir, 'dest'); + const outsideDir = path.join(tempDir, 'outside'); + setupLinkedDir(destDir, outsideDir); + + const tarBuffer = await createTarBuffer([ + { name: 'entry', type: 'file', content: 'content' }, + ]); + + await compressing.tar.uncompress(tarBuffer, destDir); + + assert.strictEqual( + fs.existsSync(path.join(outsideDir, 'other.txt')), + false, + 'The linked directory in the target should be resolved, not taken literally' + ); + }); + + it('should behave the same in tgz extraction', async () => { + const destDir = path.join(tempDir, 'dest'); + const outsideDir = path.join(tempDir, 'outside'); + setupLinkedDir(destDir, outsideDir); + + const tarBuffer = await createTarBuffer([ + { name: 'entry', type: 'file', content: 'content' }, + ]); + await compressing.tgz.uncompress(await gzipBuffer(tarBuffer), destDir); + + assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false); + }); + + it('should behave the same in zip extraction', async () => { + const destDir = path.join(tempDir, 'dest'); + const outsideDir = path.join(tempDir, 'outside'); + setupLinkedDir(destDir, outsideDir); + + const zipBuffer = await createZipBuffer([ + { name: 'entry', content: 'content' }, + ]); + await compressing.zip.uncompress(zipBuffer, destDir); + + assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false); + }); + }); + + describe('a symlink at the entry destination', () => { + it('should be replaced by the entry instead of written through', async () => { + const destDir = path.join(tempDir, 'dest'); + fs.mkdirSync(destDir, { recursive: true }); + fs.symlinkSync(path.join(destDir, 'hop'), path.join(destDir, 'entry')); + fs.symlinkSync(path.join(destDir, 'final.txt'), path.join(destDir, 'hop')); + + const tarBuffer = await createTarBuffer([ + { name: 'entry', type: 'file', content: 'content' }, + ]); + + await compressing.tar.uncompress(tarBuffer, destDir); + + assert.strictEqual( + fs.lstatSync(path.join(destDir, 'entry')).isSymbolicLink(), + false, + 'The symlink at the destination should have been replaced by a regular file' + ); + assert.strictEqual(fs.readFileSync(path.join(destDir, 'entry'), 'utf8'), 'content'); + assert.strictEqual( + fs.existsSync(path.join(destDir, 'final.txt')), + false, + 'The chain should not have been followed to its target' + ); + }); + + it('should leave the file the symlink points at untouched', async () => { + const destDir = path.join(tempDir, 'dest'); + fs.mkdirSync(destDir, { recursive: true }); + const target = path.join(destDir, 'target.txt'); + fs.writeFileSync(target, 'ORIGINAL_CONTENT'); + fs.symlinkSync(target, path.join(destDir, 'entry')); + + const tarBuffer = await createTarBuffer([ + { name: 'entry', type: 'file', content: 'new content' }, + ]); + + await compressing.tar.uncompress(tarBuffer, destDir); + + assert.strictEqual( + fs.readFileSync(target, 'utf8'), + 'ORIGINAL_CONTENT', + 'Writing an entry must not reach through a symlink to its target' + ); + assert.strictEqual(fs.readFileSync(path.join(destDir, 'entry'), 'utf8'), 'new content'); + }); + }); + + describe('linked directories inside destDir', () => { + it('should still be traversed when writing an entry beneath them', async () => { + const destDir = path.join(tempDir, 'dest'); + const realDir = path.join(destDir, 'real'); + fs.mkdirSync(realDir, { recursive: true }); + fs.symlinkSync(realDir, path.join(destDir, 'linkDir')); + + const tarBuffer = await createTarBuffer([ + { name: 'linkDir/final.txt', type: 'file', content: 'content' }, + ]); + + await compressing.tar.uncompress(tarBuffer, destDir); + + assert.strictEqual( + fs.readFileSync(path.join(realDir, 'final.txt'), 'utf8'), + 'content', + 'A linked directory inside destDir should still be traversed' + ); + }); + }); + + describe('an extraction directory reached through a symlink', () => { + // destDir is given as linkBase/dest while its real path is realBase/dest, the + // shape /var -> /private/var produces on macOS. A link target written in the + // real namespace must still be recognised as living inside destDir. + // + // Skipped on Windows, where a dangling link resolves differently and the entry + // is skipped regardless. That behaviour predates this change, and the namespace + // divergence covered here is a POSIX shape. + const itPosix = process.platform === 'win32' ? it.skip : it; + + itPosix('should accept a dangling target written in the real namespace', async () => { + const realBase = path.join(tempDir, 'realBase'); + const linkBase = path.join(tempDir, 'linkBase'); + fs.mkdirSync(path.join(realBase, 'dest'), { recursive: true }); + fs.symlinkSync(realBase, linkBase); + + const destDir = path.join(linkBase, 'dest'); + // realpathSync, not the realBase path: tempDir may itself sit behind a symlink. + const realDest = fs.realpathSync(path.join(realBase, 'dest')); + fs.symlinkSync(path.join(realDest, 'final.txt'), path.join(destDir, 'entry')); + + const tarBuffer = await createTarBuffer([ + { name: 'entry', type: 'file', content: 'content' }, + ]); + + await compressing.tar.uncompress(tarBuffer, destDir); + + assert.strictEqual( + fs.readFileSync(path.join(destDir, 'entry'), 'utf8'), + 'content', + 'A target inside destDir should be accepted whichever namespace names it' + ); + }); + }); + + describe('symlink cycles', () => { + it('should terminate rather than loop', async () => { + const destDir = path.join(tempDir, 'dest'); + fs.mkdirSync(destDir, { recursive: true }); + fs.symlinkSync(path.join(destDir, 'b'), path.join(destDir, 'entry')); + fs.symlinkSync(path.join(destDir, 'entry'), path.join(destDir, 'b')); + + const tarBuffer = await createTarBuffer([ + { name: 'entry', type: 'file', content: 'content' }, + ]); + + await compressing.tar.uncompress(tarBuffer, destDir); + + assert.strictEqual(fs.lstatSync(path.join(destDir, 'entry')).isSymbolicLink(), true); + }); + }); +}); diff --git a/test/util.js b/test/util.js index 2c26f10..50685da 100644 --- a/test/util.js +++ b/test/util.js @@ -40,5 +40,33 @@ function createTarBuffer(entries) { }); } +/** + * Create a ZIP buffer with given file entries + * @param {Array<{name: string, content?: string}>} entries - Files to put in the archive + * @returns {Promise} The archive contents + */ +function createZipBuffer(entries) { + return new Promise((resolve, reject) => { + // An empty archive never finalizes, so the promise would never settle. + if (!entries || entries.length === 0) { + return reject(new Error('createZipBuffer requires at least one entry')); + } + + const compressing = require('..'); + const zipStream = new compressing.zip.Stream(); + const chunks = []; + + // Listeners first, so an entry rejected during addEntry() settles the promise. + zipStream.on('data', chunk => chunks.push(chunk)); + zipStream.on('end', () => resolve(Buffer.concat(chunks))); + zipStream.on('error', reject); + + for (const entry of entries) { + zipStream.addEntry(Buffer.from(entry.content || ''), { relativePath: entry.name }); + } + }); +} + exports.pipelinePromise = pipelinePromise; +exports.createZipBuffer = createZipBuffer; exports.createTarBuffer = createTarBuffer;