Switched to new dataflow and added a test (but it doesn't produce results yet)

aegilops · aegilops · commit 01798f63f8f5 · 2023-06-28T17:14:39.000+01:00
diff --git a/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExec.ql b/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExec.ql
@@ -11,27 +11,16 @@
  *       external/cwe/cwe-078
  */
 
-import DataFlow::PathGraph
 import CommandInjectionRuntimeExec
+import ExecUserFlow::PathGraph
 
-class RemoteSource extends Source instanceof RemoteFlowSource {}
+class RemoteSource extends Source instanceof RemoteFlowSource { }
 
 from
-  DataFlow::PathNode source, DataFlow::PathNode sink, ExecTaintConfiguration2 conf,
-  MethodAccess call, DataFlow::Node sourceCmd, DataFlow::Node sinkCmd,
-  ExecTaintConfiguration confCmd
+  ExecUserFlow::PathNode source, ExecUserFlow::PathNode sink,
+  MethodAccess call, DataFlow::Node sourceCmd, DataFlow::Node sinkCmd
 where
-  call.getMethod() instanceof RuntimeExecMethod and
-  // this is a command-accepting call to exec, e.g. rt.exec(new String[]{"/bin/sh", ...})
-  (
-    confCmd.hasFlow(sourceCmd, sinkCmd) and
-    sinkCmd.asExpr() = call.getArgument(0)
-  ) and
-  // it is tainted by untrusted user input
-  (
-    conf.hasFlow(source.getNode(), sink.getNode()) and
-    sink.getNode().asExpr() = call.getArgument(0)
-  )
+  callIsTaintedByUserInputAndDangerousCommand(call, source, sink, sourceCmd, sinkCmd)
 select sink, source, sink,
   "Call to dangerous java.lang.Runtime.exec() with command '$@' with arg from untrusted input '$@'",
   sourceCmd, sourceCmd.toString(), source.getNode(), source.toString()
diff --git a/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExec.qll b/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExec.qll
@@ -1,11 +1,10 @@
 import java
 import semmle.code.java.frameworks.javaee.ejb.EJBRestrictions
 import semmle.code.java.dataflow.DataFlow
-private import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.dataflow.FlowSources
 
 // a static string of an unsafe executable tainting arg 0 of Runtime.exec()
-class ExecTaintConfiguration extends TaintTracking::Configuration {
+deprecated class ExecTaintConfiguration extends TaintTracking::Configuration {
   ExecTaintConfiguration() { this = "ExecTaintConfiguration" }
 
   override predicate isSource(DataFlow::Node source) {
@@ -33,12 +32,41 @@ class ExecTaintConfiguration extends TaintTracking::Configuration {
   }
 }
 
+module ExecCmdFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source.asExpr() instanceof StringLiteral and
+    source.asExpr().(StringLiteral).getValue() instanceof UnSafeExecutable
+  }
+
+  predicate isSink(DataFlow::Node sink) {
+    exists(RuntimeExecMethod method, MethodAccess call |
+      call.getMethod() = method and
+      sink.asExpr() = call.getArgument(0) and
+      sink.asExpr().getType() instanceof Array
+    )
+  }
+
+  predicate isBarrier(DataFlow::Node node) {
+    node.asExpr().getFile().isSourceFile() and
+    (
+      node instanceof AssignToNonZeroIndex or
+      node instanceof ArrayInitAtNonZeroIndex or
+      node instanceof StreamConcatAtNonZeroIndex or
+      node.getType() instanceof PrimitiveType or
+      node.getType() instanceof BoxedType
+    )
+  }
+}
+
+/** Tracks flow of unvalidated user input that is used in Runtime.Exec */
+module ExecCmdFlow = TaintTracking::Global<ExecCmdFlowConfig>;
+
 abstract class Source extends DataFlow::Node {
   Source() { this = this }
 }
 
 // taint flow from user data to args of the command
-class ExecTaintConfiguration2 extends TaintTracking::Configuration {
+deprecated class ExecTaintConfiguration2 extends TaintTracking::Configuration {
   ExecTaintConfiguration2() { this = "ExecTaintConfiguration2" }
 
   override predicate isSource(DataFlow::Node source) { source instanceof Source }
@@ -60,6 +88,31 @@ class ExecTaintConfiguration2 extends TaintTracking::Configuration {
   }
 }
 
+module ExecUserFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source instanceof Source
+  }
+
+  predicate isSink(DataFlow::Node sink) {
+    exists(RuntimeExecMethod method, MethodAccess call |
+      call.getMethod() = method and
+      sink.asExpr() = call.getArgument(_) and
+      sink.asExpr().getType() instanceof Array
+    )
+  }
+
+  predicate isBarrier(DataFlow::Node node) {
+    node.asExpr().getFile().isSourceFile() and
+    (
+      node.getType() instanceof PrimitiveType or
+      node.getType() instanceof BoxedType
+    )
+  }
+}
+
+/** Tracks flow of unvalidated user input that is used in Runtime.Exec */
+module ExecUserFlow = TaintTracking::Global<ExecUserFlowConfig>;
+
 // array[3] = node
 class AssignToNonZeroIndex extends DataFlow::Node {
   AssignToNonZeroIndex() {
@@ -101,3 +154,17 @@ class UnSafeExecutable extends string {
     not this = "netsh.exe"
   }
 }
+
+predicate callIsTaintedByUserInputAndDangerousCommand(MethodAccess call, ExecUserFlow::PathNode source, ExecUserFlow::PathNode sink, DataFlow::Node sourceCmd, DataFlow::Node sinkCmd) {
+  call.getMethod() instanceof RuntimeExecMethod and
+  // this is a command-accepting call to exec, e.g. rt.exec(new String[]{"/bin/sh", ...})
+  (
+    ExecCmdFlow::flow(sourceCmd, sinkCmd) and
+    sinkCmd.asExpr() = call.getArgument(0)
+  ) and
+  // it is tainted by untrusted user input
+  (
+    ExecUserFlow::flowPath(source, sink) and
+    sink.getNode().asExpr() = call.getArgument(0)
+  )
+}
diff --git a/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExecLocal.ql b/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExecLocal.ql
@@ -12,27 +12,16 @@
  *       external/cwe/cwe-078
  */
 
-import DataFlow::PathGraph
 import CommandInjectionRuntimeExec
+import ExecUserFlow::PathGraph
 
-class LocalSource extends Source instanceof LocalUserInput {}
+class LocalSource extends Source instanceof LocalUserInput { }
 
 from
-  DataFlow::PathNode source, DataFlow::PathNode sink, ExecTaintConfiguration2 conf,
-  MethodAccess call, DataFlow::Node sourceCmd, DataFlow::Node sinkCmd,
-  ExecTaintConfiguration confCmd
+  ExecUserFlow::PathNode source, ExecUserFlow::PathNode sink,
+  MethodAccess call, DataFlow::Node sourceCmd, DataFlow::Node sinkCmd
 where
-  call.getMethod() instanceof RuntimeExecMethod and
-  // this is a command-accepting call to exec, e.g. rt.exec(new String[]{"/bin/sh", ...})
-  (
-    confCmd.hasFlow(sourceCmd, sinkCmd) and
-    sinkCmd.asExpr() = call.getArgument(0)
-  ) and
-  // it is tainted by untrusted user input
-  (
-    conf.hasFlow(source.getNode(), sink.getNode()) and
-    sink.getNode().asExpr() = call.getArgument(0)
-  )
+  callIsTaintedByUserInputAndDangerousCommand(call, source, sink, sourceCmd, sinkCmd)
 select sink, source, sink,
   "Call to dangerous java.lang.Runtime.exec() with command '$@' with arg from untrusted input '$@'",
   sourceCmd, sourceCmd.toString(), source.getNode(), source.toString()
diff --git a/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExecTest.ql b/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExecTest.ql
diff --git a/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExecTestPath.ql b/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExecTestPath.ql
diff --git a/java/ql/test/experimental/query-tests/security/CWE-078/CommandInjectionRuntimeExecLocal.qlref b/java/ql/test/experimental/query-tests/security/CWE-078/CommandInjectionRuntimeExecLocal.qlref
@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExecLocal.ql
diff --git a/java/ql/test/experimental/query-tests/security/CWE-078/RuntimeExecTest.java b/java/ql/test/experimental/query-tests/security/CWE-078/RuntimeExecTest.java
@@ -0,0 +1,42 @@
+/* Tests for command injection query
+ * 
+ * This is suitable for testing static analysis tools, as long as they treat local input as an attack surface (which can be prone to false positives)
+ * 
+ * (C) Copyright GitHub, 2023
+ * 
+ */
+
+import java.util.stream.Stream;
+import java.io.IOException;
+import java.util.Arrays;
+
+public class RuntimeExecTest {
+    public static void test(String[] args) {
+        System.out.println("Command injection test");
+
+        try {
+            // 1. array literal
+            String[] commandArray1 = new String[]{"/bin/sh", args[2], args[3], args[4]};
+            Runtime.getRuntime().exec(commandArray1);
+
+            // 2. array assignment after it is created
+            String[] commandArray2 = new String[4];
+            commandArray2[0] = "/bin/sh";
+            commandArray2[1] = args[2];
+            commandArray2[2] = args[3];
+            commandArray2[3] = args[4];
+            Runtime.getRuntime().exec(commandArray2);
+
+            // 3. Stream concatenation
+            Runtime.getRuntime().exec(
+                Stream.concat(
+                    Arrays.stream(new String[]{"/bin/sh"}),
+                    Arrays.stream(new String[]{args[2], args[3], args[4]})
+                ).toArray(String[]::new)
+            );
+
+        } catch (Exception e) {
+            System.err.println("ERROR: " + e.getMessage());
+        }
+    }
+}

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExecLocal.ql`