diff --git a/.changeset/strange-bin-segments.md b/.changeset/strange-bin-segments.md new file mode 100644 index 0000000000..35951c1863 --- /dev/null +++ b/.changeset/strange-bin-segments.md @@ -0,0 +1,6 @@ +--- +"@pnpm/bins.resolver": patch +"pnpm": patch +--- + +Reject reserved manifest `bin` names (`""`, `"."`, `".."`, and scoped forms such as `@scope/..`) when resolving a package's bins. These names previously passed the bin-name guard and, when joined to the global bin directory during global remove/update/add operations, could resolve to the global bin directory itself or its parent and have it recursively deleted. diff --git a/bins/resolver/src/index.ts b/bins/resolver/src/index.ts index dc97de1f47..601832fe43 100644 --- a/bins/resolver/src/index.ts +++ b/bins/resolver/src/index.ts @@ -66,7 +66,13 @@ function commandsFromBin (bin: PackageBin, pkgName: string, pkgPath: string): Co const binName = commandName[0] === '@' ? commandName.slice(commandName.indexOf('/') + 1) : commandName - // Validate: must be safe (no path traversal) - only allow URL-safe chars or $ + // Validate: must be safe (no path traversal). Reject empty and the + // filesystem-relative names "." and ".." (these survive encodeURIComponent + // unchanged but resolve to the bin directory itself or its parent when + // joined to a target dir), then only allow URL-safe chars or $. + if (binName === '' || binName === '.' || binName === '..') { + continue + } if (binName !== encodeURIComponent(binName) && binName !== '$') { continue } diff --git a/bins/resolver/test/index.ts b/bins/resolver/test/index.ts index ac39a60a9a..b473113fe9 100644 --- a/bins/resolver/test/index.ts +++ b/bins/resolver/test/index.ts @@ -147,6 +147,26 @@ test('skip scoped bin names with path traversal', async () => { ]) }) +test('skip reserved bin names', async () => { + expect( + await getBinsFromPackageManifest({ + name: 'malicious', + version: '1.0.0', + bin: { + '': './empty.js', + '.': './dot.js', + '..': './dot-dot.js', + '@scope/..': './scoped-dot-dot.js', + good: './good', + }, + }, process.cwd())).toStrictEqual([ + { + name: 'good', + path: path.resolve('good'), + }, + ]) +}) + test('skip directories.bin with path traversal', async () => { // Security test: malicious packages can try to escape the package root // using directories.bin to chmod files at arbitrary locations diff --git a/global/commands/test/globalRemove.test.ts b/global/commands/test/globalRemove.test.ts new file mode 100644 index 0000000000..0323e9d608 --- /dev/null +++ b/global/commands/test/globalRemove.test.ts @@ -0,0 +1,47 @@ +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' + +import { expect, jest, test } from '@jest/globals' + +const removeBin = jest.fn<(cmd: string) => Promise>().mockResolvedValue(undefined) + +jest.unstable_mockModule('@pnpm/bins.remover', () => ({ removeBin })) + +const { handleGlobalRemove } = await import('../src/globalRemove.js') + +// A malicious global package whose manifest declares reserved bin keys must not +// reach the deletion sink: `path.join(globalBinDir, '.')` is the bin directory +// itself and `path.join(globalBinDir, '..')` is its parent, so removing either +// would wipe out unrelated files. Only the safe `good` shim may be deleted. +test('global remove ignores reserved manifest bin names', async () => { + const globalDir = fs.mkdtempSync(path.join(os.tmpdir(), 'global-remove-')) + const globalBinDir = path.join(globalDir, 'bin') + const installDir = path.join(globalDir, 'install') + const depDir = path.join(installDir, 'node_modules', 'evil') + fs.mkdirSync(depDir, { recursive: true }) + fs.writeFileSync( + path.join(installDir, 'package.json'), + JSON.stringify({ name: 'global', version: '1.0.0', dependencies: { evil: '1.0.0' } }) + ) + fs.writeFileSync( + path.join(depDir, 'package.json'), + JSON.stringify({ + name: 'evil', + version: '1.0.0', + bin: { + '': './empty.js', + '.': './dot.js', + '..': './dot-dot.js', + '@scope/..': './scoped-dot-dot.js', + good: './good.js', + }, + }) + ) + fs.symlinkSync(installDir, path.join(globalDir, 'hash')) + + await handleGlobalRemove({ globalPkgDir: globalDir, bin: globalBinDir }, ['evil']) + + expect(removeBin).toHaveBeenCalledTimes(1) + expect(removeBin).toHaveBeenCalledWith(path.join(globalBinDir, 'good')) +}) diff --git a/pacquet/crates/cmd-shim/src/bin_resolver.rs b/pacquet/crates/cmd-shim/src/bin_resolver.rs index 8dc928269d..d10505749e 100644 --- a/pacquet/crates/cmd-shim/src/bin_resolver.rs +++ b/pacquet/crates/cmd-shim/src/bin_resolver.rs @@ -171,11 +171,15 @@ fn commands_from_bin(bin: &Value, pkg_name: Option<&str>, pkg_path: &Path) -> Ve /// /// `encodeURIComponent` leaves the following bytes unescaped: /// `A-Z a-z 0-9 - _ . ! ~ * ' ( )`. +/// +/// `.` and `..` survive `encodeURIComponent` unchanged but resolve to the bin +/// directory itself or its parent when joined to a target dir, so they are +/// rejected explicitly. fn is_safe_bin_name(name: &str) -> bool { if name == "$" { return true; } - if name.is_empty() { + if name.is_empty() || name == "." || name == ".." { return false; } name.bytes().all(|byte| { diff --git a/pacquet/crates/cmd-shim/src/bin_resolver/tests.rs b/pacquet/crates/cmd-shim/src/bin_resolver/tests.rs index 13a72ea734..471f908d83 100644 --- a/pacquet/crates/cmd-shim/src/bin_resolver/tests.rs +++ b/pacquet/crates/cmd-shim/src/bin_resolver/tests.rs @@ -362,6 +362,28 @@ fn empty_bin_key_is_rejected() { assert_eq!(commands[0].name, "good"); } +/// The relative names `.` and `..` survive the URL-safe guard's character +/// set (`.` is unescaped by `encodeURIComponent`) but resolve to the bin +/// directory itself or its parent when joined to a target dir, so +/// `is_safe_bin_name` must reject them. Scoped forms like `@scope/..` +/// collapse to `..` after the scope strip and must be rejected too. +#[test] +fn reserved_relative_bin_names_are_rejected() { + let manifest = json!({ + "name": "malicious", + "version": "1.0.0", + "bin": { + ".": "dot.js", + "..": "dotdot.js", + "@scope/..": "scoped-dotdot.js", + "good": "good.js", + }, + }); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); + assert_eq!(commands.len(), 1); + assert_eq!(commands[0].name, "good"); +} + /// [`lexical_normalize`] drops `.` (CurDir) segments. This is a direct /// test on the helper. The integration-style test below covers the same /// arm via `directories.bin`, but a direct assertion makes the CurDir