From 6e230e10f8205436e617581b9b2ac7844634d4a5 Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Wed, 10 May 2023 15:28:01 -0400 Subject: [PATCH 1/6] C++: include stack-allocated arrays in off-by-one query --- .../CWE/CWE-193/ConstantSizeArrayOffByOne.ql | 49 ++++++++++--------- .../ConstantSizeArrayOffByOne.expected | 4 ++ .../CWE/CWE-193/constant-size/test.cpp | 8 +++ 3 files changed, 39 insertions(+), 22 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql b/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql index 88db396f2cf2..0f8609c3c43b 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql +++ b/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; 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; +module ArrayAddressToDerefFlow = DataFlow::GlobalWithState; 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 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 index 7d3df8cb7cb6..831280a015e4 100644 --- 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 @@ -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 | 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 index a33f43bfa497..64b32b9814e6 100644 --- 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 @@ -120,3 +120,11 @@ void testEqRefinement2() { } } } + +void testStackAllocated() { + char *arr[MAX_SIZE]; + + for(int i = 0; i <= MAX_SIZE; i++) { + arr[i] = 0; // BAD + } +} From df4d156a36489ab02f15eb8a2c1ae07dee097ad4 Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Thu, 1 Jun 2023 11:28:12 -0400 Subject: [PATCH 2/6] C++: remove unneeded exists variables --- .../Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql b/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql index 0f8609c3c43b..646044bc4f92 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql @@ -146,12 +146,12 @@ module ArrayAddressToDerefConfig implements DataFlow::StateConfigSig { predicate isAdditionalFlowStep( DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2 ) { - exists(PointerArithmeticInstruction pai, Variable v, int size, int delta | + exists(PointerArithmeticInstruction pai, Variable v | state1 = TArray(v) and state2 = TOverflowArithmetic(pai) and pai.getLeft() = node1.asInstruction() and node2.asInstruction() = pai and - pointerArithOverflow(pai, v, size, _, delta) + pointerArithOverflow(pai, v, _, _, _) ) } } From c9c93ca701b64095507b7be45494dd20145b668f Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Thu, 1 Jun 2023 11:37:52 -0400 Subject: [PATCH 3/6] C++: test for strncmp false positives --- .../ConstantSizeArrayOffByOne.expected | 10 ++++++++++ .../Security/CWE/CWE-193/constant-size/test.cpp | 16 +++++++++++++++- 2 files changed, 25 insertions(+), 1 deletion(-) 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 index 831280a015e4..88e32ccf921d 100644 --- 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 @@ -12,6 +12,10 @@ edges | 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 | +| test.cpp:134:25:134:27 | arr | test.cpp:136:9:136:16 | ... += ... | +| test.cpp:136:9:136:16 | ... += ... | test.cpp:138:13:138:15 | arr | +| test.cpp:143:18:143:21 | asdf | test.cpp:134:25:134:27 | arr | +| test.cpp:143:18:143:21 | asdf | test.cpp:143:18:143:21 | asdf | 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 | @@ -36,6 +40,11 @@ nodes | 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 | +| test.cpp:134:25:134:27 | arr | semmle.label | arr | +| test.cpp:136:9:136:16 | ... += ... | semmle.label | ... += ... | +| test.cpp:138:13:138:15 | arr | semmle.label | arr | +| test.cpp:143:18:143:21 | asdf | semmle.label | asdf | +| test.cpp:143:18:143:21 | asdf | semmle.label | asdf | 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 | @@ -48,3 +57,4 @@ subpaths | 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 | +| test.cpp:136:9:136:16 | PointerAdd: ... += ... | test.cpp:143:18:143:21 | asdf | test.cpp:138:13:138:15 | arr | This pointer arithmetic may have an off-by-2 error allowing it to overrun $@ at this $@. | test.cpp:142:10:142:13 | asdf | asdf | test.cpp:138:12:138:15 | Load: * ... | read | 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 index 64b32b9814e6..f799518f6ec3 100644 --- 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 @@ -85,7 +85,7 @@ void testCharIndex(BigArray *arr) { char *charBuf = (char*) arr->buf; charBuf[MAX_SIZE_BYTES - 1] = 0; // GOOD - charBuf[MAX_SIZE_BYTES] = 0; // BAD [FALSE NEGATIVE] + charBuf[MAX_SIZE_BYTES] = 0; // BAD } void testEqRefinement() { @@ -128,3 +128,17 @@ void testStackAllocated() { arr[i] = 0; // BAD } } + +int strncmp(const char*, const char*, int); + +char testStrncmp2(char *arr) { + if(strncmp(arr, "", 6) == 0) { + arr += 6; + } + return *arr; // GOOD [FALSE POSITIVE] +} + +void testStrncmp1() { + char asdf[5]; + testStrncmp2(asdf); +} From e32f7d84a5303da643fe4eb398ad2568cb57949c Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Sun, 25 Jun 2023 00:35:43 +0100 Subject: [PATCH 4/6] C++: Speed up analysis on 'Samate' by avoiding the 'Variable' column in the dataflow stages of the query. --- .../CWE/CWE-193/ConstantSizeArrayOffByOne.ql | 81 ++++++++++++------- .../ConstantSizeArrayOffByOne.expected | 5 ++ 2 files changed, 55 insertions(+), 31 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql b/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql index 9a17d4c37acc..b3258a6e627c 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql @@ -78,28 +78,39 @@ predicate isInvalidPointerDerefSink2(DataFlow::Node sink, Instruction i, string ) } +predicate arrayTypeCand(ArrayType arrayType) { any(Variable v).getUnspecifiedType() = arrayType } + pragma[nomagic] predicate arrayTypeHasSizes(ArrayType arr, int baseTypeSize, int arraySize) { + arrayTypeCand(arr) and arr.getBaseType().getSize() = baseTypeSize and arr.getArraySize() = arraySize } -predicate pointerArithOverflow0( - PointerArithmeticInstruction pai, Variable v, int size, int bound, int delta -) { - not v.getNamespace() instanceof StdNamespace and - arrayTypeHasSizes(v.getUnspecifiedType(), pai.getElementSize(), size) and - semBounded(getSemanticExpr(pai.getRight()), any(SemZeroBound b), bound, true, _) and +bindingset[pai] +pragma[inline_late] +predicate constantUpperBounded(PointerArithmeticInstruction pai, int delta) { + semBounded(getSemanticExpr(pai.getRight()), any(SemZeroBound b), delta, true, _) +} + +bindingset[pai, size] +predicate pointerArithOverflow0Impl(PointerArithmeticInstruction pai, int size, int bound, int delta) { + constantUpperBounded(pai, bound) and delta = bound - size and delta >= 0 and size != 0 and size != 1 } +predicate pointerArithOverflow0(PointerArithmeticInstruction pai, int delta) { + exists(int size, int bound | + arrayTypeHasSizes(_, pai.getElementSize(), size) and + pointerArithOverflow0Impl(pai, size, bound, delta) + ) +} + module PointerArithmeticToDerefConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node source) { - pointerArithOverflow0(source.asInstruction(), _, _, _, _) - } + predicate isSource(DataFlow::Node source) { pointerArithOverflow0(source.asInstruction(), _) } predicate isBarrierIn(DataFlow::Node node) { isSource(node) } @@ -110,30 +121,38 @@ module PointerArithmeticToDerefConfig implements DataFlow::ConfigSig { module PointerArithmeticToDerefFlow = DataFlow::Global; -predicate pointerArithOverflow( - PointerArithmeticInstruction pai, Variable v, int size, int bound, int delta -) { - pointerArithOverflow0(pai, v, size, bound, delta) and +predicate pointerArithOverflow(PointerArithmeticInstruction pai, int delta) { + pointerArithOverflow0(pai, delta) and PointerArithmeticToDerefFlow::flow(DataFlow::instructionNode(pai), _) } +bindingset[v] +predicate finalPointerArithOverflow(Variable v, PointerArithmeticInstruction pai, int delta) { + exists(int size | + arrayTypeHasSizes(pragma[only_bind_out](v.getUnspecifiedType()), pai.getElementSize(), size) and + pointerArithOverflow0Impl(pai, size, _, delta) + ) +} + +predicate isSourceImpl(DataFlow::Node source, Variable v) { + ( + source.asInstruction().(FieldAddressInstruction).getField() = v + or + source.asInstruction().(VariableAddressInstruction).getAstVariable() = v + ) and + exists(v.getUnspecifiedType().(ArrayType).getArraySize()) +} + module ArrayAddressToDerefConfig implements DataFlow::StateConfigSig { newtype FlowState = - additional TArray(Variable v) { pointerArithOverflow(_, v, _, _, _) } or + additional TArray() or additional TOverflowArithmetic(PointerArithmeticInstruction pai) { - pointerArithOverflow(pai, _, _, _, _) + pointerArithOverflow(pai, _) } predicate isSource(DataFlow::Node source, FlowState state) { - exists(Variable v | - ( - source.asInstruction().(FieldAddressInstruction).getField() = v - or - source.asInstruction().(VariableAddressInstruction).getAstVariable() = v - ) and - exists(v.getUnspecifiedType().(ArrayType).getArraySize()) and - state = TArray(v) - ) + isSourceImpl(source, _) and + state = TArray() } predicate isSink(DataFlow::Node sink, FlowState state) { @@ -152,12 +171,12 @@ module ArrayAddressToDerefConfig implements DataFlow::StateConfigSig { predicate isAdditionalFlowStep( DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2 ) { - exists(PointerArithmeticInstruction pai, Variable v | - state1 = TArray(v) and + exists(PointerArithmeticInstruction pai | + state1 = TArray() and state2 = TOverflowArithmetic(pai) and pai.getLeft() = node1.asInstruction() and node2.asInstruction() = pai and - pointerArithOverflow(pai, v, _, _, _) + pointerArithOverflow(pai, _) ) } } @@ -168,11 +187,11 @@ from Variable v, ArrayAddressToDerefFlow::PathNode source, PointerArithmeticInstruction pai, ArrayAddressToDerefFlow::PathNode sink, Instruction deref, string operation, int delta where - ArrayAddressToDerefFlow::flowPath(source, sink) and + ArrayAddressToDerefFlow::flowPath(pragma[only_bind_into](source), pragma[only_bind_into](sink)) and isInvalidPointerDerefSink2(sink.getNode(), deref, operation) and - source.getState() = ArrayAddressToDerefConfig::TArray(v) and - sink.getState() = ArrayAddressToDerefConfig::TOverflowArithmetic(pai) and - pointerArithOverflow(pai, v, _, _, delta) + pragma[only_bind_out](sink.getState()) = ArrayAddressToDerefConfig::TOverflowArithmetic(pai) and + isSourceImpl(source.getNode(), v) and + finalPointerArithOverflow(v, pai, delta) select pai, source, sink, "This pointer arithmetic may have an off-by-" + (delta + 1) + " error allowing it to overrun $@ at this $@.", v, v.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 index 88e32ccf921d..c85ad8f95c10 100644 --- 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 @@ -11,6 +11,8 @@ 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:85:34:85:36 | buf | test.cpp:87:5:87:31 | access to array | +| test.cpp:85:34:85:36 | buf | test.cpp:88:5:88:27 | access to array | | test.cpp:128:9:128:11 | arr | test.cpp:128:9:128:14 | access to array | | test.cpp:134:25:134:27 | arr | test.cpp:136:9:136:16 | ... += ... | | test.cpp:136:9:136:16 | ... += ... | test.cpp:138:13:138:15 | arr | @@ -38,6 +40,9 @@ 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:85:34:85:36 | buf | semmle.label | buf | +| test.cpp:87:5:87:31 | access to array | semmle.label | access to array | +| test.cpp:88:5:88:27 | access to array | semmle.label | access to array | | test.cpp:128:9:128:11 | arr | semmle.label | arr | | test.cpp:128:9:128:14 | access to array | semmle.label | access to array | | test.cpp:134:25:134:27 | arr | semmle.label | arr | From 3b4f2b22d65dc9de7b7df140b043243bd87bee6b Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 26 Jun 2023 11:36:56 +0100 Subject: [PATCH 5/6] C++: Fix Code Scanning errors. --- .../CWE/CWE-193/ConstantSizeArrayOffByOne.ql | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql b/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql index b3258a6e627c..eebcce6baea0 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql @@ -94,18 +94,21 @@ predicate constantUpperBounded(PointerArithmeticInstruction pai, int delta) { } bindingset[pai, size] -predicate pointerArithOverflow0Impl(PointerArithmeticInstruction pai, int size, int bound, int delta) { - constantUpperBounded(pai, bound) and - delta = bound - size and - delta >= 0 and - size != 0 and - size != 1 +predicate pointerArithOverflow0Impl(PointerArithmeticInstruction pai, int size, int delta) { + exists(int bound | + constantUpperBounded(pai, bound) and + delta = bound - size and + delta >= 0 and + size != 0 and + size != 1 + ) } +pragma[nomagic] predicate pointerArithOverflow0(PointerArithmeticInstruction pai, int delta) { - exists(int size, int bound | + exists(int size | arrayTypeHasSizes(_, pai.getElementSize(), size) and - pointerArithOverflow0Impl(pai, size, bound, delta) + pointerArithOverflow0Impl(pai, size, delta) ) } @@ -130,7 +133,7 @@ bindingset[v] predicate finalPointerArithOverflow(Variable v, PointerArithmeticInstruction pai, int delta) { exists(int size | arrayTypeHasSizes(pragma[only_bind_out](v.getUnspecifiedType()), pai.getElementSize(), size) and - pointerArithOverflow0Impl(pai, size, _, delta) + pointerArithOverflow0Impl(pai, size, delta) ) } From d68b0605cdee725fb63954734d9860e8f0d5d4f0 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 26 Jun 2023 11:37:35 +0100 Subject: [PATCH 6/6] C++: Use 'arrayTypeCand' in 'isSourceImpl' instead of checking for array size explicitly. --- .../Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql b/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql index eebcce6baea0..42afc6f21193 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql @@ -78,7 +78,10 @@ predicate isInvalidPointerDerefSink2(DataFlow::Node sink, Instruction i, string ) } -predicate arrayTypeCand(ArrayType arrayType) { any(Variable v).getUnspecifiedType() = arrayType } +predicate arrayTypeCand(ArrayType arrayType) { + any(Variable v).getUnspecifiedType() = arrayType and + exists(arrayType.getArraySize()) +} pragma[nomagic] predicate arrayTypeHasSizes(ArrayType arr, int baseTypeSize, int arraySize) { @@ -143,7 +146,7 @@ predicate isSourceImpl(DataFlow::Node source, Variable v) { or source.asInstruction().(VariableAddressInstruction).getAstVariable() = v ) and - exists(v.getUnspecifiedType().(ArrayType).getArraySize()) + arrayTypeCand(v.getUnspecifiedType()) } module ArrayAddressToDerefConfig implements DataFlow::StateConfigSig {