C++: include stack-allocated arrays in off-by-one query

Author: rdmarsh2

cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql

@@ -14,7 +14,7 @@ import semmle.code.cpp.rangeanalysis.new.internal.semantic.analysis.RangeAnalysi
 import semmle.code.cpp.rangeanalysis.new.internal.semantic.SemanticExprSpecific
 import semmle.code.cpp.ir.IR
 import semmle.code.cpp.ir.dataflow.DataFlow
-import FieldAddressToDerefFlow::PathGraph
+import ArrayAddressToDerefFlow::PathGraph
 
 pragma[nomagic]
 Instruction getABoundIn(SemBound b, IRFunction func) {
@@ -79,10 +79,10 @@ predicate isInvalidPointerDerefSink2(DataFlow::Node sink, Instruction i, string
 }
 
 predicate pointerArithOverflow0(
-  PointerArithmeticInstruction pai, Field f, int size, int bound, int delta
+  PointerArithmeticInstruction pai, Variable v, int size, int bound, int delta
 ) {
-  pai.getElementSize() = f.getUnspecifiedType().(ArrayType).getBaseType().getSize() and
-  f.getUnspecifiedType().(ArrayType).getArraySize() = size and
+  pai.getElementSize() = v.getUnspecifiedType().(ArrayType).getBaseType().getSize() and
+  v.getUnspecifiedType().(ArrayType).getArraySize() = size and
   semBounded(getSemanticExpr(pai.getRight()), any(SemZeroBound b), bound, true, _) and
   delta = bound - size and
   delta >= 0 and
@@ -105,23 +105,28 @@ module PointerArithmeticToDerefConfig implements DataFlow::ConfigSig {
 module PointerArithmeticToDerefFlow = DataFlow::Global<PointerArithmeticToDerefConfig>;
 
 predicate pointerArithOverflow(
-  PointerArithmeticInstruction pai, Field f, int size, int bound, int delta
+  PointerArithmeticInstruction pai, Variable v, int size, int bound, int delta
 ) {
-  pointerArithOverflow0(pai, f, size, bound, delta) and
+  pointerArithOverflow0(pai, v, size, bound, delta) and
   PointerArithmeticToDerefFlow::flow(DataFlow::instructionNode(pai), _)
 }
 
-module FieldAddressToDerefConfig implements DataFlow::StateConfigSig {
+module ArrayAddressToDerefConfig implements DataFlow::StateConfigSig {
   newtype FlowState =
-    additional TArray(Field f) { pointerArithOverflow(_, f, _, _, _) } or
+    additional TArray(Variable v) { pointerArithOverflow(_, v, _, _, _) } or
     additional TOverflowArithmetic(PointerArithmeticInstruction pai) {
       pointerArithOverflow(pai, _, _, _, _)
     }
 
   predicate isSource(DataFlow::Node source, FlowState state) {
-    exists(Field f |
-      source.asInstruction().(FieldAddressInstruction).getField() = f and
-      state = TArray(f)
+    exists(Variable v |
+      (
+        source.asInstruction().(FieldAddressInstruction).getField() = v
+        or
+        source.asInstruction().(VariableAddressInstruction).getAstVariable() = v
+      ) and
+      exists(v.getUnspecifiedType().(ArrayType).getArraySize()) and
+      state = TArray(v)
     )
   }
 
@@ -141,27 +146,27 @@ module FieldAddressToDerefConfig implements DataFlow::StateConfigSig {
   predicate isAdditionalFlowStep(
     DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2
   ) {
-    exists(PointerArithmeticInstruction pai, Field f |
-      state1 = TArray(f) and
+    exists(PointerArithmeticInstruction pai, Variable v, int size, int delta |
+      state1 = TArray(v) and
       state2 = TOverflowArithmetic(pai) and
       pai.getLeft() = node1.asInstruction() and
       node2.asInstruction() = pai and
-      pointerArithOverflow(pai, f, _, _, _)
+      pointerArithOverflow(pai, v, size, _, delta)
     )
   }
 }
 
-module FieldAddressToDerefFlow = DataFlow::GlobalWithState<FieldAddressToDerefConfig>;
+module ArrayAddressToDerefFlow = DataFlow::GlobalWithState<ArrayAddressToDerefConfig>;
 
 from
-  Field f, FieldAddressToDerefFlow::PathNode source, PointerArithmeticInstruction pai,
-  FieldAddressToDerefFlow::PathNode sink, Instruction deref, string operation, int delta
+  Variable v, ArrayAddressToDerefFlow::PathNode source, PointerArithmeticInstruction pai,
+  ArrayAddressToDerefFlow::PathNode sink, Instruction deref, string operation, int delta
 where
-  FieldAddressToDerefFlow::flowPath(source, sink) and
+ArrayAddressToDerefFlow::flowPath(source, sink) and
   isInvalidPointerDerefSink2(sink.getNode(), deref, operation) and
-  source.getState() = FieldAddressToDerefConfig::TArray(f) and
-  sink.getState() = FieldAddressToDerefConfig::TOverflowArithmetic(pai) and
-  pointerArithOverflow(pai, f, _, _, delta)
+  source.getState() = ArrayAddressToDerefConfig::TArray(v) and
+  sink.getState() = ArrayAddressToDerefConfig::TOverflowArithmetic(pai) and
+  pointerArithOverflow(pai, v, _, _, delta)
 select pai, source, sink,
   "This pointer arithmetic may have an off-by-" + (delta + 1) +
-    " error allowing it to overrun $@ at this $@.", f, f.getName(), deref, operation
+    " error allowing it to overrun $@ at this $@.", v, v.getName(), deref, operation

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/constant-size/ConstantSizeArrayOffByOne.expected

@@ -11,6 +11,7 @@ edges
 | test.cpp:77:32:77:34 | buf | test.cpp:77:26:77:44 | & ... |
 | test.cpp:79:27:79:34 | buf | test.cpp:70:33:70:33 | p |
 | test.cpp:79:32:79:34 | buf | test.cpp:79:27:79:34 | buf |
+| test.cpp:128:9:128:11 | arr | test.cpp:128:9:128:14 | access to array |
 nodes
 | test.cpp:35:5:35:22 | access to array | semmle.label | access to array |
 | test.cpp:35:10:35:12 | buf | semmle.label | buf |
@@ -33,6 +34,8 @@ nodes
 | test.cpp:77:32:77:34 | buf | semmle.label | buf |
 | test.cpp:79:27:79:34 | buf | semmle.label | buf |
 | test.cpp:79:32:79:34 | buf | semmle.label | buf |
+| test.cpp:128:9:128:11 | arr | semmle.label | arr |
+| test.cpp:128:9:128:14 | access to array | semmle.label | access to array |
 subpaths
 #select
 | test.cpp:35:5:35:22 | PointerAdd: access to array | test.cpp:35:10:35:12 | buf | test.cpp:35:5:35:22 | access to array | This pointer arithmetic may have an off-by-1 error allowing it to overrun $@ at this $@. | test.cpp:15:9:15:11 | buf | buf | test.cpp:35:5:35:26 | Store: ... = ... | write |
@@ -44,3 +47,4 @@ subpaths
 | test.cpp:61:9:61:19 | PointerAdd: access to array | test.cpp:61:14:61:16 | buf | test.cpp:61:9:61:19 | access to array | This pointer arithmetic may have an off-by-2 error allowing it to overrun $@ at this $@. | test.cpp:19:9:19:11 | buf | buf | test.cpp:61:9:61:23 | Store: ... = ... | write |
 | test.cpp:72:5:72:15 | PointerAdd: access to array | test.cpp:79:32:79:34 | buf | test.cpp:72:5:72:15 | access to array | This pointer arithmetic may have an off-by-1 error allowing it to overrun $@ at this $@. | test.cpp:15:9:15:11 | buf | buf | test.cpp:72:5:72:19 | Store: ... = ... | write |
 | test.cpp:77:27:77:44 | PointerAdd: access to array | test.cpp:77:32:77:34 | buf | test.cpp:66:32:66:32 | p | This pointer arithmetic may have an off-by-1 error allowing it to overrun $@ at this $@. | test.cpp:15:9:15:11 | buf | buf | test.cpp:67:5:67:10 | Store: ... = ... | write |
+| test.cpp:128:9:128:14 | PointerAdd: access to array | test.cpp:128:9:128:11 | arr | test.cpp:128:9:128:14 | access to array | This pointer arithmetic may have an off-by-1 error allowing it to overrun $@ at this $@. | test.cpp:125:11:125:13 | arr | arr | test.cpp:128:9:128:18 | Store: ... = ... | write |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/constant-size/test.cpp

@@ -120,3 +120,11 @@ void testEqRefinement2() {
         }
     }
 }
+
+void testStackAllocated() {
+    char *arr[MAX_SIZE];
+
+    for(int i = 0; i <= MAX_SIZE; i++) {
+        arr[i] = 0; // BAD
+    }
+}