Fix/exec path vulnerability

.changeset/lucky-guards-block.md

@@ -0,0 +1,16 @@
+---
+"@simple-git/argv-parser": patch
+---
+
+Vulnerability detection expanded to cover configuration delivered through path-taking global options, where
+the dangerous value is a file on disk rather than a token `simple-git` can inspect:
+
+- `--exec-path` names the directory `git` loads built-in commands and remote helpers from, and is blocked
+  under the new `allowUnsafeExec` category along with the `GIT_EXEC_PATH` environment variable (previously
+  grouped under `allowUnsafeConfigPaths`)
+- `--git-dir`, `--work-tree` and `-C` cause `git` to read the configuration of the repository they name, and
+  are blocked under `allowUnsafeConfigPaths`
+
+These options are only detected when supplied before the git sub-command and with a value - used as getters
+(`git.raw('rev-parse', '--git-dir')`) or as task options (`git.raw('commit', '-C', 'HEAD~1')`) they are
+unaffected.

.nvmrc

@@ -1 +1 @@
-v24.10.0
+v26.4.0

docs/PLUGIN-UNSAFE-ACTIONS.md

@@ -426,12 +426,17 @@ await simpleGit({ unsafe: { allowUnsafeMergeDriver: true } })
    .raw('-c', 'mergetool.vimdiff.path=/usr/bin/vim', 'mergetool');
 ```
 
-### Configuration paths via environment variables
+### Configuration paths
 
-The `GIT_CONFIG_GLOBAL`, `GIT_CONFIG_SYSTEM`, `GIT_CONFIG`, `GIT_EXEC_PATH`, and `PREFIX` environment
-variables override the paths `git` uses to locate its configuration files and built-in commands. Controlling
-these paths allows an attacker to supply an entirely malicious git configuration or replace git's built-in
-commands with arbitrary binaries.
+The `GIT_CONFIG_GLOBAL`, `GIT_CONFIG_SYSTEM`, `GIT_CONFIG` and `PREFIX` environment variables override the
+paths `git` uses to locate its configuration files, as do the `--git-dir`, `--work-tree` and `-C` global
+options. Controlling any of these paths allows an attacker to supply an entirely malicious git configuration
+- including settings that enable further command-execution vectors - by naming a directory rather than by
+supplying a value `simple-git` can inspect. The configuration file itself need not be executable.
+
+Used without a value, `--git-dir` and `--work-tree` report the paths `git` resolved rather than setting them,
+so `git.checkIsRepo(CheckRepoActions.IS_REPO_ROOT)` and the equivalent `git.raw('rev-parse', '--git-dir')`
+are unaffected.
 
 ```typescript
 import { simpleGit } from 'simple-git';
@@ -441,12 +446,38 @@ await simpleGit()
    .env('GIT_CONFIG_GLOBAL', '/attacker/controlled/gitconfig')
    .clone('https://example.com/repo');
 
+// throws - git reads the configuration of whichever repository the path names
+await simpleGit()
+   .raw('--git-dir=/attacker/controlled/repo', 'ls-remote', 'https://example.com/repo.git');
+
+// throws - `git` discovers, and reads the configuration of, a repository in that directory
+await simpleGit().raw('-C', '/attacker/controlled/checkout', 'ls-remote', 'https://example.com/repo.git');
+
 // opt in to overriding git configuration paths
 await simpleGit({ unsafe: { allowUnsafeConfigPaths: true } })
    .env('GIT_CONFIG_GLOBAL', '/custom/global/gitconfig')
    .clone('https://example.com/repo');
 ```
 
+### Git executable path
+
+The `--exec-path` option and `GIT_EXEC_PATH` environment variable name the directory `git` loads its
+built-in commands and remote helpers from. An attacker-named directory containing, for example, a
+`git-remote-https` executable has that file run in place of git's own helper - reached by any operation
+using an `https://` url, including `git.clone`.
+
+```typescript
+import { simpleGit } from 'simple-git';
+
+// throws
+await simpleGit()
+   .raw('--exec-path=/attacker/controlled/bin', 'ls-remote', 'https://example.com/repo.git');
+
+// opt in to overriding the git executable path
+await simpleGit({ unsafe: { allowUnsafeExec: true } })
+   .raw('--exec-path=/opt/custom/libexec/git-core', 'ls-remote', 'https://example.com/repo.git');
+```
+
 ### Environment-based configuration
 
 Git supports injecting configuration values at runtime through a set of numbered environment variables:

packages/argv-parser/src/env/parse-env.ts

@@ -11,7 +11,7 @@ const GitEnvKeys = {
    'git_config_parameters': 'allowUnsafeConfigEnvCount',
    'git_config': 'allowUnsafeConfigPaths',
    'git_editor': 'allowUnsafeEditor',
-   'git_exec_path': 'allowUnsafeConfigPaths',
+   'git_exec_path': 'allowUnsafeExec',
    'git_external_diff': 'allowUnsafeDiffExternal',
    'git_pager': 'allowUnsafePager',
    'git_proxy_command': 'allowUnsafeGitProxy',

packages/argv-parser/src/vulnerabilities/detect-vulnerable-flags.ts

@@ -7,25 +7,51 @@ export function* detectVulnerableFlags(
 ): Generator<Vulnerability> {
    for (const flag of flags) {
       for (const helper of preventUnsafeFlags) {
-         const vulnerability = helper(task, flag.name);
+         const vulnerability = helper(task, flag);
          if (vulnerability) {
             yield vulnerability;
          }
       }
    }
 }
 
+interface PreventFlagOptions {
+   /** Label to use in the error message in place of the matcher itself */
+   name?: string;
+
+   /** Only match when the switch appears before the git sub-command */
+   globalOnly?: boolean;
+
+   /**
+    * Only match when the switch was supplied with a value - without one switches
+    * such as `--git-dir` and `--exec-path` are getters rather than setters.
+    */
+   withValue?: boolean;
+}
+
 function preventFlagBuilder(
    task: string | null,
    flag: string | RegExp,
    category: VulnerabilityCategory,
-   name = String(flag)
+   { name = String(flag), globalOnly = false, withValue = false }: PreventFlagOptions = {}
 ) {
    const regex = typeof flag === 'string' ? new RegExp(`\\s*${flag.toLowerCase()}`) : flag;
    const message = `Use of ${task ? `${task} with option ` : ''}${name} is not permitted without enabling ${category}`;
 
-   return function preventFlag(currentTask: string | null, flagName: string): Vulnerability | void {
-      if ((!task || currentTask === task) && regex.test(flagName)) {
+   return function preventFlag(currentTask: string | null, flag: Flag): Vulnerability | void {
+      if (task && currentTask !== task) {
+         return;
+      }
+
+      if (globalOnly && !flag.isGlobal) {
+         return;
+      }
+
+      if (withValue && flag.value === undefined) {
+         return;
+      }
+
+      if (regex.test(flag.name)) {
          return {
             category,
             message,
@@ -34,15 +60,20 @@ function preventFlagBuilder(
    };
 }
 
+const pathTakingGlobal: PreventFlagOptions = { globalOnly: true, withValue: true };
+
 const preventUnsafeFlags = [
-   preventFlagBuilder(
-      null,
-      /--(upload|receive)-pack/,
-      'allowUnsafePack',
-      '--upload-pack or --receive-pack'
-   ),
+   preventFlagBuilder(null, /--(upload|receive)-pack/, 'allowUnsafePack', {
+      name: '--upload-pack or --receive-pack',
+   }),
    preventFlagBuilder('clone', /^-\w*u/, 'allowUnsafePack'),
    preventFlagBuilder('clone', '--u', 'allowUnsafePack'),
-   preventFlagBuilder('push', '--exec', 'allowUnsafePack'),
+   preventFlagBuilder('push', /^--exec$/, 'allowUnsafePack', { name: '--exec' }),
    preventFlagBuilder(null, '--template', 'allowUnsafeTemplateDir'),
+   preventFlagBuilder(null, '--exec-path', 'allowUnsafeExec', pathTakingGlobal),
+   // `git` reads the configuration of whichever repository these name, so the
+   // directory alone is enough to deliver config the argv guards never see
+   preventFlagBuilder(null, '--git-dir', 'allowUnsafeConfigPaths', pathTakingGlobal),
+   preventFlagBuilder(null, '--work-tree', 'allowUnsafeConfigPaths', pathTakingGlobal),
+   preventFlagBuilder(null, /^-C$/, 'allowUnsafeConfigPaths', { ...pathTakingGlobal, name: '-C' }),
 ];

packages/argv-parser/src/vulnerabilities/vulnerability.types.ts

@@ -87,6 +87,12 @@ export interface VulnerabilityCategoryFlags {
     */
    allowUnsafeGitProxy: boolean;
 
+   /**
+    * Use of the `--exec-path` and similar options can override the binaries executed during
+    * `git` operations.
+    */
+   allowUnsafeExec: boolean;
+
    /**
     * Using a `-c` switch to enable custom hooks path commands to be run automatically
     * exposes and attack vector for running arbitrary commands.

packages/argv-parser/test/parse-env.spec.ts

@@ -32,7 +32,7 @@ describe('parseEnv', () => {
       ['GIT_CONFIG', '/tmp/malicious', 'allowUnsafeConfigPaths'],
       ['GIT_EDITOR', '/tmp/malicious', 'allowUnsafeEditor'],
       ['GIT_SEQUENCE_EDITOR', '/tmp/malicious', 'allowUnsafeEditor'],
-      ['GIT_EXEC_PATH', '/tmp/malicious', 'allowUnsafeConfigPaths'],
+      ['GIT_EXEC_PATH', '/tmp/malicious', 'allowUnsafeExec'],
       ['GIT_EXTERNAL_DIFF', '/tmp/malicious', 'allowUnsafeDiffExternal'],
       ['GIT_PAGER', '/tmp/malicious', 'allowUnsafePager'],
       ['GIT_PROXY_COMMAND', '/tmp/malicious', 'allowUnsafeGitProxy'],

packages/argv-parser/test/vulnerability-analysis.spec.ts

@@ -195,4 +195,44 @@ describe('VulnerabilityAnalysis', () => {
          );
       });
    });
+   describe('path-taking global flags', () => {
+      it.each<[string, VulnerabilityCategory]>([
+         ['--exec-path', 'allowUnsafeExec'],
+         ['--git-dir', 'allowUnsafeConfigPaths'],
+         ['--work-tree', 'allowUnsafeConfigPaths'],
+      ])('detects %s supplied with a value', (flag, category) => {
+         const expected = oneVulnerability(category);
+
+         expect(parseArgv(`${flag}=/tmp/evil`, 'ls-remote', 'url').vulnerabilities).toEqual(
+            expected
+         );
+         expect(parseArgv(flag, '/tmp/evil', 'ls-remote', 'url').vulnerabilities).toEqual(expected);
+      });
+
+      it('detects -C supplied with a value', () => {
+         expect(parseArgv('-C', '/tmp/evil', 'ls-remote', 'url').vulnerabilities).toEqual(
+            oneVulnerability('allowUnsafeConfigPaths')
+         );
+      });
+
+      it.each([['--exec-path'], ['--git-dir'], ['--work-tree']])(
+         'allows %s as a getter, where no value is consumed',
+         (flag) => {
+            expect(parseArgv('rev-parse', flag).vulnerabilities).toEqual(noVulnerabilities());
+         }
+      );
+
+      it.each([
+         ['apply', '-C', '3'],
+         ['commit', '-C', 'HEAD~1'],
+      ])('allows %s %s as a task option', (task, flag, value) => {
+         expect(parseArgv(task, flag, value).vulnerabilities).toEqual(noVulnerabilities());
+      });
+
+      it('does not confuse -C with the inline config -c', () => {
+         expect(parseArgv('-c', 'user.name=bob', 'status').vulnerabilities).toEqual(
+            noVulnerabilities()
+         );
+      });
+   });
 });

simple-git/test/integration/vulnerabilities/exec-path.spec.ts

@@ -0,0 +1,36 @@
+import { chmod } from 'node:fs/promises';
+import { describe, expect, it } from 'vitest';
+import { exists, FILE } from '@kwsites/file-exists';
+import { promiseError } from '@kwsites/promise-result';
+import { assertGitError, createTestContext, newSimpleGit } from '@simple-git/test-utils';
+
+describe('--exec-path', () => {
+   it('blocks: global flag without being allowed', async () => {
+      const context = await createTestContext();
+      const root = await context.dir('poc-workdir');
+      const pwnd = context.path('new-exec-path-pwned');
+
+      await newSimpleGit(root).init();
+
+      // `git` loads the remote helper for a url's scheme out of its exec-path, so an
+      // attacker-named directory containing `git-remote-https` is arbitrary execution
+      const execPath = await context.dir('evil-exec-path');
+      const helper = await context.file(
+         ['evil-exec-path', 'git-remote-https'],
+         `#!/bin/sh\ntouch ${pwnd}\nexit 1\n`
+      );
+      await chmod(helper, 0o755);
+
+      const err = await promiseError(
+         newSimpleGit(root).raw(
+            `--exec-path=${execPath}`,
+            'ls-remote',
+            'https://example.com/repo.git'
+         )
+      );
+
+      expect(exists(pwnd, FILE)).toBe(false);
+
+      assertGitError(err, 'allowUnsafeExec');
+   });
+});

simple-git/test/integration/vulnerabilities/git-dir.spec.ts

@@ -0,0 +1,110 @@
+import { chmod, writeFile } from 'node:fs/promises';
+import { describe, expect, it } from 'vitest';
+import { exists, FILE } from '@kwsites/file-exists';
+import { promiseError } from '@kwsites/promise-result';
+import {
+   assertGitError,
+   createTestContext,
+   newSimpleGit,
+   type SimpleGitTestContext,
+} from '@simple-git/test-utils';
+
+/**
+ * Plants a repo skeleton at `dir` whose config runs `payload` whenever git opens an
+ * ssh connection, rewriting `https://` urls to ssh so that a url supplied by the
+ * application itself is enough to reach it. Note the config is plain text with no
+ * execute bit - only the directory name needs to be attacker controlled.
+ */
+async function plantRepo(context: SimpleGitTestContext, dir: string[], payload: string) {
+   const gitDir = await context.dir(...dir);
+
+   await writeFile(
+      context.path(...dir, 'config'),
+      `[core]\n\tsshCommand = ${payload}\n[url "ssh://x.invalid/"]\n\tinsteadOf = https://\n`
+   );
+   await writeFile(context.path(...dir, 'HEAD'), 'ref: refs/heads/main\n');
+   await context.dir(...dir, 'objects');
+   await context.dir(...dir, 'refs');
+
+   return gitDir;
+}
+
+async function plantPayload(context: SimpleGitTestContext, pwnd: string) {
+   const payload = await context.file('payload.sh', `#!/bin/sh\ntouch ${pwnd}\nexit 1\n`);
+   await chmod(payload, 0o755);
+
+   return payload;
+}
+
+describe('config through a path-taking global flag', () => {
+   it('allows: --git-dir when used as a getter', async () => {
+      const context = await createTestContext();
+      const root = await context.dir('poc-workdir');
+      const pwnd = context.path('new-git-dir-pwned');
+
+      await newSimpleGit(root).init();
+      await plantRepo(context, ['evil-git-dir'], await plantPayload(context, pwnd));
+
+      const err = await promiseError(newSimpleGit(root).raw('rev-parse', `--git-dir`));
+
+      expect(exists(pwnd, FILE)).toBe(false);
+
+      expect(err).toBe(undefined);
+   });
+
+   it('blocks: --git-dir at an attacker-named directory', async () => {
+      const context = await createTestContext();
+      const root = await context.dir('poc-workdir');
+      const pwnd = context.path('new-git-dir-pwned');
+
+      await newSimpleGit(root).init();
+      const gitDir = await plantRepo(context, ['evil-git-dir'], await plantPayload(context, pwnd));
+
+      const err = await promiseError(
+         newSimpleGit(root).raw(`--git-dir=${gitDir}`, 'ls-remote', 'https://example.com/repo.git')
+      );
+
+      expect(exists(pwnd, FILE)).toBe(false);
+
+      assertGitError(err, 'allowUnsafeConfigPaths');
+   });
+
+   it('blocks: --git-dir supplied as a separate token', async () => {
+      const context = await createTestContext();
+      const root = await context.dir('poc-workdir');
+      const pwnd = context.path('new-git-dir-pwned');
+
+      await newSimpleGit(root).init();
+      const gitDir = await plantRepo(context, ['evil-git-dir'], await plantPayload(context, pwnd));
+
+      const err = await promiseError(
+         newSimpleGit(root).raw('--git-dir', gitDir, 'ls-remote', 'https://example.com/repo.git')
+      );
+
+      expect(exists(pwnd, FILE)).toBe(false);
+
+      assertGitError(err, 'allowUnsafeConfigPaths');
+   });
+
+   it('blocks: -C into a directory holding a planted repo', async () => {
+      const context = await createTestContext();
+      const root = await context.dir('poc-workdir');
+      const pwnd = context.path('new-change-dir-pwned');
+
+      await newSimpleGit(root).init();
+      await plantRepo(context, ['evil-work-dir', '.git'], await plantPayload(context, pwnd));
+
+      const err = await promiseError(
+         newSimpleGit(root).raw(
+            '-C',
+            context.path('evil-work-dir'),
+            'ls-remote',
+            'https://example.com/repo.git'
+         )
+      );
+
+      expect(exists(pwnd, FILE)).toBe(false);
+
+      assertGitError(err, 'allowUnsafeConfigPaths');
+   });
+});