-
-
Notifications
You must be signed in to change notification settings - Fork 313
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Create the
unsafe
plugin to configure how simple-git
treats known…
… 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
Showing
7 changed files
with
159 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
41 changes: 41 additions & 0 deletions
41
simple-git/src/lib/plugins/block-unsafe-operations-plugin.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; | ||
}, | ||
}; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 | ||
); | ||
}); | ||
}); |