Merge pull request #12685 from atorralba/atorralba/java/command-injection-mad

Java: Add command-injection sink kind and refactor command injection queries

java/ql/lib/change-notes/2023-03-28-cmdi-queries-models-as-data.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* A new models as data sink kind `command-injection` has been added.
+* The queries `java/command-line-injection` and `java/concatenated-command-line` now can be extended using the `command-injection` models as data sink kind.

java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll

@@ -272,7 +272,7 @@ module ModelValidation {
           "groovy", "xss", "ognl-injection", "intent-start", "pending-intent-sent",
           "url-open-stream", "url-redirect", "create-file", "read-file", "write-file",
           "set-hostname-verifier", "header-splitting", "information-leak", "xslt", "jexl",
-          "bean-validation", "ssti", "fragment-injection"
+          "bean-validation", "ssti", "fragment-injection", "command-injection"
         ] and
       not kind.matches("regex-use%") and
       not kind.matches("qltest%") and

java/ql/lib/semmle/code/java/security/CommandLineQuery.qll

@@ -7,9 +7,114 @@
  * query.
  */
 
-import semmle.code.java.dataflow.FlowSources
-import semmle.code.java.security.ExternalProcess
-import semmle.code.java.security.CommandArguments
+import java
+private import semmle.code.java.dataflow.FlowSources
+private import semmle.code.java.dataflow.ExternalFlow
+private import semmle.code.java.security.ExternalProcess
+private import semmle.code.java.security.CommandArguments
+
+/** A sink for command injection vulnerabilities. */
+abstract class CommandInjectionSink extends DataFlow::Node { }
+
+/** A sanitizer for command injection vulnerabilities. */
+abstract class CommandInjectionSanitizer extends DataFlow::Node { }
+
+/**
+ * A unit class for adding additional taint steps.
+ *
+ * Extend this class to add additional taint steps that should apply to configurations related to command injection.
+ */
+class CommandInjectionAdditionalTaintStep extends Unit {
+  /**
+   * Holds if the step from `node1` to `node2` should be considered a taint
+   * step for configurations related to command injection.
+   */
+  abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
+}
+
+private class DefaultCommandInjectionSink extends CommandInjectionSink {
+  DefaultCommandInjectionSink() {
+    this.asExpr() instanceof ArgumentToExec or sinkNode(this, "command-injection")
+  }
+}
+
+private class DefaultCommandInjectionSanitizer extends CommandInjectionSanitizer {
+  DefaultCommandInjectionSanitizer() {
+    this.getType() instanceof PrimitiveType
+    or
+    this.getType() instanceof BoxedType
+    or
+    isSafeCommandArgument(this.asExpr())
+  }
+}
+
+/**
+ * A taint-tracking configuration for unvalidated user input that is used to run an external process.
+ */
+module RemoteUserInputToArgumentToExecFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof CommandInjectionSink }
+
+  predicate isBarrier(DataFlow::Node node) { node instanceof CommandInjectionSanitizer }
+
+  predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
+    any(CommandInjectionAdditionalTaintStep s).step(n1, n2)
+  }
+}
+
+/**
+ * Taint-tracking flow for unvalidated user input that is used to run an external process.
+ */
+module RemoteUserInputToArgumentToExecFlow =
+  TaintTracking::Global<RemoteUserInputToArgumentToExecFlowConfig>;
+
+/**
+ * A taint-tracking configuration for unvalidated local user input that is used to run an external process.
+ */
+module LocalUserInputToArgumentToExecFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node src) { src instanceof LocalUserInput }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof CommandInjectionSink }
+
+  predicate isBarrier(DataFlow::Node node) { node instanceof CommandInjectionSanitizer }
+
+  predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
+    any(CommandInjectionAdditionalTaintStep s).step(n1, n2)
+  }
+}
+
+/**
+ * Taint-tracking flow for unvalidated local user input that is used to run an external process.
+ */
+module LocalUserInputToArgumentToExecFlow =
+  TaintTracking::Global<LocalUserInputToArgumentToExecFlowConfig>;
+
+/**
+ * Implementation of `ExecTainted.ql`. It is extracted to a QLL
+ * so that it can be excluded from `ExecUnescaped.ql` to avoid
+ * reporting overlapping results.
+ */
+predicate execIsTainted(
+  RemoteUserInputToArgumentToExecFlow::PathNode source,
+  RemoteUserInputToArgumentToExecFlow::PathNode sink, Expr execArg
+) {
+  RemoteUserInputToArgumentToExecFlow::flowPath(source, sink) and
+  sink.getNode().asExpr() = execArg
+}
+
+/**
+ * DEPRECATED: Use `execIsTainted` instead.
+ *
+ * Implementation of `ExecTainted.ql`. It is extracted to a QLL
+ * so that it can be excluded from `ExecUnescaped.ql` to avoid
+ * reporting overlapping results.
+ */
+deprecated predicate execTainted(DataFlow::PathNode source, DataFlow::PathNode sink, Expr execArg) {
+  exists(RemoteUserInputToArgumentToExecFlowConfig conf |
+    conf.hasFlowPath(source, sink) and sink.getNode().asExpr() = execArg
+  )
+}
 
 /**
  * DEPRECATED: Use `RemoteUserInputToArgumentToExecFlow` instead.
@@ -23,64 +128,11 @@ deprecated class RemoteUserInputToArgumentToExecFlowConfig extends TaintTracking
 
   override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
 
-  override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof ArgumentToExec }
+  override predicate isSink(DataFlow::Node sink) { sink instanceof CommandInjectionSink }
 
-  override predicate isSanitizer(DataFlow::Node node) {
-    node.getType() instanceof PrimitiveType
-    or
-    node.getType() instanceof BoxedType
-    or
-    isSafeCommandArgument(node.asExpr())
+  override predicate isSanitizer(DataFlow::Node node) { node instanceof CommandInjectionSanitizer }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
+    any(CommandInjectionAdditionalTaintStep s).step(n1, n2)
   }
 }
-
-/**
- * A taint-tracking configuration for unvalidated user input that is used to run an external process.
- */
-module RemoteUserInputToArgumentToExecFlowConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
-
-  predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof ArgumentToExec }
-
-  predicate isBarrier(DataFlow::Node node) {
-    node.getType() instanceof PrimitiveType
-    or
-    node.getType() instanceof BoxedType
-    or
-    isSafeCommandArgument(node.asExpr())
-  }
-}
-
-/**
- * Taint-tracking flow for unvalidated user input that is used to run an external process.
- */
-module RemoteUserInputToArgumentToExecFlow =
-  TaintTracking::Global<RemoteUserInputToArgumentToExecFlowConfig>;
-
-/**
- * DEPRECATED: Use `execIsTainted` instead.
- *
- * Implementation of `ExecTainted.ql`. It is extracted to a QLL
- * so that it can be excluded from `ExecUnescaped.ql` to avoid
- * reporting overlapping results.
- */
-deprecated predicate execTainted(
-  DataFlow::PathNode source, DataFlow::PathNode sink, ArgumentToExec execArg
-) {
-  exists(RemoteUserInputToArgumentToExecFlowConfig conf |
-    conf.hasFlowPath(source, sink) and sink.getNode() = DataFlow::exprNode(execArg)
-  )
-}
-
-/**
- * Implementation of `ExecTainted.ql`. It is extracted to a QLL
- * so that it can be excluded from `ExecUnescaped.ql` to avoid
- * reporting overlapping results.
- */
-predicate execIsTainted(
-  RemoteUserInputToArgumentToExecFlow::PathNode source,
-  RemoteUserInputToArgumentToExecFlow::PathNode sink, ArgumentToExec execArg
-) {
-  RemoteUserInputToArgumentToExecFlow::flowPath(source, sink) and
-  sink.getNode() = DataFlow::exprNode(execArg)
-}

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

@@ -13,14 +13,12 @@
  */
 
 import java
-import semmle.code.java.dataflow.FlowSources
-import semmle.code.java.security.ExternalProcess
 import semmle.code.java.security.CommandLineQuery
 import RemoteUserInputToArgumentToExecFlow::PathGraph
 
 from
   RemoteUserInputToArgumentToExecFlow::PathNode source,
-  RemoteUserInputToArgumentToExecFlow::PathNode sink, ArgumentToExec execArg
+  RemoteUserInputToArgumentToExecFlow::PathNode sink, Expr execArg
 where execIsTainted(source, sink, execArg)
 select execArg, source, sink, "This command line depends on a $@.", source.getNode(),
   "user-provided value"

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

@@ -12,35 +12,12 @@
  *       external/cwe/cwe-088
  */
 
-import semmle.code.java.Expr
-import semmle.code.java.dataflow.FlowSources
-import semmle.code.java.security.ExternalProcess
-import semmle.code.java.security.CommandArguments
-
-module LocalUserInputToArgumentToExecFlowConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node src) { src instanceof LocalUserInput }
-
-  predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof ArgumentToExec }
-
-  predicate isBarrier(DataFlow::Node node) {
-    node.getType() instanceof PrimitiveType
-    or
-    node.getType() instanceof BoxedType
-    or
-    isSafeCommandArgument(node.asExpr())
-  }
-}
-
-module LocalUserInputToArgumentToExecFlow =
-  TaintTracking::Global<LocalUserInputToArgumentToExecFlowConfig>;
-
+import semmle.code.java.security.CommandLineQuery
 import LocalUserInputToArgumentToExecFlow::PathGraph
 
 from
   LocalUserInputToArgumentToExecFlow::PathNode source,
-  LocalUserInputToArgumentToExecFlow::PathNode sink, ArgumentToExec execArg
-where
-  LocalUserInputToArgumentToExecFlow::flowPath(source, sink) and
-  sink.getNode().asExpr() = execArg
-select execArg, source, sink, "This command line depends on a $@.", source.getNode(),
-  "user-provided value"
+  LocalUserInputToArgumentToExecFlow::PathNode sink
+where LocalUserInputToArgumentToExecFlow::flowPath(source, sink)
+select sink.getNode().asExpr(), source, sink, "This command line depends on a $@.",
+  source.getNode(), "user-provided value"

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

@@ -13,7 +13,6 @@
  */
 
 import java
-import semmle.code.java.security.ExternalProcess
 import semmle.code.java.security.CommandLineQuery
 
 /**

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

@@ -13,16 +13,14 @@
  */
 
 import java
-import semmle.code.java.dataflow.FlowSources
-import semmle.code.java.security.ExternalProcess
 import semmle.code.java.security.CommandLineQuery
-import JSchOSInjection
 import RemoteUserInputToArgumentToExecFlow::PathGraph
+import JSchOSInjection
 
 // This is a clone of query `java/command-line-injection` that also includes experimental sinks.
 from
   RemoteUserInputToArgumentToExecFlow::PathNode source,
-  RemoteUserInputToArgumentToExecFlow::PathNode sink, ArgumentToExec execArg
+  RemoteUserInputToArgumentToExecFlow::PathNode sink, Expr execArg
 where execIsTainted(source, sink, execArg)
 select execArg, source, sink, "This command line depends on a $@.", source.getNode(),
   "user-provided value"