C++: Fix struct field conflation in IR data flow

The virtual-dispatch code for globals was missing any relationship
between the union field access and the global variable, which meant it
propagated function-pointer flow between any two fields of a global
struct. This resulted in false positives from
`cpp/tainted-format-string` on projects using SDL, such as
WohlSoft/PGE-Project.

In addition to fixing that bug, this commit also brings the code up to
date with the new style of modeling flow through global variables:
`DataFlow::Node.asVariable()`.

Author: jbj

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

@@ -77,49 +77,48 @@ private module VirtualDispatch {
         // Local flow
         DataFlow::localFlowStep(src, other) and
         allowFromArg = allowOtherFromArg
-      )
-      or
-      // Flow through global variable
-      exists(StoreInstruction store |
-        store = src.asInstruction() and
-        (
-          exists(Variable var |
-            var = store.getDestinationAddress().(VariableAddressInstruction).getASTVariable() and
-            this.flowsFromGlobal(var)
+        or
+        // Flow from global variable to load.
+        exists(LoadInstruction load, GlobalOrNamespaceVariable var |
+          var = src.asVariable() and
+          other.asInstruction() = load and
+          // The `allowFromArg` concept doesn't play a role when `src` is a
+          // global variable, so we just set it to a single arbitrary value for
+          // performance.
+          allowFromArg = true
+        |
+          // Load directly from the global variable
+          load.getSourceAddress().(VariableAddressInstruction).getASTVariable() = var
+          or
+          // Load from a field on a global union
+          exists(FieldAddressInstruction fa |
+            fa = load.getSourceAddress() and
+            fa.getObjectAddress().(VariableAddressInstruction).getASTVariable() = var and
+            fa.getField().getDeclaringType() instanceof Union
           )
+        )
+        or
+        // Flow from store to global variable. These cases are similar to the
+        // above but have `StoreInstruction` instead of `LoadInstruction` and
+        // have the roles swapped between `other` and `src`.
+        exists(StoreInstruction store, GlobalOrNamespaceVariable var |
+          var = other.asVariable() and
+          store = src.asInstruction() and
+          // Setting `allowFromArg` to `true` like in the base case means we
+          // treat a store to a global variable like the dispatch itself: flow
+          // may come from anywhere.
+          allowFromArg = true
+        |
+          // Store directly to the global variable
+          store.getDestinationAddress().(VariableAddressInstruction).getASTVariable() = var
           or
-          exists(Variable var, FieldAccess a |
-            var =
-              store
-                  .getDestinationAddress()
-                  .(FieldAddressInstruction)
-                  .getObjectAddress()
-                  .(VariableAddressInstruction)
-                  .getASTVariable() and
-            this.flowsFromGlobalUnionField(var, a)
+          // Store to a field on a global union
+          exists(FieldAddressInstruction fa |
+            fa = store.getDestinationAddress() and
+            fa.getObjectAddress().(VariableAddressInstruction).getASTVariable() = var and
+            fa.getField().getDeclaringType() instanceof Union
           )
-        ) and
-        allowFromArg = true
-      )
-    }
-
-    private predicate flowsFromGlobal(GlobalOrNamespaceVariable var) {
-      exists(LoadInstruction load |
-        this.flowsFrom(DataFlow::instructionNode(load), _) and
-        load.getSourceAddress().(VariableAddressInstruction).getASTVariable() = var
-      )
-    }
-
-    private predicate flowsFromGlobalUnionField(Variable var, FieldAccess a) {
-      a.getTarget().getDeclaringType() instanceof Union and
-      exists(LoadInstruction load |
-        this.flowsFrom(DataFlow::instructionNode(load), _) and
-        load
-            .getSourceAddress()
-            .(FieldAddressInstruction)
-            .getObjectAddress()
-            .(VariableAddressInstruction)
-            .getASTVariable() = var
+        )
       )
     }
   }

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

@@ -25,8 +25,8 @@ void assignGlobals() {
 };
 
 void testStruct() {
-  globalStruct.sinkPtr(atoi(getenv("TAINTED"))); // should reach sinkParam [NOT DETECTED in AST]
-  globalStruct.notSinkPtr(atoi(getenv("TAINTED"))); // shouldn't reach sinkParam [FALSE POSITIVE in IR]
+  globalStruct.sinkPtr(atoi(getenv("TAINTED"))); // should reach sinkParam [NOT DETECTED]
+  globalStruct.notSinkPtr(atoi(getenv("TAINTED"))); // shouldn't reach sinkParam
 
   globalUnion.sinkPtr(atoi(getenv("TAINTED"))); // should reach sinkParam
   globalUnion.notSinkPtr(atoi(getenv("TAINTED"))); // should reach sinkParam

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

@@ -98,19 +98,13 @@
 | 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 | shared.h:5:23:5:31 | sinkparam |
-| dispatch.cpp:28:29:28:34 | call to getenv | dispatch.cpp:7:20:7:28 | sinkParam |
-| dispatch.cpp:28:29:28:34 | call to getenv | dispatch.cpp:8:8:8:16 | 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 *)... |
-| dispatch.cpp:28:29:28:34 | call to getenv | shared.h:6:15:6:23 | sinkparam |
 | dispatch.cpp:28:29:28:34 | call to getenv | shared.h:8:22:8:25 | nptr |
-| dispatch.cpp:29:32:29:37 | call to getenv | dispatch.cpp:7:20:7:28 | sinkParam |
-| dispatch.cpp:29:32:29:37 | call to getenv | dispatch.cpp:8:8:8:16 | sinkParam |
 | dispatch.cpp:29:32:29:37 | call to getenv | dispatch.cpp:29:27:29:30 | call to atoi |
 | dispatch.cpp:29:32:29:37 | call to getenv | dispatch.cpp:29:32:29:37 | call to getenv |
 | dispatch.cpp:29:32:29:37 | call to getenv | dispatch.cpp:29:32:29:48 | (const char *)... |
-| dispatch.cpp:29:32:29:37 | call to getenv | shared.h:6:15:6:23 | sinkparam |
 | dispatch.cpp:29:32:29:37 | call to getenv | shared.h:8:22:8:25 | nptr |
 | dispatch.cpp:31:28:31:33 | call to getenv | dispatch.cpp:7:20:7:28 | sinkParam |
 | dispatch.cpp:31:28:31:33 | call to getenv | dispatch.cpp:8:8:8:16 | sinkParam |

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

@@ -20,12 +20,6 @@
 | 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 | shared.h:5:23:5:31 | sinkparam | IR only |
-| dispatch.cpp:28:29:28:34 | call to getenv | dispatch.cpp:7:20:7:28 | sinkParam | IR only |
-| dispatch.cpp:28:29:28:34 | call to getenv | dispatch.cpp:8:8:8:16 | sinkParam | IR only |
-| dispatch.cpp:28:29:28:34 | call to getenv | shared.h:6:15:6:23 | sinkparam | IR only |
-| dispatch.cpp:29:32:29:37 | call to getenv | dispatch.cpp:7:20:7:28 | sinkParam | IR only |
-| dispatch.cpp:29:32:29:37 | call to getenv | dispatch.cpp:8:8:8:16 | sinkParam | IR only |
-| dispatch.cpp:29:32:29:37 | 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 |