Skip to content

Commit

Permalink
Create the unsafe plugin to configure how simple-git treats known…
Browse files Browse the repository at this point in the history
… potentially unsafe operations.

Snyk recognised a potential vulnerability caused by passing unsanitised user data into commands that operate on a git remote (eg `clone`, `fetch`, `pull` and `push` etc) whereby an inline configuration argument could be used to enable the `ext::` protocol to switch out the remote for an arbitrary named binary run on the host machine.

While this highlights the need to sanitise user input in the application that consumes it before passing it through to any library, `simple-git` will now limit the use of these options unless the developer explicitly opts in to them with a new `allowUnsafeProtocolOverride` option.
  • Loading branch information
steveukx committed Nov 12, 2022
1 parent 3324eed commit 6b3c631
Show file tree
Hide file tree
Showing 7 changed files with 159 additions and 1 deletion.
38 changes: 38 additions & 0 deletions docs/PLUGIN-UNSAFE-ACTIONS.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
## Unsafe Actions

As `simple-git` passes generated arguments through to a child process of the calling node.js process, it is recommended
that any parameter sourced from user input is validated before being passed to the `simple-git` api.

In some cases where there is an elevated potential for harm `simple-git` will throw an exception unless you have
explicitly opted in to the potentially unsafe action.

### Overriding allowed protocols

A standard installation of `git` permits `file`, `http` and `ssh` protocols for a remote. A range of
[git remote helpers](https://git-scm.com/docs/gitremote-helpers) other than these default few can be
used by referring to te helper name in the remote protocol - for example the git file descriptor transport
[git-remote-fd](https://git-scm.com/docs/git-remote-fd) would be used in a remote protocol such as:

```
git fetch "fd::<infd>[,<outfd>][/<anything>]"
```

To avoid accidentally triggering a helper transport by passing through unsanitised user input to a function
that expects a remote, the use of `-c protocol.fd.allow=always` (or any variant of protocol permission changes)
will cause `simple-git` to throw unless it has been configured with:

```typescript
import { simpleGit } from 'simple-git';

// throws
await simpleGit()
.raw('clone', 'ext::git-server-alias foo %G/repo', '-c', 'protocol.ext.allow=always');

// allows calling clone with a helper transport
await simpleGit({ unsafe: { allowUnsafeProtocolOverride: true } })
.raw('clone', 'ext::git-server-alias foo %G/repo', '-c', 'protocol.ext.allow=always');
```

> *Be advised* helper transports can be used to call arbitrary binaries on the host machine.
> Do not allow them in applications where you are not in control of the input parameters.
6 changes: 5 additions & 1 deletion simple-git/readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,9 @@ await git.pull();
- [Timeout](https://github.com/steveukx/git-js/blob/main/docs/PLUGIN-TIMEOUT.md)
Automatically kill the wrapped `git` process after a rolling timeout.

- [Unsafe](https://github.com/steveukx/git-js/blob/main/docs/PLUGIN-UNSAFE-ACTIONS.md)
Selectively opt out of `simple-git` safety precautions - for advanced users and use cases.

## Using Task Promises

Each task in the API returns the `simpleGit` instance for chaining together multiple tasks, and each
Expand Down Expand Up @@ -436,7 +439,8 @@ application hasn't been making use of non-documented APIs by importing from a su

See also:

- [release notes v2](https://github.com/steveukx/git-js/blob/main/docs/RELEASE-NOTES-V2.md)
- [release notes v3](https://github.com/steveukx/git-js/blob/main/simple-git/CHANGELOG.md)
- [release notes v2](https://github.com/steveukx/git-js/blob/main/docs/RELEASE-NOTES-V2.md)

# Concurrent / Parallel Requests

Expand Down
2 changes: 2 additions & 0 deletions simple-git/src/lib/git-factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { SimpleGitFactory } from '../../typings';
import * as api from './api';
import {
abortPlugin,
blockUnsafeOperationsPlugin,
commandConfigPrefixingPlugin,
completionDetectionPlugin,
errorDetectionHandler,
Expand Down Expand Up @@ -55,6 +56,7 @@ export function gitInstanceFactory(
plugins.add(commandConfigPrefixingPlugin(config.config));
}

plugins.add(blockUnsafeOperationsPlugin(config.unsafe));
plugins.add(completionDetectionPlugin(config.completion));
config.abort && plugins.add(abortPlugin(config.abort));
config.progress && plugins.add(progressMonitorPlugin(config.progress));
Expand Down
41 changes: 41 additions & 0 deletions simple-git/src/lib/plugins/block-unsafe-operations-plugin.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import type { SimpleGitPlugin } from './simple-git-plugin';

import { GitPluginError } from '../errors/git-plugin-error';
import type { SimpleGitPluginConfig } from '../types';

function isConfigSwitch(arg: string) {
return arg.trim().toLowerCase() === '-c';
}

function preventProtocolOverride(arg: string, next: string) {
if (!isConfigSwitch(arg)) {
return;
}

if (!/^\s*protocol(.[a-z]+)?.allow/.test(next)) {
return;
}

throw new GitPluginError(
undefined,
'unsafe',
'Configuring protocol.allow is not permitted without enabling allowUnsafeExtProtocol'
);
}

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

allowUnsafeProtocolOverride || preventProtocolOverride(current, next);
});

return args;
},
};
}
1 change: 1 addition & 0 deletions simple-git/src/lib/plugins/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
export * from './abort-plugin';
export * from './block-unsafe-operations-plugin';
export * from './command-config-prefixing-plugin';
export * from './completion-detection.plugin';
export * from './error-detection.plugin';
Expand Down
16 changes: 16 additions & 0 deletions simple-git/src/lib/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,22 @@ export interface SimpleGitPluginConfig {
};

spawnOptions: Pick<SpawnOptions, 'uid' | 'gid'>;

unsafe: {
/**
* By default `simple-git` prevents the use of inline configuration
* options to override the protocols available for the `git` child
* process to prevent accidental security vulnerabilities when
* unsanitised user data is passed directly into operations such as
* `git.addRemote`, `git.clone` or `git.raw`.
*
* 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;
};
}

/**
Expand Down
56 changes: 56 additions & 0 deletions simple-git/test/integration/plugin.unsafe.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import { promiseError, promiseResult } from '@kwsites/promise-result';
import {
assertGitError,
createTestContext,
newSimpleGit,
SimpleGitTestContext,
} from '@simple-git/test-utils';

import { GitPluginError } from '../..';

describe('add', () => {
let context: SimpleGitTestContext;

beforeEach(async () => (context = await createTestContext()));

it('allows overriding protocol when opting in to unsafe practices', async () => {
const { threw } = await promiseResult(
newSimpleGit(context.root, { unsafe: { allowUnsafeProtocolOverride: true } }).raw(
'-c',
'protocol.ext.allow=always',
'init'
)
);

expect(threw).toBe(false);
});

it('prevents overriding protocol.ext.allow before the method of a command', async () => {
assertGitError(
await promiseError(context.git.raw('-c', 'protocol.ext.allow=always', 'init')),
'Configuring protocol.allow is not permitted',
GitPluginError
);
});

it('prevents overriding protocol.ext.allow after the method of a command', async () => {
assertGitError(
await promiseError(context.git.raw('init', '-c', 'protocol.ext.allow=always')),
'Configuring protocol.allow is not permitted',
GitPluginError
);
});

it('prevents adding a remote with vulnerable ext transport', async () => {
assertGitError(
await promiseError(
context.git.clone(`ext::sh -c touch% /tmp/pwn% >&2`, '/tmp/example-new-repo', [
'-c',
'protocol.ext.allow=always',
])
),
'Configuring protocol.allow is not permitted',
GitPluginError
);
});
});

0 comments on commit 6b3c631

Please sign in to comment.