fix: do not write linkpaths through symlinks

Prevent any `Link` or `SymbolicLink` entry from being created if its
`linkpath` would target a location that is through a symbolic link from
the current working directory.

This matches the behavior of `bsdtar` for hard links, and is somewhat
more restrictive in applying the same logic to symbolic links as well.

Unpacking links with targets that extend through symlink folders is
allowed if `preservePaths` option is enabled, as this disables all
protective link checking by design, and is only designed for use with
trusted input.

Fix: GHSA-83g3-92jg-28cx

Author: isaacs

src/process-umask.ts

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

src/unpack.ts

@@ -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) {

test/process-umask.ts

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

test/unpack.js

@@ -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()
+})