Merge pull request #155 from AikidoSec/shell_inj_add_sinks

Add missing sinks spawn and spawnSync for shell injection
AikidoSec · Apr 11, 2024 · 1e4edd6 · 1e4edd6
2 parents 2f3d3e0 + ef882c8
commit 1e4edd6
Show file tree

Hide file tree

Showing 2 changed files with 81 additions and 2 deletions.
diff --git a/library/sinks/ChildProcess.test.ts b/library/sinks/ChildProcess.test.ts
@@ -38,7 +38,7 @@ t.test("it works", async (t) => {
 
   agent.start([new ChildProcess()]);
 
-  const { exec, execSync } = require("child_process");
+  const { exec, execSync, spawn, spawnSync } = require("child_process");
 
   const runCommandsWithInvalidArgs = () => {
     throws(
@@ -50,6 +50,16 @@ t.test("it works", async (t) => {
       () => execSync(),
       /argument must be of type string. Received undefined/
     );
+
+    throws(
+      () => spawn().unref(),
+      /argument must be of type string. Received undefined/
+    );
+
+    throws(
+      () => spawnSync(),
+      /argument must be of type string. Received undefined/
+    );
   };
 
   runCommandsWithInvalidArgs();
@@ -61,6 +71,12 @@ t.test("it works", async (t) => {
   const runSafeCommands = () => {
     exec("ls", (err, stdout, stderr) => {}).unref();
     execSync("ls", (err, stdout, stderr) => {});
+
+    spawn("ls", ["-la"], {}, (err, stdout, stderr) => {}).unref();
+    spawnSync("ls", ["-la"], {}, (err, stdout, stderr) => {});
+
+    spawn("ls", ["-la"], { shell: false }, (err, stdout, stderr) => {}).unref();
+    spawnSync("ls", ["-la"], { shell: false }, (err, stdout, stderr) => {});
   };
 
   runSafeCommands();
@@ -80,4 +96,50 @@ t.test("it works", async (t) => {
       "Aikido runtime has blocked a Shell injection: child_process.execSync(...) originating from body.file.matches"
     );
   });
+
+  runWithContext(unsafeContext, () => {
+    throws(
+      () =>
+        spawn(
+          "ls `echo .`",
+          [],
+          { shell: true },
+          (err, stdout, stderr) => {}
+        ).unref(),
+      "Aikido runtime has blocked a Shell injection: child_process.spawn(...) originating from body.file.matches"
+    );
+
+    throws(
+      () =>
+        spawn(
+          "ls `echo .`",
+          [],
+          { shell: "/bin/sh" },
+          (err, stdout, stderr) => {}
+        ).unref(),
+      "Aikido runtime has blocked a Shell injection: child_process.spawn(...) originating from body.file.matches"
+    );
+
+    throws(
+      () =>
+        spawnSync(
+          "ls `echo .`",
+          [],
+          { shell: true },
+          (err, stdout, stderr) => {}
+        ),
+      "Aikido runtime has blocked a Shell injection: child_process.spawnSync(...) originating from body.file.matches"
+    );
+
+    throws(
+      () =>
+        spawnSync(
+          "ls `echo .`",
+          [],
+          { shell: "/bin/sh" },
+          (err, stdout, stderr) => {}
+        ),
+      "Aikido runtime has blocked a Shell injection: child_process.spawnSync(...) originating from body.file.matches"
+    );
+  });
 });
diff --git a/library/sinks/ChildProcess.ts b/library/sinks/ChildProcess.ts
@@ -2,6 +2,7 @@ import { getContext } from "../agent/Context";
 import { Hooks } from "../agent/hooks/Hooks";
 import { InterceptorResult } from "../agent/hooks/MethodInterceptor";
 import { Wrapper } from "../agent/Wrapper";
+import { isPlainObject } from "../helpers/isPlainObject";
 import { checkContextForShellInjection } from "../vulnerabilities/shell-injection/checkContextForShellInjection";
 
 export class ChildProcess implements Wrapper {
@@ -12,6 +13,20 @@ export class ChildProcess implements Wrapper {
       return undefined;
     }
 
+    // Ignore calls to spawn or spawnSync if shell option is not enabled
+    if (name === "spawn" || name === "spawnSync") {
+      const unsafeShellOption = args.find(
+        (arg) =>
+          isPlainObject(arg) &&
+          "shell" in arg &&
+          (arg.shell === true || typeof arg.shell === "string")
+      );
+
+      if (!unsafeShellOption) {
+        return undefined;
+      }
+    }
+
     if (args.length > 0 && typeof args[0] === "string") {
       const command = args[0];
 
@@ -31,6 +46,8 @@ export class ChildProcess implements Wrapper {
     childProcess
       .addSubject((exports) => exports)
       .inspect("exec", (args) => this.inspectExec(args, "exec"))
-      .inspect("execSync", (args) => this.inspectExec(args, "execSync"));
+      .inspect("execSync", (args) => this.inspectExec(args, "execSync"))
+      .inspect("spawn", (args) => this.inspectExec(args, "spawn"))
+      .inspect("spawnSync", (args) => this.inspectExec(args, "spawnSync"));
   }
 }