Skip to content

Commit d762810

Browse files
authored
Fix/vulnerability rebase exec (#1198)
* Add catch for `git rebase --exec` and `-x` variants * Changeset * Unsafe docs * Lint * Add `allowUnsafeCommandBinaries` detection to configuring `trailer.<token>.cmd` and `trailer.<token>.command`.
1 parent 76f308e commit d762810

8 files changed

Lines changed: 94 additions & 0 deletions

File tree

‎.changeset/safe-rebase-exec.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
---
2+
"@simple-git/argv-parser": patch
3+
"simple-git": patch
4+
---
5+
6+
Add `allowUnsafeExec` detection to `rebase -x` and `rebase --exec`.
7+
8+
Thanks to @gdegrange for the vulnerability report.

‎.changeset/safe-trailer-config.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
"@simple-git/argv-parser": patch
3+
---
4+
5+
Add `allowUnsafeCommandBinaries` detection to configuring `trailer.<token>.cmd` and `trailer.<token>.command`.
6+
7+
Thanks to @sec-reex for the vulnerability report.

‎docs/PLUGIN-UNSAFE-ACTIONS.md‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,25 @@ await simpleGit({ unsafe: { allowAbbreviatedOptions: true } })
3232
.raw('fetch', '--conf=user.name=me');
3333
```
3434

35+
### Command Binaries
36+
37+
Options that allow supplying the path to executable binaries and writing to configuration options that set the path
38+
to executable binaries are caught as potentially unsafe RCE vectors, enabled through the
39+
`allowUnsafeCommandBinaries` option.
40+
41+
```typescript
42+
import { simpleGit } from 'simple-git';
43+
44+
// throws the GitPluginError for allowUnsafeCommandBinaries
45+
await simpleGit().raw('rebase', '--exec', '/custom/path');
46+
await simpleGit().raw('config', 'trailer.foo.command', '/custom/path');
47+
48+
// opt in to using custom paths to executable binaries
49+
await simpleGit({ unsafe: { allowUnsafeCommandBinaries: true } })
50+
.raw('config', 'trailer.foo.command', '/custom/path')
51+
.raw('rebase', '--exec', '/custom/path');
52+
```
53+
3554
### Custom upload and receive packs
3655

3756
Instead of using the default `git-receive-pack` and `git-upload-pack` binaries to parse incoming and outgoing
@@ -478,6 +497,8 @@ await simpleGit({ unsafe: { allowUnsafeExec: true } })
478497
.raw('--exec-path=/opt/custom/libexec/git-core', 'ls-remote', 'https://example.com/repo.git');
479498
```
480499

500+
The `allowUnsafeExec` category also applies to `--exec` or `-x` options when rebasing.
501+
481502
### Environment-based configuration
482503

483504
Git supports injecting configuration values at runtime through a set of numbered environment variables:

‎packages/argv-parser/src/tokens/flag-specs.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,22 @@ const COMMANDS: Record<string, FlagSpec> = {
8888
short: new Map(),
8989
long: new Set(['exec', 'receive-pack']),
9090
},
91+
rebase: {
92+
short: new Map([
93+
['X', true], // -X <option> strategy option
94+
['f', false], // -f force-rebase
95+
['i', false], // -i interactive
96+
['k', false], // -k keep-base
97+
['m', false], // -m merge
98+
['n', false], // -n no-stat
99+
['q', false], // -q quiet
100+
['r', false], // -r rebase-merges
101+
['s', true], // -s <strategy>
102+
['v', false], // -v verbose
103+
['x', true], // -x <cmd> exec
104+
]),
105+
long: new Set(['exec', 'onto', 'strategy', 'strategy-option']),
106+
},
91107
};
92108

93109
const EMPTY: FlagSpec = { short: new Map(), long: new Set() };

‎packages/argv-parser/src/vulnerabilities/detect-vulnerable-config-writes.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,5 +68,7 @@ const preventUnsafeConfig = [
6868
preventConfigBuilder('sequence.editor', 'allowUnsafeEditor'),
6969
preventExpandedConfigBuilder('submodule.update', 'allowUnsafeSubmodule'),
7070
preventExpandedConfigBuilder('tar.command', 'allowUnsafeCommandBinaries'),
71+
preventExpandedConfigBuilder('trailer.cmd', 'allowUnsafeCommandBinaries'),
72+
preventExpandedConfigBuilder('trailer.command', 'allowUnsafeCommandBinaries'),
7173
preventExpandedConfigBuilder('url.insteadOf', 'allowUnsafeUrlRewrite'),
7274
];

‎packages/argv-parser/src/vulnerabilities/detect-vulnerable-flags.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,8 @@ const preventUnsafeFlags = [
6969
preventFlagBuilder('clone', /^-\w*u/, 'allowUnsafePack'),
7070
preventFlagBuilder('clone', '--u', 'allowUnsafePack'),
7171
preventFlagBuilder('push', /^--exec$/, 'allowUnsafePack', { name: '--exec' }),
72+
// `git` accepts unambiguous abbreviations of long options, so `--ex` and `--exe` are `--exec`
73+
preventFlagBuilder('rebase', /^(-x|--ex(ec?)?)$/, 'allowUnsafeExec', { name: '-x or --exec' }),
7274
preventFlagBuilder(null, '--template', 'allowUnsafeTemplateDir'),
7375
preventFlagBuilder(null, '--exec-path', 'allowUnsafeExec', pathTakingGlobal),
7476
// `git` reads the configuration of whichever repository these name, so the

‎packages/argv-parser/test/vulnerability-analysis.spec.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,9 @@ describe('VulnerabilityAnalysis', () => {
9595
['submodule.evil.update', '!malicious', 'allowUnsafeSubmodule'],
9696
['tar.command', '!malicious', 'allowUnsafeCommandBinaries'],
9797
['tar.foo.command', '!malicious', 'allowUnsafeCommandBinaries'],
98+
['trailer.foo.command', '!malicious', 'allowUnsafeCommandBinaries'],
99+
['trailer.foo.cmd', '!malicious', 'allowUnsafeCommandBinaries'],
100+
['trailer.cmd', '!malicious', 'allowUnsafeCommandBinaries'],
98101
['url.https://evil.com.insteadOf', 'https://github.com', 'allowUnsafeUrlRewrite'],
99102
['url.https://evil.com.insteadOf', 'git@github.com:', 'allowUnsafeUrlRewrite'],
100103
])('writing %s = %s to the git config', (key, value, category) => {
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
import { promiseError } from '@kwsites/promise-result';
2+
import { describe, expect, it } from 'vitest';
3+
4+
import { assertGitError, closeWithSuccess } from '../__fixtures__';
5+
import { simpleGit } from '../../../src/lib/git-factory';
6+
7+
describe('rebase --exec', () => {
8+
it('catches bare rebase -x', async () => {
9+
const task = promiseError(simpleGit().rebase(['-x', 'cmd']));
10+
await promiseError(closeWithSuccess(''));
11+
12+
assertGitError(await task, 'allowUnsafeExec');
13+
});
14+
15+
it('catches rebase -qx', async () => {
16+
const task = promiseError(simpleGit().rebase(['-qx', 'cmd']));
17+
await promiseError(closeWithSuccess(''));
18+
19+
assertGitError(await task, 'allowUnsafeExec');
20+
});
21+
22+
it('catches rebase --exec', async () => {
23+
const task = promiseError(simpleGit().rebase(['--exec', 'cmd']));
24+
await promiseError(closeWithSuccess(''));
25+
26+
assertGitError(await task, 'allowUnsafeExec');
27+
});
28+
29+
it('allows: rebase -qm', async () => {
30+
const task = promiseError(simpleGit().rebase(['-qm', 'foo']));
31+
await promiseError(closeWithSuccess(''));
32+
33+
expect(await task).toBeUndefined();
34+
});
35+
});

0 commit comments

Comments
 (0)