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.

Author: isaacs

lib/unpack.js

@@ -18,10 +18,12 @@ const wc = require('./winchars.js')
 const stripAbsolutePath = require('./strip-absolute-path.js')
 const pathReservations = require('./path-reservations.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')
@@ -45,6 +47,8 @@ const UID = Symbol('uid')
 const GID = Symbol('gid')
 const CHECKED_CWD = Symbol('checkedCwd')
 const crypto = require('crypto')
+const platform = process.env.TESTING_TAR_FAKE_PLATFORM || process.platform
+const isWindows = platform === 'win32'
 
 // Unlinks on Windows are not atomic.
 //
@@ -63,7 +67,7 @@ const crypto = require('crypto')
 // 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')
@@ -76,7 +80,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')
@@ -90,17 +94,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)
@@ -159,7 +179,7 @@ class Unpack extends Parser {
     this.forceChown = opt.forceChown === true
 
     // turn ><?| in filenames into 0xf000-higher encoded forms
-    this.win32 = !!opt.win32 || process.platform === 'win32'
+    this.win32 = !!opt.win32 || isWindows
 
     // do not unpack over files that are newer than what's in the archive
     this.newer = !!opt.newer
@@ -470,7 +490,7 @@ class Unpack extends Parser {
       !this.unlink &&
       st.isFile() &&
       st.nlink <= 1 &&
-      process.platform !== 'win32'
+      !isWindows
   }
 
   // check if a thing is there, and if so, try to clobber it
@@ -481,13 +501,31 @@ class Unpack extends Parser {
       paths.push(entry.linkpath)
     this.reservations.reserve(paths, done => 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 => {
@@ -538,7 +576,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))
@@ -608,8 +652,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)
@@ -658,13 +701,19 @@ class UnpackSync extends Unpack {
     this[MAKEFS](er, entry)
   }
 
-  [FILE] (entry, _) {
+  [FILE] (entry, done) {
     const mode = entry.mode & 0o7777 || this.fmode
 
     const oner = er => {
-      try { fs.closeSync(fd) } catch (_) {}
-      if (er)
-        this[ONERROR](er, entry)
+      let closeError
+      try {
+        fs.closeSync(fd)
+      } catch (e) {
+        closeError = e
+      }
+      if (er || closeError)
+        this[ONERROR](er || closeError, entry)
+      done()
     }
 
     let stream
@@ -725,11 +774,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)
@@ -740,6 +792,7 @@ class UnpackSync extends Unpack {
         fs.chownSync(entry.absolute, this[UID](entry), this[GID](entry))
       } catch (er) {}
     }
+    done()
     entry.resume()
   }
 
@@ -762,9 +815,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)

test/unpack.js

@@ -2693,3 +2693,146 @@ t.test('using strip option when top level file exists', t => {
     check(t, path)
   })
 })
+
+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 = requireInject('../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()
+})