Merge pull request github#13196 from erik-krogh/indirectCommand

JS: require arguments to be shell interpreted to be flagged by indirect-command-injection

Author: erik-krogh

javascript/ql/lib/semmle/javascript/frameworks/ActionsLib.qll

@@ -78,5 +78,10 @@ private class ExecActionsCall extends SystemCommandExecution, DataFlow::CallNode
 
   override DataFlow::Node getOptionsArg() { result = this.getArgument(2) }
 
+  override predicate isShellInterpreted(DataFlow::Node arg) {
+    arg = this.getACommandArgument() and
+    not this.getArgumentList().getALocalSource() instanceof DataFlow::ArrayCreationNode
+  }
+
   override predicate isSync() { none() }
 }

javascript/ql/lib/semmle/javascript/security/dataflow/IndirectCommandInjectionCustomizations.qll

@@ -199,9 +199,13 @@ module IndirectCommandInjection {
   }
 
   /**
-   * A command argument to a function that initiates an operating system command.
+   * A command argument to a function that initiates an operating system command as a shell invocation.
    */
   private class SystemCommandExecutionSink extends Sink, DataFlow::ValueNode {
-    SystemCommandExecutionSink() { this = any(SystemCommandExecution sys).getACommandArgument() }
+    SystemCommandExecutionSink() {
+      exists(SystemCommandExecution sys |
+        sys.isShellInterpreted(this) and this = sys.getACommandArgument()
+      )
+    }
   }
 }

javascript/ql/src/change-notes/2023-05-17-indirect-shell.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `js/indirect-command-line-injection` query no longer flags command arguments that cannot be interpreted as a shell string.

javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/command-line-parameter-command-injection.js

@@ -144,4 +144,6 @@ cp.exec("cmd.sh " + require("optimist").argv.foo); // NOT OK
 
 	cp.exec("cmd.sh " + program.opts().pizzaType); // NOT OK
 	cp.exec("cmd.sh " + program.pizzaType); // NOT OK
+
+	cp.execFile(program.opts().pizzaType, ["foo", "bar"]); // OK
 });