pnpm: Binary ZIP extraction allows arbitrary file write via path traversal (Zip Slip)

@@ -0,0 +1,10 @@
+---
+"@pnpm/fetching.binary-fetcher": patch
+"pnpm": patch
+---
+
+Fix path traversal vulnerability in binary fetcher ZIP extraction
+
+- Validate ZIP entry paths before extraction to prevent writing files outside target directory
+- Validate BinaryResolution.prefix (basename) to prevent directory escape via crafted prefix
+- Both attack vectors now throw `ERR_PNPM_PATH_TRAVERSAL` error

@@ -24,8 +24,9 @@
     "!*.map"
   ],
   "scripts": {
-    "lint": "eslint \"src/**/*.ts\"",
-    "test": "pnpm run compile",
+    "_test": "cross-env NODE_OPTIONS=\"$NODE_OPTIONS --experimental-vm-modules\" jest",
+    "test": "pnpm run compile && pnpm run _test",
+    "lint": "eslint \"src/**/*.ts\" \"test/**/*.ts\"",
     "prepublishOnly": "pnpm run compile",
     "compile": "tsc --build && pnpm run lint --fix"
   },
@@ -34,6 +35,7 @@
     "@pnpm/fetcher-base": "workspace:*",
     "@pnpm/fetching-types": "workspace:*",
     "adm-zip": "catalog:",
+    "is-subdir": "catalog:",
     "rename-overwrite": "catalog:",
     "ssri": "catalog:",
     "tempy": "catalog:"
@@ -42,9 +44,11 @@
     "@pnpm/worker": "workspace:^"
   },
   "devDependencies": {
+    "@jest/globals": "catalog:",
     "@pnpm/fetching.binary-fetcher": "workspace:*",
     "@types/adm-zip": "catalog:",
-    "@types/ssri": "catalog:"
+    "@types/ssri": "catalog:",
+    "tempy": "catalog:"
   },
   "engines": {
     "node": ">=18.12"

@@ -8,6 +8,28 @@
 pnpm add @pnpm/fetching.binary-fetcher
 ```
 
+## Testing
+
+### Test Fixtures
+
+The `test/fixtures/` directory contains malicious ZIP files for testing path traversal protection:
+
+| File | Entry Path | Purpose |
+|------|------------|---------|
+| `path-traversal.zip` | `../../../.npmrc` | Tests `../` escape sequences |
+| `absolute-path.zip` | `/etc/passwd` | Tests absolute path entries |
+| `backslash-traversal.zip` | `..\..\..\evil.txt` | Tests Windows backslash traversal (Windows-only) |
+
+These fixtures are manually crafted because AdmZip's `addFile()` sanitizes paths automatically.
+
+> **Note:** The backslash test only runs on Windows because `\` is a valid filename character on Unix.
+
+### Regenerating Fixtures
+
+```bash
+node --experimental-strip-types scripts/create-fixtures.ts
+```
+
 ## License
 
 MIT

@@ -0,0 +1,100 @@
+/**
+ * Script to generate malicious ZIP fixtures for path traversal testing.
+ *
+ * AdmZip's addFile() sanitizes paths automatically, so we need to create
+ * raw ZIP files manually to test path traversal protection.
+ *
+ * Run with: node --experimental-strip-types scripts/create-fixtures.ts
+ */
+import fs from 'fs'
+import path from 'path'
+
+/**
+ * Create a minimal ZIP file with a given entry path (not sanitized).
+ * This creates a valid ZIP structure with a single uncompressed file entry.
+ */
+function createZipWithEntry (entryPath: string, content: string): Buffer {
+  const contentBuf = Buffer.from(content)
+
+  // Local file header (30 bytes + filename)
+  const localHeader = Buffer.alloc(30 + entryPath.length)
+  localHeader.writeUInt32LE(0x04034b50, 0) // Local file header signature
+  localHeader.writeUInt16LE(20, 4) // Version needed to extract
+  localHeader.writeUInt16LE(0, 6) // General purpose flags
+  localHeader.writeUInt16LE(0, 8) // Compression method (0 = store)
+  localHeader.writeUInt16LE(0, 10) // Last mod file time
+  localHeader.writeUInt16LE(0, 12) // Last mod file date
+  localHeader.writeUInt32LE(0, 14) // CRC-32 (fake but okay for tests)
+  localHeader.writeUInt32LE(contentBuf.length, 18) // Compressed size
+  localHeader.writeUInt32LE(contentBuf.length, 22) // Uncompressed size
+  localHeader.writeUInt16LE(entryPath.length, 26) // Filename length
+  localHeader.writeUInt16LE(0, 28) // Extra field length
+  localHeader.write(entryPath, 30, 'utf-8') // Filename
+
+  const cdOffset = localHeader.length + contentBuf.length
+
+  // Central directory header (46 bytes + filename)
+  const centralDir = Buffer.alloc(46 + entryPath.length)
+  centralDir.writeUInt32LE(0x02014b50, 0) // Central file header signature
+  centralDir.writeUInt16LE(20, 4) // Version made by
+  centralDir.writeUInt16LE(20, 6) // Version needed to extract
+  centralDir.writeUInt16LE(0, 8) // General purpose flags
+  centralDir.writeUInt16LE(0, 10) // Compression method
+  centralDir.writeUInt16LE(0, 12) // Last mod file time
+  centralDir.writeUInt16LE(0, 14) // Last mod file date
+  centralDir.writeUInt32LE(0, 16) // CRC-32
+  centralDir.writeUInt32LE(contentBuf.length, 20) // Compressed size
+  centralDir.writeUInt32LE(contentBuf.length, 24) // Uncompressed size
+  centralDir.writeUInt16LE(entryPath.length, 28) // Filename length
+  centralDir.writeUInt16LE(0, 30) // Extra field length
+  centralDir.writeUInt16LE(0, 32) // File comment length
+  centralDir.writeUInt16LE(0, 34) // Disk number start
+  centralDir.writeUInt16LE(0, 36) // Internal file attributes
+  centralDir.writeUInt32LE(0, 38) // External file attributes
+  centralDir.writeUInt32LE(0, 42) // Relative offset of local header
+  centralDir.write(entryPath, 46, 'utf-8')
+
+  // End of central directory record (22 bytes)
+  const endRecord = Buffer.alloc(22)
+  endRecord.writeUInt32LE(0x06054b50, 0) // End of central directory signature
+  endRecord.writeUInt16LE(0, 4) // Number of this disk
+  endRecord.writeUInt16LE(0, 6) // Disk with central directory
+  endRecord.writeUInt16LE(1, 8) // Entries on this disk
+  endRecord.writeUInt16LE(1, 10) // Total entries
+  endRecord.writeUInt32LE(centralDir.length, 12) // Size of central directory
+  endRecord.writeUInt32LE(cdOffset, 16) // Offset of central directory
+  endRecord.writeUInt16LE(0, 20) // ZIP file comment length
+
+  return Buffer.concat([localHeader, contentBuf, centralDir, endRecord])
+}
+
+// Ensure fixtures directory exists
+const fixturesDir = path.join(import.meta.dirname, '..', 'test', 'fixtures')
+fs.mkdirSync(fixturesDir, { recursive: true })
+
+// Create path traversal ZIP (../../../ prefix)
+const pathTraversalZip = createZipWithEntry(
+  '../../../.npmrc',
+  'registry=https://evil.com/\n'
+)
+fs.writeFileSync(path.join(fixturesDir, 'path-traversal.zip'), pathTraversalZip)
+console.log('Created: test/fixtures/path-traversal.zip')
+
+// Create absolute path ZIP (/etc/passwd)
+const absolutePathZip = createZipWithEntry(
+  '/etc/passwd',
+  'root:x:0:0:root:/root:/bin/bash'
+)
+fs.writeFileSync(path.join(fixturesDir, 'absolute-path.zip'), absolutePathZip)
+console.log('Created: test/fixtures/absolute-path.zip')
+
+// Create Windows-style backslash path traversal ZIP
+// This is only dangerous on Windows (on Unix, backslash is a valid filename char)
+const backslashTraversalZip = createZipWithEntry(
+  '..\\..\\..\\evil.txt',
+  'malicious content via backslash'
+)
+fs.writeFileSync(path.join(fixturesDir, 'backslash-traversal.zip'), backslashTraversalZip)
+console.log('Created: test/fixtures/backslash-traversal.zip')
+
+console.log('\nDone! Created malicious ZIP fixtures for path traversal testing.')

@@ -5,6 +5,7 @@ import { type FetchFromRegistry } from '@pnpm/fetching-types'
 import { type BinaryFetcher, type FetchFunction, type FetchResult } from '@pnpm/fetcher-base'
 import { addFilesFromDir } from '@pnpm/worker'
 import AdmZip from 'adm-zip'
+import isSubdir from 'is-subdir'
 import renameOverwrite from 'rename-overwrite'
 import tempy from 'tempy'
 import ssri from 'ssri'
@@ -139,7 +140,7 @@ async function downloadWithIntegrityCheck (
  * @param zipPath - Path to the zip file
  * @param basename - Base name of the file (without extension)
  * @param targetDir - Directory where contents should be extracted
- * @throws {PnpmError} When extraction fails
+ * @throws {PnpmError} When extraction fails or path traversal is detected
  */
 async function extractZipToTarget (
   zipPath: string,
@@ -148,8 +149,39 @@ async function extractZipToTarget (
 ): Promise<void> {
   const zip = new AdmZip(zipPath)
   const nodeDir = basename === '' ? targetDir : path.dirname(targetDir)
-  const extractedDir = path.join(nodeDir, basename)
 
-  zip.extractAllTo(nodeDir, true)
+  // Validate basename/prefix doesn't escape the target directory
+  if (basename !== '') {
+    validatePathSecurity(nodeDir, basename)
+  }
+
+  // Extract each entry with path validation to prevent path traversal attacks
+  for (const entry of zip.getEntries()) {
+    const entryPath = entry.entryName
+    validatePathSecurity(nodeDir, entryPath)
+    zip.extractEntryTo(entry, nodeDir, true, true)
+  }
+
+  const extractedDir = path.join(nodeDir, basename)
   await renameOverwrite(extractedDir, targetDir)
 }
+
+/**
+ * Validates that a path does not escape the base directory via path traversal.
+ *
+ * @param basePath - The base directory that should contain the target
+ * @param targetPath - The relative path to validate
+ * @throws {PnpmError} When path traversal is detected
+ */
+function validatePathSecurity (basePath: string, targetPath: string): void {
+  // Explicitly reject absolute paths - they should never be allowed as prefixes or entry names
+  if (path.isAbsolute(targetPath)) {
+    throw new PnpmError('PATH_TRAVERSAL',
+      `Refusing to extract path "${targetPath}" - absolute paths are not allowed`)
+  }
+  const normalizedTarget = path.resolve(basePath, targetPath)
+  if (!isSubdir(basePath, normalizedTarget) && normalizedTarget !== basePath) {
+    throw new PnpmError('PATH_TRAVERSAL',
+      `Refusing to extract path "${targetPath}" outside of target directory`)
+  }
+}

@@ -0,0 +1,223 @@
+/// <reference path="../../../__typings__/index.d.ts"/>
+import fs from 'fs'
+import path from 'path'
+import { PnpmError } from '@pnpm/error'
+import { temporaryDirectory } from 'tempy'
+import AdmZip from 'adm-zip'
+import ssri from 'ssri'
+import { downloadAndUnpackZip } from '@pnpm/fetching.binary-fetcher'
+
+// Mock fetch function that returns a ZIP buffer and simulates FetchFromRegistry
+function createMockFetch (zipBuffer: Buffer) {
+  return () => Promise.resolve({
+    body: (async function * () {
+      yield zipBuffer
+    })(),
+  })
+}
+
+describe('extractZipToTarget security', () => {
+  describe('prefix path traversal (Attack Vector 2)', () => {
+    it('should reject prefix with ../ path traversal', async () => {
+      const targetDir = temporaryDirectory()
+      const zip = new AdmZip()
+      zip.addFile('node-v20.0.0/bin/node', Buffer.from('#!/bin/sh\necho "node"'))
+      const zipBuffer = zip.toBuffer()
+      // Use real integrity so the check passes and we reach path traversal validation
+      const integrity = ssri.fromData(zipBuffer).toString()
+
+      const mockFetch = createMockFetch(zipBuffer)
+
+      await expect(
+        downloadAndUnpackZip(
+          // eslint-disable-next-line @typescript-eslint/no-explicit-any
+          mockFetch as any,
+          {
+            url: 'https://example.com/node.zip',
+            integrity,
+            basename: '../../evil',
+          },
+          targetDir
+        )
+      ).rejects.toThrow(PnpmError)
+
+      await expect(
+        downloadAndUnpackZip(
+          // eslint-disable-next-line @typescript-eslint/no-explicit-any
+          mockFetch as any,
+          {
+            url: 'https://example.com/node.zip',
+            integrity,
+            basename: '../../evil',
+          },
+          targetDir
+        )
+      ).rejects.toMatchObject({
+        code: 'ERR_PNPM_PATH_TRAVERSAL',
+      })
+    })
+
+    it('should reject absolute path prefix', async () => {
+      const targetDir = temporaryDirectory()
+      const zip = new AdmZip()
+      zip.addFile('node-v20.0.0/bin/node', Buffer.from('#!/bin/sh\necho "node"'))
+      const zipBuffer = zip.toBuffer()
+      // Use real integrity so the check passes and we reach path traversal validation
+      const integrity = ssri.fromData(zipBuffer).toString()
+
+      const mockFetch = createMockFetch(zipBuffer)
+
+      await expect(
+        downloadAndUnpackZip(
+          // eslint-disable-next-line @typescript-eslint/no-explicit-any
+          mockFetch as any,
+          {
+            url: 'https://example.com/node.zip',
+            integrity,
+            basename: '/tmp/evil',
+          },
+          targetDir
+        )
+      ).rejects.toMatchObject({
+        code: 'ERR_PNPM_PATH_TRAVERSAL',
+      })
+    })
+  })
+
+  describe('ZIP entry path traversal (Attack Vector 1)', () => {
+    it('should reject ZIP entries with ../ path traversal', async () => {
+      const targetDir = temporaryDirectory()
+      // Load fixture ZIP that has a raw malicious entry path
+      const zipBuffer = fs.readFileSync(path.join(import.meta.dirname, 'fixtures/path-traversal.zip'))
+      const integrity = ssri.fromData(zipBuffer).toString()
+
+      const mockFetch = createMockFetch(zipBuffer)
+
+      await expect(
+        downloadAndUnpackZip(
+          // eslint-disable-next-line @typescript-eslint/no-explicit-any
+          mockFetch as any,
+          {
+            url: 'https://example.com/node.zip',
+            integrity,
+            basename: '',
+          },
+          targetDir
+        )
+      ).rejects.toMatchObject({
+        code: 'ERR_PNPM_PATH_TRAVERSAL',
+      })
+
+      // Verify no files were written outside target
+      const parentDir = path.dirname(targetDir)
+      expect(fs.existsSync(path.join(parentDir, '.npmrc'))).toBe(false)
+    })
+
+    it('should reject ZIP entries with absolute paths', async () => {
+      const targetDir = temporaryDirectory()
+      // Load fixture ZIP that has a raw malicious absolute path entry
+      const zipBuffer = fs.readFileSync(path.join(import.meta.dirname, 'fixtures/absolute-path.zip'))
+      const integrity = ssri.fromData(zipBuffer).toString()
+
+      const mockFetch = createMockFetch(zipBuffer)
+
+      await expect(
+        downloadAndUnpackZip(
+          // eslint-disable-next-line @typescript-eslint/no-explicit-any
+          mockFetch as any,
+          {
+            url: 'https://example.com/node.zip',
+            integrity,
+            basename: '',
+          },
+          targetDir
+        )
+      ).rejects.toMatchObject({
+        code: 'ERR_PNPM_PATH_TRAVERSAL',
+      })
+    })
+
+    // Windows-specific: backslash is a path separator only on Windows
+    // On Unix, backslash is a valid filename character, so this test only runs on Windows
+    const isWindows = process.platform === 'win32'
+    const windowsTest = isWindows ? it : it.skip
+
+    windowsTest('should reject ZIP entries with backslash path traversal on Windows', async () => {
+      const targetDir = temporaryDirectory()
+      // Load fixture ZIP with Windows-style backslash path traversal
+      const zipBuffer = fs.readFileSync(path.join(import.meta.dirname, 'fixtures/backslash-traversal.zip'))
+      const integrity = ssri.fromData(zipBuffer).toString()
+
+      const mockFetch = createMockFetch(zipBuffer)
+
+      await expect(
+        downloadAndUnpackZip(
+          // eslint-disable-next-line @typescript-eslint/no-explicit-any
+          mockFetch as any,
+          {
+            url: 'https://example.com/node.zip',
+            integrity,
+            basename: '',
+          },
+          targetDir
+        )
+      ).rejects.toMatchObject({
+        code: 'ERR_PNPM_PATH_TRAVERSAL',
+      })
+    })
+  })
+
+  describe('legitimate ZIP extraction', () => {
+    it('should successfully extract a normal ZIP file', async () => {
+      const targetDir = temporaryDirectory()
+      const zip = new AdmZip()
+      zip.addFile('node-v20.0.0/bin/node', Buffer.from('#!/bin/sh\necho "node"'))
+      zip.addFile('node-v20.0.0/README.md', Buffer.from('# Node.js'))
+      const zipBuffer = zip.toBuffer()
+
+      // Create a mock fetch that also passes integrity check by using the actual buffer
+      const integrity = ssri.fromData(zipBuffer).toString()
+
+      const mockFetch = createMockFetch(zipBuffer)
+
+      await downloadAndUnpackZip(
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        mockFetch as any,
+        {
+          url: 'https://example.com/node.zip',
+          integrity,
+          basename: 'node-v20.0.0',
+        },
+        targetDir
+      )
+
+      // Verify files were extracted correctly
+      expect(fs.existsSync(path.join(targetDir, 'bin/node'))).toBe(true)
+      expect(fs.existsSync(path.join(targetDir, 'README.md'))).toBe(true)
+    })
+
+    it('should handle empty basename correctly', async () => {
+      const targetDir = temporaryDirectory()
+      const zip = new AdmZip()
+      zip.addFile('bin/node', Buffer.from('#!/bin/sh\necho "node"'))
+      const zipBuffer = zip.toBuffer()
+
+      const integrity = ssri.fromData(zipBuffer).toString()
+
+      const mockFetch = createMockFetch(zipBuffer)
+
+      await downloadAndUnpackZip(
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        mockFetch as any,
+        {
+          url: 'https://example.com/node.zip',
+          integrity,
+          basename: '',
+        },
+        targetDir
+      )
+
+      expect(fs.existsSync(path.join(targetDir, 'bin/node'))).toBe(true)
+    })
+  })
+})

@@ -0,0 +1,18 @@
+{
+  "extends": "../tsconfig.json",
+  "compilerOptions": {
+    "noEmit": false,
+    "outDir": "../node_modules/.test.lib",
+    "rootDir": "..",
+    "isolatedModules": true
+  },
+  "include": [
+    "**/*.ts",
+    "../../../__typings__/**/*.d.ts"
+  ],
+  "references": [
+    {
+      "path": ".."
+    }
+  ]
+}

@@ -3074,6 +3074,9 @@ importers:
       adm-zip:
         specifier: 'catalog:'
         version: 0.5.16
+      is-subdir:
+        specifier: 'catalog:'
+        version: 1.2.0
       rename-overwrite:
         specifier: 'catalog:'
         version: 6.0.3
@@ -3084,6 +3087,9 @@ importers:
         specifier: 'catalog:'
         version: 1.0.1
     devDependencies:
+      '@jest/globals':
+        specifier: 'catalog:'
+        version: 30.0.5
       '@pnpm/fetching.binary-fetcher':
         specifier: workspace:*
         version: 'link:'