diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.cpp b/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.cpp new file mode 100644 index 000000000000..408ae2d5e533 --- /dev/null +++ b/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 + } +} \ No newline at end of file diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.qhelp b/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.qhelp new file mode 100644 index 000000000000..c9e2673f0797 --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.qhelp @@ -0,0 +1,29 @@ + + + +

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.

+ + + + +

Ensure that pointer dereferences are properly guarded to ensure that they cannot be used to read or write past the end of the allocation.

+ + + +

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.

+ + + + + +
  • CERT C Coding Standard: +ARR30-C. Do not form or use out-of-bounds pointers or array subscripts.
    • +
  • +OWASP: +Buffer Overflow. +
    • + +     +     diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql b/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql new file mode 100644 index 000000000000..990c43564256 --- /dev/null +++ b/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 diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/constant-size/ConstantSizeArrayOffByOne.expected b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/constant-size/ConstantSizeArrayOffByOne.expected new file mode 100644 index 000000000000..9d68450439e9 --- /dev/null +++ b/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 | diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/constant-size/ConstantSizeArrayOffByOne.qlref b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/constant-size/ConstantSizeArrayOffByOne.qlref new file mode 100644 index 000000000000..082e8951c70d --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/constant-size/ConstantSizeArrayOffByOne.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/constant-size/test.cpp b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/constant-size/test.cpp new file mode 100644 index 000000000000..df4cd7b44911 --- /dev/null +++ b/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); +}