Prevent use of --upload-pack as a command in git.clone to avoid potential accidental command execution.

steveukx · steveukx · commit 2040de601c89 · 2022-03-29T07:21:01.000+01:00
diff --git a/.changeset/wicked-files-vanish.md b/.changeset/wicked-files-vanish.md
@@ -0,0 +1,5 @@
+---
+"simple-git": minor
+---
+
+Resolves potential command injection vulnerability by preventing use of `--upload-pack` in `git.clone`
diff --git a/simple-git/src/lib/tasks/clone.ts b/simple-git/src/lib/tasks/clone.ts
@@ -1,10 +1,9 @@
-import { straightThroughStringTask } from './task';
+import { configurationErrorTask, EmptyTask, straightThroughStringTask } from './task';
 import { OptionFlags, Options, StringTask } from '../types';
-import { append } from '../utils';
+import { append, filterString } from '../utils';
 
 export type CloneOptions = Options &
-   OptionFlags<
-      '--bare' |
+   OptionFlags<'--bare' |
       '--dissociate' |
       '--mirror' |
       '--no-checkout' |
@@ -15,25 +14,30 @@ export type CloneOptions = Options &
       '--remote-submodules' |
       '--single-branch' |
       '--shallow-submodules' |
-      '--verbose'
-      > &
+      '--verbose'> &
    OptionFlags<'--depth' | '-j' | '--jobs', number> &
    OptionFlags<'--branch' | '--origin' | '--recurse-submodules' | '--separate-git-dir' | '--shallow-exclude' | '--shallow-since' | '--template', string>
 
-export function cloneTask(repo: string | undefined, directory: string | undefined, customArgs: string[]): StringTask<string> {
+function disallowedCommand(command: string) {
+   return /^--upload-pack(=|$)/.test(command);
+}
+
+export function cloneTask(repo: string | undefined, directory: string | undefined, customArgs: string[]): StringTask<string> | EmptyTask {
    const commands = ['clone', ...customArgs];
-   if (typeof repo === 'string') {
-      commands.push(repo);
-   }
-   if (typeof directory === 'string') {
-      commands.push(directory);
+
+   filterString(repo) && commands.push(repo);
+   filterString(directory) && commands.push(directory);
+
+   const banned = commands.find(disallowedCommand);
+   if (banned) {
+      return configurationErrorTask(`git.fetch: potential exploit argument blocked.`);
    }
 
    return straightThroughStringTask(commands);
 }
 
-export function cloneMirrorTask(repo: string | undefined, directory: string | undefined, customArgs: string[]): StringTask<string> {
-   append(customArgs,'--mirror');
+export function cloneMirrorTask(repo: string | undefined, directory: string | undefined, customArgs: string[]) {
+   append(customArgs, '--mirror');
 
    return cloneTask(repo, directory, customArgs);
 }
diff --git a/simple-git/test/unit/clone.spec.ts b/simple-git/test/unit/clone.spec.ts
@@ -1,5 +1,6 @@
+import { promiseError } from '@kwsites/promise-result';
 import { SimpleGit, TaskOptions } from 'typings';
-import { assertExecutedCommands, closeWithSuccess, newSimpleGit } from './__fixtures__';
+import { assertExecutedCommands, assertGitError, closeWithSuccess, newSimpleGit } from './__fixtures__';
 
 describe('clone', () => {
    let git: SimpleGit;
@@ -15,7 +16,7 @@ describe('clone', () => {
 
    beforeEach(() => git = newSimpleGit());
 
-   it.each(cloneTests)('callbacks - %s %s', async (api, name, cloneArgs, executedCommands)=> {
+   it.each(cloneTests)('callbacks - %s %s', async (api, name, cloneArgs, executedCommands) => {
       const callback = jest.fn();
       const queue = (git[api] as any)(...cloneArgs, callback);
       await closeWithSuccess(name);
@@ -32,5 +33,30 @@ describe('clone', () => {
       expect(await queue).toBe(name);
       assertExecutedCommands(...executedCommands);
    });
+
+   describe('failures', () => {
+
+      it('disallows upload-pack as remote/branch', async () => {
+         const error = await promiseError(git.clone('origin', '--upload-pack=touch ./foo'));
+
+         assertGitError(error, 'potential exploit argument blocked');
+      });
+
+      it('disallows upload-pack as varargs', async () => {
+         const error = await promiseError(git.clone('origin', 'main', {
+            '--upload-pack': 'touch ./foo'
+         }));
+
+         assertGitError(error, 'potential exploit argument blocked');
+      });
+
+      it('disallows upload-pack as varargs', async () => {
+         const error = await promiseError(git.clone('origin', 'main', [
+            '--upload-pack', 'touch ./foo'
+         ]));
+
+         assertGitError(error, 'potential exploit argument blocked');
+      });
+   });
 });
 

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +"simple-git": minor
 +---
++
 +Resolves potential command injection vulnerability by preventing use of `--upload-pack` in `git.clone`