C++: Use TaintTracking::Configuration in TaintedAllocationSize

cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql

@@ -12,8 +12,12 @@
  */
 
 import cpp
-import semmle.code.cpp.security.TaintTracking
-import TaintedWithPath
+import semmle.code.cpp.ir.dataflow.TaintTracking
+import semmle.code.cpp.security.FlowSources
+import semmle.code.cpp.controlflow.IRGuards
+import semmle.code.cpp.ir.ValueNumbering
+import DataFlow::PathGraph
+import semmle.code.cpp.ir.IR
 
 /**
  * Holds if `alloc` is an allocation, and `tainted` is a child of it that is a
@@ -25,21 +29,107 @@ predicate allocSink(Expr alloc, Expr tainted) {
   tainted.getUnspecifiedType() instanceof IntegralType
 }
 
-class TaintedAllocationSizeConfiguration extends TaintTrackingConfiguration {
-  override predicate isSink(Element tainted) { allocSink(_, tainted) }
+class TaintedAllocationSizeConfiguration extends TaintTracking::Configuration {
+  TaintedAllocationSizeConfiguration() { this = "TaintedAllocationSizeConfiguration" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof RemoteFlowSource or source instanceof LocalFlowSource
+  }
+
+  override predicate isSink(DataFlow::Node sink) { allocSink(_, sink.asExpr()) }
+
+  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+    guard instanceof UpperBoundGuard
+    or
+    guard instanceof EqualityGuard
+  }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    node.asInstruction() instanceof BoundedVariableRead
+  }
+}
+
+class UpperBoundGuard extends DataFlow::BarrierGuard {
+  UpperBoundGuard() { this.comparesLt(_, _, _, _, _) }
+
+  override predicate checks(Instruction i, boolean b) { this.comparesLt(i.getAUse(), _, _, _, b) }
+}
+
+class EqualityGuard extends DataFlow::BarrierGuard {
+  EqualityGuard() { this.comparesEq(_, _, _, _, _) }
+
+  override predicate checks(Instruction i, boolean b) {
+    this.comparesEq(i.getAUse(), _, _, true, b)
+  }
+}
+
+private predicate readsVariable(LoadInstruction load, Variable var) {
+  load.getSourceAddress().(VariableAddressInstruction).getASTVariable() = var
+}
+
+/**
+ * A variable that has any kind of upper-bound check anywhere in the program.  This is
+ * biased towards being inclusive because there are a lot of valid ways of doing an
+ * upper bounds checks if we don't consider where it occurs, for example:
+ * ```
+ *   if (x < 10) { sink(x); }
+ *
+ *   if (10 > y) { sink(y); }
+ *
+ *   if (z > 10) { z = 10; }
+ *   sink(z);
+ * ```
+ */
+// TODO: This coarse overapproximation, ported from the old taint tracking
+// library, could be replaced with an actual semantic check that a particular
+// variable _access_ is guarded by an upper-bound check. We probably don't want
+// to do this right away since it could expose a lot of FPs that were
+// previously suppressed by this predicate by coincidence.
+private predicate hasUpperBoundsCheck(Variable var) {
+  exists(RelationalOperation oper, VariableAccess access |
+    oper.getAnOperand() = access and
+    access.getTarget() = var and
+    // Comparing to 0 is not an upper bound check
+    not oper.getAnOperand().getValue() = "0"
+  )
+}
+
+/**
+ * A read of a variable that has an upper-bound check somewhere in the
+ * program.
+ */
+class BoundedVariableRead extends LoadInstruction {
+  BoundedVariableRead() {
+    exists(Variable checkedVar |
+      readsVariable(this, checkedVar) and
+      hasUpperBoundsCheck(checkedVar)
+    )
+  }
 }
 
 predicate taintedAllocSize(
-  Expr source, Expr alloc, PathNode sourceNode, PathNode sinkNode, string taintCause
+  Expr source, Expr alloc, DataFlow::PathNode sourceNode, DataFlow::PathNode sinkNode,
+  string taintCause
 ) {
-  isUserInput(source, taintCause) and
-  exists(Expr tainted |
+  exists(Expr tainted, DataFlow::Node rawSourceNode, TaintedAllocationSizeConfiguration config |
     allocSink(alloc, tainted) and
-    taintedWithPath(source, tainted, sourceNode, sinkNode)
+    (
+      rawSourceNode.asExpr() = source or
+      rawSourceNode.asDefiningArgument() = source
+    ) and
+    sourceNode.getNode() = rawSourceNode and
+    config.hasFlowPath(sourceNode, sinkNode) and
+    tainted = sinkNode.getNode().(DataFlow::ExprNode).getConvertedExpr() and
+    (
+      taintCause = rawSourceNode.(RemoteFlowSource).getSourceType() or
+      taintCause = rawSourceNode.(LocalFlowSource).getSourceType()
+    )
   )
 }
 
-from Expr source, Expr alloc, PathNode sourceNode, PathNode sinkNode, string taintCause
+from
+  Expr source, Expr alloc, DataFlow::PathNode sourceNode, DataFlow::PathNode sinkNode,
+  string taintCause
 where taintedAllocSize(source, alloc, sourceNode, sinkNode, taintCause)
 select alloc, sourceNode, sinkNode, "This allocation size is derived from $@ and might overflow",
   source, "user input (" + taintCause + ")"

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/ModelUtil.qll

@@ -18,7 +18,7 @@ Instruction callInput(CallInstruction call, FunctionInput input) {
   or
   // A value pointed to by a positional argument
   exists(ReadSideEffectInstruction read |
-    result = read and
+    result = read.getSideEffectOperand().getAnyDef() and
     read.getPrimaryInstruction() = call and
     input.isParameterDeref(read.getIndex())
   )

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/TaintTrackingUtil.qll

@@ -67,6 +67,22 @@ private predicate localInstructionTaintStep(Instruction nodeFrom, Instruction no
     or
     t instanceof ArrayType
   )
+  or
+  // Until we have from through indirections across calls, we'll take flow out
+  // of the parameter and into its indirection.
+  exists(IRFunction f, Parameter parameter |
+    nodeFrom = getInitializeParameter(f, parameter) and
+    nodeTo = getInitializeIndirection(f, parameter)
+  )
+  or
+  // Until we have flow through indirections across calls, we'll take flow out
+  // of the indirection and into the argument.
+  // When we get proper flow through indirections across calls, this code can be
+  // moved to `adjusedSink` or possibly into the `DataFlow::ExprNode` class.
+  exists(ReadSideEffectInstruction read |
+    read.getAnOperand().(SideEffectOperand).getAnyDef() = nodeFrom and
+    read.getArgumentDef() = nodeTo
+  )
 }
 
 /**
@@ -135,3 +151,15 @@ predicate modeledInstructionTaintStep(Instruction instrIn, Instruction instrOut)
     modelMidIn.isParameter(indexMid)
   )
 }
+
+pragma[noinline]
+private InitializeIndirectionInstruction getInitializeIndirection(IRFunction f, Parameter p) {
+  result.getParameter() = p and
+  result.getEnclosingIRFunction() = f
+}
+
+pragma[noinline]
+private InitializeParameterInstruction getInitializeParameter(IRFunction f, Parameter p) {
+  result.getParameter() = p and
+  result.getEnclosingIRFunction() = f
+}

cpp/ql/src/semmle/code/cpp/models/Models.qll

@@ -1,6 +1,7 @@
 private import implementations.Allocation
 private import implementations.Deallocation
 private import implementations.Fread
+private import implementations.Getenv
 private import implementations.Gets
 private import implementations.IdentityFunction
 private import implementations.Inet

cpp/ql/src/semmle/code/cpp/models/implementations/Getenv.qll

@@ -0,0 +1,22 @@
+/**
+ * Provides an implementation class modelling the POSIX function `getenv`.
+ */
+import cpp
+import semmle.code.cpp.models.interfaces.FlowSource
+
+/**
+ * The POSIX function `getenv`.
+ */
+class Getenv extends LocalFlowFunction {
+  Getenv() {
+    this.hasGlobalName("getenv")
+  }
+
+  override predicate hasLocalFlowSource (FunctionOutput output, string description) {
+    (
+      output.isReturnValueDeref() or
+      output.isReturnValue()
+    ) and
+    description = "an environment variable"
+  }
+}

cpp/ql/src/semmle/code/cpp/models/interfaces/FlowSource.qll

@@ -11,11 +11,21 @@ import FunctionInputsAndOutputs
 import semmle.code.cpp.models.Models
 
 /**
- * A library function which returns data read from a network connection.
+ * A library function which returns data that may be read from a network connection.
  */
 abstract class RemoteFlowFunction extends Function {
   /**
    * Holds if remote data described by `description` flows from `output` of a call to this function.
    */
   abstract predicate hasRemoteFlowSource(FunctionOutput output, string description);
 }
+
+/**
+ * A library function which returns data that is directly controlled by a user.
+ */
+abstract class LocalFlowFunction extends Function {
+  /**
+   * Holds if data described by `description` flows from `output` of a call to this function.
+   */
+  abstract predicate hasLocalFlowSource(FunctionOutput output, string description);
+}

cpp/ql/src/semmle/code/cpp/security/FlowSources.qll

@@ -13,10 +13,16 @@ abstract class RemoteFlowSource extends DataFlow::Node {
   abstract string getSourceType();
 }
 
-private class TaintedReturnSource extends RemoteFlowSource {
+/** A data flow source of local user input. */
+abstract class LocalFlowSource extends DataFlow::Node  {
+/** Gets a string that describes the type of this local flow source. */
+  abstract string getSourceType();
+}
+
+private class RemoteReturnSource extends RemoteFlowSource {
   string sourceType;
 
-  TaintedReturnSource() {
+  RemoteReturnSource() {
     exists(RemoteFlowFunction func, CallInstruction instr, FunctionOutput output |
       asInstruction() = instr and
       instr.getStaticCallTarget() = func and
@@ -28,10 +34,10 @@ private class TaintedReturnSource extends RemoteFlowSource {
   override string getSourceType() { result = sourceType }
 }
 
-private class TaintedParameterSource extends RemoteFlowSource {
+private class RemoteParameterSource extends RemoteFlowSource {
   string sourceType;
 
-  TaintedParameterSource() {
+  RemoteParameterSource() {
     exists(RemoteFlowFunction func, WriteSideEffectInstruction instr, FunctionOutput output |
       asInstruction() = instr and
       instr.getPrimaryInstruction().(CallInstruction).getStaticCallTarget() = func and
@@ -42,3 +48,45 @@ private class TaintedParameterSource extends RemoteFlowSource {
 
   override string getSourceType() { result = sourceType }
 }
+
+private class LocalReturnSource extends LocalFlowSource {
+  string sourceType;
+
+  LocalReturnSource() {
+    exists(LocalFlowFunction func, CallInstruction instr, FunctionOutput output |
+      asInstruction() = instr and
+      instr.getStaticCallTarget() = func and
+      func.hasLocalFlowSource(output, sourceType) and
+      output.isReturnValue()
+    )
+  }
+
+  override string getSourceType() { result = sourceType }
+}
+
+private class LocalParameterSource extends LocalFlowSource {
+  string sourceType;
+
+  LocalParameterSource() {
+    exists(LocalFlowFunction func, WriteSideEffectInstruction instr, FunctionOutput output |
+      asInstruction() = instr and
+      instr.getPrimaryInstruction().(CallInstruction).getStaticCallTarget() = func and
+      func.hasLocalFlowSource(output, sourceType) and
+      output.isParameterDeref(instr.getIndex())
+    )
+  }
+
+  override string getSourceType() { result = sourceType }
+}
+
+private class ArgvSource extends LocalFlowSource {
+  ArgvSource() {
+    exists(Parameter argv |
+      argv.hasName("argv") and
+      argv.getFunction().hasGlobalName("main") and
+      this.asExpr() = argv.getAnAccess()
+    )
+  }
+
+  override string getSourceType() { result = "a command line argument" }
+}

cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected

@@ -27,21 +27,14 @@
 | taint.cpp:112:7:112:13 | taint.cpp:106:12:106:17 | IR only |
 | taint.cpp:130:7:130:9 | taint.cpp:127:8:127:13 | IR only |
 | taint.cpp:137:7:137:9 | taint.cpp:120:11:120:16 | AST only |
-| taint.cpp:173:8:173:13 | taint.cpp:164:19:164:24 | AST only |
-| taint.cpp:181:8:181:9 | taint.cpp:185:11:185:16 | AST only |
-| taint.cpp:195:7:195:7 | taint.cpp:192:23:192:28 | AST only |
 | taint.cpp:195:7:195:7 | taint.cpp:193:6:193:6 | AST only |
 | taint.cpp:229:3:229:6 | taint.cpp:223:10:223:15 | AST only |
 | taint.cpp:233:8:233:8 | taint.cpp:223:10:223:15 | AST only |
 | taint.cpp:236:3:236:6 | taint.cpp:223:10:223:15 | AST only |
 | taint.cpp:244:3:244:6 | taint.cpp:223:10:223:15 | AST only |
-| taint.cpp:256:8:256:8 | taint.cpp:223:10:223:15 | AST only |
 | taint.cpp:261:7:261:7 | taint.cpp:258:7:258:12 | AST only |
 | taint.cpp:351:7:351:7 | taint.cpp:330:6:330:11 | AST only |
 | taint.cpp:352:7:352:7 | taint.cpp:330:6:330:11 | AST only |
-| taint.cpp:372:7:372:7 | taint.cpp:365:24:365:29 | AST only |
-| taint.cpp:374:7:374:7 | taint.cpp:365:24:365:29 | AST only |
-| taint.cpp:391:7:391:7 | taint.cpp:385:27:385:32 | AST only |
 | taint.cpp:423:7:423:7 | taint.cpp:422:14:422:19 | AST only |
 | taint.cpp:424:9:424:17 | taint.cpp:422:14:422:19 | AST only |
 | taint.cpp:429:7:429:7 | taint.cpp:428:13:428:18 | IR only |

cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected

@@ -16,17 +16,25 @@
 | taint.cpp:151:7:151:12 | call to select | taint.cpp:151:20:151:25 | call to source |
 | taint.cpp:167:8:167:13 | call to source | taint.cpp:167:8:167:13 | call to source |
 | taint.cpp:168:8:168:14 | tainted | taint.cpp:164:19:164:24 | call to source |
+| taint.cpp:173:8:173:13 | array to pointer conversion | taint.cpp:164:19:164:24 | call to source |
+| taint.cpp:181:8:181:9 | * ... | taint.cpp:185:11:185:16 | call to source |
+| taint.cpp:195:7:195:7 | x | taint.cpp:192:23:192:28 | source |
 | taint.cpp:210:7:210:7 | x | taint.cpp:207:6:207:11 | call to source |
 | taint.cpp:215:7:215:7 | x | taint.cpp:207:6:207:11 | call to source |
 | taint.cpp:216:7:216:7 | y | taint.cpp:207:6:207:11 | call to source |
 | taint.cpp:250:8:250:8 | a | taint.cpp:223:10:223:15 | call to source |
+| taint.cpp:256:8:256:8 | (reference dereference) | taint.cpp:223:10:223:15 | call to source |
+| taint.cpp:256:8:256:8 | a | taint.cpp:223:10:223:15 | call to source |
 | taint.cpp:280:7:280:7 | t | taint.cpp:275:6:275:11 | call to source |
 | taint.cpp:289:7:289:7 | t | taint.cpp:275:6:275:11 | call to source |
 | taint.cpp:290:7:290:7 | x | taint.cpp:275:6:275:11 | call to source |
 | taint.cpp:291:7:291:7 | y | taint.cpp:275:6:275:11 | call to source |
 | taint.cpp:337:7:337:7 | t | taint.cpp:330:6:330:11 | call to source |
 | taint.cpp:350:7:350:7 | t | taint.cpp:330:6:330:11 | call to source |
+| taint.cpp:372:7:372:7 | a | taint.cpp:365:24:365:29 | source |
+| taint.cpp:374:7:374:7 | c | taint.cpp:365:24:365:29 | source |
 | taint.cpp:382:7:382:7 | a | taint.cpp:377:23:377:28 | source |
+| taint.cpp:391:7:391:7 | a | taint.cpp:385:27:385:32 | source |
 | taint.cpp:429:7:429:7 | b | taint.cpp:428:13:428:18 | call to source |
 | taint.cpp:430:9:430:14 | member | taint.cpp:428:13:428:18 | call to source |
 | taint.cpp:465:7:465:7 | x | taint.cpp:462:6:462:11 | call to source |