Merge pull request #3364 from github/rdmarsh/cpp/use-taint-configuration-dtt

C++: use TaintTracking::Configuration in DefaultTaintTracking

Author: jbj

config/identical-files.json

@@ -36,6 +36,7 @@
     "cpp/ql/src/semmle/code/cpp/dataflow/internal/tainttracking2/TaintTrackingImpl.qll",
     "cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/tainttracking1/TaintTrackingImpl.qll",
     "cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/tainttracking2/TaintTrackingImpl.qll",
+    "cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/tainttracking3/TaintTrackingImpl.qll",
     "csharp/ql/src/semmle/code/csharp/dataflow/internal/tainttracking1/TaintTrackingImpl.qll",
     "csharp/ql/src/semmle/code/csharp/dataflow/internal/tainttracking2/TaintTrackingImpl.qll",
     "csharp/ql/src/semmle/code/csharp/dataflow/internal/tainttracking3/TaintTrackingImpl.qll",

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

@@ -0,0 +1,15 @@
+/**
+ * Provides a `TaintTracking3` module, which is a copy of the `TaintTracking`
+ * module. Use this class when data-flow configurations or taint-tracking
+ * configurations must depend on each other. Two classes extending
+ * `DataFlow::Configuration` should never depend on each other, but one of them
+ * should instead depend on a `DataFlow2::Configuration`, a
+ * `DataFlow3::Configuration`, or a `DataFlow4::Configuration`. The
+ * `TaintTracking::Configuration` class extends `DataFlow::Configuration`, and
+ * `TaintTracking2::Configuration` extends `DataFlow2::Configuration`.
+ *
+ * See `semmle.code.cpp.ir.dataflow.TaintTracking` for the full documentation.
+ */
+module TaintTracking3 {
+  import semmle.code.cpp.ir.dataflow.internal.tainttracking3.TaintTrackingImpl
+}

cpp/ql/src/semmle/code/cpp/ir/dataflow/TaintTracking3.qll

@@ -9,30 +9,18 @@ private import semmle.code.cpp.ir.dataflow.DataFlow
 /**
  * Gets the instruction that goes into `input` for `call`.
  */
-DataFlow::Node callInput(CallInstruction call, FunctionInput input) {
-  // A positional argument
+Operand callInput(CallInstruction call, FunctionInput input) {
+  // An argument or qualifier
   exists(int index |
-    result.asInstruction() = call.getPositionalArgument(index) and
-    input.isParameter(index)
+    result = call.getArgumentOperand(index) and
+    input.isParameterOrQualifierAddress(index)
   )
   or
-  // A value pointed to by a positional argument
+  // A value pointed to by an argument or qualifier
   exists(ReadSideEffectInstruction read |
-    result.asOperand() = read.getSideEffectOperand() and
+    result = read.getSideEffectOperand() and
     read.getPrimaryInstruction() = call and
-    input.isParameterDeref(read.getIndex())
-  )
-  or
-  // The qualifier pointer
-  result.asInstruction() = call.getThisArgument() and
-  input.isQualifierAddress()
-  or
-  // The qualifier object
-  exists(ReadSideEffectInstruction read |
-    result.asOperand() = read.getSideEffectOperand() and
-    read.getPrimaryInstruction() = call and
-    read.getIndex() = -1 and
-    input.isQualifierObject()
+    input.isParameterDerefOrQualifierObject(read.getIndex())
   )
 }
 
@@ -44,19 +32,11 @@ Instruction callOutput(CallInstruction call, FunctionOutput output) {
   result = call and
   output.isReturnValue()
   or
-  // The side effect of a call on the value pointed to by a positional argument
-  exists(WriteSideEffectInstruction effect |
-    result = effect and
-    effect.getPrimaryInstruction() = call and
-    output.isParameterDeref(effect.getIndex())
-  )
-  or
-  // The side effect of a call on the qualifier object
+  // The side effect of a call on the value pointed to by an argument or qualifier
   exists(WriteSideEffectInstruction effect |
     result = effect and
     effect.getPrimaryInstruction() = call and
-    effect.getIndex() = -1 and
-    output.isQualifierObject()
+    output.isParameterDerefOrQualifierObject(effect.getIndex())
   )
   // TODO: return value dereference
 }

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

@@ -21,53 +21,104 @@ predicate localTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
  */
 cached
 predicate localAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
-  localInstructionTaintStep(nodeFrom.asInstruction(), nodeTo.asInstruction())
+  operandToInstructionTaintStep(nodeFrom.asOperand(), nodeTo.asInstruction())
   or
-  modeledTaintStep(nodeFrom, nodeTo)
+  instructionToOperandTaintStep(nodeFrom.asInstruction(), nodeTo.asOperand())
+}
+
+private predicate instructionToOperandTaintStep(Instruction fromInstr, Operand toOperand) {
+  // Propagate flow from the definition of an operand to the operand, even when the overlap is inexact.
+  // We only do this in certain cases:
+  // 1. The instruction's result must not be conflated, and
+  // 2. The instruction's result type is one the types where we expect element-to-object flow. Currently
+  // this is array types and union types. This matches the other two cases of element-to-object flow in
+  // `DefaultTaintTracking`.
+  toOperand.getAnyDef() = fromInstr and
+  not fromInstr.isResultConflated() and
+  (
+    fromInstr.getResultType() instanceof ArrayType or
+    fromInstr.getResultType() instanceof Union
+  )
+  or
+  exists(ReadSideEffectInstruction readInstr |
+    fromInstr = readInstr.getArgumentDef() and
+    toOperand = readInstr.getSideEffectOperand()
+  )
+  or
+  toOperand.(LoadOperand).getAnyDef() = fromInstr
 }
 
 /**
  * Holds if taint propagates from `nodeFrom` to `nodeTo` in exactly one local
  * (intra-procedural) step.
  */
-private predicate localInstructionTaintStep(Instruction nodeFrom, Instruction nodeTo) {
+private predicate operandToInstructionTaintStep(Operand opFrom, Instruction instrTo) {
   // Taint can flow through expressions that alter the value but preserve
   // more than one bit of it _or_ expressions that follow data through
   // pointer indirections.
-  nodeTo.getAnOperand().getAnyDef() = nodeFrom and
+  instrTo.getAnOperand() = opFrom and
   (
-    nodeTo instanceof ArithmeticInstruction
-    or
-    nodeTo instanceof BitwiseInstruction
+    instrTo instanceof ArithmeticInstruction
     or
-    nodeTo instanceof PointerArithmeticInstruction
+    instrTo instanceof BitwiseInstruction
     or
-    nodeTo instanceof FieldAddressInstruction
+    instrTo instanceof PointerArithmeticInstruction
     or
     // The `CopyInstruction` case is also present in non-taint data flow, but
     // that uses `getDef` rather than `getAnyDef`. For taint, we want flow
     // from a definition of `myStruct` to a `myStruct.myField` expression.
-    nodeTo instanceof CopyInstruction
+    instrTo instanceof CopyInstruction
   )
   or
-  nodeTo.(LoadInstruction).getSourceAddress() = nodeFrom
-  or
-  // Flow through partial reads of arrays and unions
-  nodeTo.(LoadInstruction).getSourceValueOperand().getAnyDef() = nodeFrom and
-  not nodeFrom.isResultConflated() and
+  // Unary instructions tend to preserve enough information in practice that we
+  // want taint to flow through.
+  // The exception is `FieldAddressInstruction`. Together with the rules below for
+  // `LoadInstruction`s and `ChiInstruction`s, flow through `FieldAddressInstruction`
+  // could cause flow into one field to come out an unrelated field.
+  // This would happen across function boundaries, where the IR would not be able to
+  // match loads to stores.
+  instrTo.(UnaryInstruction).getUnaryOperand() = opFrom and
   (
-    nodeFrom.getResultType() instanceof ArrayType or
-    nodeFrom.getResultType() instanceof Union
+    not instrTo instanceof FieldAddressInstruction
+    or
+    instrTo.(FieldAddressInstruction).getField().getDeclaringType() instanceof Union
   )
   or
+  instrTo.(LoadInstruction).getSourceAddressOperand() = opFrom
+  or
   // Flow from an element to an array or union that contains it.
-  nodeTo.(ChiInstruction).getPartial() = nodeFrom and
-  not nodeTo.isResultConflated() and
-  exists(Type t | nodeTo.getResultLanguageType().hasType(t, false) |
+  instrTo.(ChiInstruction).getPartialOperand() = opFrom and
+  not instrTo.isResultConflated() and
+  exists(Type t | instrTo.getResultLanguageType().hasType(t, false) |
     t instanceof Union
     or
     t instanceof ArrayType
   )
+  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.getSideEffectOperand() = opFrom and
+    read.getArgumentDef() = instrTo
+  )
+  or
+  // Until we have from through indirections across calls, we'll take flow out
+  // of the parameter and into its indirection.
+  // `InitializeIndirectionInstruction` only has a single operand: the address of the
+  // value whose indirection we are initializing. When initializing an indirection of a parameter `p`,
+  // the IR looks like this:
+  // ```
+  // m1 = InitializeParameter[p] : &r1
+  // r2 = Load[p] : r2, m1
+  // m3 = InitializeIndirection[p] : &r2
+  // ```
+  // So by having flow from `r2` to `m3` we're enabling flow from `m1` to `m3`. This relies on the
+  // `LoadOperand`'s overlap being exact.
+  instrTo.(InitializeIndirectionInstruction).getAnOperand() = opFrom
+  or
+  modeledTaintStep(opFrom, instrTo)
 }
 
 /**
@@ -110,17 +161,19 @@ predicate defaultTaintSanitizer(DataFlow::Node node) { none() }
  * Holds if taint can flow from `instrIn` to `instrOut` through a call to a
  * modeled function.
  */
-predicate modeledTaintStep(DataFlow::Node nodeIn, DataFlow::Node nodeOut) {
+predicate modeledTaintStep(Operand nodeIn, Instruction nodeOut) {
   exists(CallInstruction call, TaintFunction func, FunctionInput modelIn, FunctionOutput modelOut |
     (
       nodeIn = callInput(call, modelIn)
       or
       exists(int n |
-        modelIn.isParameterDeref(n) and
-        nodeIn = callInput(call, any(InParameter inParam | inParam.getIndex() = n))
+        modelIn.isParameterDerefOrQualifierObject(n) and
+        if n = -1
+        then nodeIn = callInput(call, any(InQualifierObject inQualifier))
+        else nodeIn = callInput(call, any(InParameter inParam | inParam.getIndex() = n))
       )
     ) and
-    nodeOut.asInstruction() = callOutput(call, modelOut) and
+    nodeOut = callOutput(call, modelOut) and
     call.getStaticCallTarget() = func and
     func.hasTaintFlow(modelIn, modelOut)
   )
@@ -135,11 +188,29 @@ predicate modeledTaintStep(DataFlow::Node nodeIn, DataFlow::Node nodeOut) {
     int indexMid, InParameter modelMidIn, OutReturnValue modelOut
   |
     nodeIn = callInput(call, modelIn) and
-    nodeOut.asInstruction() = callOutput(call, modelOut) and
+    nodeOut = callOutput(call, modelOut) and
     call.getStaticCallTarget() = func and
     func.(TaintFunction).hasTaintFlow(modelIn, modelMidOut) and
     func.(DataFlowFunction).hasDataFlow(modelMidIn, modelOut) and
     modelMidOut.isParameterDeref(indexMid) and
     modelMidIn.isParameter(indexMid)
   )
+  or
+  // Taint flow from a pointer argument to an output, when the model specifies flow from the deref
+  // to that output, but the deref is not modeled in the IR for the caller.
+  exists(
+    CallInstruction call, ReadSideEffectInstruction read, Function func, FunctionInput modelIn,
+    FunctionOutput modelOut
+  |
+    read.getSideEffectOperand() = callInput(call, modelIn) and
+    read.getArgumentDef() = nodeIn.getDef() and
+    not read.getSideEffect().isResultModeled() and
+    call.getStaticCallTarget() = func and
+    (
+      func.(DataFlowFunction).hasDataFlow(modelIn, modelOut)
+      or
+      func.(TaintFunction).hasTaintFlow(modelIn, modelOut)
+    ) and
+    nodeOut = callOutput(call, modelOut)
+  )
 }

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

@@ -0,0 +1,115 @@
+/**
+ * Provides an implementation of global (interprocedural) taint tracking.
+ * This file re-exports the local (intraprocedural) taint-tracking analysis
+ * from `TaintTrackingParameter::Public` and adds a global analysis, mainly
+ * exposed through the `Configuration` class. For some languages, this file
+ * exists in several identical copies, allowing queries to use multiple
+ * `Configuration` classes that depend on each other without introducing
+ * mutual recursion among those configurations.
+ */
+
+import TaintTrackingParameter::Public
+private import TaintTrackingParameter::Private
+
+/**
+ * A configuration of interprocedural taint tracking analysis. This defines
+ * sources, sinks, and any other configurable aspect of the analysis. Each
+ * use of the taint tracking library must define its own unique extension of
+ * this abstract class.
+ *
+ * A taint-tracking configuration is a special data flow configuration
+ * (`DataFlow::Configuration`) that allows for flow through nodes that do not
+ * necessarily preserve values but are still relevant from a taint tracking
+ * perspective. (For example, string concatenation, where one of the operands
+ * is tainted.)
+ *
+ * To create a configuration, extend this class with a subclass whose
+ * characteristic predicate is a unique singleton string. For example, write
+ *
+ * ```ql
+ * class MyAnalysisConfiguration extends TaintTracking::Configuration {
+ *   MyAnalysisConfiguration() { this = "MyAnalysisConfiguration" }
+ *   // Override `isSource` and `isSink`.
+ *   // Optionally override `isSanitizer`.
+ *   // Optionally override `isSanitizerIn`.
+ *   // Optionally override `isSanitizerOut`.
+ *   // Optionally override `isSanitizerGuard`.
+ *   // Optionally override `isAdditionalTaintStep`.
+ * }
+ * ```
+ *
+ * Then, to query whether there is flow between some `source` and `sink`,
+ * write
+ *
+ * ```ql
+ * exists(MyAnalysisConfiguration cfg | cfg.hasFlow(source, sink))
+ * ```
+ *
+ * Multiple configurations can coexist, but it is unsupported to depend on
+ * another `TaintTracking::Configuration` or a `DataFlow::Configuration` in the
+ * overridden predicates that define sources, sinks, or additional steps.
+ * Instead, the dependency should go to a `TaintTracking2::Configuration` or a
+ * `DataFlow2::Configuration`, `DataFlow3::Configuration`, etc.
+ */
+abstract class Configuration extends DataFlow::Configuration {
+  bindingset[this]
+  Configuration() { any() }
+
+  /**
+   * Holds if `source` is a relevant taint source.
+   *
+   * The smaller this predicate is, the faster `hasFlow()` will converge.
+   */
+  // overridden to provide taint-tracking specific qldoc
+  abstract override predicate isSource(DataFlow::Node source);
+
+  /**
+   * Holds if `sink` is a relevant taint sink.
+   *
+   * The smaller this predicate is, the faster `hasFlow()` will converge.
+   */
+  // overridden to provide taint-tracking specific qldoc
+  abstract override predicate isSink(DataFlow::Node sink);
+
+  /** Holds if the node `node` is a taint sanitizer. */
+  predicate isSanitizer(DataFlow::Node node) { none() }
+
+  final override predicate isBarrier(DataFlow::Node node) {
+    isSanitizer(node) or
+    defaultTaintSanitizer(node)
+  }
+
+  /** Holds if taint propagation into `node` is prohibited. */
+  predicate isSanitizerIn(DataFlow::Node node) { none() }
+
+  final override predicate isBarrierIn(DataFlow::Node node) { isSanitizerIn(node) }
+
+  /** Holds if taint propagation out of `node` is prohibited. */
+  predicate isSanitizerOut(DataFlow::Node node) { none() }
+
+  final override predicate isBarrierOut(DataFlow::Node node) { isSanitizerOut(node) }
+
+  /** Holds if taint propagation through nodes guarded by `guard` is prohibited. */
+  predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { none() }
+
+  final override predicate isBarrierGuard(DataFlow::BarrierGuard guard) { isSanitizerGuard(guard) }
+
+  /**
+   * Holds if the additional taint propagation step from `node1` to `node2`
+   * must be taken into account in the analysis.
+   */
+  predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { none() }
+
+  final override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+    isAdditionalTaintStep(node1, node2) or
+    defaultAdditionalTaintStep(node1, node2)
+  }
+
+  /**
+   * Holds if taint may flow from `source` to `sink` for this configuration.
+   */
+  // overridden to provide taint-tracking specific qldoc
+  override predicate hasFlow(DataFlow::Node source, DataFlow::Node sink) {
+    super.hasFlow(source, sink)
+  }
+}

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/tainttracking3/TaintTrackingImpl.qll

@@ -0,0 +1,5 @@
+import semmle.code.cpp.ir.dataflow.internal.TaintTrackingUtil as Public
+
+module Private {
+  import semmle.code.cpp.ir.dataflow.DataFlow3::DataFlow3 as DataFlow
+}