C++: Change the behavior of 'isUserInput' so that the 'argv' paramter (and not all of the uses of 'argv') is user input. This removes the duplicate paths.

Author: MathiasVP

cpp/ql/lib/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

@@ -47,14 +47,24 @@ predicate predictableOnlyFlow(string name) {
     ]
 }
 
-private DataFlow::Node getNodeForSource(Expr source) {
-  isUserInput(source, _) and
-  result = getNodeForExpr(source)
+private DataFlow::Node getNodeForSource(Element source) {
+  (
+    // It is user input
+    isUserInput(source, _) and
+    // But `isUserInput` also marks all uses of `argv` as user input, and we don't want those.
+    not argv(source.(VariableAccess).getTarget())
+    or
+    // Instead, we want the actual parameter as user input.
+    argv(source)
+  ) and
+  result = getNodeForElement(source)
 }
 
-private DataFlow::Node getNodeForExpr(Expr node) {
+private DataFlow::Node getNodeForElement(Element node) {
   result = DataFlow::exprNode(node)
   or
+  result.asParameter() = node
+  or
   // Some of the sources in `isUserInput` are intended to match the value of
   // an expression, while others (those modeled below) are intended to match
   // the taint that propagates out of an argument, like the `char *` argument
@@ -194,13 +204,7 @@ private module Cached {
   cached
   predicate nodeIsBarrierIn(DataFlow::Node node) {
     // don't use dataflow into taint sources, as this leads to duplicate results.
-    exists(Expr source | isUserInput(source, _) |
-      node = DataFlow::exprNode(source)
-      or
-      // This case goes together with the similar (but not identical) rule in
-      // `getNodeForSource`.
-      node = DataFlow::definitionByReferenceNodeFromArgument(source)
-    )
+    node = getNodeForSource(_)
     or
     // don't use dataflow into binary instructions if both operands are unpredictable
     exists(BinaryInstruction iTo |
@@ -303,7 +307,7 @@ private import Cached
  * If you need that you must call `taintedIncludingGlobalVars`.
  */
 cached
-predicate tainted(Expr source, Element tainted) {
+predicate tainted(Element source, Element tainted) {
   exists(DefaultTaintTrackingCfg cfg, DataFlow::Node sink |
     cfg.hasFlow(getNodeForSource(source), sink) and
     tainted = adjustedSink(sink)
@@ -326,7 +330,7 @@ predicate tainted(Expr source, Element tainted) {
  * through a global variable, then `globalVar = ""`.
  */
 cached
-predicate taintedIncludingGlobalVars(Expr source, Element tainted, string globalVar) {
+predicate taintedIncludingGlobalVars(Element source, Element tainted, string globalVar) {
   tainted(source, tainted) and
   globalVar = ""
   or
@@ -376,13 +380,13 @@ module TaintedWithPath {
    */
   class TaintTrackingConfiguration extends TSingleton {
     /** Override this to specify which elements are sources in this configuration. */
-    predicate isSource(Expr source) { exists(getNodeForSource(source)) }
+    predicate isSource(Element source) { exists(getNodeForSource(source)) }
 
     /** Override this to specify which elements are sinks in this configuration. */
     abstract predicate isSink(Element e);
 
     /** Override this to specify which expressions are barriers in this configuration. */
-    predicate isBarrier(Expr e) { nodeIsBarrier(getNodeForExpr(e)) }
+    predicate isBarrier(Expr e) { nodeIsBarrier(getNodeForElement(e)) }
 
     /**
      * Override this predicate to `any()` to allow taint to flow through global
@@ -398,8 +402,8 @@ module TaintedWithPath {
     AdjustedConfiguration() { this = "AdjustedConfiguration" }
 
     override predicate isSource(DataFlow::Node source) {
-      exists(TaintTrackingConfiguration cfg, Expr e |
-        cfg.isSource(e) and source = getNodeForExpr(e)
+      exists(TaintTrackingConfiguration cfg, Element e |
+        cfg.isSource(e) and source = getNodeForElement(e)
       )
     }
 
@@ -419,7 +423,9 @@ module TaintedWithPath {
     }
 
     override predicate isSanitizer(DataFlow::Node node) {
-      exists(TaintTrackingConfiguration cfg, Expr e | cfg.isBarrier(e) and node = getNodeForExpr(e))
+      exists(TaintTrackingConfiguration cfg, Element e |
+        cfg.isBarrier(e) and node = getNodeForElement(e)
+      )
     }
 
     override predicate isSanitizerIn(DataFlow::Node node) { nodeIsBarrierIn(node) }
@@ -447,7 +453,7 @@ module TaintedWithPath {
       exists(AdjustedConfiguration cfg, DataFlow3::Node sourceNode, DataFlow3::Node sinkNode |
         cfg.hasFlow(sourceNode, sinkNode)
       |
-        sourceNode = getNodeForExpr(e) and
+        sourceNode = getNodeForElement(e) and
         exists(TaintTrackingConfiguration ttCfg | ttCfg.isSource(e))
         or
         e = adjustedSink(sinkNode) and
@@ -506,7 +512,7 @@ module TaintedWithPath {
   }
 
   private class EndpointPathNode extends PathNode, TEndpointPathNode {
-    Expr inner() { this = TEndpointPathNode(result) }
+    Element inner() { this = TEndpointPathNode(result) }
 
     override string toString() { result = this.inner().toString() }
 
@@ -543,14 +549,14 @@ module TaintedWithPath {
     // Same for the first node
     exists(WrapPathNode sourceNode |
       DataFlow3::PathGraph::edges(sourceNode.inner(), b.(WrapPathNode).inner()) and
-      sourceNode.inner().getNode() = getNodeForExpr(a.(InitialPathNode).inner())
+      sourceNode.inner().getNode() = getNodeForElement(a.(InitialPathNode).inner())
     )
     or
     // Finally, handle the case where the path goes directly from a source to a
     // sink, meaning that they both need to be translated.
     exists(WrapPathNode sinkNode, WrapPathNode sourceNode |
       DataFlow3::PathGraph::edges(sourceNode.inner(), sinkNode.inner()) and
-      sourceNode.inner().getNode() = getNodeForExpr(a.(InitialPathNode).inner()) and
+      sourceNode.inner().getNode() = getNodeForElement(a.(InitialPathNode).inner()) and
       b.(FinalPathNode).inner() = adjustedSink(sinkNode.inner().getNode())
     )
   }
@@ -575,15 +581,15 @@ module TaintedWithPath {
     exists(WrapPathNode sourceNode |
       DataFlow3::PathGraph::subpaths(sourceNode.inner(), par.(WrapPathNode).inner(),
         ret.(WrapPathNode).inner(), out.(WrapPathNode).inner()) and
-      sourceNode.inner().getNode() = getNodeForExpr(arg.(InitialPathNode).inner())
+      sourceNode.inner().getNode() = getNodeForElement(arg.(InitialPathNode).inner())
     )
     or
     // Finally, handle the case where the path goes directly from a source to a
     // sink, meaning that they both need to be translated.
     exists(WrapPathNode sinkNode, WrapPathNode sourceNode |
       DataFlow3::PathGraph::subpaths(sourceNode.inner(), par.(WrapPathNode).inner(),
         ret.(WrapPathNode).inner(), sinkNode.inner()) and
-      sourceNode.inner().getNode() = getNodeForExpr(arg.(InitialPathNode).inner()) and
+      sourceNode.inner().getNode() = getNodeForElement(arg.(InitialPathNode).inner()) and
       out.(FinalPathNode).inner() = adjustedSink(sinkNode.inner().getNode())
     )
   }
@@ -603,10 +609,10 @@ module TaintedWithPath {
    * user input in a way that users can probably control the exact output of
    * the computation.
    */
-  predicate taintedWithPath(Expr source, Element tainted, PathNode sourceNode, PathNode sinkNode) {
+  predicate taintedWithPath(Element source, Element tainted, PathNode sourceNode, PathNode sinkNode) {
     exists(AdjustedConfiguration cfg, DataFlow3::Node flowSource, DataFlow3::Node flowSink |
       source = sourceNode.(InitialPathNode).inner() and
-      flowSource = getNodeForExpr(source) and
+      flowSource = getNodeForElement(source) and
       cfg.hasFlow(flowSource, flowSink) and
       tainted = adjustedSink(flowSink) and
       tainted = sinkNode.(FinalPathNode).inner()

cpp/ql/lib/semmle/code/cpp/security/Security.qll

@@ -117,22 +117,22 @@ class SecurityOptions extends string {
    * computed from user input. Such expressions are treated as
    * sources of taint.
    */
-  predicate isUserInput(Expr expr, string cause) {
+  predicate isUserInput(Element e, string cause) {
     exists(FunctionCall fc, int i |
       this.userInputArgument(fc, i) and
-      expr = fc.getArgument(i) and
+      e = fc.getArgument(i) and
       cause = fc.getTarget().getName()
     )
     or
     exists(FunctionCall fc |
       this.userInputReturned(fc) and
-      expr = fc and
+      e = fc and
       cause = fc.getTarget().getName()
     )
     or
-    commandLineArg(expr) and cause = "argv"
+    argv(e) and cause = "argv"
     or
-    expr.(EnvironmentRead).getSourceDescription() = cause
+    e.(EnvironmentRead).getSourceDescription() = cause
   }
 
   /**
@@ -188,8 +188,8 @@ predicate userInputReturned(FunctionCall functionCall) {
 }
 
 /** Convenience accessor for SecurityOptions.isUserInput */
-predicate isUserInput(Expr expr, string cause) {
-  exists(SecurityOptions opts | opts.isUserInput(expr, cause))
+predicate isUserInput(Element e, string cause) {
+  exists(SecurityOptions opts | opts.isUserInput(e, cause))
 }
 
 /** Convenience accessor for SecurityOptions.isProcessOperationArgument */

cpp/ql/lib/semmle/code/cpp/security/TaintTrackingImpl.qll

@@ -395,7 +395,7 @@ private predicate taintedWithArgsAndGlobalVars(
  * This doesn't include data flow through global variables.
  * If you need that you must call taintedIncludingGlobalVars.
  */
-predicate tainted(Expr source, Element tainted) {
+predicate tainted(Element source, Element tainted) {
   taintedWithArgsAndGlobalVars(source, tainted, _, "")
 }
 

cpp/ql/src/Critical/OverflowDestination.ql

@@ -45,9 +45,9 @@ predicate sourceSized(FunctionCall fc, Expr src) {
   )
 }
 
-from FunctionCall fc, Expr vuln, Expr taintSource
+from FunctionCall fc, Expr vuln
 where
   sourceSized(fc, vuln) and
-  tainted(taintSource, vuln)
+  tainted(_, vuln)
 select fc,
   "To avoid overflow, this operation should be bounded by destination-buffer size, not source-buffer size."

cpp/ql/src/Security/CWE/CWE-022/TaintedPath.ql

@@ -53,7 +53,7 @@ class TaintedPathConfiguration extends TaintTrackingConfiguration {
 }
 
 from
-  FileFunction fileFunction, Expr taintedArg, Expr taintSource, PathNode sourceNode,
+  FileFunction fileFunction, Expr taintedArg, Element taintSource, PathNode sourceNode,
   PathNode sinkNode, string taintCause, string callChain
 where
   fileFunction.outermostWrapperFunctionCall(taintedArg, callChain) and

cpp/ql/src/Security/CWE/CWE-079/CgiXss.ql

@@ -30,7 +30,7 @@ class QueryString extends EnvironmentRead {
 }
 
 class Configuration extends TaintTrackingConfiguration {
-  override predicate isSource(Expr source) { source instanceof QueryString }
+  override predicate isSource(Element source) { source instanceof QueryString }
 
   override predicate isSink(Element tainted) {
     exists(PrintStdoutCall call | call.getAnArgument() = tainted)

cpp/ql/src/Security/CWE/CWE-089/SqlTainted.ql

@@ -43,8 +43,8 @@ class Configuration extends TaintTrackingConfiguration {
 }
 
 from
-  SQLLikeFunction runSql, Expr taintedArg, Expr taintSource, PathNode sourceNode, PathNode sinkNode,
-  string taintCause, string callChain
+  SQLLikeFunction runSql, Expr taintedArg, Element taintSource, PathNode sourceNode,
+  PathNode sinkNode, string taintCause, string callChain
 where
   runSql.outermostWrapperFunctionCall(taintedArg, callChain) and
   taintedWithPath(taintSource, taintedArg, sourceNode, sinkNode) and

cpp/ql/src/Security/CWE/CWE-114/UncontrolledProcessOperation.ql

@@ -29,7 +29,7 @@ class Configuration extends TaintTrackingConfiguration {
   override predicate isSink(Element arg) { isProcessOperationExplanation(arg, _) }
 }
 
-from string processOperation, Expr arg, Expr source, PathNode sourceNode, PathNode sinkNode
+from string processOperation, Expr arg, Element source, PathNode sourceNode, PathNode sinkNode
 where
   isProcessOperationExplanation(arg, processOperation) and
   taintedWithPath(source, arg, sourceNode, sinkNode)

cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql

@@ -94,7 +94,7 @@ class Configuration extends TaintTrackingConfiguration {
  * an argument. Thus, we get the desired alerts.
  */
 
-from BufferWrite bw, Expr inputSource, Expr tainted, PathNode sourceNode, PathNode sinkNode
+from BufferWrite bw, Element inputSource, Expr tainted, PathNode sourceNode, PathNode sinkNode
 where
   taintedWithPath(inputSource, tainted, sourceNode, sinkNode) and
   unboundedWriteSource(tainted, bw)

cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql

@@ -32,7 +32,7 @@ predicate linearBoundControls(BasicBlock controlled, SsaDefinition def, StackVar
   )
 }
 
-from Expr origin, ArrayExpr arrayExpr, VariableAccess offsetExpr
+from Element origin, ArrayExpr arrayExpr, VariableAccess offsetExpr
 where
   tainted(origin, offsetExpr) and
   offsetExpr = arrayExpr.getArrayOffset() and