C++: IR dataflow edges through outparams

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

@@ -199,7 +199,27 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
   // Flow through pointer dereference
   i2.(LoadInstruction).getSourceAddress() = i1
   or
-  i2.(UnaryInstruction).getUnary() = i1
+  // Flow through partial reads of arrays and unions
+  i2.(LoadInstruction).getSourceValueOperand().getAnyDef() = i1 and
+  not i1.isResultConflated() and
+  (
+    i1.getResultType() instanceof ArrayType or
+    i1.getResultType() instanceof Union
+  )
+  or
+  // Unary instructions tend to preserve enough information in practice that we
+  // want taint to flow through.
+  // The exception is `FieldAddressInstruction`. Together with the rule for
+  // `LoadInstruction` above and for `ChiInstruction` below, 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.
+  i2.(UnaryInstruction).getUnary() = i1 and
+  (
+    not i2 instanceof FieldAddressInstruction
+    or
+    i2.(FieldAddressInstruction).getField().getDeclaringType() instanceof Union
+  )
   or
   // Flow out of definition-by-reference
   i2.(ChiInstruction).getPartial() = i1.(WriteSideEffectInstruction) and
@@ -213,7 +233,7 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
     or
     t instanceof ArrayType
     or
-    // Buffers or unknown size
+    // Buffers of unknown size
     t instanceof UnknownType
   )
   or

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

@@ -70,8 +70,7 @@ private module VirtualDispatch {
         // Call return
         exists(DataFlowCall call, ReturnKind returnKind |
           other = getAnOutNode(call, returnKind) and
-          src.(ReturnNode).getKind() = returnKind and
-          call.getStaticCallTarget() = src.getEnclosingCallable()
+          returnNodeWithKindAndEnclosingCallable(src, returnKind, call.getStaticCallTarget())
         ) and
         allowFromArg = false
         or
@@ -125,6 +124,18 @@ private module VirtualDispatch {
     }
   }
 
+  /**
+   * A ReturnNode with its ReturnKind and its enclosing callable.
+   *
+   * Used to fix a join ordering issue in flowsFrom.
+   */
+  private predicate returnNodeWithKindAndEnclosingCallable(
+    ReturnNode node, ReturnKind kind, DataFlowCallable callable
+  ) {
+    node.getKind() = kind and
+    node.getEnclosingCallable() = callable
+  }
+
   /** Call through a function pointer. */
   private class DataSensitiveExprCall extends DataSensitiveCall {
     DataSensitiveExprCall() { not exists(this.getStaticCallTarget()) }

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

@@ -24,31 +24,86 @@ class ArgumentNode extends InstructionNode {
   DataFlowCall getCall() { this.argumentOf(result, _) }
 }
 
-private newtype TReturnKind = TNormalReturnKind()
+private newtype TReturnKind =
+  TNormalReturnKind() or
+  TIndirectReturnKind(ParameterIndex index)
 
 /**
  * A return kind. A return kind describes how a value can be returned
  * from a callable. For C++, this is simply a function return.
  */
 class ReturnKind extends TReturnKind {
   /** Gets a textual representation of this return kind. */
-  string toString() { result = "return" }
+  abstract string toString();
+}
+
+private class NormalReturnKind extends ReturnKind, TNormalReturnKind {
+  override string toString() { result = "return" }
+}
+
+private class IndirectReturnKind extends ReturnKind, TIndirectReturnKind {
+  ParameterIndex index;
+
+  IndirectReturnKind() { this = TIndirectReturnKind(index) }
+
+  override string toString() { result = "outparam[" + index.toString() + "]" }
 }
 
 /** A data flow node that occurs as the result of a `ReturnStmt`. */
 class ReturnNode extends InstructionNode {
-  ReturnNode() { exists(ReturnValueInstruction ret | this.getInstruction() = ret.getReturnValue()) }
+  Instruction primary;
+
+  ReturnNode() {
+    exists(ReturnValueInstruction ret | instr = ret.getReturnValue() and primary = ret)
+    or
+    exists(ReturnIndirectionInstruction rii |
+      instr = rii.getSideEffectOperand().getAnyDef() and primary = rii
+    )
+  }
 
   /** Gets the kind of this returned value. */
-  ReturnKind getKind() { result = TNormalReturnKind() }
+  abstract ReturnKind getKind();
+}
+
+class ReturnValueNode extends ReturnNode {
+  override ReturnValueInstruction primary;
+
+  override ReturnKind getKind() { result = TNormalReturnKind() }
+}
+
+class ReturnIndirectionNode extends ReturnNode {
+  override ReturnIndirectionInstruction primary;
+
+  override ReturnKind getKind() { result = TIndirectReturnKind(primary.getParameter().getIndex()) }
 }
 
 /** A data flow node that represents the output of a call. */
 class OutNode extends InstructionNode {
-  override CallInstruction instr;
+  OutNode() {
+    instr instanceof CallInstruction or
+    instr instanceof WriteSideEffectInstruction
+  }
 
   /** Gets the underlying call. */
-  DataFlowCall getCall() { result = instr }
+  abstract DataFlowCall getCall();
+
+  abstract ReturnKind getReturnKind();
+}
+
+private class CallOutNode extends OutNode {
+  override CallInstruction instr;
+
+  override DataFlowCall getCall() { result = instr }
+
+  override ReturnKind getReturnKind() { result instanceof NormalReturnKind }
+}
+
+private class SideEffectOutNode extends OutNode {
+  override WriteSideEffectInstruction instr;
+
+  override DataFlowCall getCall() { result = instr.getPrimaryInstruction() }
+
+  override ReturnKind getReturnKind() { result = TIndirectReturnKind(instr.getIndex()) }
 }
 
 /**
@@ -57,7 +112,7 @@ class OutNode extends InstructionNode {
  */
 OutNode getAnOutNode(DataFlowCall call, ReturnKind kind) {
   result.getCall() = call and
-  kind = TNormalReturnKind()
+  result.getReturnKind() = kind
 }
 
 /**

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll

@@ -525,7 +525,7 @@ class ReturnValueInstruction extends ReturnInstruction {
   final Instruction getReturnValue() { result = getReturnValueOperand().getDef() }
 }
 
-class ReturnIndirectionInstruction extends Instruction {
+class ReturnIndirectionInstruction extends VariableInstruction {
   ReturnIndirectionInstruction() { getOpcode() instanceof Opcode::ReturnIndirection }
 
   final SideEffectOperand getSideEffectOperand() { result = getAnOperand() }
@@ -535,6 +535,12 @@ class ReturnIndirectionInstruction extends Instruction {
   final AddressOperand getSourceAddressOperand() { result = getAnOperand() }
 
   final Instruction getSourceAddress() { result = getSourceAddressOperand().getDef() }
+
+  /**
+   * Gets the parameter for which this instruction reads the final pointed-to value within the
+   * function.
+   */
+  final Language::Parameter getParameter() { result = var.(IRUserVariable).getVariable() }
 }
 
 class CopyInstruction extends Instruction {

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll

@@ -525,7 +525,7 @@ class ReturnValueInstruction extends ReturnInstruction {
   final Instruction getReturnValue() { result = getReturnValueOperand().getDef() }
 }
 
-class ReturnIndirectionInstruction extends Instruction {
+class ReturnIndirectionInstruction extends VariableInstruction {
   ReturnIndirectionInstruction() { getOpcode() instanceof Opcode::ReturnIndirection }
 
   final SideEffectOperand getSideEffectOperand() { result = getAnOperand() }
@@ -535,6 +535,12 @@ class ReturnIndirectionInstruction extends Instruction {
   final AddressOperand getSourceAddressOperand() { result = getAnOperand() }
 
   final Instruction getSourceAddress() { result = getSourceAddressOperand().getDef() }
+
+  /**
+   * Gets the parameter for which this instruction reads the final pointed-to value within the
+   * function.
+   */
+  final Language::Parameter getParameter() { result = var.(IRUserVariable).getVariable() }
 }
 
 class CopyInstruction extends Instruction {

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedFunction.qll

@@ -744,4 +744,9 @@ class TranslatedReadEffect extends TranslatedElement, TTranslatedReadEffect {
     operandTag = sideEffectOperand() and
     result = getUnknownType()
   }
+
+  final override IRVariable getInstructionVariable(InstructionTag tag) {
+    tag = OnlyInstructionTag() and
+    result = getIRUserVariable(getFunction(), param)
+  }
 }

cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll

@@ -525,7 +525,7 @@ class ReturnValueInstruction extends ReturnInstruction {
   final Instruction getReturnValue() { result = getReturnValueOperand().getDef() }
 }
 
-class ReturnIndirectionInstruction extends Instruction {
+class ReturnIndirectionInstruction extends VariableInstruction {
   ReturnIndirectionInstruction() { getOpcode() instanceof Opcode::ReturnIndirection }
 
   final SideEffectOperand getSideEffectOperand() { result = getAnOperand() }
@@ -535,6 +535,12 @@ class ReturnIndirectionInstruction extends Instruction {
   final AddressOperand getSourceAddressOperand() { result = getAnOperand() }
 
   final Instruction getSourceAddress() { result = getSourceAddressOperand().getDef() }
+
+  /**
+   * Gets the parameter for which this instruction reads the final pointed-to value within the
+   * function.
+   */
+  final Language::Parameter getParameter() { result = var.(IRUserVariable).getVariable() }
 }
 
 class CopyInstruction extends Instruction {

cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp

@@ -86,4 +86,14 @@ namespace std {
 
 void test_std_move() {
   sink(std::move(getenv("VAR")));
+}
+
+void flow_to_outparam(char ** ret, char *arg) {
+    *ret = arg; 
+}
+
+void test_outparams() {
+    char *p2 = nullptr;
+    flow_to_outparam(&p2, getenv("VAR"));
+    sink(p2); // tainted
 }

cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected

@@ -101,6 +101,14 @@
 | defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:18:88:23 | call to getenv |
 | defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:18:88:30 | (reference to) |
 | defaulttainttracking.cpp:88:18:88:23 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:91:42:91:44 | arg |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:92:12:92:14 | arg |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:96:11:96:12 | p2 |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:97:27:97:32 | call to getenv |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:98:10:98:11 | (const char *)... |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:98:10:98:11 | p2 |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 |
 | globals.cpp:5:20:5:25 | call to getenv | globals.cpp:2:17:2:25 | sinkParam |
 | globals.cpp:5:20:5:25 | call to getenv | globals.cpp:5:12:5:16 | local |
 | globals.cpp:5:20:5:25 | call to getenv | globals.cpp:5:20:5:25 | call to getenv |

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

@@ -15,6 +15,14 @@
 | defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:8:88:32 | (reference dereference) | IR only |
 | defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:18:88:30 | (reference to) | IR only |
 | defaulttainttracking.cpp:88:18:88:23 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 | IR only |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 | IR only |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:91:31:91:33 | ret | AST only |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:92:5:92:8 | * ... | AST only |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:92:6:92:8 | ret | AST only |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:96:11:96:12 | p2 | IR only |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:98:10:98:11 | (const char *)... | IR only |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:98:10:98:11 | p2 | IR only |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 | IR only |
 | globals.cpp:13:15:13:20 | call to getenv | globals.cpp:13:5:13:11 | global1 | AST only |
 | globals.cpp:23:15:23:20 | call to getenv | globals.cpp:23:5:23:11 | global2 | AST only |
 | test_diff.cpp:104:12:104:15 | argv | test_diff.cpp:104:11:104:20 | (...) | IR only |

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

@@ -460,3 +460,13 @@ void throughStmtExpr(int source1, int clean1) {
   });
   sink(local); // tainted
 }
+
+void intOutparamSource(int *p) {
+  *p = source();
+}
+
+void viaOutparam() {
+  int x = 0;
+  intOutparamSource(&x);
+  sink(x); // tainted [FALSE NEGATIVE]
+}