diff --git a/.changeset/git-fetcher-reject-non-sha-commits.md b/.changeset/git-fetcher-reject-non-sha-commits.md new file mode 100644 index 0000000000..258ded7ed8 --- /dev/null +++ b/.changeset/git-fetcher-reject-non-sha-commits.md @@ -0,0 +1,6 @@ +--- +"@pnpm/fetching.git-fetcher": patch +"pnpm": patch +--- + +Reject git resolutions whose `commit` field is not a 40-character hexadecimal SHA before invoking `git`. A malicious lockfile could otherwise smuggle a value such as `--upload-pack=` through `git fetch` / `git checkout`, which on SSH or local-file transports executes the supplied command. diff --git a/fetching/git-fetcher/src/index.ts b/fetching/git-fetcher/src/index.ts index 2a402e07bf..1d35259886 100644 --- a/fetching/git-fetcher/src/index.ts +++ b/fetching/git-fetcher/src/index.ts @@ -26,6 +26,9 @@ export function createGitFetcher (createOpts: CreateGitFetcherOptions): { git: G const ignoreScripts = createOpts.ignoreScripts ?? false const gitFetcher: GitFetcher = async (cafs, resolution, opts) => { + if (!isValidCommitHash(resolution.commit)) { + throw new PnpmError('INVALID_GIT_COMMIT', `Invalid git commit hash "${resolution.commit}" for repository "${resolution.repo}". Expected a 40-character hexadecimal SHA.`) + } const tempLocation = await cafs.tempDir() if (allowedHosts.size > 0 && shouldUseShallow(resolution.repo, allowedHosts)) { await execGit(['init'], { cwd: tempLocation }) @@ -78,6 +81,10 @@ export function createGitFetcher (createOpts: CreateGitFetcherOptions): { git: G } } +function isValidCommitHash (commit: string): boolean { + return /^[0-9a-f]{40}$/i.test(commit) +} + function shouldUseShallow (repoUrl: string, allowedHosts: Set): boolean { try { const { host } = new URL(repoUrl) diff --git a/fetching/git-fetcher/test/index.ts b/fetching/git-fetcher/test/index.ts index 1abd567538..fdc11afe28 100644 --- a/fetching/git-fetcher/test/index.ts +++ b/fetching/git-fetcher/test/index.ts @@ -227,7 +227,7 @@ test('fail when preparing a git-hosted package', async () => { ).rejects.toThrow('Failed to prepare git-hosted package fetched from "https://github.com/pnpm-e2e/prepare-script-fails.git": @pnpm.e2e/prepare-script-fails@1.0.0 npm-install: `npm install`') }) -test('fail when preparing a git-hosted package with a partial commit', async () => { +test('reject a partial commit before invoking git', async () => { const storeDir = temporaryDirectory() const fetch = createGitFetcher({ storeIndex: createStoreIndex(storeDir), @@ -241,7 +241,24 @@ test('fail when preparing a git-hosted package with a partial commit', async () }, { filesIndexFile: path.join(storeDir, 'index.json'), }) - ).rejects.toThrow(/received commit [0-9a-f]{40} does not match expected value deadbeef/) + ).rejects.toThrow('Invalid git commit hash "deadbeef"') + expect(jest.mocked(execa)).not.toHaveBeenCalled() +}) + +test('reject a commit value that looks like a git option', async () => { + const storeDir = temporaryDirectory() + const fetch = createGitFetcher({ storeIndex: createStoreIndex(storeDir) }).git + await expect( + fetch(createCafsStore(storeDir), + { + commit: '--upload-pack=touch /tmp/pwned', + repo: 'file:///tmp/repo.git', + type: 'git', + }, { + filesIndexFile: path.join(storeDir, 'index.json'), + }) + ).rejects.toThrow('Invalid git commit hash "--upload-pack=touch /tmp/pwned"') + expect(jest.mocked(execa)).not.toHaveBeenCalled() }) test('do not build the package when scripts are ignored', async () => { diff --git a/pacquet/crates/git-fetcher/src/error.rs b/pacquet/crates/git-fetcher/src/error.rs index 5a617a975d..2545d66160 100644 --- a/pacquet/crates/git-fetcher/src/error.rs +++ b/pacquet/crates/git-fetcher/src/error.rs @@ -89,6 +89,17 @@ pub enum GitFetcherError { #[diagnostic(code(GIT_CHECKOUT_FAILED))] CheckoutMismatch { expected: String, received: String }, + /// `resolution.commit` is not a 40-character hexadecimal SHA. A + /// commit value beginning with `-` would otherwise be parsed by + /// `git fetch` / `git checkout` as an option (e.g. `--upload-pack`), + /// allowing a malicious lockfile to execute arbitrary commands on + /// SSH or local-file transports. + #[display( + "Invalid git commit hash {commit:?} for repository {repo:?}. Expected a 40-character hexadecimal SHA." + )] + #[diagnostic(code(INVALID_GIT_COMMIT))] + InvalidCommit { commit: String, repo: String }, + #[display("I/O error during git fetch: {_0}")] #[diagnostic(code(pacquet_git_fetcher::io))] Io(#[error(source)] std::io::Error), diff --git a/pacquet/crates/git-fetcher/src/fetcher.rs b/pacquet/crates/git-fetcher/src/fetcher.rs index 1552df0c89..b34f3fddf6 100644 --- a/pacquet/crates/git-fetcher/src/fetcher.rs +++ b/pacquet/crates/git-fetcher/src/fetcher.rs @@ -106,6 +106,12 @@ impl<'a> GitFetcher<'a> { } fn run_sync(self) -> Result { + if !is_valid_commit_hash(self.commit) { + return Err(GitFetcherError::InvalidCommit { + commit: self.commit.to_string(), + repo: self.repo.to_string(), + }); + } let temp = tempfile::tempdir().map_err(GitFetcherError::Io)?; let temp_location = temp.path(); @@ -226,6 +232,13 @@ fn wrap_prepare_error(_repo: &str, err: PreparePackageError) -> GitFetcherError GitFetcherError::Prepare(err) } +/// True iff `commit` is exactly a 40-character hexadecimal git SHA. +/// Rejects everything else (short SHAs, ref names, option-shaped +/// strings like `--upload-pack=…`) before the value reaches `git`. +fn is_valid_commit_hash(commit: &str) -> bool { + commit.len() == 40 && commit.bytes().all(|b| b.is_ascii_hexdigit()) +} + /// True iff `repo` parses to a host that pacquet should clone via the /// shallow `init` + `fetch --depth 1` path. Mirrors upstream's /// [`shouldUseShallow`](https://github.com/pnpm/pnpm/blob/94240bc046/fetching/git-fetcher/src/index.ts#L81-L91). diff --git a/pacquet/crates/git-fetcher/src/fetcher/tests.rs b/pacquet/crates/git-fetcher/src/fetcher/tests.rs index 819337926b..e9f20b50a1 100644 --- a/pacquet/crates/git-fetcher/src/fetcher/tests.rs +++ b/pacquet/crates/git-fetcher/src/fetcher/tests.rs @@ -1,4 +1,4 @@ -use super::{GitFetcher, exec_git, extract_host, should_use_shallow}; +use super::{GitFetcher, exec_git, extract_host, is_valid_commit_hash, should_use_shallow}; use crate::error::GitFetcherError; use pacquet_executor::ScriptsPrependNodePath; use pacquet_reporter::SilentReporter; @@ -84,6 +84,55 @@ fn should_use_shallow_matches_known_host() { assert!(!should_use_shallow("https://example.com/x/y.git", &hosts)); } +#[test] +fn is_valid_commit_hash_accepts_full_sha() { + assert!(is_valid_commit_hash("c9b30e71d704cd30fa71f2edd1ecc7dcc4985493")); + assert!(is_valid_commit_hash("C9B30E71D704CD30FA71F2EDD1ECC7DCC4985493")); +} + +#[test] +fn is_valid_commit_hash_rejects_short_or_option_shaped_values() { + assert!(!is_valid_commit_hash("deadbeef")); + assert!(!is_valid_commit_hash("")); + assert!(!is_valid_commit_hash("--upload-pack=touch /tmp/pwned")); + assert!(!is_valid_commit_hash("c9b30e71d704cd30fa71f2edd1ecc7dcc4985493 ")); + // 40 chars but contains a non-hex digit. + assert!(!is_valid_commit_hash("c9b30e71d704cd30fa71f2edd1ecc7dcc498549z")); +} + +#[tokio::test(flavor = "multi_thread")] +async fn fetcher_rejects_option_shaped_commit() { + let store_root = tempdir().unwrap(); + let store_dir = StoreDir::from(store_root.path().to_path_buf()); + let err = GitFetcher { + repo: "file:///tmp/githost", + commit: "--upload-pack=touch /tmp/pwned", + path: None, + git_shallow_hosts: &[], + allow_build: deny_all_builds(), + ignore_scripts: false, + unsafe_perm: true, + user_agent: None, + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + script_shell: None, + node_execpath: None, + npm_execpath: None, + store_dir: &store_dir, + package_id: "pkg@1.0.0", + requester: "/test", + store_index_writer: None, + files_index_file: "pkg@1.0.0\tbuilt", + git_bin: None, + } + .run::() + .await + .unwrap_err(); + assert!( + matches!(err, GitFetcherError::InvalidCommit { .. }), + "expected InvalidCommit, got {err:?}", + ); +} + #[test] fn extract_host_handles_user_authority_and_port() { assert_eq!(extract_host("https://github.com/foo/bar"), Some("github.com"));