Merge pull request #14571 from MathiasVP/fix-indirect-taint

C++: Fix indirect taint

Author: jketema

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

@@ -32,25 +32,34 @@ DataFlow::Node callInput(CallInstruction call, FunctionInput input) {
 }
 
 /**
- * Gets the instruction that holds the `output` for `call`.
+ * Gets the node that represents the output of `call` with kind `output` at
+ * indirection index `indirectionIndex`.
  */
-Node callOutput(CallInstruction call, FunctionOutput output) {
+private Node callOutputWithIndirectionIndex(
+  CallInstruction call, FunctionOutput output, int indirectionIndex
+) {
   // The return value
   simpleOutNode(result, call) and
-  output.isReturnValue()
+  output.isReturnValue() and
+  indirectionIndex = 0
   or
   // The side effect of a call on the value pointed to by an argument or qualifier
-  exists(int index, int indirectionIndex |
+  exists(int index |
     result.(IndirectArgumentOutNode).getArgumentIndex() = index and
-    result.(IndirectArgumentOutNode).getIndirectionIndex() = indirectionIndex and
+    result.(IndirectArgumentOutNode).getIndirectionIndex() = indirectionIndex - 1 and
     result.(IndirectArgumentOutNode).getCallInstruction() = call and
-    output.isParameterDerefOrQualifierObject(index, indirectionIndex)
+    output.isParameterDerefOrQualifierObject(index, indirectionIndex - 1)
   )
   or
-  exists(int ind |
-    result = getIndirectReturnOutNode(call, ind) and
-    output.isReturnValueDeref(ind)
-  )
+  result = getIndirectReturnOutNode(call, indirectionIndex) and
+  output.isReturnValueDeref(indirectionIndex)
+}
+
+/**
+ * Gets the instruction that holds the `output` for `call`.
+ */
+Node callOutput(CallInstruction call, FunctionOutput output) {
+  result = callOutputWithIndirectionIndex(call, output, _)
 }
 
 DataFlow::Node callInput(CallInstruction call, FunctionInput input, int d) {
@@ -76,19 +85,15 @@ private IndirectReturnOutNode getIndirectReturnOutNode(CallInstruction call, int
  */
 bindingset[d]
 Node callOutput(CallInstruction call, FunctionOutput output, int d) {
-  exists(DataFlow::Node n | n = callOutput(call, output) and d > 0 |
+  exists(DataFlow::Node n, int indirectionIndex |
+    n = callOutputWithIndirectionIndex(call, output, indirectionIndex) and d > 0
+  |
     // The return value
-    result = getIndirectReturnOutNode(n.asInstruction(), d)
+    result = callOutputWithIndirectionIndex(call, output, indirectionIndex + d)
     or
     // If there isn't an indirect out node for the call with indirection `d` then
     // we conflate this with the underlying `CallInstruction`.
-    not exists(getIndirectReturnOutNode(call, d)) and
+    not exists(getIndirectReturnOutNode(call, indirectionIndex + d)) and
     n = result
-    or
-    // The side effect of a call on the value pointed to by an argument or qualifier
-    exists(Operand operand, int indirectionIndex |
-      Ssa::outNodeHasAddressAndIndex(n, operand, indirectionIndex) and
-      Ssa::outNodeHasAddressAndIndex(result, operand, indirectionIndex + d)
-    )
   )
 }

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternalsCommon.qll

@@ -228,7 +228,7 @@ private class PointerWrapperTypeIndirection extends Indirection instanceof Point
   override predicate isAdditionalDereference(Instruction deref, Operand address) {
     exists(CallInstruction call |
       operandForFullyConvertedCall(getAUse(deref), call) and
-      this = call.getStaticCallTarget().getClassAndName("operator*") and
+      this = call.getStaticCallTarget().getClassAndName(["operator*", "operator->", "get"]) and
       address = call.getThisArgumentOperand()
     )
   }

cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-ir-consistency.expected

@@ -20,12 +20,8 @@ reverseRead
 argHasPostUpdate
 postWithInFlow
 | test.cpp:384:10:384:13 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
-| test.cpp:384:10:384:13 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
-| test.cpp:391:10:391:13 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
 | test.cpp:391:10:391:13 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
 | test.cpp:400:10:400:13 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
-| test.cpp:400:10:400:13 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
-| test.cpp:407:10:407:13 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
 | test.cpp:407:10:407:13 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
 viableImplInCallContextTooLarge
 uniqueParameterNodeAtPosition

cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected

@@ -44,8 +44,6 @@ reverseRead
 argHasPostUpdate
 postWithInFlow
 | realistic.cpp:54:16:54:47 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
-| realistic.cpp:54:16:54:47 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
-| realistic.cpp:60:16:60:18 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
 | realistic.cpp:60:16:60:18 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
 viableImplInCallContextTooLarge
 uniqueParameterNodeAtPosition

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

@@ -2199,6 +2199,19 @@ WARNING: Module TaintTracking has been deprecated and may be removed in future (
 | map.cpp:436:55:436:59 | def | map.cpp:436:19:436:60 | call to pair | TAINT |
 | map.cpp:436:63:436:67 | first | map.cpp:436:7:436:67 | call to iterator |  |
 | map.cpp:437:7:437:9 | m35 | map.cpp:437:7:437:9 | call to unordered_map |  |
+| map.cpp:446:23:446:23 | call to map | map.cpp:448:3:448:3 | m |  |
+| map.cpp:446:23:446:23 | call to map | map.cpp:449:12:449:12 | m |  |
+| map.cpp:446:23:446:23 | call to map | map.cpp:451:1:451:1 | m |  |
+| map.cpp:447:12:447:26 | call to indirect_source | map.cpp:448:10:448:10 | p |  |
+| map.cpp:448:3:448:3 | m | map.cpp:448:4:448:4 | call to operator[] | TAINT |
+| map.cpp:448:3:448:3 | ref arg m | map.cpp:449:12:449:12 | m |  |
+| map.cpp:448:3:448:3 | ref arg m | map.cpp:451:1:451:1 | m |  |
+| map.cpp:448:3:448:10 | ... = ... | map.cpp:448:4:448:4 | call to operator[] [post update] |  |
+| map.cpp:448:4:448:4 | call to operator[] [post update] | map.cpp:448:3:448:3 | ref arg m | TAINT |
+| map.cpp:448:10:448:10 | p | map.cpp:448:3:448:10 | ... = ... |  |
+| map.cpp:449:12:449:12 | m | map.cpp:449:13:449:13 | call to operator[] | TAINT |
+| map.cpp:449:12:449:12 | ref arg m | map.cpp:451:1:451:1 | m |  |
+| map.cpp:449:13:449:13 | call to operator[] | map.cpp:450:8:450:8 | q |  |
 | movableclass.cpp:8:2:8:15 | this | movableclass.cpp:8:27:8:31 | constructor init of field v [pre-this] |  |
 | movableclass.cpp:8:21:8:22 | _v | movableclass.cpp:8:29:8:30 | _v |  |
 | movableclass.cpp:8:29:8:30 | _v | movableclass.cpp:8:27:8:31 | constructor init of field v | TAINT |

cpp/ql/test/library-tests/dataflow/taint-tests/map.cpp

@@ -436,3 +436,16 @@ void test_unordered_map()
 	sink(m35.emplace(std::pair<char *, char *>(source(), "def")).first); // $ MISSING: ast,ir
 	sink(m35); // $ MISSING: ast,ir
 }
+
+namespace {
+	int* indirect_source();
+	void indirect_sink(int*);
+}
+
+void test_indirect_taint() {
+  std::map<int, int*> m;
+  int* p = indirect_source();
+  m[1] = p;
+  int* q = m[1];
+  sink(q); // $ ir MISSING: ast
+}

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

@@ -84,6 +84,8 @@ module IRTest {
       or
       source.asIndirectExpr().(FunctionCall).getTarget().getName() = "source"
       or
+      source.asIndirectExpr().(FunctionCall).getTarget().getName() = "indirect_source"
+      or
       source.asParameter().getName().matches("source%")
       or
       exists(FunctionCall fc |