Skip to content

Commit

Permalink
Prevent use of --upload-pack as a command in git.clone to avoid p…
Browse files Browse the repository at this point in the history
…otential accidental command execution.
  • Loading branch information
steveukx committed Mar 29, 2022
1 parent 9bf9baa commit 2040de6
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 16 deletions.
5 changes: 5 additions & 0 deletions .changeset/wicked-files-vanish.md
Original file line number Diff 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`
32 changes: 18 additions & 14 deletions simple-git/src/lib/tasks/clone.ts
Original file line number Diff line number Diff line change
@@ -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' |
Expand All @@ -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);
}
30 changes: 28 additions & 2 deletions simple-git/test/unit/clone.spec.ts
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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);
Expand All @@ -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');
});
});
});

0 comments on commit 2040de6

Please sign in to comment.