CVE-2021-32803: Improper Link Resolution Before File Access ('Link Following') (CWE-59)
value: The npm package "tar" (aka node-tar) before versions 6.1.2, 5.0.7, 4.4.15, and 3.2.3 has an arbitrary File Creation/Overwrite vulnerability via insufficient symlink protection. `node-tar` aims to guarantee that any file whose location would be modified by a symbolic link is not extracted. This is, in part, achieved by ensuring that extracted directories are not symlinks. Additionally, in order to prevent unnecessary `stat` calls to determine whether a given path is a directory, paths are cached when directories are created. This logic was insufficient when extracting tar files that contained both a directory and a symlink with the same name as the directory. This order of operations resulted in the directory being created and added to the `node-tar` directory cache. When a directory is present in the directory cache, subsequent calls to mkdir for that directory are skipped. However, this is also where `node-tar` checks for symlinks occur. By first creating a directory, and then replacing that directory with a symlink, it was thus possible to bypass `node-tar` symlink checks on directories, essentially allowing an untrusted tar file to symlink into an arbitrary location and subsequently extracting arbitrary files into that location, thus allowing arbitrary file creation and overwrite. This issue was addressed in releases 3.2.3, 4.4.15, 5.0.7 and 6.1.2.
CWE description: The product attempts to access a file based on the filename, but it does not properly prevent that filename from identifying a link or shortcut that resolves to an unintended resource.
CVSS v3 base score: 8.1
Published: 2021-08-03T19:15Z
Repository: https://github.com/npm/node-tar
Language: JavaScript
Files changed: lib/unpack.js, test/unpack.js
Commit date: 2021-07-26 16:10:30 -0700
Commit message: Remove paths from dirCache when no longer dirs

Diff: diff --git a/lib/unpack.js b/lib/unpack.js
index 216fa71..edaf783 100644
--- a/lib/unpack.js
+++ b/lib/unpack.js
@@ -463,10 +463,23 @@ class Unpack extends Parser {
       paths.push(entry.linkpath)
     this.reservations.reserve(paths, done => this[CHECKFS2](entry, done))
   }
 
   [CHECKFS2] (entry, done) {
+    // 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') {
+      for (const path of this.dirCache.keys()) {
+        if (path === entry.absolute ||
+            path.indexOf(entry.absolute + '/') === 0 ||
+            path.indexOf(entry.absolute + '\\') === 0)
+          this.dirCache.delete(path)
+      }
+    }
+
     this[MKDIR](path.dirname(entry.absolute), this.dmode, er => {
       if (er) {
         done()
         return this[ONERROR](er, entry)
       }
@@ -527,10 +540,19 @@ class Unpack extends Parser {
   }
 }
 
 class UnpackSync extends Unpack {
   [CHECKFS] (entry) {
+    if (entry.type !== 'Directory') {
+      for (const path of this.dirCache.keys()) {
+        if (path === entry.absolute ||
+            path.indexOf(entry.absolute + '/') === 0 ||
+            path.indexOf(entry.absolute + '\\') === 0)
+          this.dirCache.delete(path)
+      }
+    }
+
     const er = this[MKDIR](path.dirname(entry.absolute), this.dmode, neverCalled)
     if (er)
       return this[ONERROR](er, entry)
     try {
       const st = fs.lstatSync(entry.absolute)
diff --git a/test/unpack.js b/test/unpack.js
index 1b2a033..db7c8f5 100644
--- a/test/unpack.js
+++ b/test/unpack.js
@@ -2603,5 +2603,58 @@ t.test('handle errors on fs.close', t => {
     .end(data)
   t.throws(() => new UnpackSync({
     cwd: dir + '/sync', strict: true,
   }).end(data), poop, 'sync')
 })
+
+t.test('drop entry from dirCache if no longer a directory', t => {
+  const dir = path.resolve(unpackdir, 'dir-cache-error')
+  mkdirp.sync(dir + '/sync/y')
+  mkdirp.sync(dir + '/async/y')
+  const data = makeTar([
+    {
+      path: 'x',
+      type: 'Directory',
+    },
+    {
+      path: 'x',
+      type: 'SymbolicLink',
+      linkpath: './y',
+    },
+    {
+      path: 'x/ginkoid',
+      type: 'File',
+      size: 'ginkoid'.length,
+    },
+    'ginkoid',
+    '',
+    '',
+  ])
+  t.plan(2)
+  const WARNINGS = {}
+  const check = (t, path) => {
+    t.equal(fs.statSync(path + '/x').isDirectory(), true)
+    t.equal(fs.lstatSync(path + '/x').isSymbolicLink(), true)
+    t.equal(fs.statSync(path + '/y').isDirectory(), true)
+    t.strictSame(fs.readdirSync(path + '/y'), [])
+    t.throws(() => fs.readFileSync(path + '/x/ginkoid'), { code: 'ENOENT' })
+    t.strictSame(WARNINGS[path], [
+      'TAR_ENTRY_ERROR',
+      'Cannot extract through symbolic link',
+    ])
+    t.end()
+  }
+  t.test('async', t => {
+    const path = dir + '/async'
+    new Unpack({ cwd: path })
+      .on('warn', (code, msg) => WARNINGS[path] = [code, msg])
+      .on('end', () => check(t, path))
+      .end(data)
+  })
+  t.test('sync', t => {
+    const path = dir + '/sync'
+    new UnpackSync({ cwd: path })
+      .on('warn', (code, msg) => WARNINGS[path] = [code, msg])
+      .end(data)
+    check(t, path)
+  })
+})

Fixed code:     // 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') {
      for (const path of this.dirCache.keys()) {
        if (path === entry.absolute ||
            path.indexOf(entry.absolute + '/') === 0 ||
            path.indexOf(entry.absolute + '\\') === 0)
          this.dirCache.delete(path)
      }
    }

    if (entry.type !== 'Directory') {
      for (const path of this.dirCache.keys()) {
        if (path === entry.absolute ||
            path.indexOf(entry.absolute + '/') === 0 ||
            path.indexOf(entry.absolute + '\\') === 0)
          this.dirCache.delete(path)
      }
    }


t.test('drop entry from dirCache if no longer a directory', t => {
  const dir = path.resolve(unpackdir, 'dir-cache-error')
  mkdirp.sync(dir + '/sync/y')
  mkdirp.sync(dir + '/async/y')
  const data = makeTar([
    {
      path: 'x',
      type: 'Directory',
    },
    {
      path: 'x',
      type: 'SymbolicLink',
      linkpath: './y',
    },
    {
      path: 'x/ginkoid',
      type: 'File',
      size: 'ginkoid'.length,
    },
    'ginkoid',
    '',
    '',
  ])
  t.plan(2)
  const WARNINGS = {}
  const check = (t, path) => {
    t.equal(fs.statSync(path + '/x').isDirectory(), true)
    t.equal(fs.lstatSync(path + '/x').isSymbolicLink(), true)
    t.equal(fs.statSync(path + '/y').isDirectory(), true)
    t.strictSame(fs.readdirSync(path + '/y'), [])
    t.throws(() => fs.readFileSync(path + '/x/ginkoid'), { code: 'ENOENT' })
    t.strictSame(WARNINGS[path], [
      'TAR_ENTRY_ERROR',
      'Cannot extract through symbolic link',
    ])
    t.end()
  }
  t.test('async', t => {
    const path = dir + '/async'
    new Unpack({ cwd: path })
      .on('warn', (code, msg) => WARNINGS[path] = [code, msg])
      .on('end', () => check(t, path))
      .end(data)
  })
  t.test('sync', t => {
    const path = dir + '/sync'
    new UnpackSync({ cwd: path })
      .on('warn', (code, msg) => WARNINGS[path] = [code, msg])
      .end(data)
    check(t, path)
  })
})