Merge pull request github#12316 from MathiasVP/no-taint-indirect-direct-conflation

C++: Remove indirect -> direct taint-flow

Author: MathiasVP

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

@@ -441,10 +441,6 @@ class OperandNode extends Node, Node0 {
 Type stripPointer(Type t) {
   result = any(Ssa::Indirection ind | ind.getType() = t).getBaseType()
   or
-  // These types have a sensible base type, but don't receive additional
-  // dataflow nodes representing their indirections. So for now we special case them.
-  result = t.(ArrayType).getBaseType()
-  or
   result = t.(PointerToMemberType).getBaseType()
   or
   result = t.(FunctionPointerIshType).getBaseType()
@@ -813,7 +809,7 @@ private class PostIndirectReturnOutNode extends IndirectReturnOutNode, PostUpdat
  *
  * Returns `t`, but stripped of the outer-most `indirectionIndex` number of indirections.
  */
-Type getTypeImpl(Type t, int indirectionIndex) {
+private Type getTypeImpl0(Type t, int indirectionIndex) {
   indirectionIndex = 0 and
   result = t
   or
@@ -823,12 +819,30 @@ Type getTypeImpl(Type t, int indirectionIndex) {
     // We need to avoid the case where `stripPointer(t) = t` (which can happen on
     // iterators that specify a `value_type` that is the iterator itself). Such a type
     // would create an infinite loop otherwise. For these cases we simply don't produce
-    // a result for `getType`.
+    // a result for `getTypeImpl`.
     stripped.getUnspecifiedType() != t.getUnspecifiedType() and
-    result = getTypeImpl(stripped, indirectionIndex - 1)
+    result = getTypeImpl0(stripped, indirectionIndex - 1)
   )
 }
 
+/**
+ * INTERNAL: Do not use.
+ *
+ * Returns `t`, but stripped of the outer-most `indirectionIndex` number of indirections.
+ *
+ * If `indirectionIndex` cannot be stripped off `t`, an `UnknownType` is returned.
+ */
+bindingset[indirectionIndex]
+Type getTypeImpl(Type t, int indirectionIndex) {
+  result = getTypeImpl0(t, indirectionIndex)
+  or
+  // If we cannot produce the right type we return an error type.
+  // This can sometimes happen when we don't know the real
+  // type of a void pointer.
+  not exists(getTypeImpl0(t, indirectionIndex)) and
+  result instanceof UnknownType
+}
+
 /**
  * INTERNAL: Do not use.
  *
@@ -988,7 +1002,7 @@ private predicate indirectExprNodeShouldBeIndirectOperand(RawIndirectOperand nod
       e = e0.getFullyConverted()
     )
     or
-    not indirectExprNodeShouldBeIndirectOperand0(_, _, e.getUnconverted()) and
+    not indirectExprNodeShouldBeIndirectOperand0(_, node, _) and
     e = instr.getConvertedResultExpression()
   )
 }

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

@@ -19,6 +19,7 @@ private import semmle.code.cpp.ir.dataflow.TaintTracking
 private import semmle.code.cpp.ir.dataflow.TaintTracking2
 private import semmle.code.cpp.ir.dataflow.TaintTracking3
 private import semmle.code.cpp.ir.dataflow.internal.ModelUtil
+private import semmle.code.cpp.ir.dataflow.internal.DataFlowPrivate
 
 /**
  * A predictable instruction is one where an external user can predict
@@ -75,6 +76,20 @@ private DataFlow::Node getNodeForExpr(Expr node) {
   not argv(node.(VariableAccess).getTarget())
 }
 
+private predicate conflatePointerAndPointee(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+  // Flow from `op` to `*op`.
+  exists(Operand operand, int indirectionIndex |
+    nodeHasOperand(nodeFrom, operand, indirectionIndex) and
+    nodeHasOperand(nodeTo, operand, indirectionIndex - 1)
+  )
+  or
+  // Flow from `instr` to `*instr`.
+  exists(Instruction instr, int indirectionIndex |
+    nodeHasInstruction(nodeFrom, instr, indirectionIndex) and
+    nodeHasInstruction(nodeTo, instr, indirectionIndex - 1)
+  )
+}
+
 private class DefaultTaintTrackingCfg extends TaintTracking::Configuration {
   DefaultTaintTrackingCfg() { this = "DefaultTaintTrackingCfg" }
 
@@ -85,6 +100,10 @@ private class DefaultTaintTrackingCfg extends TaintTracking::Configuration {
   override predicate isSanitizer(DataFlow::Node node) { nodeIsBarrier(node) }
 
   override predicate isSanitizerIn(DataFlow::Node node) { nodeIsBarrierIn(node) }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+    conflatePointerAndPointee(nodeFrom, nodeTo)
+  }
 }
 
 private class ToGlobalVarTaintTrackingCfg extends TaintTracking::Configuration {
@@ -417,6 +436,8 @@ module TaintedWithPath {
     }
 
     override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
+      conflatePointerAndPointee(n1, n2)
+      or
       // Steps into and out of global variables
       exists(TaintTrackingConfiguration cfg | cfg.taintThroughGlobals() |
         writesVariable(n1.asInstruction(), n2.asVariable().(GlobalOrNamespaceVariable))