diff --git a/.changeset/fix-binary-fetcher-path-traversal.md b/.changeset/fix-binary-fetcher-path-traversal.md new file mode 100644 index 0000000000..e9d0d17311 --- /dev/null +++ b/.changeset/fix-binary-fetcher-path-traversal.md @@ -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 diff --git a/fetching/binary-fetcher/README.md b/fetching/binary-fetcher/README.md index ece0b75948..d9123c78f1 100644 --- a/fetching/binary-fetcher/README.md +++ b/fetching/binary-fetcher/README.md @@ -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 diff --git a/fetching/binary-fetcher/package.json b/fetching/binary-fetcher/package.json index 26d64fe345..91806717b4 100644 --- a/fetching/binary-fetcher/package.json +++ b/fetching/binary-fetcher/package.json @@ -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": "tsgo --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": ">=20.19" diff --git a/fetching/binary-fetcher/scripts/create-fixtures.ts b/fetching/binary-fetcher/scripts/create-fixtures.ts new file mode 100644 index 0000000000..0dab052405 --- /dev/null +++ b/fetching/binary-fetcher/scripts/create-fixtures.ts @@ -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.') diff --git a/fetching/binary-fetcher/src/index.ts b/fetching/binary-fetcher/src/index.ts index 6bf858c3f3..4c0d5b87d6 100644 --- a/fetching/binary-fetcher/src/index.ts +++ b/fetching/binary-fetcher/src/index.ts @@ -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 { temporaryDirectory } 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 { 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`) + } +} diff --git a/fetching/binary-fetcher/test/fixtures/absolute-path.zip b/fetching/binary-fetcher/test/fixtures/absolute-path.zip new file mode 100644 index 0000000000..8f1554e34b Binary files /dev/null and b/fetching/binary-fetcher/test/fixtures/absolute-path.zip differ diff --git a/fetching/binary-fetcher/test/fixtures/backslash-traversal.zip b/fetching/binary-fetcher/test/fixtures/backslash-traversal.zip new file mode 100644 index 0000000000..88bce5df7c Binary files /dev/null and b/fetching/binary-fetcher/test/fixtures/backslash-traversal.zip differ diff --git a/fetching/binary-fetcher/test/fixtures/path-traversal.zip b/fetching/binary-fetcher/test/fixtures/path-traversal.zip new file mode 100644 index 0000000000..85184f3228 Binary files /dev/null and b/fetching/binary-fetcher/test/fixtures/path-traversal.zip differ diff --git a/fetching/binary-fetcher/test/index.ts b/fetching/binary-fetcher/test/index.ts new file mode 100644 index 0000000000..a4c208835e --- /dev/null +++ b/fetching/binary-fetcher/test/index.ts @@ -0,0 +1,223 @@ +/// +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) + }) + }) +}) diff --git a/fetching/binary-fetcher/test/tsconfig.json b/fetching/binary-fetcher/test/tsconfig.json new file mode 100644 index 0000000000..67ce5e1d0e --- /dev/null +++ b/fetching/binary-fetcher/test/tsconfig.json @@ -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": ".." + } + ] +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 97ac0fac67..0ab9bae842 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -3114,6 +3114,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.2 @@ -3124,6 +3127,9 @@ importers: specifier: 'catalog:' version: 3.0.0 devDependencies: + '@jest/globals': + specifier: 'catalog:' + version: 30.0.5 '@pnpm/fetching.binary-fetcher': specifier: workspace:* version: 'link:'