Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/unsafe pack #881

Merged
merged 2 commits into from Dec 22, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Next
Unsafe operations plugin extended to prevent the use of potential att…
…ack 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 committed Dec 22, 2022
commit 95459310e5b8f96e20bb77ef1a6559036b779e13
7 changes: 6 additions & 1 deletion packages/test-utils/src/expectations.ts
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
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
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
@@ -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
97 changes: 97 additions & 0 deletions simple-git/test/unit/plugin.unsafe.spec.ts
@@ -0,0 +1,97 @@
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");
});

});