Merge pull request github#3532 from MathiasVP/remove-field-conflation-from-ir-fieldflow

C++: Remove field conflation caused by IR field flow

Author: jbj

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

@@ -1,6 +1,7 @@
 import cpp
 import semmle.code.cpp.security.Security
 private import semmle.code.cpp.ir.dataflow.DataFlow
+private import semmle.code.cpp.ir.dataflow.internal.DataFlowUtil
 private import semmle.code.cpp.ir.dataflow.DataFlow2
 private import semmle.code.cpp.ir.dataflow.DataFlow3
 private import semmle.code.cpp.ir.IR

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

@@ -458,9 +458,9 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
   // for now.
   iTo.getAnOperand().(ChiTotalOperand).getDef() = iFrom
   or
-  // The next two rules allow flow from partial definitions in setters to succeeding loads in the caller.
-  // First, we add flow from write side-effects to non-conflated chi instructions through their
-  // partial operands. Consider the following example:
+  // Add flow from write side-effects to non-conflated chi instructions through their
+  // partial operands. From there, a `readStep` will find subsequent reads of that field.
+  // Consider the following example:
   // ```
   // void setX(Point* p, int new_x) {
   //   p->x = new_x;
@@ -470,14 +470,9 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
   // ```
   // Here, a `WriteSideEffectInstruction` will provide a new definition for `p->x` after the call to
   // `setX`, which will be melded into `p` through a chi instruction.
-  iTo.getAnOperand().(ChiPartialOperand).getDef() = iFrom.(WriteSideEffectInstruction) and
-  not iTo.isResultConflated()
-  or
-  // Next, we add flow from non-conflated chi instructions to loads (even when they are not precise).
-  // This ensures that loads of `p->x` gets data flow from the `WriteSideEffectInstruction` above.
-  exists(ChiInstruction chi | iFrom = chi |
-    not chi.isResultConflated() and
-    iTo.(LoadInstruction).getSourceValueOperand().getAnyDef() = chi
+  exists(ChiInstruction chi | chi = iTo |
+    chi.getPartialOperand().getDef() = iFrom.(WriteSideEffectInstruction) and
+    not chi.isResultConflated()
   )
   or
   // Flow from stores to structs with a single field to a load of that field.

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

@@ -115,5 +115,5 @@ void test_conflated_fields3() {
   XY xy;
   xy.x = 0;
   taint_y(&xy);
-  sink(xy.x); // not tainted [FALSE POSITIVE]
+  sink(xy.x); // not tainted
 }

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

@@ -103,8 +103,6 @@
 | defaulttainttracking.cpp:110:17:110:22 | call to getenv | defaulttainttracking.cpp:110:17:110:32 | (int)... |
 | defaulttainttracking.cpp:110:17:110:22 | call to getenv | defaulttainttracking.cpp:110:17:110:32 | access to array |
 | defaulttainttracking.cpp:110:17:110:22 | call to getenv | defaulttainttracking.cpp:111:12:111:18 | tainted |
-| defaulttainttracking.cpp:110:17:110:22 | call to getenv | defaulttainttracking.cpp:118:11:118:11 | x |
-| defaulttainttracking.cpp:110:17:110:22 | call to getenv | shared.h:6:15:6:23 | sinkparam |
 | dispatch.cpp:28:29:28:34 | call to getenv | dispatch.cpp:28:24:28:27 | call to atoi |
 | dispatch.cpp:28:29:28:34 | call to getenv | dispatch.cpp:28:29:28:34 | call to getenv |
 | dispatch.cpp:28:29:28:34 | call to getenv | dispatch.cpp:28:29:28:45 | (const char *)... |

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

@@ -21,8 +21,6 @@
 | 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 | shared.h:5:23:5:31 | sinkparam | IR only |
 | defaulttainttracking.cpp:110:17:110:22 | call to getenv | defaulttainttracking.cpp:111:8:111:8 | y | AST only |
-| defaulttainttracking.cpp:110:17:110:22 | call to getenv | defaulttainttracking.cpp:118:11:118:11 | x | IR only |
-| defaulttainttracking.cpp:110:17:110:22 | call to getenv | shared.h:6:15:6:23 | sinkparam | 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/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected

@@ -1,13 +1,4 @@
 edges
-| field_conflation.c:12:22:12:27 | call to getenv | field_conflation.c:13:3:13:18 | Chi |
-| field_conflation.c:12:22:12:34 | (const char *)... | field_conflation.c:13:3:13:18 | Chi |
-| field_conflation.c:13:3:13:18 | Chi | field_conflation.c:19:15:19:17 | taint_array output argument |
-| field_conflation.c:19:15:19:17 | taint_array output argument | field_conflation.c:20:10:20:13 | (unsigned long)... |
-| field_conflation.c:19:15:19:17 | taint_array output argument | field_conflation.c:20:13:20:13 | x |
-| field_conflation.c:19:15:19:17 | taint_array output argument | field_conflation.c:20:13:20:13 | x |
-| field_conflation.c:19:15:19:17 | taint_array output argument | field_conflation.c:20:13:20:13 | x |
-| field_conflation.c:20:13:20:13 | x | field_conflation.c:20:10:20:13 | (unsigned long)... |
-| field_conflation.c:20:13:20:13 | x | field_conflation.c:20:13:20:13 | x |
 | test.cpp:39:21:39:24 | argv | test.cpp:42:38:42:44 | (size_t)... |
 | test.cpp:39:21:39:24 | argv | test.cpp:42:38:42:44 | (size_t)... |
 | test.cpp:39:21:39:24 | argv | test.cpp:42:38:42:44 | tainted |
@@ -89,15 +80,6 @@ edges
 | test.cpp:309:19:309:32 | (const char *)... | test.cpp:314:10:314:27 | ... * ... |
 | test.cpp:309:19:309:32 | (const char *)... | test.cpp:314:10:314:27 | ... * ... |
 nodes
-| field_conflation.c:12:22:12:27 | call to getenv | semmle.label | call to getenv |
-| field_conflation.c:12:22:12:34 | (const char *)... | semmle.label | (const char *)... |
-| field_conflation.c:13:3:13:18 | Chi | semmle.label | Chi |
-| field_conflation.c:19:15:19:17 | taint_array output argument | semmle.label | taint_array output argument |
-| field_conflation.c:20:10:20:13 | (unsigned long)... | semmle.label | (unsigned long)... |
-| field_conflation.c:20:10:20:13 | (unsigned long)... | semmle.label | (unsigned long)... |
-| field_conflation.c:20:13:20:13 | x | semmle.label | x |
-| field_conflation.c:20:13:20:13 | x | semmle.label | x |
-| field_conflation.c:20:13:20:13 | x | semmle.label | x |
 | test.cpp:39:21:39:24 | argv | semmle.label | argv |
 | test.cpp:39:21:39:24 | argv | semmle.label | argv |
 | test.cpp:42:38:42:44 | (size_t)... | semmle.label | (size_t)... |
@@ -187,7 +169,6 @@ nodes
 | test.cpp:314:10:314:27 | ... * ... | semmle.label | ... * ... |
 | test.cpp:314:10:314:27 | ... * ... | semmle.label | ... * ... |
 #select
-| field_conflation.c:20:3:20:8 | call to malloc | field_conflation.c:12:22:12:27 | call to getenv | field_conflation.c:20:13:20:13 | x | This allocation size is derived from $@ and might overflow | field_conflation.c:12:22:12:27 | call to getenv | user input (getenv) |
 | test.cpp:42:31:42:36 | call to malloc | test.cpp:39:21:39:24 | argv | test.cpp:42:38:42:44 | tainted | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
 | test.cpp:43:31:43:36 | call to malloc | test.cpp:39:21:39:24 | argv | test.cpp:43:38:43:63 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
 | test.cpp:45:31:45:36 | call to malloc | test.cpp:39:21:39:24 | argv | test.cpp:45:38:45:63 | ... + ... | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/field_conflation.c

@@ -17,5 +17,5 @@ void test_conflated_fields3(void) {
   struct XY xy;
   xy.x = 4;
   taint_array(&xy);
-  malloc(xy.x); // not tainted [FALSE POSITIVE]
+  malloc(xy.x); // not tainted
 }