Merge pull request #4272 from jbj/dataflow-partial-access

C++: Add AST flow through arrays

Author: MathiasVP

cpp/ql/src/semmle/code/cpp/dataflow/internal/AddressFlow.qll

@@ -29,7 +29,7 @@ private predicate stdIdentityFunction(Function f) { f.hasQualifiedName("std", ["
  */
 private predicate stdAddressOf(Function f) { f.hasQualifiedName("std", "addressof") }
 
-private predicate lvalueToLvalueStep(Expr lvalueIn, Expr lvalueOut) {
+private predicate lvalueToLvalueStepPure(Expr lvalueIn, Expr lvalueOut) {
   lvalueIn.getConversion() = lvalueOut.(ParenthesisExpr)
   or
   // When an object is implicitly converted to a reference to one of its base
@@ -42,6 +42,10 @@ private predicate lvalueToLvalueStep(Expr lvalueIn, Expr lvalueOut) {
   // such casts.
   lvalueIn.getConversion() = lvalueOut and
   lvalueOut.(CStyleCast).isImplicit()
+}
+
+private predicate lvalueToLvalueStep(Expr lvalueIn, Expr lvalueOut) {
+  lvalueToLvalueStepPure(lvalueIn, lvalueOut)
   or
   // C++ only
   lvalueIn = lvalueOut.(PrefixCrementOperation).getOperand().getFullyConverted()
@@ -214,6 +218,69 @@ private predicate referenceToUpdate(Expr reference, Expr outer, ControlFlowNode
   )
 }
 
+private predicate lvalueFromVariableAccess(VariableAccess va, Expr lvalue) {
+  // Base case for non-reference types.
+  lvalue = va and
+  not va.getConversion() instanceof ReferenceDereferenceExpr
+  or
+  // Base case for reference types where we pretend that they are
+  // non-reference types. The type of the target of `va` can be `ReferenceType`
+  // or `FunctionReferenceType`.
+  lvalue = va.getConversion().(ReferenceDereferenceExpr)
+  or
+  // lvalue -> lvalue
+  exists(Expr prev |
+    lvalueFromVariableAccess(va, prev) and
+    lvalueToLvalueStep(prev, lvalue)
+  )
+  or
+  // pointer -> lvalue
+  exists(Expr prev |
+    pointerFromVariableAccess(va, prev) and
+    pointerToLvalueStep(prev, lvalue)
+  )
+  or
+  // reference -> lvalue
+  exists(Expr prev |
+    referenceFromVariableAccess(va, prev) and
+    referenceToLvalueStep(prev, lvalue)
+  )
+}
+
+private predicate pointerFromVariableAccess(VariableAccess va, Expr pointer) {
+  // pointer -> pointer
+  exists(Expr prev |
+    pointerFromVariableAccess(va, prev) and
+    pointerToPointerStep(prev, pointer)
+  )
+  or
+  // reference -> pointer
+  exists(Expr prev |
+    referenceFromVariableAccess(va, prev) and
+    referenceToPointerStep(prev, pointer)
+  )
+  or
+  // lvalue -> pointer
+  exists(Expr prev |
+    lvalueFromVariableAccess(va, prev) and
+    lvalueToPointerStep(prev, pointer)
+  )
+}
+
+private predicate referenceFromVariableAccess(VariableAccess va, Expr reference) {
+  // reference -> reference
+  exists(Expr prev |
+    referenceFromVariableAccess(va, prev) and
+    referenceToReferenceStep(prev, reference)
+  )
+  or
+  // lvalue -> reference
+  exists(Expr prev |
+    lvalueFromVariableAccess(va, prev) and
+    lvalueToReferenceStep(prev, reference)
+  )
+}
+
 /**
  * Holds if `node` is a control-flow node that may modify `inner` (or what it
  * points to) through `outer`. The two expressions may be `Conversion`s. Plain
@@ -236,7 +303,7 @@ predicate valueToUpdate(Expr inner, Expr outer, ControlFlowNode node) {
   (
     inner instanceof VariableAccess and
     // Don't track non-field assignments
-    (assignmentTo(outer, _) implies inner instanceof FieldAccess)
+    not (assignmentTo(outer, _) and outer.(VariableAccess).getTarget() instanceof StackVariable)
     or
     inner instanceof ThisExpr
     or
@@ -245,3 +312,27 @@ predicate valueToUpdate(Expr inner, Expr outer, ControlFlowNode node) {
     // can't do anything useful with those at the moment.
   )
 }
+
+/**
+ * Holds if `e` is a fully-converted expression that evaluates to an lvalue
+ * derived from `va` and is used for reading from or assigning to. This is in
+ * contrast with a variable access that is used for taking an address (`&x`)
+ * or simply discarding its value (`x;`).
+ *
+ * This analysis does not propagate across assignments or calls, and unlike
+ * `variableAccessedAsValue` in `semmle.code.cpp.dataflow.EscapesTree` it
+ * propagates through array accesses but not field accesses. The analysis is
+ * also not concerned with whether the lvalue `e` is converted to an rvalue --
+ * to examine that, use the relevant member predicates on `Expr`.
+ *
+ * If `va` has reference type, the analysis concerns the value pointed to by
+ * the reference rather than the reference itself. The expression `e` may be a
+ * `Conversion`.
+ */
+predicate variablePartiallyAccessed(VariableAccess va, Expr e) {
+  lvalueFromVariableAccess(va, e) and
+  not lvalueToLvalueStepPure(e, _) and
+  not lvalueToPointerStep(e, _) and
+  not lvalueToReferenceStep(e, _) and
+  not e = any(ExprInVoidContext eivc | e = eivc.getConversion*())
+}

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

@@ -6,6 +6,7 @@ private import cpp
 private import semmle.code.cpp.dataflow.internal.FlowVar
 private import semmle.code.cpp.models.interfaces.DataFlow
 private import semmle.code.cpp.controlflow.Guards
+private import semmle.code.cpp.dataflow.internal.AddressFlow
 
 cached
 private newtype TNode =
@@ -610,6 +611,15 @@ private predicate exprToExprStep_nocfg(Expr fromExpr, Expr toExpr) {
   or
   toExpr.(AddressOfExpr).getOperand() = fromExpr
   or
+  // This rule enables flow from an array to its elements. Example: `a` to
+  // `a[i]` or `*a`, where `a` is an array type. It does not enable flow from a
+  // pointer to its indirection as in `p[i]` where `p` is a pointer type.
+  exists(Expr toConverted |
+    variablePartiallyAccessed(fromExpr, toConverted) and
+    toExpr = toConverted.getUnconverted() and
+    not toExpr = fromExpr
+  )
+  or
   toExpr.(BuiltInOperationBuiltInAddressOf).getOperand() = fromExpr
   or
   // The following case is needed to track the qualifier object for flow

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

@@ -48,6 +48,6 @@ void following_pointers(
 
   int stackArray[2] = { source(), source() };
   stackArray[0] = source();
-  sink(stackArray); // no flow
+  sink(stackArray); // flow
 }
 

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

@@ -13,6 +13,7 @@
 | example.c:24:13:24:18 | coords [post update] | example.c:26:19:26:24 | coords |
 | example.c:24:13:24:30 | ... = ... | example.c:24:2:24:30 | ... = ... |
 | example.c:24:13:24:30 | ... = ... | example.c:24:20:24:20 | y [post update] |
+| example.c:24:20:24:20 | y | example.c:24:13:24:30 | ... = ... |
 | example.c:24:24:24:30 | ... + ... | example.c:24:13:24:30 | ... = ... |
 | example.c:26:2:26:25 | ... = ... | example.c:26:9:26:9 | x [post update] |
 | example.c:26:13:26:16 | call to getX | example.c:26:2:26:25 | ... = ... |

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

@@ -428,7 +428,7 @@ void intPointerSourceCaller2() {
   int local[1];
   intPointerSource(local);
   sink(local); // tainted
-  sink(*local); // clean
+  sink(*local); // tainted
 }
 
 void intArraySourceCaller() {
@@ -441,7 +441,7 @@ void intArraySourceCaller2() {
   int local[2];
   intArraySource(local, 2);
   sink(local); // tainted
-  sink(*local); // clean
+  sink(*local); // tainted
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -468,5 +468,5 @@ void intOutparamSource(int *p) {
 void viaOutparam() {
   int x = 0;
   intOutparamSource(&x);
-  sink(x); // tainted [FALSE NEGATIVE]
+  sink(x); // tainted
 }

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

@@ -16,6 +16,7 @@
 | clang.cpp:30:27:30:34 | call to getFirst | clang.cpp:28:27:28:32 | call to source |
 | clang.cpp:37:10:37:11 | m2 | clang.cpp:34:32:34:37 | call to source |
 | clang.cpp:45:17:45:18 | m2 | clang.cpp:43:35:43:40 | call to source |
+| clang.cpp:51:8:51:17 | stackArray | clang.cpp:50:19:50:24 | call to source |
 | dispatch.cpp:11:38:11:38 | x | dispatch.cpp:37:19:37:24 | call to source |
 | dispatch.cpp:11:38:11:38 | x | dispatch.cpp:45:18:45:23 | call to source |
 | dispatch.cpp:35:16:35:25 | call to notSource1 | dispatch.cpp:9:37:9:42 | call to source |
@@ -79,12 +80,17 @@
 | test.cpp:424:8:424:12 | local | test.cpp:423:20:423:25 | ref arg & ... |
 | test.cpp:430:8:430:12 | local | test.cpp:428:7:428:11 | local |
 | test.cpp:430:8:430:12 | local | test.cpp:429:20:429:24 | ref arg local |
+| test.cpp:431:8:431:13 | * ... | test.cpp:428:7:428:11 | local |
+| test.cpp:431:8:431:13 | * ... | test.cpp:429:20:429:24 | ref arg local |
 | test.cpp:437:8:437:12 | local | test.cpp:435:7:435:11 | local |
 | test.cpp:437:8:437:12 | local | test.cpp:436:18:436:23 | ref arg & ... |
 | test.cpp:443:8:443:12 | local | test.cpp:441:7:441:11 | local |
 | test.cpp:443:8:443:12 | local | test.cpp:442:18:442:22 | ref arg local |
+| test.cpp:444:8:444:13 | * ... | test.cpp:441:7:441:11 | local |
+| test.cpp:444:8:444:13 | * ... | test.cpp:442:18:442:22 | ref arg local |
 | test.cpp:450:9:450:22 | (statement expression) | test.cpp:449:26:449:32 | source1 |
 | test.cpp:461:8:461:12 | local | test.cpp:449:26:449:32 | source1 |
+| test.cpp:471:8:471:8 | x | test.cpp:465:8:465:13 | call to source |
 | true_upon_entry.cpp:21:8:21:8 | x | true_upon_entry.cpp:17:11:17:16 | call to source |
 | true_upon_entry.cpp:29:8:29:8 | x | true_upon_entry.cpp:27:9:27:14 | call to source |
 | true_upon_entry.cpp:39:8:39:8 | x | true_upon_entry.cpp:33:11:33:16 | call to source |

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

@@ -2,6 +2,7 @@
 | BarrierGuard.cpp:60:11:60:16 | BarrierGuard.cpp:62:14:62:14 | AST only |
 | clang.cpp:12:9:12:20 | clang.cpp:22:8:22:20 | AST only |
 | clang.cpp:39:42:39:47 | clang.cpp:41:18:41:19 | IR only |
+| clang.cpp:50:19:50:24 | clang.cpp:51:8:51:17 | AST only |
 | dispatch.cpp:16:37:16:42 | dispatch.cpp:32:16:32:24 | IR only |
 | dispatch.cpp:16:37:16:42 | dispatch.cpp:40:15:40:23 | IR only |
 | dispatch.cpp:22:37:22:42 | dispatch.cpp:31:16:31:24 | IR only |
@@ -41,11 +42,16 @@
 | test.cpp:422:7:422:11 | test.cpp:424:8:424:12 | AST only |
 | test.cpp:423:20:423:25 | test.cpp:424:8:424:12 | AST only |
 | test.cpp:428:7:428:11 | test.cpp:430:8:430:12 | AST only |
+| test.cpp:428:7:428:11 | test.cpp:431:8:431:13 | AST only |
 | test.cpp:429:20:429:24 | test.cpp:430:8:430:12 | AST only |
+| test.cpp:429:20:429:24 | test.cpp:431:8:431:13 | AST only |
 | test.cpp:435:7:435:11 | test.cpp:437:8:437:12 | AST only |
 | test.cpp:436:18:436:23 | test.cpp:437:8:437:12 | AST only |
 | test.cpp:441:7:441:11 | test.cpp:443:8:443:12 | AST only |
+| test.cpp:441:7:441:11 | test.cpp:444:8:444:13 | AST only |
 | test.cpp:442:18:442:22 | test.cpp:443:8:443:12 | AST only |
+| test.cpp:442:18:442:22 | test.cpp:444:8:444:13 | AST only |
+| test.cpp:465:8:465:13 | test.cpp:471:8:471:8 | AST only |
 | true_upon_entry.cpp:9:11:9:16 | true_upon_entry.cpp:13:8:13:8 | IR only |
 | true_upon_entry.cpp:62:11:62:16 | true_upon_entry.cpp:66:8:66:8 | IR only |
 | true_upon_entry.cpp:98:11:98:16 | true_upon_entry.cpp:105:8:105:8 | IR only |

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

@@ -0,0 +1,51 @@
+void sink(void *o);
+void *user_input(void);
+
+void local_array() {
+  void *arr[10] = { 0 };
+  arr[0] = user_input();
+  sink(arr[0]); // $ast,ir
+  sink(arr[1]); // $f+:ast
+  sink(*arr); // $ast,ir
+  sink(*&arr[0]); // $ast,ir
+}
+
+void local_array_convoluted_assign() {
+  void *arr[10] = { 0 };
+  *&arr[0] = user_input();
+  sink(arr[0]); // $ast,ir
+  sink(arr[1]); // $f+:ast
+}
+
+struct inner {
+  void *data;
+  int unrelated;
+};
+
+struct middle {
+  inner arr[10];
+  inner *ptr;
+};
+
+struct outer {
+  middle nested;
+  middle *indirect;
+};
+
+void nested_array_1(outer o) {
+  o.nested.arr[1].data = user_input();
+  sink(o.nested.arr[1].data); // $ast,ir
+  sink(o.nested.arr[0].data); // $f+:ast
+}
+
+void nested_array_2(outer o) {
+  o.indirect->arr[1].data = user_input();
+  sink(o.indirect->arr[1].data); // $ast $f-:ir
+  sink(o.indirect->arr[0].data); // $f+:ast
+}
+
+void nested_array_3(outer o) {
+  o.indirect->ptr[1].data = user_input();
+  sink(o.indirect->ptr[1].data); // $f-:ast,ir
+  sink(o.indirect->ptr[0].data);
+}

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

@@ -109,11 +109,11 @@ void test_outer_with_ptr(Outer *pouter) {
 
   sink(outer.inner_nested.a); // $ast,ir
   sink(outer.inner_ptr->a); // $ast $f-:ir
-  sink(outer.a); // $f-:ast $f-:ir
+  sink(outer.a); // $ast $f-:ir
 
   sink(pouter->inner_nested.a); // $ast,ir
   sink(pouter->inner_ptr->a); // $ast $f-:ir
-  sink(pouter->a); // $f-:ast $f-:ir
+  sink(pouter->a); // $ast $f-:ir
 }
 
 void test_outer_with_ref(Outer *pouter) {

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

@@ -30,6 +30,12 @@ argHasPostUpdate
 | D.cpp:43:24:43:40 | new | ArgumentNode is missing PostUpdateNode. |
 | D.cpp:50:24:50:40 | new | ArgumentNode is missing PostUpdateNode. |
 | D.cpp:57:25:57:41 | new | ArgumentNode is missing PostUpdateNode. |
+| arrays.cpp:7:8:7:13 | access to array | ArgumentNode is missing PostUpdateNode. |
+| arrays.cpp:8:8:8:13 | access to array | ArgumentNode is missing PostUpdateNode. |
+| arrays.cpp:9:8:9:11 | * ... | ArgumentNode is missing PostUpdateNode. |
+| arrays.cpp:10:8:10:15 | * ... | ArgumentNode is missing PostUpdateNode. |
+| arrays.cpp:16:8:16:13 | access to array | ArgumentNode is missing PostUpdateNode. |
+| arrays.cpp:17:8:17:13 | access to array | ArgumentNode is missing PostUpdateNode. |
 | by_reference.cpp:51:8:51:8 | s | ArgumentNode is missing PostUpdateNode. |
 | by_reference.cpp:57:8:57:8 | s | ArgumentNode is missing PostUpdateNode. |
 | by_reference.cpp:63:8:63:8 | s | ArgumentNode is missing PostUpdateNode. |