C++: Add barriers to 'cpp/overrun-write'.

Author: MathiasVP

cpp/ql/src/Security/CWE/CWE-119/OverrunWriteProductFlow.ql

@@ -20,6 +20,7 @@ import semmle.code.cpp.models.interfaces.Allocation
 import semmle.code.cpp.models.interfaces.ArrayFunction
 import semmle.code.cpp.rangeanalysis.new.internal.semantic.analysis.RangeAnalysis
 import semmle.code.cpp.rangeanalysis.new.internal.semantic.SemanticExprSpecific
+import semmle.code.cpp.security.ProductFlowUtils.ProductFlowUtils
 import semmle.code.cpp.rangeanalysis.new.RangeAnalysisUtil
 import StringSizeFlow::PathGraph1
 import codeql.util.Unit
@@ -109,17 +110,24 @@ module ValidState {
 
 import ValidState
 
-/**
- * Holds if `node2` is a dataflow node that represents an addition of two operands `op1`
- * and `op2` such that:
- * 1. `node1` is the dataflow node that represents `op1`, and
- * 2. the value of `op2` can be upper bounded by `delta.`
- */
-predicate isAdditionalFlowStep2(DataFlow::Node node1, DataFlow::Node node2, int delta) {
-  exists(AddInstruction add, Operand op |
-    add.hasOperands(node1.asOperand(), op) and
-    semBounded(getSemanticExpr(op.getDef()), any(SemZeroBound zero), delta, true, _) and
-    node2.asInstruction() = add
+module SizeBarrierInput implements SizeBarrierInputSig {
+  int fieldFlowBranchLimit() { result = 2 }
+
+  predicate isSource(DataFlow::Node source) {
+    exists(int state |
+      hasSize(_, source, state) and
+      validState(source, _, state)
+    )
+  }
+}
+
+predicate isSinkPairImpl(
+  CallInstruction c, DataFlow::Node bufSink, DataFlow::Node sizeSink, int delta, Expr eBuf
+) {
+  exists(Instruction sizeBound, Instruction sizeInstr |
+    isSinkPairImpl0(c, bufSink, sizeSink, delta, eBuf, sizeBound, sizeInstr) and
+    not sizeBound = SizeBarrier<SizeBarrierInput>::getABarrierInstruction(delta) and
+    not sizeInstr = SizeBarrier<SizeBarrierInput>::getABarrierInstruction(delta)
   )
 }
 
@@ -154,6 +162,10 @@ module StringSizeConfig implements ProductFlow::StateConfigSig {
   }
 
   predicate isBarrierOut2(DataFlow::Node node) { DataFlow::flowsToBackEdge(node) }
+
+  predicate isBarrier2(DataFlow::Node node, FlowState2 state) {
+    node = SizeBarrier<SizeBarrierInput>::getABarrierNode(state)
+  }
 }
 
 module StringSizeFlow = ProductFlow::GlobalWithState<StringSizeConfig>;

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

@@ -49,8 +49,6 @@ edges
 | test.cpp:262:15:262:30 | call to malloc | test.cpp:266:12:266:12 | p | provenance |  |
 | test.cpp:264:9:264:30 | ... = ... | test.cpp:266:12:266:12 | p | provenance |  |
 | test.cpp:264:13:264:30 | call to malloc | test.cpp:264:9:264:30 | ... = ... | provenance |  |
-| test.cpp:271:14:271:27 | new[] | test.cpp:271:14:271:27 | new[] | provenance |  |
-| test.cpp:271:14:271:27 | new[] | test.cpp:276:12:276:13 | xs | provenance |  |
 nodes
 | test.cpp:16:11:16:21 | **mk_string_t [string] | semmle.label | **mk_string_t [string] |
 | test.cpp:18:5:18:7 | *str [post update] [string] | semmle.label | *str [post update] [string] |
@@ -107,9 +105,6 @@ nodes
 | test.cpp:264:9:264:30 | ... = ... | semmle.label | ... = ... |
 | test.cpp:264:13:264:30 | call to malloc | semmle.label | call to malloc |
 | test.cpp:266:12:266:12 | p | semmle.label | p |
-| test.cpp:271:14:271:27 | new[] | semmle.label | new[] |
-| test.cpp:271:14:271:27 | new[] | semmle.label | new[] |
-| test.cpp:276:12:276:13 | xs | semmle.label | xs |
 subpaths
 | test.cpp:242:22:242:27 | buffer | test.cpp:235:40:235:45 | buffer | test.cpp:235:27:235:31 | *p_str [Return] [string] | test.cpp:242:16:242:19 | set_string output argument [string] |
 | test.cpp:242:22:242:27 | buffer | test.cpp:235:40:235:45 | buffer | test.cpp:235:27:235:31 | *p_str [string] | test.cpp:242:16:242:19 | set_string output argument [string] |
@@ -129,4 +124,3 @@ subpaths
 | test.cpp:243:5:243:10 | call to memset | test.cpp:241:20:241:38 | 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:14:249:33 | 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 |
 | test.cpp:266:5:266:10 | call to memset | test.cpp:262:15:262:30 | call to malloc | test.cpp:266:12:266:12 | p | This write may overflow $@ by 1 element. | test.cpp:266:12:266:12 | p | p |
-| test.cpp:276:5:276:10 | call to memset | test.cpp:271:14:271:27 | new[] | test.cpp:276:12:276:13 | xs | This write may overflow $@ by 1 element. | test.cpp:276:12:276:13 | xs | xs |

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

@@ -273,6 +273,6 @@ void test8(unsigned size, unsigned src_pos)
     src_pos = size;
   }
   if (src_pos < size - 1) {
-    memset(xs, 0, src_pos + 1); // GOOD [FALSE POSITIVE]
+    memset(xs, 0, src_pos + 1); // GOOD
   }
 }