diff --git a/packages/bug-detectors/internal/command-injection.ts b/packages/bug-detectors/internal/command-injection.ts index aab5e61f..41a18f56 100644 --- a/packages/bug-detectors/internal/command-injection.ts +++ b/packages/bug-detectors/internal/command-injection.ts @@ -45,6 +45,9 @@ for (const functionName of functionNames) { // - the command/file path to execute in execFile/execFileSync // - the module path to fork in fork const firstArgument = params[0] as string; + if (typeof firstArgument !== "string") { + return; + } if (firstArgument.includes(goal)) { reportFinding( `Command Injection in ${functionName}(): called with '${firstArgument}'`, diff --git a/packages/bug-detectors/internal/path-traversal.ts b/packages/bug-detectors/internal/path-traversal.ts index b7dbe8ce..81938ea6 100644 --- a/packages/bug-detectors/internal/path-traversal.ts +++ b/packages/bug-detectors/internal/path-traversal.ts @@ -130,14 +130,8 @@ for (const module of modulesToHook) { return; } // The first argument of the original function is typically - // a path or a file name. - const firstArgument = params[0] as string; - if (firstArgument.includes(goal)) { - reportFinding( - `Path Traversal in ${functionName}(): called with '${firstArgument}'`, - ); - } - guideTowardsContainment(firstArgument, goal, hookId); + // a path or a file name. For some functions, it can be a URL or a Buffer. + detectFindingAndGuideFuzzing(params[0], goal, hookId, functionName); }; registerBeforeHook(functionName, module.moduleName, false, beforeHook); @@ -174,19 +168,15 @@ for (const module of functionsWithTwoTargets) { if (params === undefined || params.length < 2) { return; } - // The first two arguments are paths. - const firstArgument = params[0] as string; - const secondArgument = params[1] as string; - if (firstArgument.includes(goal) || secondArgument.includes(goal)) { - reportFinding( - `Path Traversal in ${functionName}(): called with '${firstArgument}'` + - ` and '${secondArgument}'`, - ); - } - guideTowardsContainment(firstArgument, goal, hookId); // We don't want to confuse the fuzzer guidance with the same hookId for both function arguments. // Therefore, we use an extra hookId for the second argument. - guideTowardsContainment(secondArgument, goal, extraHookId); + detectFindingAndGuideFuzzing(params[0], goal, hookId, functionName); + detectFindingAndGuideFuzzing( + params[1], + goal, + extraHookId, + functionName, + ); }; }; @@ -198,3 +188,24 @@ for (const module of functionsWithTwoTargets) { ); } } + +function detectFindingAndGuideFuzzing( + input: unknown, + goal: string, + hookId: number, + functionName: string, +) { + if ( + typeof input === "string" || + input instanceof URL || + input instanceof Buffer + ) { + const argument = input.toString(); + if (argument.includes(goal)) { + reportFinding( + `Path Traversal in ${functionName}(): called with '${argument}'`, + ); + } + guideTowardsContainment(argument, goal, hookId); + } +} diff --git a/tests/bug-detectors/command-injection.test.js b/tests/bug-detectors/command-injection.test.js index e3a7d07c..fa937947 100644 --- a/tests/bug-detectors/command-injection.test.js +++ b/tests/bug-detectors/command-injection.test.js @@ -171,3 +171,19 @@ describe("Command injection", () => { expect(fs.existsSync(friendlyFilePath)).toBeTruthy(); }); }); + +describe("Invalid arguments", () => { + const bugDetectorDirectory = path.join(__dirname, "command-injection"); + const errorMessage = /TypeError: The ".*" argument must be of type string./; + + it("invalid args to exec", () => { + const fuzzTest = new FuzzTestBuilder() + .runs(0) + .sync(false) + .fuzzEntryPoint("execInvalid") + .dir(bugDetectorDirectory) + .build(); + expect(() => fuzzTest.execute()).toThrow(); + expect(fuzzTest.stderr).toMatch(errorMessage); + }); +}); diff --git a/tests/bug-detectors/command-injection/fuzz.js b/tests/bug-detectors/command-injection/fuzz.js index 83c4ce7d..b97f61ab 100644 --- a/tests/bug-detectors/command-injection/fuzz.js +++ b/tests/bug-detectors/command-injection/fuzz.js @@ -92,3 +92,7 @@ module.exports.forkEVIL = function (data) { module.exports.forkFRIENDLY = function (data) { child_process.fork("makeFRIENDLY.js"); }; + +module.exports.execInvalid = async function (data) { + child_process.exec(0); +}; diff --git a/tests/bug-detectors/path-traversal.test.js b/tests/bug-detectors/path-traversal.test.js index fa7251a3..b4337025 100644 --- a/tests/bug-detectors/path-traversal.test.js +++ b/tests/bug-detectors/path-traversal.test.js @@ -186,3 +186,33 @@ describe("Path Traversal", () => { fuzzTest.execute(); }); }); + +describe("Path Traversal invalid input", () => { + const bugDetectorDirectory = path.join(__dirname, "path-traversal"); + const errorMessage = + /TypeError: The ".*" argument must be of type string or an instance of Buffer or URL./; + + it("Invalid args to open", () => { + const fuzzTest = new FuzzTestBuilder() + .runs(0) + .sync(true) + .fuzzEntryPoint("invalidArgsToOpen") + .dir(bugDetectorDirectory) + .build(); + expect(() => fuzzTest.execute()).toThrow(); + expect(fuzzTest.stderr).toMatch(errorMessage); + }); + + it("Invalid first arg to cp", () => { + const fuzzTest = new FuzzTestBuilder() + .runs(0) + .sync(true) + .fuzzEntryPoint("invalidArgsToCp") + .dir(bugDetectorDirectory) + .build(); + expect(() => fuzzTest.execute()).toThrow(); + // 'TypeError: The "src" argument must be of type string or an instance of Buffer or URL.', + // however the string "src" may vary from system to system, so a regexp is better + expect(fuzzTest.stderr).toMatch(errorMessage); + }); +}); diff --git a/tests/bug-detectors/path-traversal/fuzz.js b/tests/bug-detectors/path-traversal/fuzz.js index 3e0ce866..f32c8544 100644 --- a/tests/bug-detectors/path-traversal/fuzz.js +++ b/tests/bug-detectors/path-traversal/fuzz.js @@ -100,3 +100,11 @@ module.exports.PathTraversalJoinEvilAsync = makeFnCalledOnce(async (data) => { module.exports.PathTraversalJoinSafeAsync = makeFnCalledOnce(async (data) => { path.join(safe_path, "SAFE"); }); + +module.exports.invalidArgsToOpen = makeFnCalledOnce((data) => { + fs.openSync(0); +}); + +module.exports.invalidArgsToCp = makeFnCalledOnce((data) => { + fs.cp(0, 0, () => {}); +});