Fix/block unsafe 2603

.changeset/shaky-readers-share.md

@@ -0,0 +1,7 @@
+---
+"simple-git": patch
+---
+
+Enhanced `git -c` checks in `unsafe` plugin.
+
+Thanks to @JohannesLks for identifying the issue

simple-git/src/lib/plugins/block-unsafe-operations-plugin.ts

@@ -16,22 +16,40 @@ function isCloneSwitch(char: string, arg: string | unknown) {
    return /^[\dlsqvnobucj]+\b/.test(token);
 }
 
-function preventProtocolOverride(arg: string, next: string) {
-   if (!isConfigSwitch(arg)) {
-      return;
-   }
-
-   if (!/^\s*protocol(.[a-z]+)?.allow/i.test(next)) {
-      return;
-   }
+function preventConfigBuilder(
+   config: string | RegExp,
+   setting: keyof SimpleGitPluginConfig['unsafe'],
+   message = String(config)
+) {
+   const regex = typeof config === 'string' ? new RegExp(`\\s*${config}`, 'i') : config;
 
-   throw new GitPluginError(
-      undefined,
-      'unsafe',
-      'Configuring protocol.allow is not permitted without enabling allowUnsafeExtProtocol'
-   );
+   return function preventCommand(
+      options: Partial<SimpleGitPluginConfig['unsafe']>,
+      arg: string,
+      next: string
+   ) {
+      if (options[setting] !== true && isConfigSwitch(arg) && regex.test(next)) {
+         throw new GitPluginError(
+            undefined,
+            'unsafe',
+            `Configuring ${message} is not permitted without enabling ${setting}`
+         );
+      }
+   };
 }
 
+const preventUnsafeConfig = [
+   preventConfigBuilder(
+      /^\s*protocol(.[a-z]+)?.allow/i,
+      'allowUnsafeProtocolOverride',
+      'protocol.allow'
+   ),
+   preventConfigBuilder('core.sshCommand', 'allowUnsafeSshCommand'),
+   preventConfigBuilder('core.gitProxy', 'allowUnsafeGitProxy'),
+   preventConfigBuilder('core.hooksPath', 'allowUnsafeHooksPath'),
+   preventConfigBuilder('diff.external', 'allowUnsafeDiffExternal'),
+];
+
 function preventUploadPack(arg: string, method: string) {
    if (/^\s*--(upload|receive)-pack/.test(arg)) {
       throw new GitPluginError(
@@ -59,17 +77,17 @@ function preventUploadPack(arg: string, method: string) {
 }
 
 export function blockUnsafeOperationsPlugin({
-   allowUnsafeProtocolOverride = false,
    allowUnsafePack = false,
+   ...options
 }: SimpleGitPluginConfig['unsafe'] = {}): SimpleGitPlugin<'spawn.args'> {
    return {
       type: 'spawn.args',
       action(args, context) {
          args.forEach((current, index) => {
             const next = index < args.length ? args[index + 1] : '';
 
-            allowUnsafeProtocolOverride || preventProtocolOverride(current, next);
             allowUnsafePack || preventUploadPack(current, context.method);
+            preventUnsafeConfig.forEach((helper) => helper(options, current, next));
          });
 
          return args;

simple-git/src/lib/types/index.ts

@@ -137,7 +137,7 @@ export interface SimpleGitPluginConfig {
       allowUnsafeCustomBinary?: boolean;
 
       /**
-       * By default `simple-git` prevents the use of inline configuration
+       * By default, `simple-git` prevents the use of inline configuration
        * options to override the protocols available for the `git` child
        * process to prevent accidental security vulnerabilities when
        * unsanitised user data is passed directly into operations such as
@@ -156,6 +156,30 @@ export interface SimpleGitPluginConfig {
        * Enable this override to permit the use of these arguments.
        */
       allowUnsafePack?: boolean;
+
+      /**
+       * Using a `-c` switch to enable custom SSH commands opens up a potential
+       * attack vector for running arbitrary commands.
+       */
+      allowUnsafeSshCommand?: boolean;
+
+      /**
+       * Using a `-c` switch to enable custom proxy command for the `git://` transport
+       * exposes and attack vector for running arbitrary commands.
+       */
+      allowUnsafeGitProxy?: boolean;
+
+      /**
+       * Using a `-c` switch to enable custom hooks path commands to be run automatically
+       * exposes and attack vector for running arbitrary commands.
+       */
+      allowUnsafeHooksPath?: boolean;
+
+      /**
+       * Using a `-c` switch to enable setting binary for processing diffs
+       * exposes and attack vector for running arbitrary commands.
+       */
+      allowUnsafeDiffExternal?: boolean;
    };
 }
 

simple-git/test/unit/plugins/plugin.unsafe.spec.ts

@@ -1,10 +1,5 @@
 import { promiseError } from '@kwsites/promise-result';
-import {
-   assertExecutedCommands,
-   assertGitError,
-   closeWithSuccess,
-   newSimpleGit,
-} from '../__fixtures__';
+import { assertExecutedCommands, assertGitError, closeWithSuccess, newSimpleGit } from '../__fixtures__';
 
 describe('blockUnsafeOperationsPlugin', () => {
    it.each([
@@ -13,12 +8,12 @@ describe('blockUnsafeOperationsPlugin', () => {
       ['Protocol.Allow=always'],
       ['PROTOCOL.allow=always'],
       ['protocol.ALLOW=always'],
-   ])('blocks protocol overide in format %s', async (cmd) => {
+   ])('blocks protocol override in format %s', async (cmd) => {
       const task = ['config', '-c', cmd, 'config', '--list'];
 
       assertGitError(
          await promiseError(newSimpleGit().raw(...task)),
-         'allowUnsafeExtProtocol'
+         'allowUnsafeProtocolOverride',
       );
 
       const err = promiseError(
@@ -38,18 +33,44 @@ describe('blockUnsafeOperationsPlugin', () => {
    ])('allows %s %s only when using override', async (cmd, option) => {
       assertGitError(
          await promiseError(newSimpleGit({ unsafe: {} }).raw(cmd, option)),
-         'allowUnsafePack'
+         'allowUnsafePack',
       );
 
       const err = promiseError(
-         newSimpleGit({ unsafe: { allowUnsafePack: true } }).raw(cmd, option)
+         newSimpleGit({ unsafe: { allowUnsafePack: true } }).raw(cmd, option),
       );
 
       await closeWithSuccess();
       expect(await err).toBeUndefined();
       assertExecutedCommands(cmd, option);
    });
 
+   describe.each([
+      ['allowUnsafeSshCommand', `core.sshCommand=sh -c 'id > pwned'`],
+      ['allowUnsafeGitProxy', `core.gitProxy=sh -c 'id > pwned'`],
+      ['allowUnsafeHooksPath', `core.hooksPath=sh -c 'id > pwned'`],
+      ['allowUnsafeDiffExternal', `diff.external=sh -c 'id > pwned'`],
+   ])('unsafe config option - %s', (setting, command) => {
+
+      it('blocks by default', async () => {
+         const err = promiseError(
+            newSimpleGit().clone('remote', 'local', ['-c', command]),
+         );
+         await promiseError(closeWithSuccess());
+
+         assertGitError(await err, setting);
+      });
+
+      it('allows with override', async () => {
+         const err = promiseError(
+            newSimpleGit({ unsafe: { [setting]: true } }).clone('remote', 'local', ['-c', command]),
+         );
+         await closeWithSuccess();
+
+         expect(await err).toBeUndefined();
+      });
+   });
+
    it('allows -u for non-clone commands', async () => {
       const git = newSimpleGit({ unsafe: {} });
       const err = promiseError(git.raw('push', '-u', 'origin/main'));