Merge pull request #10562 from rdmarsh2/rdmarsh2/cpp/field-off-by-one

C++: prototype for off-by-one in array-typed field

Author: MathiasVP

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

@@ -0,0 +1,17 @@
+#define MAX_SIZE 1024
+
+struct FixedArray {
+  int buf[MAX_SIZE];
+};
+
+int main(){
+  FixedArray arr;
+
+  for(int i = 0; i <= MAX_SIZE; i++) {
+    arr.buf[i] = 0; // BAD
+  }
+
+  for(int i = 0; i < MAX_SIZE; i++) {
+    arr.buf[i] = 0; // GOOD
+  }
+}

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

@@ -0,0 +1,29 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>The program performs an out-of-bounds read or write operation. In addition to causing program instability, techniques exist which may allow an attacker to use this vulnerability to execute arbitrary code.</p>
+
+</overview>
+<recommendation>
+
+<p>Ensure that pointer dereferences are properly guarded to ensure that they cannot be used to read or write past the end of the allocation.</p>
+
+</recommendation>
+<example>
+<p>The first example uses a for loop which is improperly bounded by a non-strict less-than operation and will write one position past the end of the array. The second example bounds the for loop properly with a strict less-than operation.</p>
+<sample src="ConstantSizeArrayOffByOne.cpp" />
+
+</example>
+<references>
+
+<li>CERT C Coding Standard:
+<a href="https://wiki.sei.cmu.edu/confluence/display/c/ARR30-C.+Do+not+form+or+use+out-of-bounds+pointers+or+array+subscripts">ARR30-C. Do not form or use out-of-bounds pointers or array subscripts</a>.</li>
+<li>
+OWASP:
+<a href="https://owasp.org/www-community/vulnerabilities/Buffer_Overflow">Buffer Overflow</a>.
+</li>
+
+</references>
+</qhelp>

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

@@ -0,0 +1,107 @@
+/**
+ * @name Constant array overflow
+ * @description Dereferencing a pointer that points past a statically-sized array is undefined behavior
+ *              and may lead to security vulnerabilities
+ * @kind path-problem
+ * @problem.severity error
+ * @id cpp/constant-array-overflow
+ * @tags reliability
+ *       security
+ */
+
+import experimental.semmle.code.cpp.semantic.analysis.RangeAnalysis
+import experimental.semmle.code.cpp.semantic.SemanticBound
+import experimental.semmle.code.cpp.semantic.SemanticExprSpecific
+import semmle.code.cpp.ir.IR
+import experimental.semmle.code.cpp.ir.dataflow.DataFlow
+import experimental.semmle.code.cpp.ir.dataflow.DataFlow2
+import DataFlow2::PathGraph
+
+pragma[nomagic]
+Instruction getABoundIn(SemBound b, IRFunction func) {
+  result = b.getExpr(0) and
+  result.getEnclosingIRFunction() = func
+}
+
+/**
+ * Holds if `i <= b + delta`.
+ */
+pragma[nomagic]
+predicate bounded(Instruction i, Instruction b, int delta) {
+  exists(SemBound bound, IRFunction func |
+    semBounded(getSemanticExpr(i), bound, delta, true, _) and
+    b = getABoundIn(bound, func) and
+    i.getEnclosingIRFunction() = func
+  )
+}
+
+class FieldAddressToPointerArithmeticConf extends DataFlow::Configuration {
+  FieldAddressToPointerArithmeticConf() { this = "FieldAddressToPointerArithmeticConf" }
+
+  override predicate isSource(DataFlow::Node source) { isFieldAddressSource(_, source) }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(PointerAddInstruction pai | pai.getLeft() = sink.asInstruction())
+  }
+}
+
+predicate isFieldAddressSource(Field f, DataFlow::Node source) {
+  source.asInstruction().(FieldAddressInstruction).getField() = f
+}
+
+/**
+ * Holds if `sink` is a sink for `InvalidPointerToDerefConf` and `i` is a `StoreInstruction` that
+ * writes to an address that non-strictly upper-bounds `sink`, or `i` is a `LoadInstruction` that
+ * reads from an address that non-strictly upper-bounds `sink`.
+ */
+predicate isInvalidPointerDerefSink(DataFlow::Node sink, Instruction i, string operation) {
+  exists(AddressOperand addr, int delta |
+    bounded(addr.getDef(), sink.asInstruction(), delta) and
+    delta >= 0 and
+    i.getAnOperand() = addr
+  |
+    i instanceof StoreInstruction and
+    operation = "write"
+    or
+    i instanceof LoadInstruction and
+    operation = "read"
+  )
+}
+
+predicate isConstantSizeOverflowSource(Field f, PointerAddInstruction pai, int delta) {
+  exists(
+    int size, int bound, FieldAddressToPointerArithmeticConf conf, DataFlow::Node source,
+    DataFlow::InstructionNode sink
+  |
+    conf.hasFlow(source, sink) and
+    isFieldAddressSource(f, source) and
+    pai.getLeft() = sink.asInstruction() and
+    f.getUnspecifiedType().(ArrayType).getArraySize() = size and
+    semBounded(getSemanticExpr(pai.getRight()), any(SemZeroBound b), bound, true, _) and
+    delta = bound - size and
+    delta >= 0 and
+    size != 0 and
+    size != 1
+  )
+}
+
+class PointerArithmeticToDerefConf extends DataFlow2::Configuration {
+  PointerArithmeticToDerefConf() { this = "PointerArithmeticToDerefConf" }
+
+  override predicate isSource(DataFlow::Node source) {
+    isConstantSizeOverflowSource(_, source.asInstruction(), _)
+  }
+
+  override predicate isSink(DataFlow::Node sink) { isInvalidPointerDerefSink(sink, _, _) }
+}
+
+from
+  Field f, DataFlow2::PathNode source, DataFlow2::PathNode sink, Instruction deref,
+  PointerArithmeticToDerefConf conf, string operation, int delta
+where
+  conf.hasFlowPath(source, sink) and
+  isInvalidPointerDerefSink(sink.getNode(), deref, operation) and
+  isConstantSizeOverflowSource(f, source.getNode().asInstruction(), delta)
+select source, source, sink,
+  "This pointer arithmetic may have an off-by-" + (delta + 1) +
+    " error allowing it to overrun $@ at this $@.", f, f.getName(), deref, operation

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

@@ -0,0 +1,37 @@
+edges
+| test.cpp:66:32:66:32 | p | test.cpp:66:32:66:32 | Load |
+| test.cpp:66:32:66:32 | p | test.cpp:67:5:67:6 | * ... |
+| test.cpp:66:32:66:32 | p | test.cpp:67:6:67:6 | Load |
+| test.cpp:77:26:77:44 | & ... | test.cpp:66:32:66:32 | p |
+| test.cpp:77:26:77:44 | & ... | test.cpp:66:32:66:32 | p |
+| test.cpp:77:27:77:44 | access to array | test.cpp:77:26:77:44 | & ... |
+nodes
+| test.cpp:35:5:35:22 | access to array | semmle.label | access to array |
+| test.cpp:36:5:36:24 | access to array | semmle.label | access to array |
+| test.cpp:43:9:43:19 | access to array | semmle.label | access to array |
+| test.cpp:49:5:49:22 | access to array | semmle.label | access to array |
+| test.cpp:50:5:50:24 | access to array | semmle.label | access to array |
+| test.cpp:57:9:57:19 | access to array | semmle.label | access to array |
+| test.cpp:61:9:61:19 | access to array | semmle.label | access to array |
+| test.cpp:66:32:66:32 | Load | semmle.label | Load |
+| test.cpp:66:32:66:32 | p | semmle.label | p |
+| test.cpp:66:32:66:32 | p | semmle.label | p |
+| test.cpp:67:5:67:6 | * ... | semmle.label | * ... |
+| test.cpp:67:6:67:6 | Load | semmle.label | Load |
+| test.cpp:72:5:72:15 | access to array | semmle.label | access to array |
+| test.cpp:77:26:77:44 | & ... | semmle.label | & ... |
+| test.cpp:77:27:77:44 | access to array | semmle.label | access to array |
+subpaths
+#select
+| test.cpp:35:5:35:22 | access to array | test.cpp:35:5:35:22 | access to array | 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 |
+| test.cpp:36:5:36:24 | access to array | test.cpp:36:5:36:24 | access to array | test.cpp:36:5:36:24 | access to array | This pointer arithmetic may have an off-by-2 error allowing it to overrun $@ at this $@. | test.cpp:15:9:15:11 | buf | buf | test.cpp:36:5:36:28 | Store: ... = ... | write |
+| test.cpp:43:9:43:19 | access to array | test.cpp:43:9:43:19 | access to array | test.cpp:43:9:43:19 | 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:43:9:43:23 | Store: ... = ... | write |
+| test.cpp:49:5:49:22 | access to array | test.cpp:49:5:49:22 | access to array | test.cpp:49:5:49:22 | access to array | This pointer arithmetic may have an off-by-1 error allowing it to overrun $@ at this $@. | test.cpp:19:9:19:11 | buf | buf | test.cpp:49:5:49:26 | Store: ... = ... | write |
+| test.cpp:50:5:50:24 | access to array | test.cpp:50:5:50:24 | access to array | test.cpp:50:5:50:24 | 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:50:5:50:28 | Store: ... = ... | write |
+| test.cpp:57:9:57:19 | access to array | test.cpp:57:9:57:19 | access to array | test.cpp:57:9:57:19 | access to array | This pointer arithmetic may have an off-by-1 error allowing it to overrun $@ at this $@. | test.cpp:19:9:19:11 | buf | buf | test.cpp:57:9:57:23 | Store: ... = ... | write |
+| test.cpp:61:9:61:19 | access to array | test.cpp:61:9:61:19 | access to array | 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 | access to array | test.cpp:72:5:72:15 | access to array | 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 | access to array | test.cpp:77:27:77:44 | access to array | test.cpp:66:32:66:32 | Load | 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:77:27:77:44 | access to array | test.cpp:77:27:77:44 | access to array | 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:77:27:77:44 | access to array | test.cpp:77:27:77:44 | access to array | test.cpp:67:5:67:6 | * ... | 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:77:27:77:44 | access to array | test.cpp:77:27:77:44 | access to array | test.cpp:67:6:67:6 | Load | 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 |

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

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql

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

@@ -0,0 +1,80 @@
+#define MAX_SIZE 1024
+
+struct ZeroArray {
+    int size;
+    int buf[0];
+};
+
+struct OneArray {
+    int size;
+    int buf[1];
+};
+
+struct BigArray {
+    int size;
+    int buf[MAX_SIZE];
+};
+
+struct ArrayAndFields {
+    int buf[MAX_SIZE];
+    int field1;
+    int field2;
+};
+
+// tests for dynamic-size trailing arrays
+void testZeroArray(ZeroArray *arr) {
+    arr->buf[0] = 0;
+}
+
+void testOneArray(OneArray *arr) {
+    arr->buf[1] = 0;
+}
+
+void testBig(BigArray *arr) {
+    arr->buf[MAX_SIZE-1] = 0; // GOOD
+    arr->buf[MAX_SIZE] = 0;   // BAD
+    arr->buf[MAX_SIZE+1] = 0; // BAD
+
+    for(int i = 0; i < MAX_SIZE; i++) {
+        arr->buf[i] = 0; // GOOD
+    }
+    
+    for(int i = 0; i <= MAX_SIZE; i++) {
+        arr->buf[i] = 0; // BAD
+    }
+}
+
+void testFields(ArrayAndFields *arr) {
+    arr->buf[MAX_SIZE-1] = 0; // GOOD
+    arr->buf[MAX_SIZE] = 0;   // BAD?
+    arr->buf[MAX_SIZE+1] = 0; // BAD?
+
+    for(int i = 0; i < MAX_SIZE; i++) {
+        arr->buf[i] = 0; // GOOD
+    }
+    
+    for(int i = 0; i <= MAX_SIZE; i++) {
+        arr->buf[i] = 0; // BAD?
+    }
+
+    for(int i = 0; i < MAX_SIZE+2; i++) {
+        arr->buf[i] = 0; // BAD?
+    }
+    // is this different if it's a memcpy?
+}
+
+void assignThroughPointer(int *p) {
+    *p = 0; // ??? should the result go at a flow source?
+}
+
+void addToPointerAndAssign(int *p) {
+    p[MAX_SIZE-1] = 0; // GOOD
+    p[MAX_SIZE] = 0; // BAD
+}
+
+void testInterproc(BigArray *arr) {
+    assignThroughPointer(&arr->buf[MAX_SIZE-1]); // GOOD
+    assignThroughPointer(&arr->buf[MAX_SIZE]); // BAD
+
+    addToPointerAndAssign(arr->buf);
+}