Fix up for code review

Author: aegilops

java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExec.ql

@@ -14,12 +14,10 @@
 import CommandInjectionRuntimeExec
 import ExecUserFlow::PathGraph
 
-class RemoteSource extends Source instanceof RemoteFlowSource { }
-
 from
-  ExecUserFlow::PathNode source, ExecUserFlow::PathNode sink, MethodAccess call,
-  DataFlow::Node sourceCmd, DataFlow::Node sinkCmd
-where callIsTaintedByUserInputAndDangerousCommand(call, source, sink, sourceCmd, sinkCmd)
+  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()

java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExec.qll

@@ -3,108 +3,51 @@ import semmle.code.java.frameworks.javaee.ejb.EJBRestrictions
 import semmle.code.java.dataflow.DataFlow
 import semmle.code.java.dataflow.FlowSources
 
-// a static string of an unsafe executable tainting arg 0 of Runtime.exec()
-deprecated class ExecTaintConfiguration extends TaintTracking::Configuration {
-  ExecTaintConfiguration() { this = "ExecTaintConfiguration" }
-
-  override predicate isSource(DataFlow::Node source) {
-    source.asExpr() instanceof StringLiteral and
-    source.asExpr().(StringLiteral).getValue() instanceof UnSafeExecutable
-  }
-
-  override 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
-    )
-  }
-
-  override predicate isSanitizer(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
-    )
-  }
-}
-
 module ExecCmdFlowConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
-    source.asExpr() instanceof StringLiteral and
-    source.asExpr().(StringLiteral).getValue() instanceof UnSafeExecutable
+    source.asExpr().(CompileTimeConstantExpr).getStringValue() instanceof UnSafeExecutable
   }
 
   predicate isSink(DataFlow::Node sink) {
-    exists(RuntimeExecMethod method, MethodAccess call |
-      call.getMethod() = method and
+    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.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
-    )
+    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
-deprecated class ExecTaintConfiguration2 extends TaintTracking::Configuration {
-  ExecTaintConfiguration2() { this = "ExecTaintConfiguration2" }
+abstract class Source extends DataFlow::Node { }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof Source }
-
-  override predicate isSink(DataFlow::Node sink) {
-    exists(RuntimeExecMethod method, MethodAccess call |
-      call.getMethod() = method and
-      sink.asExpr() = call.getArgument(_) and
-      sink.asExpr().getType() instanceof Array
-    )
-  }
+class RemoteSource extends Source instanceof RemoteFlowSource { }
 
-  override predicate isSanitizer(DataFlow::Node node) {
-    node.asExpr().getFile().isSourceFile() and
-    (
-      node.getType() instanceof PrimitiveType or
-      node.getType() instanceof BoxedType
-    )
-  }
-}
+class LocalSource extends Source instanceof LocalUserInput { }
 
 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
+    exists(MethodAccess call |
+      call.getMethod() instanceof RuntimeExecMethod 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
-    )
+    node.getType() instanceof PrimitiveType or
+    node.getType() instanceof BoxedType
   }
 }
 
@@ -116,7 +59,7 @@ class AssignToNonZeroIndex extends DataFlow::Node {
   AssignToNonZeroIndex() {
     exists(AssignExpr assign, ArrayAccess access |
       assign.getDest() = access and
-      access.getIndexExpr().(IntegerLiteral).getValue() != "0" and
+      access.getIndexExpr().(IntegerLiteral).getValue().toInt() != 0 and
       assign.getSource() = this.asExpr()
     )
   }
@@ -143,7 +86,7 @@ class StreamConcatAtNonZeroIndex extends DataFlow::Node {
   }
 }
 
-// allow list of executables that execute their arguments
+// list of executables that execute their arguments
 // TODO: extend with data extensions
 class UnSafeExecutable extends string {
   bindingset[this]
@@ -154,17 +97,15 @@ class UnSafeExecutable extends string {
 }
 
 predicate callIsTaintedByUserInputAndDangerousCommand(
-  MethodAccess call, ExecUserFlow::PathNode source, ExecUserFlow::PathNode sink,
-  DataFlow::Node sourceCmd, DataFlow::Node sinkCmd
+  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", ...})
-  (
+  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
-  (
+    sinkCmd.asExpr() = call.getArgument(0) and
+    // it is tainted by untrusted user input
     ExecUserFlow::flowPath(source, sink) and
     sink.getNode().asExpr() = call.getArgument(0)
   )

java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExecLocal.ql

@@ -15,12 +15,10 @@
 import CommandInjectionRuntimeExec
 import ExecUserFlow::PathGraph
 
-class LocalSource extends Source instanceof LocalUserInput { }
-
 from
-  ExecUserFlow::PathNode source, ExecUserFlow::PathNode sink, MethodAccess call,
-  DataFlow::Node sourceCmd, DataFlow::Node sinkCmd
-where callIsTaintedByUserInputAndDangerousCommand(call, source, sink, sourceCmd, sinkCmd)
+  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()