Merge pull request github#12698 from egregius313/egregius313/java/refactor-commandline-query-and-request-forgery

Java: Refactor CommandLineQuery.qll and RequestForgeryConfig.qll

Author: egregius313

java/ql/lib/change-notes/2023-03-28-deprecated-exectainted.md

@@ -0,0 +1,5 @@
+---
+category: deprecated
+---
+* The `execTainted` predicate in `CommandLineQuery.qll` has been deprecated and replaced with the predicate `execIsTainted`.
+

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

@@ -12,9 +12,11 @@ import semmle.code.java.security.ExternalProcess
 import semmle.code.java.security.CommandArguments
 
 /**
+ * DEPRECATED: Use `RemoteUserInputToArgumentToExecFlow` instead.
+ *
  * A taint-tracking configuration for unvalidated user input that is used to run an external process.
  */
-class RemoteUserInputToArgumentToExecFlowConfig extends TaintTracking::Configuration {
+deprecated class RemoteUserInputToArgumentToExecFlowConfig extends TaintTracking::Configuration {
   RemoteUserInputToArgumentToExecFlowConfig() {
     this = "ExecCommon::RemoteUserInputToArgumentToExecFlowConfig"
   }
@@ -33,12 +35,52 @@ class RemoteUserInputToArgumentToExecFlowConfig extends TaintTracking::Configura
 }
 
 /**
+ * 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.
  */
-predicate execTainted(DataFlow::PathNode source, DataFlow::PathNode sink, ArgumentToExec execArg) {
+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/lib/semmle/code/java/security/RequestForgeryConfig.qll

@@ -35,7 +35,7 @@ deprecated class RequestForgeryConfiguration extends TaintTracking::Configuratio
 /**
  * A taint-tracking configuration characterising request-forgery risks.
  */
-private module RequestForgeryConfig implements DataFlow::ConfigSig {
+module RequestForgeryConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
     source instanceof RemoteFlowSource and
     // Exclude results of remote HTTP requests: fetching something else based on that result

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

@@ -16,9 +16,11 @@ import java
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.security.ExternalProcess
 import semmle.code.java.security.CommandLineQuery
-import DataFlow::PathGraph
+import RemoteUserInputToArgumentToExecFlow::PathGraph
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, ArgumentToExec execArg
-where execTainted(source, sink, execArg)
+from
+  RemoteUserInputToArgumentToExecFlow::PathNode source,
+  RemoteUserInputToArgumentToExecFlow::PathNode sink, ArgumentToExec 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/ExecUnescaped.ql

@@ -48,5 +48,5 @@ predicate builtFromUncontrolledConcat(Expr expr) {
 from StringArgumentToExec argument
 where
   builtFromUncontrolledConcat(argument) and
-  not execTainted(_, _, argument)
+  not execIsTainted(_, _, argument)
 select argument, "Command line is built with string concatenation."

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

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