Merge pull request #13484 from aegilops/java/experimental/command-inj…

…ection Java: Experimental version of Java Command Injection query
github · Aug 4, 2023 · 33eaeb9 · 33eaeb9
2 parents 51c8331 + 5db569d
commit 33eaeb9
Show file tree

Hide file tree

Showing 9 changed files with 339 additions and 0 deletions.
diff --git a/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExec.java b/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExec.java
@@ -0,0 +1,9 @@
+class Test {
+    public static void main(String[] args) {
+        String script = System.getenv("SCRIPTNAME");
+        if (script != null) {
+            // BAD: The script to be executed by /bin/sh is controlled by the user.
+            Runtime.getRuntime().exec(new String[]{"/bin/sh", script});
+        }
+    }
+}
diff --git a/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExec.qhelp b/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExec.qhelp
@@ -0,0 +1,41 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Code that passes remote user input to an arugment of a call of <code>Runtime.exec</code> that
+executes a scripting executable will allow the user to execute malicious code.</p>
+
+</overview>
+<recommendation>
+
+<p>If possible, use hard-coded string literals to specify the command or script to run,
+or library to load. Instead of passing the user input directly to the
+process or library function, examine the user input and then choose
+among hard-coded string literals.</p>
+
+<p>If the applicable libraries or commands cannot be determined at
+compile time, then add code to verify that the user input string is
+safe before using it.</p>
+
+</recommendation>
+<example>
+
+<p>The following example shows code that takes a shell script that can be changed
+maliciously by a user, and passes it straight to the array going into <code>Runtime.exec</code>
+without examining it first.</p>
+
+<sample src="CommandInjectionRuntimeExec.java" />
+
+</example>
+<references>
+
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
+</li>
+<li>SEI CERT Oracle Coding Standard for Java:
+  <a href="https://wiki.sei.cmu.edu/confluence/display/java/IDS07-J.+Sanitize+untrusted+data+passed+to+the+Runtime.exec()+method">IDS07-J. Sanitize untrusted data passed to the Runtime.exec() method</a>.</li>
+
+</references>
+</qhelp>
diff --git a/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExec.ql b/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExec.ql
@@ -0,0 +1,25 @@
+/**
+ * @name Command Injection into Runtime.exec() with dangerous command
+ * @description High sensitvity and precision version of java/command-line-injection, designed to find more cases of command injection in rare cases that the default query does not find
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 6.1
+ * @precision high
+ * @id java/command-line-injection-extra
+ * @tags security
+ *       experimental
+ *       external/cwe/cwe-078
+ */
+
+import CommandInjectionRuntimeExec
+import ExecUserFlow::PathGraph
+
+class RemoteSource extends Source instanceof RemoteFlowSource { }
+
+from
+  ExecUserFlow::PathNode source, ExecUserFlow::PathNode sink, DataFlow::Node sourceCmd,
+  DataFlow::Node sinkCmd
+where callIsTaintedByUserInputAndDangerousCommand(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
@@ -0,0 +1,108 @@
+import java
+import semmle.code.java.frameworks.javaee.ejb.EJBRestrictions
+import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.dataflow.FlowSources
+
+module ExecCmdFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source.asExpr().(CompileTimeConstantExpr).getStringValue() instanceof UnSafeExecutable
+  }
+
+  predicate isSink(DataFlow::Node sink) {
+    exists(MethodAccess call |
+      call.getMethod() instanceof RuntimeExecMethod and
+      sink.asExpr() = call.getArgument(0) and
+      sink.asExpr().getType() instanceof Array
+    )
+  }
+
+  predicate isBarrier(DataFlow::Node node) {
+    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 { }
+
+module ExecUserFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+  predicate isSink(DataFlow::Node sink) {
+    exists(MethodAccess call |
+      call.getMethod() instanceof RuntimeExecMethod and
+      sink.asExpr() = call.getArgument(_) and
+      sink.asExpr().getType() instanceof Array
+    )
+  }
+
+  predicate isBarrier(DataFlow::Node node) {
+    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() {
+    exists(AssignExpr assign, ArrayAccess access |
+      assign.getDest() = access and
+      access.getIndexExpr().(IntegerLiteral).getValue().toInt() != 0 and
+      assign.getSource() = this.asExpr()
+    )
+  }
+}
+
+// String[] array = {"a", "b, "c"};
+class ArrayInitAtNonZeroIndex extends DataFlow::Node {
+  ArrayInitAtNonZeroIndex() {
+    exists(ArrayInit init, int index |
+      init.getInit(index) = this.asExpr() and
+      index != 0
+    )
+  }
+}
+
+// Stream.concat(Arrays.stream(array_1), Arrays.stream(array_2))
+class StreamConcatAtNonZeroIndex extends DataFlow::Node {
+  StreamConcatAtNonZeroIndex() {
+    exists(MethodAccess call, int index |
+      call.getMethod().getQualifiedName() = "java.util.stream.Stream.concat" and
+      call.getArgument(index) = this.asExpr() and
+      index != 0
+    )
+  }
+}
+
+// list of executables that execute their arguments
+// TODO: extend with data extensions
+class UnSafeExecutable extends string {
+  bindingset[this]
+  UnSafeExecutable() {
+    this.regexpMatch("^(|.*/)([a-z]*sh|javac?|python.*|perl|[Pp]ower[Ss]hell|php|node|deno|bun|ruby|osascript|cmd|Rscript|groovy)(\\.exe)?$") and
+    not this = "netsh.exe"
+  }
+}
+
+predicate callIsTaintedByUserInputAndDangerousCommand(
+  ExecUserFlow::PathNode source, ExecUserFlow::PathNode sink, DataFlow::Node sourceCmd,
+  DataFlow::Node sinkCmd
+) {
+  exists(MethodAccess call |
+    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.qhelp b/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExecLocal.qhelp
@@ -0,0 +1,41 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Code that passes local user input to an arugment of a call of <code>Runtime.exec</code> that
+executes a scripting executable will allow the user to execute malicious code.</p>
+
+</overview>
+<recommendation>
+
+<p>If possible, use hard-coded string literals to specify the command or script to run,
+or library to load. Instead of passing the user input directly to the
+process or library function, examine the user input and then choose
+among hard-coded string literals.</p>
+
+<p>If the applicable libraries or commands cannot be determined at
+compile time, then add code to verify that the user input string is
+safe before using it.</p>
+
+</recommendation>
+<example>
+
+<p>The following example shows code that takes a shell script that can be changed
+maliciously by a user, and passes it straight to the array going into <code>Runtime.exec</code>
+without examining it first.</p>
+
+<sample src="CommandInjectionRuntimeExec.java" />
+
+</example>
+<references>
+
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
+</li>
+<li>SEI CERT Oracle Coding Standard for Java:
+  <a href="https://wiki.sei.cmu.edu/confluence/display/java/IDS07-J.+Sanitize+untrusted+data+passed+to+the+Runtime.exec()+method">IDS07-J. Sanitize untrusted data passed to the Runtime.exec() method</a>.</li>
+
+</references>
+</qhelp>
diff --git a/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExecLocal.ql b/java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExecLocal.ql
@@ -0,0 +1,26 @@
+/**
+ * @name Command Injection into Runtime.exec() with dangerous command
+ * @description High sensitvity and precision version of java/command-line-injection, designed to find more cases of command injection in rare cases that the default query does not find
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 6.1
+ * @precision high
+ * @id java/command-line-injection-extra-local
+ * @tags security
+ *       experimental
+ *       local
+ *       external/cwe/cwe-078
+ */
+
+import CommandInjectionRuntimeExec
+import ExecUserFlow::PathGraph
+
+class LocalSource extends Source instanceof LocalUserInput { }
+
+from
+  ExecUserFlow::PathNode source, ExecUserFlow::PathNode sink, DataFlow::Node sourceCmd,
+  DataFlow::Node sinkCmd
+where callIsTaintedByUserInputAndDangerousCommand(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/.../test/experimental/query-tests/security/CWE-078/CommandInjectionRuntimeExecLocal.expected b/.../test/experimental/query-tests/security/CWE-078/CommandInjectionRuntimeExecLocal.expected
@@ -0,0 +1,41 @@
+edges
+| RuntimeExecTest.java:17:25:17:51 | getenv(...) : String | RuntimeExecTest.java:22:67:22:72 | script : String |
+| RuntimeExecTest.java:17:25:17:51 | getenv(...) : String | RuntimeExecTest.java:25:66:25:71 | script : String |
+| RuntimeExecTest.java:17:25:17:51 | getenv(...) : String | RuntimeExecTest.java:31:36:31:41 | script : String |
+| RuntimeExecTest.java:17:25:17:51 | getenv(...) : String | RuntimeExecTest.java:38:52:38:57 | script : String |
+| RuntimeExecTest.java:22:43:22:73 | {...} : String[] [[]] : String | RuntimeExecTest.java:22:43:22:73 | new String[] |
+| RuntimeExecTest.java:22:67:22:72 | script : String | RuntimeExecTest.java:22:43:22:73 | {...} : String[] [[]] : String |
+| RuntimeExecTest.java:25:42:25:72 | {...} : String[] [[]] : String | RuntimeExecTest.java:26:43:26:55 | commandArray1 |
+| RuntimeExecTest.java:25:66:25:71 | script : String | RuntimeExecTest.java:25:42:25:72 | {...} : String[] [[]] : String |
+| RuntimeExecTest.java:31:17:31:29 | commandArray2 [post update] : String[] [[]] : String | RuntimeExecTest.java:32:43:32:55 | commandArray2 |
+| RuntimeExecTest.java:31:36:31:41 | script : String | RuntimeExecTest.java:31:17:31:29 | commandArray2 [post update] : String[] [[]] : String |
+| RuntimeExecTest.java:36:21:39:21 | concat(...) : Stream [<element>] : String | RuntimeExecTest.java:36:21:39:44 | toArray(...) : String[] [[]] : String |
+| RuntimeExecTest.java:36:21:39:44 | toArray(...) : String[] [[]] : String | RuntimeExecTest.java:36:21:39:44 | toArray(...) |
+| RuntimeExecTest.java:38:25:38:59 | stream(...) : Stream [<element>] : String | RuntimeExecTest.java:36:21:39:21 | concat(...) : Stream [<element>] : String |
+| RuntimeExecTest.java:38:39:38:58 | new String[] : String[] [[]] : String | RuntimeExecTest.java:38:25:38:59 | stream(...) : Stream [<element>] : String |
+| RuntimeExecTest.java:38:39:38:58 | {...} : String[] [[]] : String | RuntimeExecTest.java:38:39:38:58 | new String[] : String[] [[]] : String |
+| RuntimeExecTest.java:38:52:38:57 | script : String | RuntimeExecTest.java:38:39:38:58 | {...} : String[] [[]] : String |
+nodes
+| RuntimeExecTest.java:17:25:17:51 | getenv(...) : String | semmle.label | getenv(...) : String |
+| RuntimeExecTest.java:22:43:22:73 | new String[] | semmle.label | new String[] |
+| RuntimeExecTest.java:22:43:22:73 | {...} : String[] [[]] : String | semmle.label | {...} : String[] [[]] : String |
+| RuntimeExecTest.java:22:67:22:72 | script : String | semmle.label | script : String |
+| RuntimeExecTest.java:25:42:25:72 | {...} : String[] [[]] : String | semmle.label | {...} : String[] [[]] : String |
+| RuntimeExecTest.java:25:66:25:71 | script : String | semmle.label | script : String |
+| RuntimeExecTest.java:26:43:26:55 | commandArray1 | semmle.label | commandArray1 |
+| RuntimeExecTest.java:31:17:31:29 | commandArray2 [post update] : String[] [[]] : String | semmle.label | commandArray2 [post update] : String[] [[]] : String |
+| RuntimeExecTest.java:31:36:31:41 | script : String | semmle.label | script : String |
+| RuntimeExecTest.java:32:43:32:55 | commandArray2 | semmle.label | commandArray2 |
+| RuntimeExecTest.java:36:21:39:21 | concat(...) : Stream [<element>] : String | semmle.label | concat(...) : Stream [<element>] : String |
+| RuntimeExecTest.java:36:21:39:44 | toArray(...) | semmle.label | toArray(...) |
+| RuntimeExecTest.java:36:21:39:44 | toArray(...) : String[] [[]] : String | semmle.label | toArray(...) : String[] [[]] : String |
+| RuntimeExecTest.java:38:25:38:59 | stream(...) : Stream [<element>] : String | semmle.label | stream(...) : Stream [<element>] : String |
+| RuntimeExecTest.java:38:39:38:58 | new String[] : String[] [[]] : String | semmle.label | new String[] : String[] [[]] : String |
+| RuntimeExecTest.java:38:39:38:58 | {...} : String[] [[]] : String | semmle.label | {...} : String[] [[]] : String |
+| RuntimeExecTest.java:38:52:38:57 | script : String | semmle.label | script : String |
+subpaths
+#select
+| RuntimeExecTest.java:22:43:22:73 | new String[] | RuntimeExecTest.java:17:25:17:51 | getenv(...) : String | RuntimeExecTest.java:22:43:22:73 | new String[] | Call to dangerous java.lang.Runtime.exec() with command '$@' with arg from untrusted input '$@' | RuntimeExecTest.java:22:56:22:64 | "/bin/sh" | "/bin/sh" | RuntimeExecTest.java:17:25:17:51 | getenv(...) | getenv(...) : String |
+| RuntimeExecTest.java:26:43:26:55 | commandArray1 | RuntimeExecTest.java:17:25:17:51 | getenv(...) : String | RuntimeExecTest.java:26:43:26:55 | commandArray1 | Call to dangerous java.lang.Runtime.exec() with command '$@' with arg from untrusted input '$@' | RuntimeExecTest.java:25:55:25:63 | "/bin/sh" | "/bin/sh" | RuntimeExecTest.java:17:25:17:51 | getenv(...) | getenv(...) : String |
+| RuntimeExecTest.java:32:43:32:55 | commandArray2 | RuntimeExecTest.java:17:25:17:51 | getenv(...) : String | RuntimeExecTest.java:32:43:32:55 | commandArray2 | Call to dangerous java.lang.Runtime.exec() with command '$@' with arg from untrusted input '$@' | RuntimeExecTest.java:30:36:30:44 | "/bin/sh" | "/bin/sh" | RuntimeExecTest.java:17:25:17:51 | getenv(...) | getenv(...) : String |
+| RuntimeExecTest.java:36:21:39:44 | toArray(...) | RuntimeExecTest.java:17:25:17:51 | getenv(...) : String | RuntimeExecTest.java:36:21:39:44 | toArray(...) | Call to dangerous java.lang.Runtime.exec() with command '$@' with arg from untrusted input '$@' | RuntimeExecTest.java:37:52:37:60 | "/bin/sh" | "/bin/sh" | RuntimeExecTest.java:17:25:17:51 | getenv(...) | getenv(...) : String |
diff --git a/.../ql/test/experimental/query-tests/security/CWE-078/CommandInjectionRuntimeExecLocal.qlref b/.../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,47 @@
+/* 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() {
+        System.out.println("Command injection test");
+
+        String script = System.getenv("SCRIPTNAME");
+
+        if (script != null) {
+            try {
+                // 1. array literal in the args
+                Runtime.getRuntime().exec(new String[]{"/bin/sh", script});
+
+                // 2. array literal with dataflow
+                String[] commandArray1 = new String[]{"/bin/sh", script};
+                Runtime.getRuntime().exec(commandArray1);
+
+                // 3. array assignment after it is created
+                String[] commandArray2 = new String[4];
+                commandArray2[0] = "/bin/sh";
+                commandArray2[1] = script;
+                Runtime.getRuntime().exec(commandArray2);
+
+                // 4. Stream concatenation
+                Runtime.getRuntime().exec(
+                    Stream.concat(
+                        Arrays.stream(new String[]{"/bin/sh"}),
+                        Arrays.stream(new String[]{script})
+                    ).toArray(String[]::new)
+                );
+
+            } catch (Exception e) {
+                System.err.println("ERROR: " + e.getMessage());
+            }
+        }
+    }
+}