node-tar Vulnerable to Arbitrary File Overwrite and Symlink Poisoning via Insufficient Path Sanitization

@@ -33,6 +33,7 @@ const SYMLINK = Symbol('symlink')
 const HARDLINK = Symbol('hardlink')
 const UNSUPPORTED = Symbol('unsupported')
 const CHECKPATH = Symbol('checkPath')
+const STRIPABSOLUTEPATH = Symbol('stripAbsolutePath')
 const MKDIR = Symbol('mkdir')
 const ONERROR = Symbol('onError')
 const PENDING = Symbol('pending')
@@ -263,6 +264,46 @@ export class Unpack extends Parser {
     }
   }
 
+  // return false if we need to skip this file
+  // return true if the field was successfully sanitized
+  [STRIPABSOLUTEPATH](
+    entry: ReadEntry,
+    field: 'path' | 'linkpath',
+  ): boolean {
+    const path = entry[field]
+    if (!path || this.preservePaths) return true
+
+    const parts = path.split('/')
+    if (
+      parts.includes('..') ||
+      /* c8 ignore next */
+      (isWindows && /^[a-z]:\.\.$/i.test(parts[0] ?? ''))
+    ) {
+      this.warn('TAR_ENTRY_ERROR', `${field} contains '..'`, {
+        entry,
+        [field]: path,
+      })
+      // not ok!
+      return false
+    }
+
+    // strip off the root
+    const [root, stripped] = stripAbsolutePath(path)
+    if (root) {
+      // ok, but triggers warning about stripping root
+      entry[field] = String(stripped)
+      this.warn(
+        'TAR_ENTRY_INFO',
+        `stripping ${root} from absolute ${field}`,
+        {
+          entry,
+          [field]: path,
+        },
+      )
+    }
+    return true
+  }
+
   [CHECKPATH](entry: ReadEntry) {
     const p = normalizeWindowsPath(entry.path)
     const parts = p.split('/')
@@ -295,32 +336,11 @@ export class Unpack extends Parser {
       return false
     }
 
-    if (!this.preservePaths) {
-      if (
-        parts.includes('..') ||
-        /* c8 ignore next */
-        (isWindows && /^[a-z]:\.\.$/i.test(parts[0] ?? ''))
-      ) {
-        this.warn('TAR_ENTRY_ERROR', `path contains '..'`, {
-          entry,
-          path: p,
-        })
-        return false
-      }
-
-      // strip off the root
-      const [root, stripped] = stripAbsolutePath(p)
-      if (root) {
-        entry.path = String(stripped)
-        this.warn(
-          'TAR_ENTRY_INFO',
-          `stripping ${root} from absolute path`,
-          {
-            entry,
-            path: p,
-          },
-        )
-      }
+    if (
+      !this[STRIPABSOLUTEPATH](entry, 'path') ||
+      !this[STRIPABSOLUTEPATH](entry, 'linkpath')
+    ) {
+      return false
     }
 
     if (path.isAbsolute(entry.path)) {

@@ -0,0 +1,48 @@
+import { readFileSync, readlinkSync, writeFileSync } from 'fs'
+import { resolve } from 'path'
+import t from 'tap'
+import { Header, x } from 'tar'
+
+const targetSym = '/some/absolute/path'
+
+const getExploitTar = () => {
+  const exploitTar = Buffer.alloc(512 + 512 + 1024)
+
+  new Header({
+    path: 'exploit_hard',
+    type: 'Link',
+    size: 0,
+    linkpath: resolve(t.testdirName, 'secret.txt'),
+  }).encode(exploitTar, 0)
+
+  new Header({
+    path: 'exploit_sym',
+    type: 'SymbolicLink',
+    size: 0,
+    linkpath: targetSym,
+  }).encode(exploitTar, 512)
+
+  return exploitTar
+}
+
+const dir = t.testdir({
+  'secret.txt': 'ORIGINAL DATA',
+  'exploit.tar': getExploitTar(),
+  out_repro: {},
+})
+
+const out = resolve(dir, 'out_repro')
+const tarFile = resolve(dir, 'exploit.tar')
+
+t.test('verify that linkpaths get sanitized properly', async t => {
+  await x({
+    cwd: out,
+    file: tarFile,
+    preservePaths: false,
+  })
+
+  writeFileSync(resolve(out, 'exploit_hard'), 'OVERWRITTEN')
+  t.equal(readFileSync(resolve(dir, 'secret.txt'), 'utf8'), 'ORIGINAL DATA')
+
+  t.not(readlinkSync(resolve(out, 'exploit_sym')), targetSym)
+})