Merge pull request #8491 from jketema/command-line-injection-with-flow-state

C++: Use flow states in `cpp/command-line-injection`

Author: MathiasVP

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

@@ -19,9 +19,9 @@ import semmle.code.cpp.security.Security
 import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 import semmle.code.cpp.ir.IR
 import semmle.code.cpp.ir.dataflow.TaintTracking
-import semmle.code.cpp.ir.dataflow.TaintTracking2
 import semmle.code.cpp.security.FlowSources
 import semmle.code.cpp.models.implementations.Strcat
+import DataFlow::PathGraph
 
 Expr sinkAsArgumentIndirection(DataFlow::Node sink) {
   result =
@@ -66,154 +66,70 @@ predicate interestingConcatenation(DataFlow::Node fst, DataFlow::Node snd) {
   )
 }
 
-class TaintToConcatenationConfiguration extends TaintTracking::Configuration {
-  TaintToConcatenationConfiguration() { this = "TaintToConcatenationConfiguration" }
-
-  override predicate isSource(DataFlow::Node source) { source instanceof FlowSource }
-
-  override predicate isSink(DataFlow::Node sink) { interestingConcatenation(sink, _) }
-
-  override predicate isSanitizer(DataFlow::Node node) {
-    node.asInstruction().getResultType() instanceof IntegralType
-    or
-    node.asInstruction().getResultType() instanceof FloatingPointType
-  }
+class ConcatState extends DataFlow::FlowState {
+  ConcatState() { this = "ConcatState" }
 }
 
-class ExecTaintConfiguration extends TaintTracking2::Configuration {
-  ExecTaintConfiguration() { this = "ExecTaintConfiguration" }
+class ExecState extends DataFlow::FlowState {
+  DataFlow::Node fst;
+  DataFlow::Node snd;
 
-  override predicate isSource(DataFlow::Node source) {
-    exists(DataFlow::Node prevSink, TaintToConcatenationConfiguration conf |
-      conf.hasFlow(_, prevSink) and
-      interestingConcatenation(prevSink, source)
-    )
+  ExecState() {
+    this =
+      "ExecState (" + fst.getLocation() + " | " + fst + ", " + snd.getLocation() + " | " + snd + ")" and
+    interestingConcatenation(fst, snd)
   }
 
-  override predicate isSink(DataFlow::Node sink) {
-    shellCommand(sinkAsArgumentIndirection(sink), _)
-  }
+  DataFlow::Node getFstNode() { result = fst }
 
-  override predicate isSanitizerOut(DataFlow::Node node) {
-    isSink(node) // Prevent duplicates along a call chain, since `shellCommand` will include wrappers
-  }
+  DataFlow::Node getSndNode() { result = snd }
 }
 
-module StitchedPathGraph {
-  // There's a different PathNode class for each DataFlowImplN.qll, so we can't simply combine the
-  // PathGraph predicates directly. Instead, we use a newtype so there's a single type that
-  // contains both sets of PathNodes.
-  newtype TMergedPathNode =
-    TPathNode1(DataFlow::PathNode node) or
-    TPathNode2(DataFlow2::PathNode node)
-
-  // this wraps the toString and location predicates so we can use the merged node type in a
-  // selection
-  class MergedPathNode extends TMergedPathNode {
-    string toString() {
-      exists(DataFlow::PathNode n |
-        this = TPathNode1(n) and
-        result = n.toString()
-      )
-      or
-      exists(DataFlow2::PathNode n |
-        this = TPathNode2(n) and
-        result = n.toString()
-      )
-    }
-
-    DataFlow::Node getNode() {
-      exists(DataFlow::PathNode n |
-        this = TPathNode1(n) and
-        result = n.getNode()
-      )
-      or
-      exists(DataFlow2::PathNode n |
-        this = TPathNode2(n) and
-        result = n.getNode()
-      )
-    }
-
-    DataFlow::PathNode getPathNode1() { this = TPathNode1(result) }
+class ExecTaintConfiguration extends TaintTracking::Configuration {
+  ExecTaintConfiguration() { this = "ExecTaintConfiguration" }
 
-    DataFlow2::PathNode getPathNode2() { this = TPathNode2(result) }
+  override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
+    source instanceof FlowSource and
+    state instanceof ConcatState
+  }
 
-    predicate hasLocationInfo(
-      string filepath, int startline, int startcolumn, int endline, int endcolumn
-    ) {
-      exists(DataFlow::PathNode n |
-        this = TPathNode1(n) and
-        n.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
-      )
-      or
-      exists(DataFlow2::PathNode n |
-        this = TPathNode2(n) and
-        n.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
-      )
-    }
+  override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
+    shellCommand(sinkAsArgumentIndirection(sink), _) and
+    state instanceof ExecState
   }
 
-  query predicate edges(MergedPathNode a, MergedPathNode b) {
-    exists(DataFlow::PathNode an, DataFlow::PathNode bn |
-      a = TPathNode1(an) and
-      b = TPathNode1(bn) and
-      DataFlow::PathGraph::edges(an, bn)
-    )
-    or
-    exists(DataFlow2::PathNode an, DataFlow2::PathNode bn |
-      a = TPathNode2(an) and
-      b = TPathNode2(bn) and
-      DataFlow2::PathGraph::edges(an, bn)
-    )
-    or
-    // This is where paths from the two configurations are connected. `interestingConcatenation`
-    // is the only thing in this module that's actually specific to the query - everything else is
-    // just using types and predicates from the DataFlow library.
-    interestingConcatenation(a.getNode(), b.getNode()) and
-    a instanceof TPathNode1 and
-    b instanceof TPathNode2
+  override predicate isAdditionalTaintStep(
+    DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
+    DataFlow::FlowState state2
+  ) {
+    state1 instanceof ConcatState and
+    state2.(ExecState).getFstNode() = node1 and
+    state2.(ExecState).getSndNode() = node2
   }
 
-  query predicate nodes(MergedPathNode mpn, string key, string val) {
-    // here we just need the union of the underlying `nodes` predicates
-    exists(DataFlow::PathNode n |
-      mpn = TPathNode1(n) and
-      DataFlow::PathGraph::nodes(n, key, val)
-    )
-    or
-    exists(DataFlow2::PathNode n |
-      mpn = TPathNode2(n) and
-      DataFlow2::PathGraph::nodes(n, key, val)
-    )
+  override predicate isSanitizer(DataFlow::Node node, DataFlow::FlowState state) {
+    (
+      node.asInstruction().getResultType() instanceof IntegralType
+      or
+      node.asInstruction().getResultType() instanceof FloatingPointType
+    ) and
+    state instanceof ConcatState
   }
 
-  query predicate subpaths(
-    MergedPathNode arg, MergedPathNode par, MergedPathNode ret, MergedPathNode out
-  ) {
-    // just forward subpaths from the underlying libraries. This might be slightly awkward when
-    // the concatenation is deep in a call chain.
-    DataFlow::PathGraph::subpaths(arg.getPathNode1(), par.getPathNode1(), ret.getPathNode1(),
-      out.getPathNode1())
-    or
-    DataFlow2::PathGraph::subpaths(arg.getPathNode2(), par.getPathNode2(), ret.getPathNode2(),
-      out.getPathNode2())
+  override predicate isSanitizerOut(DataFlow::Node node, DataFlow::FlowState state) {
+    isSink(node, state) // Prevent duplicates along a call chain, since `shellCommand` will include wrappers
   }
 }
 
-import StitchedPathGraph
-
 from
-  DataFlow::PathNode sourceNode, DataFlow::PathNode concatSink, DataFlow2::PathNode concatSource,
-  DataFlow2::PathNode sinkNode, string taintCause, string callChain,
-  TaintToConcatenationConfiguration conf1, ExecTaintConfiguration conf2
+  ExecTaintConfiguration conf, DataFlow::PathNode sourceNode, DataFlow::PathNode sinkNode,
+  string taintCause, string callChain, DataFlow::Node concatResult
 where
+  conf.hasFlowPath(sourceNode, sinkNode) and
   taintCause = sourceNode.getNode().(FlowSource).getSourceType() and
-  conf1.hasFlowPath(sourceNode, concatSink) and
-  interestingConcatenation(concatSink.getNode(), concatSource.getNode()) and // this loses call context
-  conf2.hasFlowPath(concatSource, sinkNode) and
-  shellCommand(sinkAsArgumentIndirection(sinkNode.getNode()), callChain)
-select sinkAsArgumentIndirection(sinkNode.getNode()), TPathNode1(sourceNode).(MergedPathNode),
-  TPathNode2(sinkNode).(MergedPathNode),
+  shellCommand(sinkAsArgumentIndirection(sinkNode.getNode()), callChain) and
+  concatResult = sinkNode.getState().(ExecState).getSndNode()
+select sinkAsArgumentIndirection(sinkNode.getNode()), sourceNode, sinkNode,
   "This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to "
-    + callChain, sourceNode, "user input (" + taintCause + ")", concatSource,
-  concatSource.toString()
+    + callChain, sourceNode, "user input (" + taintCause + ")", concatResult,
+  concatResult.toString()

cpp/ql/src/change-notes/2022-03-21-command-line-injection-with-flow-states.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `cpp/command-line-injection` query now takes into account calling contexts across string concatenations. This removes false positives due to mismatched calling contexts before and after string concatenations.

cpp/ql/test/query-tests/Security/CWE/CWE-078/SAMATE/ExecTainted/ExecTainted.expected

@@ -3,7 +3,6 @@ edges
 | tests.cpp:33:34:33:39 | call to getenv | tests.cpp:38:39:38:49 | environment indirection |
 | tests.cpp:38:25:38:36 | strncat output argument | tests.cpp:26:15:26:23 | ReturnValue |
 | tests.cpp:38:39:38:49 | environment indirection | tests.cpp:38:25:38:36 | strncat output argument |
-| tests.cpp:38:39:38:49 | environment indirection | tests.cpp:38:25:38:36 | strncat output argument |
 | tests.cpp:51:12:51:20 | call to badSource | tests.cpp:53:16:53:19 | data indirection |
 nodes
 | tests.cpp:26:15:26:23 | ReturnValue | semmle.label | ReturnValue |