Merge pull request #3501 from jbj/dispatch-global-union

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

Author: alexet

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/defaulttainttracking.cpp

@@ -1,13 +1,13 @@
-int atoi(const char *nptr);
-char *getenv(const char *name);
-char *strcat(char * s1, const char * s2);
+#include "shared.h"
+
+
+
+
+
+
+
 
-char *strdup(const char *);
-char *_strdup(const char *);
-char *unmodeled_function(const char *);
 
-void sink(const char *);
-void sink(int);
 
 int main(int argc, char *argv[]) {
 

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

@@ -0,0 +1,35 @@
+#include "shared.h"
+
+using SinkFunction = void (*)(int);
+
+void notSink(int notSinkParam);
+
+void callsSink(int sinkParam) {
+  sink(sinkParam);
+}
+
+struct {
+  SinkFunction sinkPtr, notSinkPtr;
+} globalStruct;
+
+union {
+  SinkFunction sinkPtr, notSinkPtr;
+} globalUnion;
+
+SinkFunction globalSinkPtr;
+
+void assignGlobals() {
+  globalStruct.sinkPtr = callsSink;
+  globalUnion.sinkPtr = callsSink;
+  globalSinkPtr = callsSink;
+};
+
+void testStruct() {
+  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
+
+  globalSinkPtr(atoi(getenv("TAINTED"))); // should reach sinkParam
+}

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

@@ -1,4 +1,6 @@
-| globals.cpp:13:15:13:20 | call to getenv | globals.cpp:2:17:2:25 | sinkParam | global1 |
+| globals.cpp:13:15:13:20 | call to getenv | globals.cpp:12:10:12:16 | (const char *)... | global1 |
 | globals.cpp:13:15:13:20 | call to getenv | globals.cpp:12:10:12:16 | global1 | global1 |
-| globals.cpp:23:15:23:20 | call to getenv | globals.cpp:2:17:2:25 | sinkParam | global2 |
+| globals.cpp:13:15:13:20 | call to getenv | shared.h:5:23:5:31 | sinkparam | global1 |
+| globals.cpp:23:15:23:20 | call to getenv | globals.cpp:19:10:19:16 | (const char *)... | global2 |
 | globals.cpp:23:15:23:20 | call to getenv | globals.cpp:19:10:19:16 | global2 | global2 |
+| globals.cpp:23:15:23:20 | call to getenv | shared.h:5:23:5:31 | sinkparam | global2 |

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

@@ -1,5 +1,5 @@
-char * getenv(const char *);
-void sink(char *sinkParam);
+#include "shared.h"
+
 
 void throughLocal() {
     char * local = getenv("VAR");

cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/shared.h

@@ -0,0 +1,14 @@
+// Common declarations in this test dir should go in this file. Otherwise, some
+// declarations will have multiple locations, which leads to confusing test
+// output.
+
+void sink(const char *sinkparam);
+void sink(int sinkparam);
+
+int atoi(const char *nptr);
+char *getenv(const char *name);
+char *strcat(char * s1, const char * s2);
+
+char *strdup(const char *string);
+char *_strdup(const char *string);
+char *unmodeled_function(const char *const_string);