fix(extract): prevent raced symlink writes outside cwd

Fix a race in file extraction where a destination path could be swapped
to a symlink after the existing `lstat()` check but before the file was
opened.

This change uses `O_NOFOLLOW` for extracted file writes on non-Windows
platforms so the final open does not follow a symlink at the destination
path.

Also adds a regression test that reproduces the race and checks that
extraction does not overwrite a file outside the target directory.

PR-URL: isaacs#456
Credit: @Jvr2022
Close: isaacs#456
Reviewed-by: @isaacs

Author: Jvr2022

src/get-write-flag.ts

@@ -12,7 +12,7 @@ const platform = process.env.__FAKE_PLATFORM__ || process.platform
 const isWindows = platform === 'win32'
 
 /* c8 ignore start */
-const { O_CREAT, O_TRUNC, O_WRONLY } = fs.constants
+const { O_CREAT, O_NOFOLLOW, O_TRUNC, O_WRONLY } = fs.constants
 const UV_FS_O_FILEMAP =
   Number(process.env.__FAKE_FS_O_FILENAME__) ||
   fs.constants.UV_FS_O_FILEMAP ||
@@ -22,7 +22,11 @@ const UV_FS_O_FILEMAP =
 const fMapEnabled = isWindows && !!UV_FS_O_FILEMAP
 const fMapLimit = 512 * 1024
 const fMapFlag = UV_FS_O_FILEMAP | O_TRUNC | O_CREAT | O_WRONLY
+const noFollowFlag =
+  !isWindows && typeof O_NOFOLLOW === 'number' ?
+    O_NOFOLLOW | O_TRUNC | O_CREAT | O_WRONLY
+  : null
 export const getWriteFlag =
-  !fMapEnabled ?
-    () => 'w'
+  noFollowFlag !== null ? () => noFollowFlag
+  : !fMapEnabled ? () => 'w'
   : (size: number) => (size < fMapLimit ? fMapFlag : 'w')

test/get-write-flag.js

@@ -13,6 +13,13 @@ const __filename = fileURLToPath(import.meta.url)
 const hasFmap = !!fs.constants.UV_FS_O_FILEMAP
 const { platform } = process
 const UV_FS_O_FILEMAP = 0x20000000
+const unixFlag =
+  typeof fs.constants.O_NOFOLLOW === 'number' ?
+    fs.constants.O_NOFOLLOW |
+    fs.constants.O_TRUNC |
+    fs.constants.O_CREAT |
+    fs.constants.O_WRONLY
+  : 'w'
 
 switch (process.argv[2]) {
   case 'win32-fmap': {
@@ -32,8 +39,8 @@ switch (process.argv[2]) {
   }
 
   case 'unix': {
-    t.equal(getWriteFlag(1), 'w')
-    t.equal(getWriteFlag(512 * 1024 + 1), 'w')
+    t.equal(getWriteFlag(1), unixFlag)
+    t.equal(getWriteFlag(512 * 1024 + 1), unixFlag)
     break
   }
 

test/symlink-race-extract.ts

@@ -0,0 +1,77 @@
+import fs, { lstatSync, readFileSync, symlinkSync, writeFileSync } from 'node:fs'
+import { resolve } from 'node:path'
+import t from 'tap'
+import { extract } from '../src/extract.js'
+import { Header } from '../src/header.js'
+
+if (typeof fs.constants.O_NOFOLLOW !== 'number') {
+  t.plan(0, 'no O_NOFOLLOW flag')
+  process.exit(0)
+}
+
+const makeTarball = () => {
+  const header = Buffer.alloc(512)
+  new Header({
+    path: 'victim.txt',
+    type: 'File',
+    size: 6,
+  }).encode(header)
+
+  return Buffer.concat([
+    header,
+    Buffer.from('PWNED\n'.padEnd(512, '\0')),
+    Buffer.alloc(1024),
+  ])
+}
+
+t.test('extract does not follow a raced-in symlink for file entries', async t => {
+  const dir = t.testdir({
+    cwd: {},
+    'target.txt': 'ORIGINAL\n',
+  })
+  const cwd = resolve(dir, 'cwd')
+  const target = resolve(dir, 'target.txt')
+  const tarball = resolve(dir, 'poc.tar')
+  const victim = resolve(cwd, 'victim.txt')
+  writeFileSync(tarball, makeTarball())
+
+  const warnings: [code: string, message: string][] = []
+  const lstat = fs.lstat
+  let raced = false
+  fs.lstat = ((path, options, cb) => {
+    const callback =
+      typeof options === 'function' ? options
+      : (cb as Parameters<typeof fs.lstat>[1] & ((
+          err: NodeJS.ErrnoException | null,
+          stats: fs.Stats,
+        ) => void))
+    if (!raced && String(path) === victim) {
+      raced = true
+      symlinkSync(target, victim)
+      const er = Object.assign(new Error('raced symlink'), {
+        code: 'ENOENT',
+      })
+      process.nextTick(() => callback(er, undefined as never))
+      return
+    }
+    return typeof options === 'function' ?
+        lstat(path, options)
+      : lstat(path, options, cb as never)
+  }) as typeof fs.lstat
+  t.teardown(() => {
+    fs.lstat = lstat
+  })
+
+  await extract({
+    cwd,
+    file: tarball,
+    onwarn: (code, message) => warnings.push([code, String(message)]),
+  })
+
+  t.equal(readFileSync(target, 'utf8'), 'ORIGINAL\n')
+  t.equal(lstatSync(victim).isSymbolicLink(), true)
+  t.match(warnings, [[
+    'TAR_ENTRY_ERROR',
+    /ELOOP|symbolic link|Too many symbolic links/,
+  ]])
+})