node-tar has Arbitrary File Read/Write via Hardlink Target Escape Through Symlink Chain in Extraction

@@ -1161,7 +1161,7 @@ export class UnpackSync extends Unpack {
       if (er) return done()
       if (st.isSymbolicLink()) {
         return onError(
-          new SymlinkError(t, path.resolve(t, parts.join('/'))),
+          new SymlinkError(t, path.resolve(cwd, parts.join('/'))),
         )
       }
     }

@@ -94,8 +94,8 @@ t.test('pack a file', t => {
 
       t.equal(sync.subarray(512).length, data.subarray(512).length)
       t.equal(
-        (sync.subarray(512).toString()),
-        (data.subarray(512).toString()),
+        sync.subarray(512).toString(),
+        data.subarray(512).toString(),
       )
       const hs = new Header(sync)
       t.match(hs, expect)

@@ -3317,7 +3317,11 @@ t.test('ignore self-referential hardlinks', async t => {
   ])
   const check = (t, warnings) => {
     t.matchSnapshot(warnings)
-    t.strictSame(fs.readdirSync(t.testdirName), [], 'nothing extracted')
+    t.strictSame(
+      fs.readdirSync(t.testdirName),
+      [],
+      'nothing extracted',
+    )
     t.end()
   }
   t.test('async', t => {
@@ -3450,10 +3454,11 @@ t.test('no linking through a symlink', t => {
         '',
         '',
       ])
-      const setup = t =>  t.testdir({
-        x: {},
-        'exploited-file': 'original content',
-      })
+      const setup = t =>
+        t.testdir({
+          x: {},
+          'exploited-file': 'original content',
+        })
       const check = t => {
         fs.writeFileSync(t.testdirName + '/x/exploit', 'pwned')
         t.equal(
@@ -3463,20 +3468,38 @@ t.test('no linking through a symlink', t => {
       }
       t.test('sync', t => {
         const cwd = setup(t)
-        t.throws(() => {
-          new UnpackSync({ cwd, strict: true }).end(exploit)
-        })
+        t.throws(
+          () => {
+            new UnpackSync({ cwd, strict: true }).end(exploit)
+          },
+          {
+            name: 'SymlinkError',
+            message: /^TAR_SYMLINK_ERROR/,
+            path: /a.b.escape.exploited-file$/,
+            symlink: /a.b.escape$/,
+          },
+        )
         check(t)
         t.end()
       })
       t.test('async', async t => {
         const cwd = setup(t)
-        await t.rejects(new Promise((res, rej) => {
-          new Unpack({ cwd, strict: true })
-            .on('finish', res)
-            .on('error', rej)
-            .end(exploit)
-        }))
+        await t.rejects(
+          new Promise(
+            (res, rej) => {
+              new Unpack({ cwd, strict: true })
+                .on('finish', res)
+                .on('error', rej)
+                .end(exploit)
+            },
+            {
+              name: 'SymlinkError',
+              message: /^TAR_SYMLINK_ERROR/,
+              path: /a.b.escape.exploited-file$/,
+              symlink: /a.b.escape$/,
+            },
+          ),
+        )
         check(t)
       })
       t.end()

@@ -0,0 +1,2 @@
+// separate file so I stop getting nagged in vim about deprecated API.
+export const umask = () => process.umask()

@@ -20,6 +20,8 @@ import { TarOptions } from './options.js'
 import { PathReservations } from './path-reservations.js'
 import { ReadEntry } from './read-entry.js'
 import { WarnData } from './warn-method.js'
+import { SymlinkError } from './symlink-error.js'
+import { umask } from './process-umask.js'
 
 const ONENTRY = Symbol('onEntry')
 const CHECKFS = Symbol('checkFs')
@@ -31,6 +33,7 @@ const DIRECTORY = Symbol('directory')
 const LINK = Symbol('link')
 const SYMLINK = Symbol('symlink')
 const HARDLINK = Symbol('hardlink')
+const ENSURE_NO_SYMLINK = Symbol('ensureNoSymlink')
 const UNSUPPORTED = Symbol('unsupported')
 const CHECKPATH = Symbol('checkPath')
 const STRIPABSOLUTEPATH = Symbol('stripAbsolutePath')
@@ -235,7 +238,7 @@ export class Unpack extends Parser {
     this.processUmask =
       !this.chmod ? 0
       : typeof opt.processUmask === 'number' ? opt.processUmask
-      : process.umask()
+      : umask()
     this.umask =
       typeof opt.umask === 'number' ? opt.umask : this.processUmask
 
@@ -332,6 +335,7 @@ export class Unpack extends Parser {
     return true
   }
 
+  // no IO, just string checking for absolute indicators
   [CHECKPATH](entry: ReadEntry) {
     const p = normalizeWindowsPath(entry.path)
     const parts = p.split('/')
@@ -663,14 +667,66 @@ export class Unpack extends Parser {
   }
 
   [SYMLINK](entry: ReadEntry, done: () => void) {
-    this[LINK](entry, String(entry.linkpath), 'symlink', done)
+    const parts = normalizeWindowsPath(
+      path.relative(
+        this.cwd,
+        path.resolve(
+          path.dirname(String(entry.absolute)),
+          String(entry.linkpath),
+        ),
+      ),
+    ).split('/')
+    this[ENSURE_NO_SYMLINK](
+      entry,
+      this.cwd,
+      parts,
+      () =>
+        this[LINK](entry, String(entry.linkpath), 'symlink', done),
+      er => {
+        this[ONERROR](er, entry)
+        done()
+      },
+    )
   }
 
   [HARDLINK](entry: ReadEntry, done: () => void) {
     const linkpath = normalizeWindowsPath(
       path.resolve(this.cwd, String(entry.linkpath)),
     )
-    this[LINK](entry, linkpath, 'link', done)
+    const parts = normalizeWindowsPath(String(entry.linkpath)).split(
+      '/',
+    )
+    this[ENSURE_NO_SYMLINK](
+      entry,
+      this.cwd,
+      parts,
+      () => this[LINK](entry, linkpath, 'link', done),
+      er => {
+        this[ONERROR](er, entry)
+        done()
+      },
+    )
+  }
+
+  [ENSURE_NO_SYMLINK](
+    entry: ReadEntry,
+    cwd: string,
+    parts: string[],
+    done: () => void,
+    onError: (er: SymlinkError) => void,
+  ) {
+    const p = parts.shift()
+    if (this.preservePaths || p === undefined) return done()
+    const t = path.resolve(cwd, p)
+    fs.lstat(t, (er, st) => {
+      if (er) return done()
+      if (st?.isSymbolicLink()) {
+        return onError(
+          new SymlinkError(t, path.resolve(t, parts.join('/'))),
+        )
+      }
+      this[ENSURE_NO_SYMLINK](entry, t, parts, done, onError)
+    })
   }
 
   [PEND]() {
@@ -851,7 +907,6 @@ export class Unpack extends Parser {
     link: 'link' | 'symlink',
     done: () => void,
   ) {
-    // XXX: get the type ('symlink' or 'junction') for windows
     fs[link](linkpath, String(entry.absolute), er => {
       if (er) {
         this[ONERROR](er, entry)
@@ -864,11 +919,13 @@ export class Unpack extends Parser {
   }
 }
 
-const callSync = (fn: () => any) => {
+const callSync = <T>(
+  fn: () => T,
+): [null, T] | [NodeJS.ErrnoException, null] => {
   try {
     return [null, fn()]
   } catch (er) {
-    return [er, null]
+    return [er as NodeJS.ErrnoException, null]
   }
 }
 
@@ -1089,15 +1146,37 @@ export class UnpackSync extends Unpack {
     }
   }
 
+  [ENSURE_NO_SYMLINK](
+    _entry: ReadEntry,
+    cwd: string,
+    parts: string[],
+    done: () => void,
+    onError: (er: SymlinkError) => void,
+  ) {
+    if (this.preservePaths || !parts.length) return done()
+    let t = cwd
+    for (const p of parts) {
+      t = path.resolve(t, p)
+      const [er, st] = callSync(() => fs.lstatSync(t))
+      if (er) return done()
+      if (st.isSymbolicLink()) {
+        return onError(
+          new SymlinkError(t, path.resolve(t, parts.join('/'))),
+        )
+      }
+    }
+    done()
+  }
+
   [LINK](
     entry: ReadEntry,
     linkpath: string,
     link: 'link' | 'symlink',
     done: () => void,
   ) {
-    const ls: `${typeof link}Sync` = `${link}Sync`
+    const linkSync: `${typeof link}Sync` = `${link}Sync`
     try {
-      fs[ls](linkpath, String(entry.absolute))
+      fs[linkSync](linkpath, String(entry.absolute))
       done()
       entry.resume()
     } catch (er) {

@@ -0,0 +1,3 @@
+import t from 'tap'
+import { umask } from '../src/process-umask.js'
+t.equal(umask(), process.umask())

@@ -1,6 +1,6 @@
 import { Unpack, UnpackSync } from '../dist/esm/unpack.js'
 
-import fs, { readdirSync, statSync } from 'fs'
+import fs from 'fs'
 import { Minipass } from 'minipass'
 import * as z from 'minizlib'
 import path from 'path'
@@ -3317,7 +3317,7 @@ t.test('ignore self-referential hardlinks', async t => {
   ])
   const check = (t, warnings) => {
     t.matchSnapshot(warnings)
-    t.strictSame(readdirSync(t.testdirName), [], 'nothing extracted')
+    t.strictSame(fs.readdirSync(t.testdirName), [], 'nothing extracted')
     t.end()
   }
   t.test('async', t => {
@@ -3417,8 +3417,70 @@ t.test(
       fs.readFileSync(dir + '/bar/link.txt', 'utf8'),
       'hello world',
     )
-    t.throws(() => statSync(dir+ '/bar/badlink.txt'))
-    t.match(warnings, [['TAR_ENTRY_ERROR', 'linkpath escapes extraction directory']])
-
+    t.throws(() => fs.statSync(dir + '/bar/badlink.txt'))
+    t.match(warnings, [
+      ['TAR_ENTRY_ERROR', 'linkpath escapes extraction directory'],
+    ])
   },
 )
+
+t.test('no linking through a symlink', t => {
+  const types = ['Link', 'SymbolicLink']
+  for (const type of types) {
+    t.test(type, t => {
+      const exploit = makeTar([
+        {
+          type: 'SymbolicLink',
+          path: 'a/b/up',
+          linkpath: '../..',
+          mode: 0o755,
+        },
+        {
+          type: 'SymbolicLink',
+          path: 'a/b/escape',
+          linkpath: 'up/..',
+          mode: 0o755,
+        },
+        {
+          type,
+          path: 'exploit',
+          linkpath: 'a/b/escape/exploited-file',
+          mode: 0o755,
+        },
+        '',
+        '',
+      ])
+      const setup = t =>  t.testdir({
+        x: {},
+        'exploited-file': 'original content',
+      })
+      const check = t => {
+        fs.writeFileSync(t.testdirName + '/x/exploit', 'pwned')
+        t.equal(
+          fs.readFileSync(t.testdirName + '/exploited-file', 'utf8'),
+          'original content',
+        )
+      }
+      t.test('sync', t => {
+        const cwd = setup(t)
+        t.throws(() => {
+          new UnpackSync({ cwd, strict: true }).end(exploit)
+        })
+        check(t)
+        t.end()
+      })
+      t.test('async', async t => {
+        const cwd = setup(t)
+        await t.rejects(new Promise((res, rej) => {
+          new Unpack({ cwd, strict: true })
+            .on('finish', res)
+            .on('error', rej)
+            .end(exploit)
+        }))
+        check(t)
+      })
+      t.end()
+    })
+  }
+  t.end()
+})