From 3aaf19b2501bbddb145d92b3322c80dcaed3c35f Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 11 Aug 2021 16:21:48 -0700 Subject: [PATCH] fix: prune dirCache properly for unicode, windows This prunes the dirCache in a way that catches unicode filename matches. If a symbolic link is encountered on Windows, the entire dirCache is cleared, as 8.3 shortname collisions may result in a path escape vulnerability in the case of symbolic links. If such a collision occurs in the case of other types of entries, it is not such a big problem, because the unpack will fail. --- lib/unpack.js | 86 ++++++++++++++++++++++------- test/unpack.js | 143 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 210 insertions(+), 19 deletions(-) diff --git a/lib/unpack.js b/lib/unpack.js index cf10d073..0783949c 100644 --- a/lib/unpack.js +++ b/lib/unpack.js @@ -16,10 +16,12 @@ const wc = require('./winchars.js') const pathReservations = require('./path-reservations.js') const stripAbsolutePath = require('./strip-absolute-path.js') const normPath = require('./normalize-windows-path.js') +const stripSlash = require('./strip-trailing-slashes.js') const ONENTRY = Symbol('onEntry') const CHECKFS = Symbol('checkFs') const CHECKFS2 = Symbol('checkFs2') +const PRUNECACHE = Symbol('pruneCache') const ISREUSABLE = Symbol('isReusable') const MAKEFS = Symbol('makeFs') const FILE = Symbol('file') @@ -43,6 +45,8 @@ const GID = Symbol('gid') const CHECKED_CWD = Symbol('checkedCwd') const crypto = require('crypto') const getFlag = require('./get-write-flag.js') +const platform = process.env.TESTING_TAR_FAKE_PLATFORM || process.platform +const isWindows = platform === 'win32' // Unlinks on Windows are not atomic. // @@ -61,7 +65,7 @@ const getFlag = require('./get-write-flag.js') // See: https://github.com/npm/node-tar/issues/183 /* istanbul ignore next */ const unlinkFile = (path, cb) => { - if (process.platform !== 'win32') + if (!isWindows) return fs.unlink(path, cb) const name = path + '.DELETE.' + crypto.randomBytes(16).toString('hex') @@ -74,7 +78,7 @@ const unlinkFile = (path, cb) => { /* istanbul ignore next */ const unlinkFileSync = path => { - if (process.platform !== 'win32') + if (!isWindows) return fs.unlinkSync(path) const name = path + '.DELETE.' + crypto.randomBytes(16).toString('hex') @@ -88,17 +92,33 @@ const uint32 = (a, b, c) => : b === b >>> 0 ? b : c +// clear the cache if it's a case-insensitive unicode-squashing match. +// we can't know if the current file system is case-sensitive or supports +// unicode fully, so we check for similarity on the maximally compatible +// representation. Err on the side of pruning, since all it's doing is +// preventing lstats, and it's not the end of the world if we get a false +// positive. +// Note that on windows, we always drop the entire cache whenever a +// symbolic link is encountered, because 8.3 filenames are impossible +// to reason about, and collisions are hazards rather than just failures. +const cacheKeyNormalize = path => stripSlash(normPath(path)) + .normalize('NFKD') + .toLowerCase() + const pruneCache = (cache, abs) => { - // clear the cache if it's a case-insensitive match, since we can't - // know if the current file system is case-sensitive or not. - abs = normPath(abs).toLowerCase() + abs = cacheKeyNormalize(abs) for (const path of cache.keys()) { - const plower = path.toLowerCase() - if (plower === abs || plower.toLowerCase().indexOf(abs + '/') === 0) + const pnorm = cacheKeyNormalize(path) + if (pnorm === abs || pnorm.indexOf(abs + '/') === 0) cache.delete(path) } } +const dropCache = cache => { + for (const key of cache.keys()) + cache.delete(key) +} + class Unpack extends Parser { constructor (opt) { if (!opt) @@ -158,7 +178,7 @@ class Unpack extends Parser { this.forceChown = opt.forceChown === true // turn > this[CHECKFS2](entry, done)) } - [CHECKFS2] (entry, done) { + [PRUNECACHE] (entry) { // if we are not creating a directory, and the path is in the dirCache, // then that means we are about to delete the directory we created // previously, and it is no longer going to be a directory, and neither // is any of its children. - if (entry.type !== 'Directory') + // If a symbolic link is encountered on Windows, all bets are off. + // There is no reasonable way to sanitize the cache in such a way + // we will be able to avoid having filesystem collisions. If this + // happens with a non-symlink entry, it'll just fail to unpack, + // but a symlink to a directory, using an 8.3 shortname, can evade + // detection and lead to arbitrary writes to anywhere on the system. + if (isWindows && entry.type === 'SymbolicLink') + dropCache(this.dirCache) + else if (entry.type !== 'Directory') pruneCache(this.dirCache, entry.absolute) + } + + [CHECKFS2] (entry, fullyDone) { + this[PRUNECACHE](entry) + + const done = er => { + this[PRUNECACHE](entry) + fullyDone(er) + } const checkCwd = () => { this[MKDIR](this.cwd, this.dmode, er => { @@ -566,7 +603,13 @@ class Unpack extends Parser { return afterChmod() return fs.chmod(entry.absolute, entry.mode, afterChmod) } - // not a dir entry, have to remove it. + // Not a dir entry, have to remove it. + // NB: the only way to end up with an entry that is the cwd + // itself, in such a way that == does not detect, is a + // tricky windows absolute path with UNC or 8.3 parts (and + // preservePaths:true, or else it will have been stripped). + // In that case, the user has opted out of path protections + // explicitly, so if they blow away the cwd, c'est la vie. if (entry.absolute !== this.cwd) { return fs.rmdir(entry.absolute, er => this[MAKEFS](er, entry, done)) @@ -641,8 +684,7 @@ class UnpackSync extends Unpack { } [CHECKFS] (entry) { - if (entry.type !== 'Directory') - pruneCache(this.dirCache, entry.absolute) + this[PRUNECACHE](entry) if (!this[CHECKED_CWD]) { const er = this[MKDIR](this.cwd, this.dmode) @@ -691,7 +733,7 @@ class UnpackSync extends Unpack { this[MAKEFS](er, entry) } - [FILE] (entry, _) { + [FILE] (entry, done) { const mode = entry.mode & 0o7777 || this.fmode const oner = er => { @@ -703,6 +745,7 @@ class UnpackSync extends Unpack { } if (er || closeError) this[ONERROR](er || closeError, entry) + done() } let fd @@ -762,11 +805,14 @@ class UnpackSync extends Unpack { }) } - [DIRECTORY] (entry, _) { + [DIRECTORY] (entry, done) { const mode = entry.mode & 0o7777 || this.dmode const er = this[MKDIR](entry.absolute, mode) - if (er) - return this[ONERROR](er, entry) + if (er) { + this[ONERROR](er, entry) + done() + return + } if (entry.mtime && !this.noMtime) { try { fs.utimesSync(entry.absolute, entry.atime || new Date(), entry.mtime) @@ -777,6 +823,7 @@ class UnpackSync extends Unpack { fs.chownSync(entry.absolute, this[UID](entry), this[GID](entry)) } catch (er) {} } + done() entry.resume() } @@ -799,9 +846,10 @@ class UnpackSync extends Unpack { } } - [LINK] (entry, linkpath, link, _) { + [LINK] (entry, linkpath, link, done) { try { fs[link + 'Sync'](linkpath, entry.absolute) + done() entry.resume() } catch (er) { return this[ONERROR](er, entry) diff --git a/test/unpack.js b/test/unpack.js index 3ebbb233..d680638c 100644 --- a/test/unpack.js +++ b/test/unpack.js @@ -3001,3 +3001,146 @@ t.test('do not hang on large files that fail to open()', t => { }) }) }) + +t.test('dirCache pruning unicode normalized collisions', { + skip: isWindows && 'symlinks not fully supported', +}, t => { + const data = makeTar([ + { + type: 'Directory', + path: 'foo', + }, + { + type: 'File', + path: 'foo/bar', + size: 1, + }, + 'x', + { + type: 'Directory', + // café + path: Buffer.from([0x63, 0x61, 0x66, 0xc3, 0xa9]).toString(), + }, + { + type: 'SymbolicLink', + // cafe with a ` + path: Buffer.from([0x63, 0x61, 0x66, 0x65, 0xcc, 0x81]).toString(), + linkpath: 'foo', + }, + { + type: 'File', + path: Buffer.from([0x63, 0x61, 0x66, 0xc3, 0xa9]).toString() + '/bar', + size: 1, + }, + 'y', + '', + '', + ]) + + const check = (path, dirCache, t) => { + path = path.replace(/\\/g, '/') + t.strictSame([...dirCache.entries()], [ + [path, true], + [`${path}/foo`, true], + ]) + t.equal(fs.readFileSync(path + '/foo/bar', 'utf8'), 'x') + t.end() + } + + t.test('sync', t => { + const path = t.testdir() + const dirCache = new Map() + new UnpackSync({ cwd: path, dirCache }).end(data) + check(path, dirCache, t) + }) + t.test('async', t => { + const path = t.testdir() + const dirCache = new Map() + new Unpack({ cwd: path, dirCache }) + .on('close', () => check(path, dirCache, t)) + .end(data) + }) + + t.end() +}) + +t.test('dircache prune all on windows when symlink encountered', t => { + if (process.platform !== 'win32') { + process.env.TESTING_TAR_FAKE_PLATFORM = 'win32' + t.teardown(() => { + delete process.env.TESTING_TAR_FAKE_PLATFORM + }) + } + const symlinks = [] + const Unpack = t.mock('../lib/unpack.js', { + fs: { + ...fs, + symlink: (target, dest, cb) => { + symlinks.push(['async', target, dest]) + process.nextTick(cb) + }, + symlinkSync: (target, dest) => symlinks.push(['sync', target, dest]), + }, + }) + const UnpackSync = Unpack.Sync + + const data = makeTar([ + { + type: 'Directory', + path: 'foo', + }, + { + type: 'File', + path: 'foo/bar', + size: 1, + }, + 'x', + { + type: 'Directory', + // café + path: Buffer.from([0x63, 0x61, 0x66, 0xc3, 0xa9]).toString(), + }, + { + type: 'SymbolicLink', + // cafe with a ` + path: Buffer.from([0x63, 0x61, 0x66, 0x65, 0xcc, 0x81]).toString(), + linkpath: 'safe/actually/but/cannot/be/too/careful', + }, + { + type: 'File', + path: 'bar/baz', + size: 1, + }, + 'z', + '', + '', + ]) + + const check = (path, dirCache, t) => { + // symlink blew away all dirCache entries before it + path = path.replace(/\\/g, '/') + t.strictSame([...dirCache.entries()], [ + [`${path}/bar`, true], + ]) + t.equal(fs.readFileSync(`${path}/foo/bar`, 'utf8'), 'x') + t.equal(fs.readFileSync(`${path}/bar/baz`, 'utf8'), 'z') + t.end() + } + + t.test('sync', t => { + const path = t.testdir() + const dirCache = new Map() + new UnpackSync({ cwd: path, dirCache }).end(data) + check(path, dirCache, t) + }) + + t.test('async', t => { + const path = t.testdir() + const dirCache = new Map() + new Unpack({ cwd: path, dirCache }) + .on('close', () => check(path, dirCache, t)) + .end(data) + }) + + t.end() +})