bugfix: prevent TypeError in the bug detectors

Author: oetr

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}'`,

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);
+	}
+}

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);
+	});
+});

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);
+};

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);
+	});
+});

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, () => {});
+});