CVE-2018-20834: Improper Link Resolution Before File Access ('Link Following') (CWE-59)
value: A vulnerability was found in node-tar before version 4.4.2 (excluding version 2.2.2). An Arbitrary File Overwrite issue exists when extracting a tarball containing a hardlink to a file that already exists on the system, in conjunction with a later plain file with the same name as the hardlink. This plain file content replaces the existing file content. A patch has been applied to node-tar v2.2.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: 7.5
Published: 2019-04-30T19:29Z
Repository: https://github.com/npm/node-tar
Language: JavaScript
Files changed: lib/unpack.js, test/unpack.js
Commit date: 2018-04-30 11:42:36 -0700
Commit message: unpack: only reuse file fs entries if nlink = 1

This matches behavior in bsdtar to prevent overwriting a hardlink with
contents from a File archive entry.

Diff: diff --git a/lib/unpack.js b/lib/unpack.js
index 3e1c527..1c16d5d 100644
--- a/lib/unpack.js
+++ b/lib/unpack.js
@@ -10,10 +10,11 @@ const mkdir = require('./mkdir.js')
 const mkdirSync = mkdir.sync
 const wc = require('./winchars.js')
 
 const ONENTRY = Symbol('onEntry')
 const CHECKFS = Symbol('checkFs')
+const ISREUSABLE = Symbol('isReusable')
 const MAKEFS = Symbol('makeFs')
 const FILE = Symbol('file')
 const DIRECTORY = Symbol('directory')
 const LINK = Symbol('link')
 const SYMLINK = Symbol('symlink')
@@ -349,20 +350,31 @@ class Unpack extends Parser {
   [SKIP] (entry) {
     this[UNPEND]()
     entry.resume()
   }
 
+  // Check if we can reuse an existing filesystem entry safely and
+  // overwrite it, rather than unlinking and recreating
+  // Windows doesn't report a useful nlink, so we just never reuse entries
+  [ISREUSABLE] (entry, st) {
+    return entry.type === 'File' &&
+      !this.unlink &&
+      st.isFile() &&
+      st.nlink <= 1 &&
+      process.platform !== 'win32'
+  }
+
   // check if a thing is there, and if so, try to clobber it
   [CHECKFS] (entry) {
     this[PEND]()
     this[MKDIR](path.dirname(entry.absolute), this.dmode, er => {
       if (er)
         return this[ONERROR](er, entry)
       fs.lstat(entry.absolute, (er, st) => {
         if (st && (this.keep || this.newer && st.mtime > entry.mtime))
           this[SKIP](entry)
-        else if (er || (entry.type === 'File' && !this.unlink && st.isFile()))
+        else if (er || this[ISREUSABLE](entry, st))
           this[MAKEFS](null, entry)
         else if (st.isDirectory()) {
           if (entry.type === 'Directory') {
             if (!entry.mode || (st.mode & 0o7777) === entry.mode)
               this[MAKEFS](null, entry)
@@ -420,11 +432,11 @@ class UnpackSync extends Unpack {
       return this[ONERROR](er, entry)
     try {
       const st = fs.lstatSync(entry.absolute)
       if (this.keep || this.newer && st.mtime > entry.mtime)
         return this[SKIP](entry)
-      else if (entry.type === 'File' && !this.unlink && st.isFile())
+      else if (this[ISREUSABLE](entry, st))
         return this[MAKEFS](null, entry)
       else {
         try {
           if (st.isDirectory()) {
             if (entry.type === 'Directory') {
diff --git a/test/unpack.js b/test/unpack.js
index c7bf791..20ca79a 100644
--- a/test/unpack.js
+++ b/test/unpack.js
@@ -2284,5 +2284,68 @@ t.test('onentry option is preserved', t => {
     unpack.end(data)
   })
 
   t.end()
 })
+
+t.test('do not reuse hardlinks, only nlink=1 files', t => {
+  const basedir = path.resolve(unpackdir, 'hardlink-reuse')
+  mkdirp.sync(basedir)
+  t.teardown(() => rimraf.sync(basedir))
+
+  const now = new Date('2018-04-30T18:30:39.025Z')
+
+  const data = makeTar([
+    {
+      path: 'overwriteme',
+      type: 'File',
+      size: 4,
+      mode: 0o644,
+      mtime: now
+    },
+    'foo\n',
+    {
+      path: 'link',
+      linkpath: 'overwriteme',
+      type: 'Link',
+      mode: 0o644,
+      mtime: now
+    },
+    {
+      path: 'link',
+      type: 'File',
+      size: 4,
+      mode: 0o644,
+      mtime: now
+    },
+    'bar\n',
+    '',
+    ''
+  ])
+
+  const checks = {
+    'link': 'bar\n',
+    'overwriteme': 'foo\n'
+  }
+
+  const check = t => {
+    for (let f in checks) {
+      t.equal(fs.readFileSync(basedir + '/' + f, 'utf8'), checks[f], f)
+      t.equal(fs.statSync(basedir + '/' + f).nlink, 1, f)
+    }
+    t.end()
+  }
+
+  t.test('async', t => {
+    const u = new Unpack({ cwd: basedir })
+    u.on('close', () => check(t))
+    u.end(data)
+  })
+
+  t.test('sync', t => {
+    const u = new UnpackSync({ cwd: basedir })
+    u.end(data)
+    check(t)
+  })
+
+  t.end()
+})

Vulnerable code:         else if (er || (entry.type === 'File' && !this.unlink && st.isFile()))
      else if (entry.type === 'File' && !this.unlink && st.isFile())
Fixed code: const ISREUSABLE = Symbol('isReusable')
  // Check if we can reuse an existing filesystem entry safely and
  // overwrite it, rather than unlinking and recreating
  // Windows doesn't report a useful nlink, so we just never reuse entries
  [ISREUSABLE] (entry, st) {
    return entry.type === 'File' &&
      !this.unlink &&
      st.isFile() &&
      st.nlink <= 1 &&
      process.platform !== 'win32'
  }

        else if (er || this[ISREUSABLE](entry, st))
      else if (this[ISREUSABLE](entry, st))

t.test('do not reuse hardlinks, only nlink=1 files', t => {
  const basedir = path.resolve(unpackdir, 'hardlink-reuse')
  mkdirp.sync(basedir)
  t.teardown(() => rimraf.sync(basedir))

  const now = new Date('2018-04-30T18:30:39.025Z')

  const data = makeTar([
    {
      path: 'overwriteme',
      type: 'File',
      size: 4,
      mode: 0o644,
      mtime: now
    },
    'foo\n',
    {
      path: 'link',
      linkpath: 'overwriteme',
      type: 'Link',
      mode: 0o644,
      mtime: now
    },
    {
      path: 'link',
      type: 'File',
      size: 4,
      mode: 0o644,
      mtime: now
    },
    'bar\n',
    '',
    ''
  ])

  const checks = {
    'link': 'bar\n',
    'overwriteme': 'foo\n'
  }

  const check = t => {
    for (let f in checks) {
      t.equal(fs.readFileSync(basedir + '/' + f, 'utf8'), checks[f], f)
      t.equal(fs.statSync(basedir + '/' + f).nlink, 1, f)
    }
    t.end()
  }

  t.test('async', t => {
    const u = new Unpack({ cwd: basedir })
    u.on('close', () => check(t))
    u.end(data)
  })

  t.test('sync', t => {
    const u = new UnpackSync({ cwd: basedir })
    u.end(data)
    check(t)
  })

  t.end()
})