CVE-2022-24066: Improper Neutralization of Argument Delimiters in a Command ('Argument Injection') (CWE-88)
value: The package simple-git before 3.5.0 are vulnerable to Command Injection due to an incomplete fix of [CVE-2022-24433](https://security.snyk.io/vuln/SNYK-JS-SIMPLEGIT-2421199) which only patches against the git fetch attack vector. A similar use of the --upload-pack feature of git is also supported for git clone, which the prior fix didn't cover.
CWE description: The product constructs a string for a command to be executed by a separate component
in another control sphere, but it does not properly delimit the
intended arguments, options, or switches within that command string.
CVSS v3 base score: 9.8
Published: 2022-04-01T20:15Z
Repository: https://github.com/steveukx/git-js
Language: TypeScript
Files changed: .changeset/wicked-files-vanish.md, simple-git/src/lib/tasks/clone.ts, simple-git/test/unit/clone.spec.ts
Commit date: 2022-03-29 07:21:01 +0100
Commit message: Prevent use of `--upload-pack` as a command in `git.clone` to avoid potential accidental command execution.

Diff: diff --git a/.changeset/wicked-files-vanish.md b/.changeset/wicked-files-vanish.md
new file mode 100644
index 0000000..ab1327a
--- /dev/null
+++ 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
index 7450350..d6b88ee 100644
--- a/simple-git/src/lib/tasks/clone.ts
+++ b/simple-git/src/lib/tasks/clone.ts
@@ -1,39 +1,43 @@
-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' |
       '--no-remote-submodules' |
       '--no-shallow-submodules' |
       '--no-single-branch' |
       '--no-tags' |
       '--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
index 6fd3a55..c52a4ea 100644
--- a/simple-git/test/unit/clone.spec.ts
+++ b/simple-git/test/unit/clone.spec.ts
@@ -1,7 +1,8 @@
+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;
 
    const cloneTests: [keyof SimpleGit, string, Array<string | TaskOptions>, string[]][] = [
@@ -13,11 +14,11 @@ describe('clone', () => {
       ['mirror', 'explicitly set', ['r', 'l'], ['clone', '--mirror', 'r', 'l']],
    ];
 
    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);
 
       expect(await queue).toBe(name);
@@ -30,7 +31,32 @@ describe('clone', () => {
       await closeWithSuccess(name);
 
       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');
+      });
+   });
 });
 

Vulnerable code: import { straightThroughStringTask } from './task';
import { append } from '../utils';
   OptionFlags<
      '--bare' |
      '--verbose'
      > &
export function cloneTask(repo: string | undefined, directory: string | undefined, customArgs: string[]): StringTask<string> {
   if (typeof repo === 'string') {
      commands.push(repo);
   }
   if (typeof directory === 'string') {
      commands.push(directory);
export function cloneMirrorTask(repo: string | undefined, directory: string | undefined, customArgs: string[]): StringTask<string> {
   append(customArgs,'--mirror');
import { assertExecutedCommands, closeWithSuccess, newSimpleGit } from './__fixtures__';
   it.each(cloneTests)('callbacks - %s %s', async (api, name, cloneArgs, executedCommands)=> {
Fixed code: ---
"simple-git": minor
---

Resolves potential command injection vulnerability by preventing use of `--upload-pack` in `git.clone`
import { configurationErrorTask, EmptyTask, straightThroughStringTask } from './task';
import { append, filterString } from '../utils';
   OptionFlags<'--bare' |
      '--verbose'> &
function disallowedCommand(command: string) {
   return /^--upload-pack(=|$)/.test(command);
}

export function cloneTask(repo: string | undefined, directory: string | undefined, customArgs: string[]): StringTask<string> | EmptyTask {

   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.`);
export function cloneMirrorTask(repo: string | undefined, directory: string | undefined, customArgs: string[]) {
   append(customArgs, '--mirror');
import { promiseError } from '@kwsites/promise-result';
import { assertExecutedCommands, assertGitError, closeWithSuccess, newSimpleGit } from './__fixtures__';
   it.each(cloneTests)('callbacks - %s %s', async (api, name, cloneArgs, 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');
      });
   });