Merge pull request github#12997 from MathiasVP/sync-product-flow-across-calls

C++: Synchronize product dataflow paths on function entry points

Author: MathiasVP

cpp/ql/lib/experimental/semmle/code/cpp/dataflow/ProductFlow.qll

@@ -1,4 +1,7 @@
 import semmle.code.cpp.ir.dataflow.DataFlow
+private import semmle.code.cpp.ir.dataflow.internal.DataFlowPrivate
+private import semmle.code.cpp.ir.dataflow.internal.DataFlowUtil
+private import semmle.code.cpp.ir.dataflow.internal.DataFlowImplCommon
 private import codeql.util.Unit
 
 module ProductFlow {
@@ -352,32 +355,119 @@ module ProductFlow {
         pragma[only_bind_out](succ.getNode().getEnclosingCallable())
     }
 
+    private newtype TKind =
+      TInto(DataFlowCall call) {
+        intoImpl1(_, _, call) or
+        intoImpl2(_, _, call)
+      } or
+      TOutOf(DataFlowCall call) {
+        outImpl1(_, _, call) or
+        outImpl2(_, _, call)
+      } or
+      TJump()
+
+    private predicate intoImpl1(Flow1::PathNode pred1, Flow1::PathNode succ1, DataFlowCall call) {
+      Flow1::PathGraph::edges(pred1, succ1) and
+      pred1.getNode().(ArgumentNode).getCall() = call and
+      succ1.getNode() instanceof ParameterNode
+    }
+
+    private predicate into1(Flow1::PathNode pred1, Flow1::PathNode succ1, TKind kind) {
+      exists(DataFlowCall call |
+        kind = TInto(call) and
+        intoImpl1(pred1, succ1, call)
+      )
+    }
+
+    private predicate outImpl1(Flow1::PathNode pred1, Flow1::PathNode succ1, DataFlowCall call) {
+      Flow1::PathGraph::edges(pred1, succ1) and
+      exists(ReturnKindExt returnKind |
+        succ1.getNode() = returnKind.getAnOutNode(call) and
+        pred1.getNode().(ReturnNodeExt).getKind() = returnKind
+      )
+    }
+
+    private predicate out1(Flow1::PathNode pred1, Flow1::PathNode succ1, TKind kind) {
+      exists(DataFlowCall call |
+        outImpl1(pred1, succ1, call) and
+        kind = TOutOf(call)
+      )
+    }
+
+    private predicate intoImpl2(Flow2::PathNode pred2, Flow2::PathNode succ2, DataFlowCall call) {
+      Flow2::PathGraph::edges(pred2, succ2) and
+      pred2.getNode().(ArgumentNode).getCall() = call and
+      succ2.getNode() instanceof ParameterNode
+    }
+
+    private predicate into2(Flow2::PathNode pred2, Flow2::PathNode succ2, TKind kind) {
+      exists(DataFlowCall call |
+        kind = TInto(call) and
+        intoImpl2(pred2, succ2, call)
+      )
+    }
+
+    private predicate outImpl2(Flow2::PathNode pred2, Flow2::PathNode succ2, DataFlowCall call) {
+      Flow2::PathGraph::edges(pred2, succ2) and
+      exists(ReturnKindExt returnKind |
+        succ2.getNode() = returnKind.getAnOutNode(call) and
+        pred2.getNode().(ReturnNodeExt).getKind() = returnKind
+      )
+    }
+
+    private predicate out2(Flow2::PathNode pred2, Flow2::PathNode succ2, TKind kind) {
+      exists(DataFlowCall call |
+        kind = TOutOf(call) and
+        outImpl2(pred2, succ2, call)
+      )
+    }
+
     pragma[nomagic]
     private predicate interprocEdge1(
-      Declaration predDecl, Declaration succDecl, Flow1::PathNode pred1, Flow1::PathNode succ1
+      Declaration predDecl, Declaration succDecl, Flow1::PathNode pred1, Flow1::PathNode succ1,
+      TKind kind
     ) {
       Flow1::PathGraph::edges(pred1, succ1) and
       predDecl != succDecl and
       pred1.getNode().getEnclosingCallable() = predDecl and
-      succ1.getNode().getEnclosingCallable() = succDecl
+      succ1.getNode().getEnclosingCallable() = succDecl and
+      (
+        into1(pred1, succ1, kind)
+        or
+        out1(pred1, succ1, kind)
+        or
+        kind = TJump() and
+        not into1(pred1, succ1, _) and
+        not out1(pred1, succ1, _)
+      )
     }
 
     pragma[nomagic]
     private predicate interprocEdge2(
-      Declaration predDecl, Declaration succDecl, Flow2::PathNode pred2, Flow2::PathNode succ2
+      Declaration predDecl, Declaration succDecl, Flow2::PathNode pred2, Flow2::PathNode succ2,
+      TKind kind
     ) {
       Flow2::PathGraph::edges(pred2, succ2) and
       predDecl != succDecl and
       pred2.getNode().getEnclosingCallable() = predDecl and
-      succ2.getNode().getEnclosingCallable() = succDecl
+      succ2.getNode().getEnclosingCallable() = succDecl and
+      (
+        into2(pred2, succ2, kind)
+        or
+        out2(pred2, succ2, kind)
+        or
+        kind = TJump() and
+        not into2(pred2, succ2, _) and
+        not out2(pred2, succ2, _)
+      )
     }
 
     private predicate interprocEdgePair(
       Flow1::PathNode pred1, Flow2::PathNode pred2, Flow1::PathNode succ1, Flow2::PathNode succ2
     ) {
-      exists(Declaration predDecl, Declaration succDecl |
-        interprocEdge1(predDecl, succDecl, pred1, succ1) and
-        interprocEdge2(predDecl, succDecl, pred2, succ2)
+      exists(Declaration predDecl, Declaration succDecl, TKind kind |
+        interprocEdge1(predDecl, succDecl, pred1, succ1, kind) and
+        interprocEdge2(predDecl, succDecl, pred2, succ2, kind)
       )
     }
 

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-119/OverrunWriteProductFlow.expected

@@ -212,6 +212,15 @@ edges
 | test.cpp:214:24:214:24 | p | test.cpp:216:10:216:10 | p |
 | test.cpp:220:43:220:48 | call to malloc | test.cpp:222:15:222:20 | buffer |
 | test.cpp:222:15:222:20 | buffer | test.cpp:214:24:214:24 | p |
+| test.cpp:225:40:225:45 | buffer | test.cpp:226:5:226:26 | ... = ... |
+| test.cpp:226:5:226:26 | ... = ... | test.cpp:226:12:226:17 | p_str indirection [post update] [string] |
+| test.cpp:231:27:231:32 | call to malloc | test.cpp:232:22:232:27 | buffer |
+| test.cpp:232:16:232:19 | set_string output argument [string] | test.cpp:233:12:233:14 | str indirection [string] |
+| test.cpp:232:22:232:27 | buffer | test.cpp:225:40:225:45 | buffer |
+| test.cpp:232:22:232:27 | buffer | test.cpp:232:16:232:19 | set_string output argument [string] |
+| test.cpp:233:12:233:14 | str indirection [string] | test.cpp:233:12:233:21 | string |
+| test.cpp:233:12:233:14 | str indirection [string] | test.cpp:233:16:233:21 | string indirection |
+| test.cpp:233:16:233:21 | string indirection | test.cpp:233:12:233:21 | string |
 nodes
 | test.cpp:16:11:16:21 | mk_string_t indirection [string] | semmle.label | mk_string_t indirection [string] |
 | test.cpp:18:5:18:30 | ... = ... | semmle.label | ... = ... |
@@ -381,7 +390,17 @@ nodes
 | test.cpp:216:10:216:10 | p | semmle.label | p |
 | test.cpp:220:43:220:48 | call to malloc | semmle.label | call to malloc |
 | test.cpp:222:15:222:20 | buffer | semmle.label | buffer |
+| test.cpp:225:40:225:45 | buffer | semmle.label | buffer |
+| test.cpp:226:5:226:26 | ... = ... | semmle.label | ... = ... |
+| test.cpp:226:12:226:17 | p_str indirection [post update] [string] | semmle.label | p_str indirection [post update] [string] |
+| test.cpp:231:27:231:32 | call to malloc | semmle.label | call to malloc |
+| test.cpp:232:16:232:19 | set_string output argument [string] | semmle.label | set_string output argument [string] |
+| test.cpp:232:22:232:27 | buffer | semmle.label | buffer |
+| test.cpp:233:12:233:14 | str indirection [string] | semmle.label | str indirection [string] |
+| test.cpp:233:12:233:21 | string | semmle.label | string |
+| test.cpp:233:16:233:21 | string indirection | semmle.label | string indirection |
 subpaths
+| test.cpp:232:22:232:27 | buffer | test.cpp:225:40:225:45 | buffer | test.cpp:226:12:226:17 | p_str indirection [post update] [string] | test.cpp:232:16:232:19 | set_string output argument [string] |
 #select
 | test.cpp:42:5:42:11 | call to strncpy | test.cpp:18:19:18:24 | call to malloc | test.cpp:42:18:42:23 | string | This write may overflow $@ by 1 element. | test.cpp:42:18:42:23 | string | string |
 | test.cpp:72:9:72:15 | call to strncpy | test.cpp:18:19:18:24 | call to malloc | test.cpp:72:22:72:27 | string | This write may overflow $@ by 1 element. | test.cpp:72:22:72:27 | string | string |
@@ -398,4 +417,4 @@ subpaths
 | test.cpp:199:9:199:15 | call to strncpy | test.cpp:147:19:147:24 | call to malloc | test.cpp:199:22:199:27 | string | This write may overflow $@ by 2 elements. | test.cpp:199:22:199:27 | string | string |
 | test.cpp:203:9:203:15 | call to strncpy | test.cpp:147:19:147:24 | call to malloc | test.cpp:203:22:203:27 | string | This write may overflow $@ by 2 elements. | test.cpp:203:22:203:27 | string | string |
 | test.cpp:207:9:207:15 | call to strncpy | test.cpp:147:19:147:24 | call to malloc | test.cpp:207:22:207:27 | string | This write may overflow $@ by 3 elements. | test.cpp:207:22:207:27 | string | string |
-| test.cpp:216:3:216:8 | call to memset | test.cpp:220:43:220:48 | call to malloc | test.cpp:216:10:216:10 | p | This write may overflow $@ by 5 elements. | test.cpp:216:10:216:10 | p | p |
+| test.cpp:233:5:233:10 | call to memset | test.cpp:231:27:231:32 | call to malloc | test.cpp:233:12:233:21 | string | This write may overflow $@ by 1 element. | test.cpp:233:16:233:21 | string | string |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-119/test.cpp

@@ -220,4 +220,15 @@ void test_missing_call_context(unsigned char *unrelated_buffer, unsigned size) {
   unsigned char* buffer = (unsigned char*)malloc(size);
   call_memset(unrelated_buffer, size + 5);
   call_memset(buffer, size);
-}
+}
+
+void set_string(string_t* p_str, char* buffer) {
+    p_str->string = buffer;
+}
+
+void test_flow_through_setter(unsigned size) {
+    string_t str;
+    char* buffer = (char*)malloc(size);
+    set_string(&str, buffer);
+    memset(str.string, 0, size + 1); // BAD
+}