Merge branch 'main' into precompute-states-in-overrun-write

Author: MathiasVP

cpp/ql/src/experimental/Likely Bugs/OverrunWriteProductFlow.ql

@@ -254,6 +254,10 @@ module StringSizeConfig implements ProductFlow::StateConfigSig {
 
   predicate isBarrier2(DataFlow::Node node, FlowState2 state) { none() }
 
+  predicate isBarrierOut2(DataFlow::Node node) {
+    node = any(DataFlow::SsaPhiNode phi).getAnInput(true)
+  }
+
   predicate isAdditionalFlowStep1(
     DataFlow::Node node1, FlowState1 state1, DataFlow::Node node2, FlowState1 state2
   ) {

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

@@ -83,6 +83,7 @@ edges
 | test.cpp:243:12:243:14 | str indirection [string] | test.cpp:243:16:243:21 | string indirection |
 | test.cpp:243:16:243:21 | string indirection | test.cpp:243:12:243:21 | string |
 | test.cpp:249:20:249:27 | call to my_alloc | test.cpp:250:12:250:12 | p |
+| test.cpp:256:17:256:22 | call to malloc | test.cpp:257:12:257:12 | p |
 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 | ... = ... |
@@ -159,6 +160,8 @@ nodes
 | test.cpp:243:16:243:21 | string indirection | semmle.label | string indirection |
 | test.cpp:249:20:249:27 | call to my_alloc | semmle.label | call to my_alloc |
 | test.cpp:250:12:250:12 | p | semmle.label | p |
+| test.cpp:256:17:256:22 | call to malloc | semmle.label | call to malloc |
+| test.cpp:257:12:257:12 | p | semmle.label | p |
 subpaths
 | test.cpp:242:22:242:27 | buffer | test.cpp:235:40:235:45 | buffer | test.cpp:236:12:236:17 | p_str indirection [post update] [string] | test.cpp:242:16:242:19 | set_string output argument [string] |
 #select
@@ -177,6 +180,5 @@ 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:232:3:232:8 | call to memset | test.cpp:228:43:228:48 | call to malloc | test.cpp:232:10:232:15 | buffer | This write may overflow $@ by 2 elements. | test.cpp:232:10:232:15 | buffer | buffer |
 | test.cpp:243:5:243:10 | call to memset | test.cpp:241:27:241:32 | call to malloc | test.cpp:243:12:243:21 | string | This write may overflow $@ by 1 element. | test.cpp:243:16:243:21 | string | string |
 | test.cpp:250:5:250:10 | call to memset | test.cpp:249:20:249:27 | call to my_alloc | test.cpp:250:12:250:12 | p | This write may overflow $@ by 1 element. | test.cpp:250:12:250:12 | p | p |

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

@@ -229,7 +229,7 @@ void repeated_alerts(unsigned size, unsigned offset) {
   while(unknown()) {
     ++size;
   }
-  memset(buffer, 0, size); // BAD
+  memset(buffer, 0, size); // BAD [NOT DETECTED]
 }
 
 void set_string(string_t* p_str, char* buffer) {
@@ -248,4 +248,12 @@ void* my_alloc(unsigned size);
 void foo(unsigned size) {
     int* p = (int*)my_alloc(size); // BAD
     memset(p, 0, size + 1);
+}
+
+void test6(unsigned long n, char *p) {
+  while (unknown()) {
+    n++;
+    p = (char *)malloc(n);
+    memset(p, 0, n); // GOOD
+  }
 }

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected

@@ -649,6 +649,10 @@ edges
 | test.cpp:280:13:280:24 | new[] | test.cpp:281:14:281:15 | xs |
 | test.cpp:290:13:290:24 | new[] | test.cpp:291:14:291:15 | xs |
 | test.cpp:290:13:290:24 | new[] | test.cpp:292:30:292:30 | x |
+| test.cpp:304:15:304:26 | new[] | test.cpp:307:5:307:6 | xs |
+| test.cpp:304:15:304:26 | new[] | test.cpp:308:5:308:6 | xs |
+| test.cpp:308:5:308:6 | xs | test.cpp:308:5:308:11 | access to array |
+| test.cpp:308:5:308:11 | access to array | test.cpp:308:5:308:29 | Store: ... = ... |
 #select
 | test.cpp:6:14:6:15 | Load: * ... | test.cpp:4:15:4:20 | call to malloc | test.cpp:6:14:6:15 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:4:15:4:20 | call to malloc | call to malloc | test.cpp:5:19:5:22 | size | size |
 | test.cpp:8:14:8:21 | Load: * ... | test.cpp:4:15:4:20 | call to malloc | test.cpp:8:14:8:21 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@ + 1. | test.cpp:4:15:4:20 | call to malloc | call to malloc | test.cpp:5:19:5:22 | size | size |
@@ -672,3 +676,4 @@ edges
 | test.cpp:254:9:254:16 | Store: ... = ... | test.cpp:248:24:248:30 | call to realloc | test.cpp:254:9:254:16 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:248:24:248:30 | call to realloc | call to realloc | test.cpp:254:11:254:11 | i | i |
 | test.cpp:264:13:264:14 | Load: * ... | test.cpp:260:13:260:24 | new[] | test.cpp:264:13:264:14 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:260:13:260:24 | new[] | new[] | test.cpp:261:19:261:21 | len | len |
 | test.cpp:274:5:274:10 | Store: ... = ... | test.cpp:270:13:270:24 | new[] | test.cpp:274:5:274:10 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:270:13:270:24 | new[] | new[] | test.cpp:271:19:271:21 | len | len |
+| test.cpp:308:5:308:29 | Store: ... = ... | test.cpp:304:15:304:26 | new[] | test.cpp:308:5:308:29 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:304:15:304:26 | new[] | new[] | test.cpp:308:8:308:10 | ... + ... | ... + ... |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp

@@ -293,4 +293,18 @@ void test20(unsigned len)
   {
     *x = 0; // GOOD
   }
-}
+}
+
+void* test21_get(int n);
+
+void test21() {
+  int n = 0;
+  while (test21_get(n)) n+=2;
+
+  void** xs = new void*[n];
+
+  for (int i = 0; i < n; i += 2) {
+    xs[i] = test21_get(i);
+    xs[i+1] = test21_get(i+1);
+  }
+}

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

@@ -125,6 +125,8 @@ postWithInFlow
 | test.cpp:681:3:681:3 | s [post update] | PostUpdateNode should not be the target of local flow. |
 | test.cpp:689:3:689:3 | s [post update] | PostUpdateNode should not be the target of local flow. |
 | test.cpp:690:3:690:3 | s [post update] | PostUpdateNode should not be the target of local flow. |
+| test.cpp:694:4:694:6 | buf [inner post update] | PostUpdateNode should not be the target of local flow. |
+| test.cpp:704:23:704:25 | buf [inner post update] | PostUpdateNode should not be the target of local flow. |
 viableImplInCallContextTooLarge
 uniqueParameterNodeAtPosition
 uniqueParameterNodePosition

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

@@ -690,3 +690,16 @@ void test_static_local_9() {
   s = 0;
 }
 
+void increment_buf(int** buf) { // $ ast-def=buf ir-def=*buf ir-def=**buf
+  *buf += 10;
+  sink(buf); // $ SPURIOUS: ast,ir // should only be flow to the indirect argument, but there's also flow to the non-indirect argument
+}
+
+void call_increment_buf(int** buf) { // $ ast-def=buf
+  increment_buf(buf);
+}
+
+void test_conflation_regression(int* source) { // $ ast-def=source
+  int* buf = source;
+  call_increment_buf(&buf);
+}

cpp/ql/test/library-tests/ir/range-analysis/test.cpp

@@ -49,3 +49,13 @@
     return 0;
   }
 
+  void* f3_get(int n);
+
+  void f3() {
+    int n = 0;
+    while (f3_get(n)) n+=2;
+
+    for (int i = 0; i < n; i += 2) {
+      range(i); // $ range=>=0 SPURIOUS: range="<=call to f3_get-1" range="<=call to f3_get-2"
+    }
+  }

csharp/ql/consistency-queries/DataFlowConsistency.ql

@@ -72,5 +72,5 @@ private class MyConsistencyConfiguration extends ConsistencyConfiguration {
 
   override predicate reverseReadExclude(Node n) { n.asExpr() = any(AwaitExpr ae).getExpr() }
 
-  override predicate identityLocalStepExclude(Node n) { n.getLocation().getFile().fromLibrary() }
+  override predicate identityLocalStepExclude(Node n) { none() }
 }

csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll

@@ -335,7 +335,8 @@ module LocalFlow {
     exists(ControlFlow::BasicBlock bb, int i |
       SsaImpl::lastRefBeforeRedefExt(def, bb, i, next.getDefinitionExt()) and
       def.definesAt(_, bb, i, _) and
-      def = getSsaDefinitionExt(nodeFrom)
+      def = getSsaDefinitionExt(nodeFrom) and
+      nodeFrom != next
     )
   }
 
@@ -414,7 +415,8 @@ module LocalFlow {
     ) {
       exists(CIL::BasicBlock bb, int i | CilSsaImpl::lastRefBeforeRedefExt(def, bb, i, next) |
         def.definesAt(_, bb, i, _) and
-        def = nodeFrom.(CilSsaDefinitionExtNode).getDefinition()
+        def = nodeFrom.(CilSsaDefinitionExtNode).getDefinition() and
+        def != next
         or
         nodeFrom = TCilExprNode(bb.getNode(i).(CIL::ReadAccess))
       )
@@ -440,7 +442,8 @@ module LocalFlow {
       exists(CIL::ReadAccess readFrom, CIL::ReadAccess readTo |
         CilSsaImpl::hasAdjacentReadsExt(def, readFrom, readTo) and
         nodeTo = TCilExprNode(readTo) and
-        nodeFrom = TCilExprNode(readFrom)
+        nodeFrom = TCilExprNode(readFrom) and
+        nodeFrom != nodeTo
       )
       or
       // Flow into phi (read) node
@@ -483,7 +486,8 @@ module LocalFlow {
     or
     hasNodePath(any(LocalExprStepConfiguration x), nodeFrom, nodeTo)
     or
-    ThisFlow::adjacentThisRefs(nodeFrom, nodeTo)
+    ThisFlow::adjacentThisRefs(nodeFrom, nodeTo) and
+    nodeFrom != nodeTo
     or
     ThisFlow::adjacentThisRefs(nodeFrom.(PostUpdateNode).getPreUpdateNode(), nodeTo)
     or
@@ -541,7 +545,8 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
   exists(SsaImpl::DefinitionExt def |
     LocalFlow::localSsaFlowStepUseUse(def, nodeFrom, nodeTo) and
     not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom, _) and
-    not LocalFlow::usesInstanceField(def)
+    not LocalFlow::usesInstanceField(def) and
+    nodeFrom != nodeTo
   )
   or
   // Flow into phi (read)/uncertain SSA definition node from read
@@ -880,7 +885,8 @@ private module Cached {
   predicate localFlowStepImpl(Node nodeFrom, Node nodeTo) {
     LocalFlow::localFlowStepCommon(nodeFrom, nodeTo)
     or
-    LocalFlow::localSsaFlowStepUseUse(_, nodeFrom, nodeTo)
+    LocalFlow::localSsaFlowStepUseUse(_, nodeFrom, nodeTo) and
+    nodeFrom != nodeTo
     or
     exists(SsaImpl::DefinitionExt def |
       LocalFlow::localSsaFlowStep(def, nodeFrom, nodeTo) and