From bb22ffb2c907e0284f887ab625e12b88c31f4bf4 Mon Sep 17 00:00:00 2001 From: Ron <67099202+ron-checkmarx@users.noreply.github.com> Date: Thu, 17 Dec 2020 13:48:28 +0200 Subject: [PATCH 1/3] Use child_process.execFile instead of child_process.exec --- src/index.ts | 46 ++++++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/src/index.ts b/src/index.ts index 67739cb..bc9b590 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,4 +1,4 @@ -import { exec, execSync, ExecSyncOptions, ExecException } from "child_process"; +import { execFile, execFileSync, ExecSyncOptions, ExecException } from "child_process"; import { existsSync } from "fs"; import createDebugger from "debug"; @@ -126,8 +126,8 @@ const defaultOptions = { }; /** Add optional parameter to command */ -function addOptional( - command: string, +function addOptionalArgument( + command: string[], options: GitlogOptions ) { let commandWithOptions = command; @@ -140,9 +140,9 @@ function addOptional( "committer", ] as const; - for (let i = cmdOptional.length; i--; ) { + for (let i = cmdOptional.length; i--;) { if (options[cmdOptional[i]]) { - commandWithOptions += ` --${cmdOptional[i]}="${options[cmdOptional[i]]}"`; + commandWithOptions.push(`--${cmdOptional[i]}=${options[cmdOptional[i]]}`); } } @@ -234,30 +234,30 @@ const parseCommits = ( }; /** Run "git log" and return the result as JSON */ -function createCommand( +function createCommandArguments( options: GitlogOptions ) { // Start constructing command - let command = "git log -l0 "; + let command: string[] = ["log", "-l0"]; if (options.findCopiesHarder) { - command += "--find-copies-harder "; + command.push("--find-copies-harder"); } if (options.all) { - command += "--all "; + command.push("--all"); } if (options.includeMergeCommitFiles) { - command += "-m "; + command.push("-m"); } - command += `-n ${options.number}`; + command.push(`-n ${options.number}`); - command = addOptional(command, options); + command = addOptionalArgument(command, options); // Start of custom format - command += ' --pretty="@begin@'; + let prettyArgument: string = '--pretty=@begin@'; // Iterating through the fields and adding them to the custom format if (options.fields) { @@ -266,29 +266,31 @@ function createCommand( throw new Error(`Unknown field: ${field}`); } - command += delimiter + fieldMap[field]; + prettyArgument += delimiter + fieldMap[field]; }); } // Close custom format - command += '@end@"'; + prettyArgument += '@end@'; + command.push(prettyArgument); // Append branch (revision range) if specified if (options.branch) { - command += ` ${options.branch}`; + command.push(options.branch); } // File and file status if (options.nameStatus && !options.fileLineRange) { - command += " --name-status"; + command.push("--name-status"); } if (options.fileLineRange) { - command += ` -L ${options.fileLineRange.startLine},${options.fileLineRange.endLine}:${options.fileLineRange.file}`; + command.push(`-L ${options.fileLineRange.startLine},${options.fileLineRange.endLine}:${options.fileLineRange.file}`); } if (options.file) { - command += ` -- ${options.file}`; + command.push("--"); + command.push(options.file); } debug("command", options.execOptions, command); @@ -342,10 +344,10 @@ function gitlog( ...userOptions, }; const execOptions = { cwd: userOptions.repo, ...userOptions.execOptions }; - const command = createCommand(options); + const commandArguments = createCommandArguments(options); if (!cb) { - const stdout = execSync(command, execOptions).toString(); + const stdout = execFileSync("git", commandArguments, execOptions).toString(); const commits = stdout.split("@begin@"); if (commits[0] === "") { @@ -356,7 +358,7 @@ function gitlog( return parseCommits(commits, options.fields, options.nameStatus); } - exec(command, execOptions, (err, stdout, stderr) => { + execFile("git", commandArguments, execOptions, (err, stdout, stderr) => { debug("stdout", stdout); const commits = stdout.split("@begin@"); From 91989ed2bc858a15a7cadc344f4c3c40c1e3a5dd Mon Sep 17 00:00:00 2001 From: Ron <67099202+ron-checkmarx@users.noreply.github.com> Date: Thu, 17 Dec 2020 13:51:51 +0200 Subject: [PATCH 2/3] test for command injection regression --- test/gitlog.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/gitlog.test.ts b/test/gitlog.test.ts index 8d8e142..77ad127 100644 --- a/test/gitlog.test.ts +++ b/test/gitlog.test.ts @@ -1,5 +1,6 @@ /* eslint-disable handle-callback-err, no-unused-expressions */ +import fs from "fs"; import { exec, execSync } from "child_process"; import gitlog, { gitlogPromise } from "../src"; @@ -320,6 +321,20 @@ describe("gitlog", () => { expect(commitsForLastLine.length).toBe(3); }); + it("should not execute shell commands", (done) => { + gitlog({ + repo: testRepoLocation, + branch: "$(touch ../exploit)" + }, () => { + const exists = fs.existsSync("./test/exploit"); + expect(exists).toBe(false); + if (exists) { + fs.unlinkSync("./test/exploit"); + } + done(); + }); + }); + afterAll(() => { execInTestDir(`${__dirname}/delete-repo.sh`); }); From df162ea2de29853d2d43a88336f3d12d0ffbad69 Mon Sep 17 00:00:00 2001 From: Ron <67099202+ron-checkmarx@users.noreply.github.com> Date: Thu, 17 Dec 2020 13:56:13 +0200 Subject: [PATCH 3/3] fix typo --- src/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/index.ts b/src/index.ts index bc9b590..aa3f2d6 100644 --- a/src/index.ts +++ b/src/index.ts @@ -126,7 +126,7 @@ const defaultOptions = { }; /** Add optional parameter to command */ -function addOptionalArgument( +function addOptionalArguments( command: string[], options: GitlogOptions ) { @@ -254,7 +254,7 @@ function createCommandArguments