Merge pull request #4295 from rdmarsh2/rdmarsh2/cpp/ir-qualifier-flow

C++: Improved qualifier flow in IR taint tracking

Author: jbj

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

@@ -717,7 +717,12 @@ private predicate modelFlow(Operand opFrom, Instruction iTo) {
         iTo = outNode and
         outNode = getSideEffectFor(call, index)
       )
-      // TODO: add write side effects for qualifiers
+      or
+      exists(WriteSideEffectInstruction outNode |
+        modelOut.isQualifierObject() and
+        iTo = outNode and
+        outNode = getSideEffectFor(call, -1)
+      )
     ) and
     (
       exists(int index |
@@ -733,7 +738,12 @@ private predicate modelFlow(Operand opFrom, Instruction iTo) {
       or
       modelIn.isQualifierAddress() and
       opFrom = call.getThisArgumentOperand()
-      // TODO: add read side effects for qualifiers
+      or
+      exists(ReadSideEffectInstruction read |
+        modelIn.isQualifierObject() and
+        read = getSideEffectFor(call, -1) and
+        opFrom = read.getSideEffectOperand()
+      )
     )
   )
 }

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

@@ -9,24 +9,31 @@ private import semmle.code.cpp.ir.dataflow.DataFlow
 /**
  * Gets the instruction that goes into `input` for `call`.
  */
-Instruction callInput(CallInstruction call, FunctionInput input) {
+DataFlow::Node callInput(CallInstruction call, FunctionInput input) {
   // A positional argument
   exists(int index |
-    result = call.getPositionalArgument(index) and
+    result.asInstruction() = call.getPositionalArgument(index) and
     input.isParameter(index)
   )
   or
   // A value pointed to by a positional argument
   exists(ReadSideEffectInstruction read |
-    result = read and
+    result.asOperand() = read.getSideEffectOperand() and
     read.getPrimaryInstruction() = call and
     input.isParameterDeref(read.getIndex())
   )
   or
   // The qualifier pointer
-  result = call.getThisArgument() and
+  result.asInstruction() = call.getThisArgument() and
   input.isQualifierAddress()
-  //TODO: qualifier deref
+  or
+  // The qualifier object
+  exists(ReadSideEffectInstruction read |
+    result.asOperand() = read.getSideEffectOperand() and
+    read.getPrimaryInstruction() = call and
+    read.getIndex() = -1 and
+    input.isQualifierObject()
+  )
 }
 
 /**
@@ -43,5 +50,13 @@ Instruction callOutput(CallInstruction call, FunctionOutput output) {
     effect.getPrimaryInstruction() = call and
     output.isParameterDeref(effect.getIndex())
   )
-  // TODO: qualifiers, return value dereference
+  or
+  // The side effect of a call on the qualifier object
+  exists(WriteSideEffectInstruction effect |
+    result = effect and
+    effect.getPrimaryInstruction() = call and
+    effect.getIndex() = -1 and
+    output.isQualifierObject()
+  )
+  // TODO: return value dereference
 }

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

@@ -19,8 +19,11 @@ predicate localTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
  * local data flow steps. That is, `nodeFrom` and `nodeTo` are likely to represent
  * different objects.
  */
+cached
 predicate localAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
   localInstructionTaintStep(nodeFrom.asInstruction(), nodeTo.asInstruction())
+  or
+  modeledTaintStep(nodeFrom, nodeTo)
 }
 
 /**
@@ -49,8 +52,6 @@ private predicate localInstructionTaintStep(Instruction nodeFrom, Instruction no
   or
   nodeTo.(LoadInstruction).getSourceAddress() = nodeFrom
   or
-  modeledInstructionTaintStep(nodeFrom, nodeTo)
-  or
   // Flow through partial reads of arrays and unions
   nodeTo.(LoadInstruction).getSourceValueOperand().getAnyDef() = nodeFrom and
   not nodeFrom.isResultConflated() and
@@ -109,10 +110,17 @@ predicate defaultTaintSanitizer(DataFlow::Node node) { none() }
  * Holds if taint can flow from `instrIn` to `instrOut` through a call to a
  * modeled function.
  */
-predicate modeledInstructionTaintStep(Instruction instrIn, Instruction instrOut) {
+predicate modeledTaintStep(DataFlow::Node nodeIn, DataFlow::Node nodeOut) {
   exists(CallInstruction call, TaintFunction func, FunctionInput modelIn, FunctionOutput modelOut |
-    instrIn = callInput(call, modelIn) and
-    instrOut = callOutput(call, modelOut) and
+    (
+      nodeIn = callInput(call, modelIn)
+      or
+      exists(int n |
+        modelIn.isParameterDeref(n) and
+        nodeIn = callInput(call, any(InParameter inParam | inParam.getIndex() = n))
+      )
+    ) and
+    nodeOut.asInstruction() = callOutput(call, modelOut) and
     call.getStaticCallTarget() = func and
     func.hasTaintFlow(modelIn, modelOut)
   )
@@ -126,8 +134,8 @@ predicate modeledInstructionTaintStep(Instruction instrIn, Instruction instrOut)
     CallInstruction call, Function func, FunctionInput modelIn, OutParameterDeref modelMidOut,
     int indexMid, InParameter modelMidIn, OutReturnValue modelOut
   |
-    instrIn = callInput(call, modelIn) and
-    instrOut = callOutput(call, modelOut) and
+    nodeIn = callInput(call, modelIn) and
+    nodeOut.asInstruction() = callOutput(call, modelOut) and
     call.getStaticCallTarget() = func and
     func.(TaintFunction).hasTaintFlow(modelIn, modelMidOut) and
     func.(DataFlowFunction).hasDataFlow(modelMidIn, modelOut) and

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

@@ -21,7 +21,11 @@ class ConversionConstructorModel extends Constructor, TaintFunction {
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
     // taint flow from the first constructor argument to the returned object
     input.isParameter(0) and
-    output.isReturnValue() // TODO: this should be `isQualifierObject` by our current definitions, but that flow is not yet supported.
+    (
+      output.isReturnValue()
+      or
+      output.isQualifierObject()
+    )
   }
 }
 
@@ -32,7 +36,11 @@ class CopyConstructorModel extends CopyConstructor, DataFlowFunction {
   override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
     // data flow from the first constructor argument to the returned object
     input.isParameter(0) and
-    output.isReturnValue() // TODO: this should be `isQualifierObject` by our current definitions, but that flow is not yet supported.
+    (
+      output.isReturnValue()
+      or
+      output.isQualifierObject()
+    )
   }
 }
 
@@ -43,7 +51,11 @@ class MoveConstructorModel extends MoveConstructor, DataFlowFunction {
   override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
     // data flow from the first constructor argument to the returned object
     input.isParameter(0) and
-    output.isReturnValue() // TODO: this should be `isQualifierObject` by our current definitions, but that flow is not yet supported.
+    (
+      output.isReturnValue()
+      or
+      output.isQualifierObject()
+    )
   }
 }
 

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

@@ -38,7 +38,11 @@ class StdSequenceContainerConstructor extends Constructor, TaintFunction {
       input.isParameterDeref(getAValueTypeParameterIndex()) or
       input.isParameter(getAnIteratorParameterIndex())
     ) and
-    output.isReturnValue() // TODO: this should be `isQualifierObject` by our current definitions, but that flow is not yet supported.
+    (
+      output.isReturnValue() // TODO: this is only needed for AST data flow, which treats constructors as returning the new object
+      or
+      output.isQualifierObject()
+    )
   }
 }
 

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

@@ -47,7 +47,11 @@ class StdStringConstructor extends Constructor, TaintFunction {
       input.isParameterDeref(getAStringParameterIndex()) or
       input.isParameter(getAnIteratorParameterIndex())
     ) and
-    output.isReturnValue() // TODO: this should be `isQualifierObject` by our current definitions, but that flow is not yet supported.
+    (
+      output.isReturnValue() // TODO: this is only needed for AST data flow, which treats constructors as returning the new object
+      or
+      output.isQualifierObject()
+    )
   }
 }
 
@@ -573,7 +577,11 @@ class StdStringStreamConstructor extends Constructor, TaintFunction {
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
     // taint flow from any parameter of string type to the returned object
     input.isParameterDeref(getAStringParameterIndex()) and
-    output.isReturnValue() // TODO: this should be `isQualifierObject` by our current definitions, but that flow is not yet supported.
+    (
+      output.isReturnValue() // TODO: this is only needed for AST data flow, which treats constructors as returning the new object
+      or
+      output.isQualifierObject()
+    )
   }
 }
 

cpp/ql/test/library-tests/dataflow/taint-tests/IRTaintTestCommon.qll

@@ -1,12 +1,13 @@
 import cpp
+import semmle.code.cpp.ir.IR
 import semmle.code.cpp.ir.dataflow.TaintTracking
 
 /** Common data flow configuration to be used by tests. */
 class TestAllocationConfig extends TaintTracking::Configuration {
   TestAllocationConfig() { this = "TestAllocationConfig" }
 
   override predicate isSource(DataFlow::Node source) {
-    source.asExpr().(FunctionCall).getTarget().getName() = "source"
+    source.(DataFlow::ExprNode).getConvertedExpr().(FunctionCall).getTarget().getName() = "source"
     or
     source.asParameter().getName().matches("source%")
     or
@@ -17,8 +18,20 @@ class TestAllocationConfig extends TaintTracking::Configuration {
   override predicate isSink(DataFlow::Node sink) {
     exists(FunctionCall call |
       call.getTarget().getName() = "sink" and
-      sink.asExpr() = call.getAnArgument()
+      sink.(DataFlow::ExprNode).getConvertedExpr() = call.getAnArgument()
+      or
+      call.getTarget().getName() = "sink" and
+      sink.asExpr() = call.getAnArgument() and
+      sink.(DataFlow::ExprNode).getConvertedExpr() instanceof ReferenceDereferenceExpr
     )
+    or
+    sink
+        .asInstruction()
+        .(ReadSideEffectInstruction)
+        .getPrimaryInstruction()
+        .(CallInstruction)
+        .getStaticCallTarget()
+        .hasName("sink")
   }
 
   override predicate isSanitizer(DataFlow::Node barrier) {