Java: Improve the ExecTainted query

java/ql/src/Security/CWE/CWE-078/ExecCommon.qll

@@ -1,5 +1,6 @@
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.security.ExternalProcess
+import semmle.code.java.security.CommandArguments
 
 private class RemoteUserInputToArgumentToExecFlowConfig extends TaintTracking::Configuration {
   RemoteUserInputToArgumentToExecFlowConfig() {
@@ -11,7 +12,11 @@ private class RemoteUserInputToArgumentToExecFlowConfig extends TaintTracking::C
   override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof ArgumentToExec }
 
   override predicate isSanitizer(DataFlow::Node node) {
-    node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType
+    node.getType() instanceof PrimitiveType
+    or
+    node.getType() instanceof BoxedType
+    or
+    isSafeCommandArgument(node.asExpr())
   }
 }
 

java/ql/src/Security/CWE/CWE-078/ExecTainted.ql

@@ -17,7 +17,7 @@ import semmle.code.java.security.ExternalProcess
 import ExecCommon
 import DataFlow::PathGraph
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, StringArgumentToExec execArg
+from DataFlow::PathNode source, DataFlow::PathNode sink, ArgumentToExec execArg
 where execTainted(source, sink, execArg)
 select execArg, source, sink, "$@ flows to here and is used in a command.", source.getNode(),
   "User-provided value"

java/ql/src/Security/CWE/CWE-078/ExecTaintedLocal.ql

@@ -14,6 +14,7 @@
 import semmle.code.java.Expr
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.security.ExternalProcess
+import semmle.code.java.security.CommandArguments
 import DataFlow::PathGraph
 
 class LocalUserInputToArgumentToExecFlowConfig extends TaintTracking::Configuration {
@@ -24,12 +25,16 @@ class LocalUserInputToArgumentToExecFlowConfig extends TaintTracking::Configurat
   override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof ArgumentToExec }
 
   override predicate isSanitizer(DataFlow::Node node) {
-    node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType
+    node.getType() instanceof PrimitiveType
+    or
+    node.getType() instanceof BoxedType
+    or
+    isSafeCommandArgument(node.asExpr())
   }
 }
 
 from
-  DataFlow::PathNode source, DataFlow::PathNode sink, StringArgumentToExec execArg,
+  DataFlow::PathNode source, DataFlow::PathNode sink, ArgumentToExec execArg,
   LocalUserInputToArgumentToExecFlowConfig conf
 where conf.hasFlowPath(source, sink) and sink.getNode().asExpr() = execArg
 select execArg, source, sink, "$@ flows to here and is used in a command.", source.getNode(),

java/ql/src/semmle/code/java/security/CommandArguments.qll

@@ -0,0 +1,194 @@
+/**
+ * Definitions for reasoning about lists and arrays that are to be used as arguments to an external process.
+ */
+
+import java
+import semmle.code.java.dataflow.SSA
+import semmle.code.java.Collections
+
+/**
+ * Holds if `ex` is used safely as an argument to a command;
+ * i.e. it's not in the first position and it's not a shell command.
+ */
+predicate isSafeCommandArgument(Expr ex) {
+  exists(ArrayInit ai, int i |
+    ex = ai.getInit(i) and
+    i > 0 and
+    not isShell(ai.getInit(0))
+  )
+  or
+  exists(CommandArgumentList cal |
+    not cal.isShell() and
+    ex = cal.getASubsequentAdd().getArgument(0)
+  )
+  or
+  exists(CommandArgArrayImmutableFirst caa |
+    not caa.isShell() and
+    ex = caa.getAWrite(any(int i | i > 0))
+  )
+}
+
+/**
+ * Holds if the given expression is the name of a shell command such as bash or python
+ */
+private predicate isShell(Expr ex) {
+  exists(string cmd | cmd = ex.(StringLiteral).getValue() |
+    cmd.regexpMatch(".*(sh|javac?|python[23]?|osascript|cmd)(\\.exe)?$")
+  )
+  or
+  exists(SsaVariable ssa |
+    ex = ssa.getAUse() and
+    isShell(ssa.getAnUltimateDefinition().(SsaExplicitUpdate).getDefiningExpr())
+  )
+  or
+  isShell(ex.(Assignment).getRhs())
+  or
+  isShell(ex.(LocalVariableDeclExpr).getInit())
+}
+
+/**
+ * A type that could be a list of strings. Includes raw `List` types.
+ */
+private class ListOfStringType extends CollectionType {
+  ListOfStringType() {
+    this.getSourceDeclaration().getASourceSupertype*().hasQualifiedName("java.util", "List") and
+    this.getElementType().getASubtype*() instanceof TypeString
+  }
+}
+
+/**
+ * A variable that could be used as a list of arguments to a command.
+ */
+private class CommandArgumentList extends SsaExplicitUpdate {
+  CommandArgumentList() {
+    this.getSourceVariable().getType() instanceof ListOfStringType and
+    forex(CollectionMutation ma | ma.getQualifier() = this.getAUse() |
+      ma.getMethod().getName().matches("add%")
+    )
+  }
+
+  /** Gets a use of the variable for which the list could be empty. */
+  private RValue getAUseBeforeFirstAdd() {
+    result = getAFirstUse()
+    or
+    exists(RValue mid |
+      mid = getAUseBeforeFirstAdd() and
+      adjacentUseUse(mid, result) and
+      not exists(MethodAccess ma |
+        mid = ma.getQualifier() and
+        ma.getMethod().hasName("add")
+      )
+    )
+  }
+
+  /**
+   * Gets an addition to this list, i.e. a call to an `add` or `addAll` method.
+   */
+  MethodAccess getAnAdd() {
+    result.getQualifier() = getAUse() and
+    result.getMethod().getName().matches("add%")
+  }
+
+  /** Gets an addition to this list which could be its first element. */
+  MethodAccess getAFirstAdd() {
+    result = getAnAdd() and
+    result.getQualifier() = getAUseBeforeFirstAdd()
+  }
+
+  /** Gets an addition to this list which is not the first element. */
+  MethodAccess getASubsequentAdd() {
+    result = getAnAdd() and
+    not result = getAFirstAdd()
+  }
+
+  /** Holds if the first element of this list is a shell command. */
+  predicate isShell() {
+    exists(MethodAccess ma | ma = getAFirstAdd() and isShell(ma.getArgument(0)))
+  }
+}
+
+/**
+ * The type `String[]`.
+ */
+private class ArrayOfStringType extends Array {
+  ArrayOfStringType() { this.getElementType() instanceof TypeString }
+}
+
+private predicate arrayLValue(ArrayAccess acc) { exists(Assignment a | a.getDest() = acc) }
+
+/**
+ * A variable that could be an array of arguments to a command.
+ */
+private class CommandArgumentArray extends SsaExplicitUpdate {
+  CommandArgumentArray() {
+    this.getSourceVariable().getType() instanceof ArrayOfStringType and
+    forall(ArrayAccess a | a.getArray() = getAUse() and arrayLValue(a) |
+      a.getIndexExpr() instanceof CompileTimeConstantExpr
+    )
+  }
+
+  /** Gets an expression that is written to the given index of this array at the given use. */
+  Expr getAWrite(int index, RValue use) {
+    exists(Assignment a, ArrayAccess acc |
+      acc.getArray() = use and
+      use = this.getAUse() and
+      index = acc.getIndexExpr().(CompileTimeConstantExpr).getIntValue() and
+      acc = a.getDest() and
+      result = a.getRhs()
+    )
+  }
+
+  /** Gets an expression that is written to the given index of this array. */
+  Expr getAWrite(int index) { result = getAWrite(index, _) }
+}
+
+/**
+ * A `CommandArgArray` whose element at index 0 is never written to, except possibly once to initialise it.
+ */
+private class CommandArgArrayImmutableFirst extends CommandArgumentArray {
+  CommandArgArrayImmutableFirst() {
+    (exists(getAWrite(0)) or exists(firstElementOf(this.getDefiningExpr()))) and
+    forall(RValue use | exists(this.getAWrite(0, use)) | use = this.getAFirstUse())
+  }
+
+  /** Gets the first element of this array. */
+  Expr getFirstElement() {
+    result = getAWrite(0)
+    or
+    not exists(getAWrite(0)) and
+    result = firstElementOf(getDefiningExpr())
+  }
+
+  /** Holds if the first element of this array is a shell command. */
+  predicate isShell() { isShell(getFirstElement()) }
+}
+
+/** Gets the first element of an imutable array of strings */
+private Expr firstElementOf(Expr arr) {
+  arr.getType() instanceof ArrayOfStringType and
+  (
+    result = firstElementOf(arr.(Assignment).getRhs())
+    or
+    result = firstElementOf(arr.(LocalVariableDeclExpr).getInit())
+    or
+    exists(CommandArgArrayImmutableFirst caa | arr = caa.getAUse() | result = caa.getFirstElement())
+    or
+    exists(MethodAccess ma, Method m |
+      arr = ma and
+      ma.getMethod() = m and
+      m.getDeclaringType().hasQualifiedName("java.util", "Arrays") and
+      m.hasName("copyOf") and
+      result = firstElementOf(ma.getArgument(0))
+    )
+    or
+    exists(Field f |
+      f.isStatic() and
+      arr.(FieldRead).getField() = f and
+      result = firstElementOf(f.getInitializer())
+    )
+    or
+    result = arr.(ArrayInit).getInit(0)
+    or
+    result = arr.(ArrayCreationExpr).getInit().getInit(0)
+  )
+}

java/ql/test/query-tests/security/CWE-078/ExecRelative.expected

@@ -0,0 +1 @@
+| Test.java:50:46:50:49 | "ls" | Command with a relative path 'ls' is executed. |

java/ql/test/query-tests/security/CWE-078/ExecRelative.qlref

@@ -0,0 +1 @@
+Security/CWE/CWE-078/ExecRelative.ql

java/ql/test/query-tests/security/CWE-078/ExecTaintedLocal.expected

@@ -0,0 +1,27 @@
+edges
+| Test.java:6:35:6:44 | arg : String | Test.java:7:44:7:69 | ... + ... |
+| Test.java:6:35:6:44 | arg : String | Test.java:10:29:10:74 | new String[] |
+| Test.java:6:35:6:44 | arg : String | Test.java:18:29:18:31 | cmd |
+| Test.java:6:35:6:44 | arg : String | Test.java:24:29:24:32 | cmd1 |
+| Test.java:28:38:28:47 | arg : String | Test.java:29:44:29:64 | ... + ... |
+| Test.java:57:27:57:39 | args : String[] | Test.java:60:20:60:22 | arg : String |
+| Test.java:57:27:57:39 | args : String[] | Test.java:61:23:61:25 | arg : String |
+| Test.java:60:20:60:22 | arg : String | Test.java:6:35:6:44 | arg : String |
+| Test.java:61:23:61:25 | arg : String | Test.java:28:38:28:47 | arg : String |
+nodes
+| Test.java:6:35:6:44 | arg : String | semmle.label | arg : String |
+| Test.java:7:44:7:69 | ... + ... | semmle.label | ... + ... |
+| Test.java:10:29:10:74 | new String[] | semmle.label | new String[] |
+| Test.java:18:29:18:31 | cmd | semmle.label | cmd |
+| Test.java:24:29:24:32 | cmd1 | semmle.label | cmd1 |
+| Test.java:28:38:28:47 | arg : String | semmle.label | arg : String |
+| Test.java:29:44:29:64 | ... + ... | semmle.label | ... + ... |
+| Test.java:57:27:57:39 | args : String[] | semmle.label | args : String[] |
+| Test.java:60:20:60:22 | arg : String | semmle.label | arg : String |
+| Test.java:61:23:61:25 | arg : String | semmle.label | arg : String |
+#select
+| Test.java:7:44:7:69 | ... + ... | Test.java:57:27:57:39 | args : String[] | Test.java:7:44:7:69 | ... + ... | $@ flows to here and is used in a command. | Test.java:57:27:57:39 | args | User-provided value |
+| Test.java:10:29:10:74 | new String[] | Test.java:57:27:57:39 | args : String[] | Test.java:10:29:10:74 | new String[] | $@ flows to here and is used in a command. | Test.java:57:27:57:39 | args | User-provided value |
+| Test.java:18:29:18:31 | cmd | Test.java:57:27:57:39 | args : String[] | Test.java:18:29:18:31 | cmd | $@ flows to here and is used in a command. | Test.java:57:27:57:39 | args | User-provided value |
+| Test.java:24:29:24:32 | cmd1 | Test.java:57:27:57:39 | args : String[] | Test.java:24:29:24:32 | cmd1 | $@ flows to here and is used in a command. | Test.java:57:27:57:39 | args | User-provided value |
+| Test.java:29:44:29:64 | ... + ... | Test.java:57:27:57:39 | args : String[] | Test.java:29:44:29:64 | ... + ... | $@ flows to here and is used in a command. | Test.java:57:27:57:39 | args | User-provided value |

java/ql/test/query-tests/security/CWE-078/ExecTaintedLocal.qlref

@@ -0,0 +1 @@
+Security/CWE/CWE-078/ExecTaintedLocal.ql

java/ql/test/query-tests/security/CWE-078/ExecUnescaped.expected

@@ -0,0 +1,2 @@
+| Test.java:7:44:7:69 | ... + ... | Command line is built with string concatenation. |
+| Test.java:29:44:29:64 | ... + ... | Command line is built with string concatenation. |

java/ql/test/query-tests/security/CWE-078/ExecUnescaped.qlref

@@ -0,0 +1 @@
+Security/CWE/CWE-078/ExecUnescaped.ql

java/ql/test/query-tests/security/CWE-078/Test.java

@@ -0,0 +1,64 @@
+import java.lang.ProcessBuilder;
+import java.util.List;
+import java.util.ArrayList;
+
+class Test {
+  public static void shellCommand(String arg) {
+    ProcessBuilder pb = new ProcessBuilder("/bin/bash -c echo " + arg);
+    pb.start();
+
+    pb = new ProcessBuilder(new String[]{"/bin/bash", "-c", "echo " + arg});
+    pb.start();
+
+    List<String> cmd = new ArrayList<String>();
+    cmd.add("/bin/bash");
+    cmd.add("-c");
+    cmd.add("echo " + arg);
+
+    pb = new ProcessBuilder(cmd);
+    pb.start();
+
+    String[] cmd1 = new String[]{"/bin/bash", "-c", "<cmd>"};
+    cmd1[1] = "echo " + arg;
+
+    pb = new ProcessBuilder(cmd1);
+    pb.start();
+  }
+
+  public static void nonShellCommand(String arg) {
+    ProcessBuilder pb = new ProcessBuilder("./customTool " + arg);
+    pb.start();
+
+    pb = new ProcessBuilder(new String[]{"./customTool", arg});
+    pb.start();
+
+    List<String> cmd = new ArrayList<String>();
+    cmd.add("./customTool");
+    cmd.add(arg);
+
+    pb = new ProcessBuilder(cmd);
+    pb.start();
+
+    String[] cmd1 = new String[]{"./customTool", "<arg>"};
+    cmd1[1] = arg;
+
+    pb = new ProcessBuilder(cmd1);
+    pb.start();
+  }
+
+  public static void relativeCommand() {
+      ProcessBuilder pb = new ProcessBuilder("ls");
+      pb.start();
+
+      pb = new ProcessBuilder("/bin/ls");
+      pb.start();
+  }
+
+  public static void main(String[] args) {
+      String arg = args.length > 1 ? args[1] : "default";
+
+      shellCommand(arg);
+      nonShellCommand(arg);
+      relativeCommand();
+  }
+}