Skip to content

Commit 0a623e5

Browse files
steveukxsteveukx
authored
Feat/unsafe pack (#881)
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.
1 parent b45d08b commit 0a623e5

File tree

6 files changed

+143
-30
lines changed

6 files changed

+143
-30
lines changed

‎.changeset/pink-and-gold.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'simple-git': minor
3+
---
4+
5+
Adds vulnerability detection to prevent use of `--upload-pack` and `--receive-pack` without explicitly opting in.

‎packages/test-utils/src/expectations.ts

+6-1
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,12 @@ export function assertGitError(
1717
errorConstructor: any = GitError
1818
) {
1919
expect(errorInstance).toBeInstanceOf(errorConstructor);
20-
expect(errorInstance).toHaveProperty('message', expect.stringMatching(message));
20+
expect(errorInstance).toHaveProperty(
21+
'message',
22+
typeof message === 'string'
23+
? expect.stringContaining(message)
24+
: expect.stringMatching(message)
25+
);
2126
}
2227

2328
export function assertGitResponseError(errorInstance: Error | unknown, git: any, equality?: any) {

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

+21-1
Original file line numberDiff line numberDiff line change
@@ -23,16 +23,36 @@ function preventProtocolOverride(arg: string, next: string) {
2323
);
2424
}
2525

26+
function preventUploadPack(arg: string, method: string) {
27+
if (/^\s*--(upload|receive)-pack/.test(arg)) {
28+
throw new GitPluginError(
29+
undefined,
30+
'unsafe',
31+
`Use of --upload-pack or --receive-pack is not permitted without enabling allowUnsafePack`
32+
);
33+
}
34+
35+
if (method === 'clone' && /^\s*-u\b/.test(arg)) {
36+
throw new GitPluginError(
37+
undefined,
38+
'unsafe',
39+
`Use of clone with option -u is not permitted without enabling allowUnsafePack`
40+
);
41+
}
42+
}
43+
2644
export function blockUnsafeOperationsPlugin({
2745
allowUnsafeProtocolOverride = false,
46+
allowUnsafePack = false,
2847
}: SimpleGitPluginConfig['unsafe'] = {}): SimpleGitPlugin<'spawn.args'> {
2948
return {
3049
type: 'spawn.args',
31-
action(args, _context) {
50+
action(args, context) {
3251
args.forEach((current, index) => {
3352
const next = index < args.length ? args[index + 1] : '';
3453

3554
allowUnsafeProtocolOverride || preventProtocolOverride(current, next);
55+
allowUnsafePack || preventUploadPack(current, context.method);
3656
});
3757

3858
return args;

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

+9-2
Original file line numberDiff line numberDiff line change
@@ -119,10 +119,17 @@ export interface SimpleGitPluginConfig {
119119
*
120120
* Enable this override to use the `ext::` protocol (see examples on
121121
* [git-scm.com](https://git-scm.com/docs/git-remote-ext#_examples)).
122-
*
123-
* See documentation for use in
124122
*/
125123
allowUnsafeProtocolOverride?: boolean;
124+
125+
/**
126+
* Given the possibility of using `--upload-pack` and `--receive-pack` as
127+
* attack vectors, the use of these in any command (or the shorthand
128+
* `-u` option in a `clone` operation) are blocked by default.
129+
*
130+
* Enable this override to permit the use of these arguments.
131+
*/
132+
allowUnsafePack?: boolean;
126133
};
127134
}
128135

‎simple-git/test/unit/clean.spec.ts

+14-26
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { SimpleGit } from 'typings';
22
import {
33
assertExecutedCommands,
4+
assertGitError,
45
assertNoExecutedTasks,
56
closeWithSuccess,
67
newSimpleGit,
@@ -95,7 +96,7 @@ describe('clean', () => {
9596
it('handles configuration errors', async () => {
9697
const err = await git.clean('X').catch((e) => e);
9798

98-
expectTheError(err).toBe(CONFIG_ERROR_MODE_REQUIRED);
99+
assertGitError(err, CONFIG_ERROR_MODE_REQUIRED, TaskConfigurationError);
99100
});
100101
});
101102

@@ -118,8 +119,8 @@ describe('clean', () => {
118119
'missing required n or f in mode',
119120
test((done) => {
120121
git.clean('x', function (err: null | Error) {
121-
expectTheError(err).toBe(CONFIG_ERROR_MODE_REQUIRED);
122-
expectNoTasksToHaveBeenRun();
122+
assertGitError(err, CONFIG_ERROR_MODE_REQUIRED, TaskConfigurationError);
123+
assertNoExecutedTasks();
123124
done();
124125
});
125126
})
@@ -129,8 +130,8 @@ describe('clean', () => {
129130
'unknown options',
130131
test((done) => {
131132
git.clean('fa', function (err: null | Error) {
132-
expectTheError(err).toBe(CONFIG_ERROR_UNKNOWN_OPTION);
133-
expectNoTasksToHaveBeenRun();
133+
assertGitError(err, CONFIG_ERROR_UNKNOWN_OPTION, TaskConfigurationError);
134+
assertNoExecutedTasks();
134135
done();
135136
});
136137
})
@@ -140,8 +141,8 @@ describe('clean', () => {
140141
'no args',
141142
test((done) => {
142143
git.clean(function (err: null | Error) {
143-
expectTheError(err).toBe(CONFIG_ERROR_MODE_REQUIRED);
144-
expectNoTasksToHaveBeenRun();
144+
assertGitError(err, CONFIG_ERROR_MODE_REQUIRED, TaskConfigurationError);
145+
assertNoExecutedTasks();
145146
done();
146147
});
147148
})
@@ -199,8 +200,8 @@ describe('clean', () => {
199200
'prevents interactive mode - shorthand option',
200201
test((done) => {
201202
git.clean('f', ['-i'], function (err: null | Error) {
202-
expectTheError(err).toBe(CONFIG_ERROR_INTERACTIVE_MODE);
203-
expectNoTasksToHaveBeenRun();
203+
assertGitError(err, CONFIG_ERROR_INTERACTIVE_MODE, TaskConfigurationError);
204+
assertNoExecutedTasks();
204205

205206
done();
206207
});
@@ -211,8 +212,8 @@ describe('clean', () => {
211212
'prevents interactive mode - shorthand mode',
212213
test((done) => {
213214
git.clean('fi', function (err: null | Error) {
214-
expectTheError(err).toBe(CONFIG_ERROR_INTERACTIVE_MODE);
215-
expectNoTasksToHaveBeenRun();
215+
assertGitError(err, CONFIG_ERROR_INTERACTIVE_MODE, TaskConfigurationError);
216+
assertNoExecutedTasks();
216217

217218
done();
218219
});
@@ -223,28 +224,15 @@ describe('clean', () => {
223224
'prevents interactive mode - longhand option',
224225
test((done) => {
225226
git.clean('f', ['--interactive'], function (err: null | Error) {
226-
expectTheError(err).toBe(CONFIG_ERROR_INTERACTIVE_MODE);
227-
expectNoTasksToHaveBeenRun();
227+
assertGitError(err, CONFIG_ERROR_INTERACTIVE_MODE, TaskConfigurationError);
228+
assertNoExecutedTasks();
228229

229230
done();
230231
});
231232
})
232233
);
233234
});
234235

235-
function expectNoTasksToHaveBeenRun() {
236-
assertNoExecutedTasks();
237-
}
238-
239-
function expectTheError<E extends Error>(err: E | null) {
240-
return {
241-
toBe(message: string) {
242-
expect(err).toBeInstanceOf(TaskConfigurationError);
243-
expect(String(err)).toMatch(message);
244-
},
245-
};
246-
}
247-
248236
function test(t: (done: Function) => void) {
249237
return async () => {
250238
await new Promise((done) => t(done));

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

+88
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
import { promiseError } from '@kwsites/promise-result';
2+
import {
3+
assertExecutedCommands,
4+
assertGitError,
5+
closeWithSuccess,
6+
newSimpleGit,
7+
} from './__fixtures__';
8+
9+
describe('blockUnsafeOperationsPlugin', () => {
10+
it.each([
11+
['cmd', '--upload-pack=touch /tmp/pwn0'],
12+
['cmd', '--receive-pack=touch /tmp/pwn1'],
13+
['clone', '-u touch /tmp/pwn'],
14+
])('allows %s %s only when using override', async (cmd, option) => {
15+
assertGitError(
16+
await promiseError(newSimpleGit({ unsafe: {} }).raw(cmd, option)),
17+
'allowUnsafePack'
18+
);
19+
20+
const err = promiseError(
21+
newSimpleGit({ unsafe: { allowUnsafePack: true } }).raw(cmd, option)
22+
);
23+
24+
await closeWithSuccess();
25+
expect(await err).toBeUndefined();
26+
assertExecutedCommands(cmd, option);
27+
});
28+
29+
it('allows -u for non-clone commands', async () => {
30+
const git = newSimpleGit({ unsafe: {} });
31+
const err = promiseError(git.raw('push', '-u', 'origin/main'));
32+
33+
await closeWithSuccess();
34+
expect(await err).toBeUndefined();
35+
assertExecutedCommands('push', '-u', 'origin/main');
36+
});
37+
38+
it('blocks -u for clone command', async () => {
39+
const git = newSimpleGit({ unsafe: {} });
40+
const err = promiseError(git.clone('-u touch /tmp/pwn', 'file:///tmp/zero12'));
41+
42+
assertGitError(await err, 'allowUnsafePack');
43+
});
44+
45+
it('allows -u for clone command with override', async () => {
46+
const git = newSimpleGit({ unsafe: { allowUnsafePack: true } });
47+
const err = promiseError(git.clone('-u touch /tmp/pwn', 'file:///tmp/zero12'));
48+
49+
await closeWithSuccess();
50+
expect(await err).toBeUndefined();
51+
assertExecutedCommands('clone', '-u touch /tmp/pwn', 'file:///tmp/zero12');
52+
});
53+
54+
it('blocks pull --upload-pack', async () => {
55+
const git = newSimpleGit({ unsafe: {} });
56+
const err = await promiseError(git.pull('--upload-pack=touch /tmp/pwn0', 'master'));
57+
58+
assertGitError(err, 'allowUnsafePack');
59+
});
60+
61+
it('blocks push --receive-pack', async () => {
62+
const git = newSimpleGit({ unsafe: {} });
63+
const err = await promiseError(git.push('--receive-pack=touch /tmp/pwn1', 'master'));
64+
65+
assertGitError(err, 'allowUnsafePack');
66+
});
67+
68+
it('blocks raw --upload-pack', async () => {
69+
const git = newSimpleGit({ unsafe: {} });
70+
const err = await promiseError(git.raw('pull', `--upload-pack=touch /tmp/pwn0`));
71+
72+
assertGitError(err, 'allowUnsafePack');
73+
});
74+
75+
it('blocks raw --receive-pack', async () => {
76+
const git = newSimpleGit({ unsafe: {} });
77+
const err = await promiseError(git.raw('push', `--receive-pack=touch /tmp/pwn1`));
78+
79+
assertGitError(err, 'allowUnsafePack');
80+
});
81+
82+
it('blocks listRemote --upload-pack', async () => {
83+
const git = newSimpleGit({ unsafe: {} });
84+
const err = await promiseError(git.listRemote(['--upload-pack=touch /tmp/pwn2', 'main']));
85+
86+
assertGitError(err, 'allowUnsafePack');
87+
});
88+
});

0 commit comments

Comments
 (0)