Skip to content

Commit

Permalink
Feat/unsafe pack (#881)
Browse files Browse the repository at this point in the history
Unsafe operations plugin extended to prevent the use of potential attack vectors `--upload-pack` or `--receive-pack` (or the shorthand version `-u` in `git clone`) without explicitly opting in with the `allowUnsafePack` option.
  • Loading branch information
steveukx authored Dec 22, 2022
1 parent b45d08b commit 0a623e5
Show file tree
Hide file tree
Showing 6 changed files with 143 additions and 30 deletions.
5 changes: 5 additions & 0 deletions .changeset/pink-and-gold.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'simple-git': minor
---

Adds vulnerability detection to prevent use of `--upload-pack` and `--receive-pack` without explicitly opting in.
7 changes: 6 additions & 1 deletion packages/test-utils/src/expectations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,12 @@ export function assertGitError(
errorConstructor: any = GitError
) {
expect(errorInstance).toBeInstanceOf(errorConstructor);
expect(errorInstance).toHaveProperty('message', expect.stringMatching(message));
expect(errorInstance).toHaveProperty(
'message',
typeof message === 'string'
? expect.stringContaining(message)
: expect.stringMatching(message)
);
}

export function assertGitResponseError(errorInstance: Error | unknown, git: any, equality?: any) {
Expand Down
22 changes: 21 additions & 1 deletion simple-git/src/lib/plugins/block-unsafe-operations-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,36 @@ function preventProtocolOverride(arg: string, next: string) {
);
}

function preventUploadPack(arg: string, method: string) {
if (/^\s*--(upload|receive)-pack/.test(arg)) {
throw new GitPluginError(
undefined,
'unsafe',
`Use of --upload-pack or --receive-pack is not permitted without enabling allowUnsafePack`
);
}

if (method === 'clone' && /^\s*-u\b/.test(arg)) {
throw new GitPluginError(
undefined,
'unsafe',
`Use of clone with option -u is not permitted without enabling allowUnsafePack`
);
}
}

export function blockUnsafeOperationsPlugin({
allowUnsafeProtocolOverride = false,
allowUnsafePack = false,
}: SimpleGitPluginConfig['unsafe'] = {}): SimpleGitPlugin<'spawn.args'> {
return {
type: 'spawn.args',
action(args, _context) {
action(args, context) {
args.forEach((current, index) => {
const next = index < args.length ? args[index + 1] : '';

allowUnsafeProtocolOverride || preventProtocolOverride(current, next);
allowUnsafePack || preventUploadPack(current, context.method);
});

return args;
Expand Down
11 changes: 9 additions & 2 deletions simple-git/src/lib/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,10 +119,17 @@ export interface SimpleGitPluginConfig {
*
* Enable this override to use the `ext::` protocol (see examples on
* [git-scm.com](https://git-scm.com/docs/git-remote-ext#_examples)).
*
* See documentation for use in
*/
allowUnsafeProtocolOverride?: boolean;

/**
* Given the possibility of using `--upload-pack` and `--receive-pack` as
* attack vectors, the use of these in any command (or the shorthand
* `-u` option in a `clone` operation) are blocked by default.
*
* Enable this override to permit the use of these arguments.
*/
allowUnsafePack?: boolean;
};
}

Expand Down
40 changes: 14 additions & 26 deletions simple-git/test/unit/clean.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { SimpleGit } from 'typings';
import {
assertExecutedCommands,
assertGitError,
assertNoExecutedTasks,
closeWithSuccess,
newSimpleGit,
Expand Down Expand Up @@ -95,7 +96,7 @@ describe('clean', () => {
it('handles configuration errors', async () => {
const err = await git.clean('X').catch((e) => e);

expectTheError(err).toBe(CONFIG_ERROR_MODE_REQUIRED);
assertGitError(err, CONFIG_ERROR_MODE_REQUIRED, TaskConfigurationError);
});
});

Expand All @@ -118,8 +119,8 @@ describe('clean', () => {
'missing required n or f in mode',
test((done) => {
git.clean('x', function (err: null | Error) {
expectTheError(err).toBe(CONFIG_ERROR_MODE_REQUIRED);
expectNoTasksToHaveBeenRun();
assertGitError(err, CONFIG_ERROR_MODE_REQUIRED, TaskConfigurationError);
assertNoExecutedTasks();
done();
});
})
Expand All @@ -129,8 +130,8 @@ describe('clean', () => {
'unknown options',
test((done) => {
git.clean('fa', function (err: null | Error) {
expectTheError(err).toBe(CONFIG_ERROR_UNKNOWN_OPTION);
expectNoTasksToHaveBeenRun();
assertGitError(err, CONFIG_ERROR_UNKNOWN_OPTION, TaskConfigurationError);
assertNoExecutedTasks();
done();
});
})
Expand All @@ -140,8 +141,8 @@ describe('clean', () => {
'no args',
test((done) => {
git.clean(function (err: null | Error) {
expectTheError(err).toBe(CONFIG_ERROR_MODE_REQUIRED);
expectNoTasksToHaveBeenRun();
assertGitError(err, CONFIG_ERROR_MODE_REQUIRED, TaskConfigurationError);
assertNoExecutedTasks();
done();
});
})
Expand Down Expand Up @@ -199,8 +200,8 @@ describe('clean', () => {
'prevents interactive mode - shorthand option',
test((done) => {
git.clean('f', ['-i'], function (err: null | Error) {
expectTheError(err).toBe(CONFIG_ERROR_INTERACTIVE_MODE);
expectNoTasksToHaveBeenRun();
assertGitError(err, CONFIG_ERROR_INTERACTIVE_MODE, TaskConfigurationError);
assertNoExecutedTasks();

done();
});
Expand All @@ -211,8 +212,8 @@ describe('clean', () => {
'prevents interactive mode - shorthand mode',
test((done) => {
git.clean('fi', function (err: null | Error) {
expectTheError(err).toBe(CONFIG_ERROR_INTERACTIVE_MODE);
expectNoTasksToHaveBeenRun();
assertGitError(err, CONFIG_ERROR_INTERACTIVE_MODE, TaskConfigurationError);
assertNoExecutedTasks();

done();
});
Expand All @@ -223,28 +224,15 @@ describe('clean', () => {
'prevents interactive mode - longhand option',
test((done) => {
git.clean('f', ['--interactive'], function (err: null | Error) {
expectTheError(err).toBe(CONFIG_ERROR_INTERACTIVE_MODE);
expectNoTasksToHaveBeenRun();
assertGitError(err, CONFIG_ERROR_INTERACTIVE_MODE, TaskConfigurationError);
assertNoExecutedTasks();

done();
});
})
);
});

function expectNoTasksToHaveBeenRun() {
assertNoExecutedTasks();
}

function expectTheError<E extends Error>(err: E | null) {
return {
toBe(message: string) {
expect(err).toBeInstanceOf(TaskConfigurationError);
expect(String(err)).toMatch(message);
},
};
}

function test(t: (done: Function) => void) {
return async () => {
await new Promise((done) => t(done));
Expand Down
88 changes: 88 additions & 0 deletions simple-git/test/unit/plugin.unsafe.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
import { promiseError } from '@kwsites/promise-result';
import {
assertExecutedCommands,
assertGitError,
closeWithSuccess,
newSimpleGit,
} from './__fixtures__';

describe('blockUnsafeOperationsPlugin', () => {
it.each([
['cmd', '--upload-pack=touch /tmp/pwn0'],
['cmd', '--receive-pack=touch /tmp/pwn1'],
['clone', '-u touch /tmp/pwn'],
])('allows %s %s only when using override', async (cmd, option) => {
assertGitError(
await promiseError(newSimpleGit({ unsafe: {} }).raw(cmd, option)),
'allowUnsafePack'
);

const err = promiseError(
newSimpleGit({ unsafe: { allowUnsafePack: true } }).raw(cmd, option)
);

await closeWithSuccess();
expect(await err).toBeUndefined();
assertExecutedCommands(cmd, option);
});

it('allows -u for non-clone commands', async () => {
const git = newSimpleGit({ unsafe: {} });
const err = promiseError(git.raw('push', '-u', 'origin/main'));

await closeWithSuccess();
expect(await err).toBeUndefined();
assertExecutedCommands('push', '-u', 'origin/main');
});

it('blocks -u for clone command', async () => {
const git = newSimpleGit({ unsafe: {} });
const err = promiseError(git.clone('-u touch /tmp/pwn', 'file:///tmp/zero12'));

assertGitError(await err, 'allowUnsafePack');
});

it('allows -u for clone command with override', async () => {
const git = newSimpleGit({ unsafe: { allowUnsafePack: true } });
const err = promiseError(git.clone('-u touch /tmp/pwn', 'file:///tmp/zero12'));

await closeWithSuccess();
expect(await err).toBeUndefined();
assertExecutedCommands('clone', '-u touch /tmp/pwn', 'file:///tmp/zero12');
});

it('blocks pull --upload-pack', async () => {
const git = newSimpleGit({ unsafe: {} });
const err = await promiseError(git.pull('--upload-pack=touch /tmp/pwn0', 'master'));

assertGitError(err, 'allowUnsafePack');
});

it('blocks push --receive-pack', async () => {
const git = newSimpleGit({ unsafe: {} });
const err = await promiseError(git.push('--receive-pack=touch /tmp/pwn1', 'master'));

assertGitError(err, 'allowUnsafePack');
});

it('blocks raw --upload-pack', async () => {
const git = newSimpleGit({ unsafe: {} });
const err = await promiseError(git.raw('pull', `--upload-pack=touch /tmp/pwn0`));

assertGitError(err, 'allowUnsafePack');
});

it('blocks raw --receive-pack', async () => {
const git = newSimpleGit({ unsafe: {} });
const err = await promiseError(git.raw('push', `--receive-pack=touch /tmp/pwn1`));

assertGitError(err, 'allowUnsafePack');
});

it('blocks listRemote --upload-pack', async () => {
const git = newSimpleGit({ unsafe: {} });
const err = await promiseError(git.listRemote(['--upload-pack=touch /tmp/pwn2', 'main']));

assertGitError(err, 'allowUnsafePack');
});
});

0 comments on commit 0a623e5

Please sign in to comment.