diff --git a/.github/workflows/nodejs.yml b/.github/workflows/nodejs.yml index e3ac637..b737e1a 100644 --- a/.github/workflows/nodejs.yml +++ b/.github/workflows/nodejs.yml @@ -15,6 +15,6 @@ jobs: uses: node-modules/github-actions/.github/workflows/node-test.yml@master with: os: 'ubuntu-latest, macos-latest, windows-latest' - version: '18, 20, 22, 24' + version: '18, 20, 22, 24, 26' secrets: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} diff --git a/lib/utils.js b/lib/utils.js index 0940071..b2ae90d 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -4,6 +4,33 @@ const fs = require('fs'); const path = require('path'); const { pipeline: pump } = require('stream'); +// 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 same way node:zip opens with O_NOFOLLOW. 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 + */ +async function unlinkSymlink(target) { + try { + const stat = await fs.promises.lstat(target); + if (stat.isSymbolicLink()) await fs.promises.unlink(target); + } catch (e) { + if (e.code !== 'ENOENT') throw e; + } +} + /** * Check if childPath is within parentPath (prevents path traversal attacks) * @param {string} childPath - The path to check @@ -28,16 +55,33 @@ function isPathWithinParent(childPath, parentPath) { * @param {string} targetPath - Absolute path to validate * @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) - * @returns {Promise} true if safe, false if any segment escapes via symlink + * @param {number} depth - Recursion depth when re-walking a dangling symlink's target + * @return {Promise} true if safe, false if any segment escapes via symlink */ -async function isRealPathSafe(targetPath, parentDir, realParentDir) { +async function isRealPathSafe(targetPath, parentDir, realParentDir, 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 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 false; + } + + const relative = path.relative(baseDir, targetPath); const segments = relative.split(path.sep); - let current = parentDir; + let current = baseDir; for (const segment of segments) { if (!segment || segment === '.') continue; current = path.join(current, segment); @@ -49,10 +93,14 @@ async function isRealPathSafe(targetPath, parentDir, realParentDir) { resolved = await fs.promises.realpath(current); } catch (e) { if (e.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. const linkTarget = await fs.promises.readlink(current); const absTarget = path.resolve(path.dirname(current), linkTarget); - return isWithinParent(absTarget); + if (!isWithinParent(absTarget)) return false; + return await isRealPathSafe(absTarget, parentDir, realParentDir, depth + 1); } // Fail closed: unexpected errors during symlink resolution are unsafe return false; @@ -197,8 +245,12 @@ exports.makeUncompressFn = StreamClass => { if (header.type === 'file') { const dir = path.dirname(destFilePath); await fs.promises.mkdir(dir, { recursive: true }); + await unlinkSymlink(destFilePath); entryCount++; - pump(stream, fs.createWriteStream(destFilePath, { mode: opts.mode || header.mode }), err => { + pump(stream, fs.createWriteStream(destFilePath, { + flags: NO_FOLLOW_WRITE_FLAGS, + mode: opts.mode || header.mode, + }), err => { if (err) return reject(err); successCount++; done(); 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 3951255..9a24298 100644 --- a/test/util.js +++ b/test/util.js @@ -5,8 +5,8 @@ const pipelinePromise = stream.promises.pipeline; /** * Create a TAR buffer with given entries - * @param {Array<{name: string, type?: string, linkname?: string, content?: string}>} entries - * @returns {Promise} + * @param {Array<{name: string, type?: string, linkname?: string, content?: string}>} entries - Entries to put in the archive + * @return {Promise} The archive contents */ function createTarBuffer(entries) { return new Promise((resolve, reject) => { @@ -31,4 +31,31 @@ function createTarBuffer(entries) { }); } -module.exports = { pipelinePromise, createTarBuffer }; +/** + * Create a ZIP buffer with given file entries + * @param {Array<{name: string, content?: string}>} entries - Files to put in the archive + * @return {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 }); + } + }); +} + +module.exports = { pipelinePromise, createTarBuffer, createZipBuffer };