Skip to content

C++: stitch paths and ignore cast arrays in constant off-by-one query #13045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rdmarsh2 merged 12 commits into from
May 26, 2023
Merged

C++: stitch paths and ignore cast arrays in constant off-by-one query #13045

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3abf5d1
C++: stitch paths in array off-by-one query
May 4, 2023
d9665e1
C++: add case test for constant off-by-one query
May 4, 2023
b7653ec
C++: ignore cast arrays in constant off-by-one query
May 4, 2023
f77c77f
C++: refactor off-by-one query to use flowstate
May 10, 2023
584adf8
C++: restrict flowstates in constant off-by-one query
May 12, 2023
bf07b0f
C++: fix cxartesian product in constant off-by-one query
May 19, 2023
604affd
C++: autoformat
May 22, 2023
4ed7450
C++: remove unneeded pragma
May 22, 2023
64d7b49
C++: Prune flow states based on 'PointerArithmeticToDerefConfig'.
MathiasVP May 24, 2023
298013a
C++: Add in-barrier on sources to reduce duplication.
MathiasVP May 24, 2023
ebc1d5f
Merge branch 'main' into rdmarsh2/cpp/improve-constant-off-by-one
MathiasVP May 24, 2023
c3fdc83
C++: Also add an out barrier on all sinks.
MathiasVP May 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 47 additions & 39 deletions cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 PointerArithmeticToDerefFlow::PathGraph
import FieldAddressToDerefFlow::PathGraph

pragma[nomagic]
Instruction getABoundIn(SemBound b, IRFunction func) {
Expand Down Expand Up @@ -42,21 +42,6 @@ bindingset[b]
pragma[inline_late]
predicate bounded2(Instruction i, Instruction b, int delta) { boundedImpl(i, b, delta) }

module FieldAddressToPointerArithmeticConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { isFieldAddressSource(_, source) }

predicate isSink(DataFlow::Node sink) {
exists(PointerAddInstruction pai | pai.getLeft() = sink.asInstruction())
}
}

module FieldAddressToPointerArithmeticFlow =
DataFlow::Global<FieldAddressToPointerArithmeticConfig>;

predicate isFieldAddressSource(Field f, DataFlow::Node source) {
source.asInstruction().(FieldAddressInstruction).getField() = f
}

bindingset[delta]
predicate isInvalidPointerDerefSinkImpl(
int delta, Instruction i, AddressOperand addr, string operation
Expand Down Expand Up @@ -93,38 +78,61 @@ predicate isInvalidPointerDerefSink2(DataFlow::Node sink, Instruction i, string
)
}

predicate isConstantSizeOverflowSource(Field f, PointerAddInstruction pai, int delta) {
exists(int size, int bound, DataFlow::Node source, DataFlow::InstructionNode sink |
FieldAddressToPointerArithmeticFlow::flow(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
)
predicate pointerArithOverflow(
PointerArithmeticInstruction pai, Field f, int size, int bound, int delta
) {
pai.getElementSize() = f.getUnspecifiedType().(ArrayType).getBaseType().getSize() and
f.getUnspecifiedType().(ArrayType).getArraySize() = size and
semBounded(getSemanticExpr(pai.getRight()), any(SemZeroBound b), bound, true, _) and
delta = bound - size
}

module PointerArithmeticToDerefConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) {
isConstantSizeOverflowSource(_, source.asInstruction(), _)
module FieldAddressToDerefConfig implements DataFlow::StateConfigSig {
newtype FlowState =
additional TArray(Field f) or
additional TOverflowArithmetic(PointerArithmeticInstruction pai)
Comment thread
MathiasVP marked this conversation as resolved.
Outdated

predicate isSource(DataFlow::Node source, FlowState state) {
exists(Field f |
source.asInstruction().(FieldAddressInstruction).getField() = f and
state = TArray(f)
)
}

pragma[inline]
predicate isSink(DataFlow::Node sink) { isInvalidPointerDerefSink1(sink, _, _) }
predicate isSink(DataFlow::Node sink, FlowState state) {
isInvalidPointerDerefSink1(sink, _, _) and
state instanceof TOverflowArithmetic
Comment thread
MathiasVP marked this conversation as resolved.
Outdated
}

predicate isBarrier(DataFlow::Node node, FlowState state) { none() }

predicate isAdditionalFlowStep(
DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2
) {
exists(PointerArithmeticInstruction pai, Field f, int size, int delta |
state1 = TArray(f) and
state2 = TOverflowArithmetic(pai) and
pai.getLeft() = node1.asInstruction() and
node2.asInstruction() = pai and
pointerArithOverflow(pai, f, size, _, delta) and
delta >= 0 and
size != 0 and
size != 1
)
}
}

module PointerArithmeticToDerefFlow = DataFlow::Global<PointerArithmeticToDerefConfig>;
module FieldAddressToDerefFlow = DataFlow::GlobalWithState<FieldAddressToDerefConfig>;

from
Field f, PointerArithmeticToDerefFlow::PathNode source,
PointerArithmeticToDerefFlow::PathNode sink, Instruction deref, string operation, int delta
Field f, FieldAddressToDerefFlow::PathNode source, PointerArithmeticInstruction pai,
FieldAddressToDerefFlow::PathNode sink, Instruction deref, string operation, int delta
where
PointerArithmeticToDerefFlow::flowPath(source, sink) and
FieldAddressToDerefFlow::flowPath(source, sink) and
isInvalidPointerDerefSink2(sink.getNode(), deref, operation) and
isConstantSizeOverflowSource(f, source.getNode().asInstruction(), delta)
select source, source, sink,
source.getState() = FieldAddressToDerefConfig::TArray(f) and
sink.getState() = FieldAddressToDerefConfig::TOverflowArithmetic(pai) and
pointerArithOverflow(pai, f, _, _, 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
92 changes: 80 additions & 12 deletions ...imental/query-tests/Security/CWE/CWE-193/constant-size/ConstantSizeArrayOffByOne.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,37 +1,105 @@
edges
| test.cpp:26:10:26:12 | buf | test.cpp:26:5:26:12 | buf |
| test.cpp:30:10:30:12 | buf | test.cpp:30:5:30:12 | buf |
| test.cpp:34:10:34:12 | buf | test.cpp:34:5:34:12 | buf |
| test.cpp:35:5:35:12 | buf | test.cpp:35:5:35:22 | access to array |
| test.cpp:35:10:35:12 | buf | test.cpp:35:5:35:12 | buf |
| test.cpp:36:5:36:12 | buf | test.cpp:36:5:36:24 | access to array |
| test.cpp:36:10:36:12 | buf | test.cpp:36:5:36:12 | buf |
| test.cpp:39:14:39:16 | buf | test.cpp:39:9:39:16 | buf |
| test.cpp:43:9:43:16 | buf | test.cpp:43:9:43:19 | access to array |
| test.cpp:43:14:43:16 | buf | test.cpp:43:9:43:16 | buf |
| test.cpp:48:10:48:12 | buf | test.cpp:48:5:48:12 | buf |
| test.cpp:49:5:49:12 | buf | test.cpp:49:5:49:22 | access to array |
| test.cpp:49:10:49:12 | buf | test.cpp:49:5:49:12 | buf |
| test.cpp:50:5:50:12 | buf | test.cpp:50:5:50:24 | access to array |
| test.cpp:50:10:50:12 | buf | test.cpp:50:5:50:12 | buf |
| test.cpp:53:14:53:16 | buf | test.cpp:53:9:53:16 | buf |
| test.cpp:57:9:57:16 | buf | test.cpp:57:9:57:19 | access to array |
| test.cpp:57:14:57:16 | buf | test.cpp:57:9:57:16 | buf |
| test.cpp:61:9:61:16 | buf | test.cpp:61:9:61:19 | access to array |
| test.cpp:61:14:61:16 | buf | test.cpp:61:9:61:16 | buf |
| test.cpp:66:32:66:32 | p | test.cpp:66:32:66:32 | p |
| 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 | p |
| test.cpp:70:33:70:33 | p | test.cpp:71:5:71:5 | p |
| test.cpp:70:33:70:33 | p | test.cpp:72:5:72:5 | p |
| test.cpp:72:5:72:5 | p | test.cpp:72:5:72:15 | access to array |
| test.cpp:76:32:76:34 | buf | test.cpp:76:27:76:34 | buf |
| 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:34 | buf | test.cpp:77:27:77:44 | access to array |
| test.cpp:77:27:77:44 | access to array | test.cpp:77:26:77:44 | & ... |
| test.cpp:77:32:77:34 | buf | test.cpp:77:27:77:34 | buf |
| 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:11 | charBuf |
| test.cpp:85:34:85:36 | buf | test.cpp:88:5:88:11 | charBuf |
nodes
| test.cpp:26:5:26:12 | buf | semmle.label | buf |
| test.cpp:26:10:26:12 | buf | semmle.label | buf |
| test.cpp:30:5:30:12 | buf | semmle.label | buf |
| test.cpp:30:10:30:12 | buf | semmle.label | buf |
| test.cpp:34:5:34:12 | buf | semmle.label | buf |
| test.cpp:34:10:34:12 | buf | semmle.label | buf |
| test.cpp:35:5:35:12 | buf | semmle.label | buf |
| test.cpp:35:5:35:22 | access to array | semmle.label | access to array |
| test.cpp:35:10:35:12 | buf | semmle.label | buf |
| test.cpp:36:5:36:12 | buf | semmle.label | buf |
| test.cpp:36:5:36:24 | access to array | semmle.label | access to array |
| test.cpp:36:10:36:12 | buf | semmle.label | buf |
| test.cpp:39:9:39:16 | buf | semmle.label | buf |
| test.cpp:39:14:39:16 | buf | semmle.label | buf |
| test.cpp:43:9:43:16 | buf | semmle.label | buf |
| test.cpp:43:9:43:19 | access to array | semmle.label | access to array |
| test.cpp:43:14:43:16 | buf | semmle.label | buf |
| test.cpp:48:5:48:12 | buf | semmle.label | buf |
| test.cpp:48:10:48:12 | buf | semmle.label | buf |
| test.cpp:49:5:49:12 | buf | semmle.label | buf |
| test.cpp:49:5:49:22 | access to array | semmle.label | access to array |
| test.cpp:49:10:49:12 | buf | semmle.label | buf |
| test.cpp:50:5:50:12 | buf | semmle.label | buf |
| test.cpp:50:5:50:24 | access to array | semmle.label | access to array |
| test.cpp:50:10:50:12 | buf | semmle.label | buf |
| test.cpp:53:9:53:16 | buf | semmle.label | buf |
| test.cpp:53:14:53:16 | buf | semmle.label | buf |
| test.cpp:57:9:57:16 | buf | semmle.label | buf |
| test.cpp:57:9:57:19 | access to array | semmle.label | access to array |
| test.cpp:57:14:57:16 | buf | semmle.label | buf |
| test.cpp:61:9:61:16 | buf | semmle.label | buf |
| test.cpp:61:9:61:19 | access to array | semmle.label | access to array |
| test.cpp:61:14:61:16 | buf | semmle.label | buf |
| test.cpp:66:32:66:32 | p | semmle.label | p |
| 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 | p | semmle.label | p |
| test.cpp:70:33:70:33 | p | semmle.label | p |
| test.cpp:71:5:71:5 | p | semmle.label | p |
| test.cpp:72:5:72:5 | p | semmle.label | p |
| test.cpp:72:5:72:15 | access to array | semmle.label | access to array |
| test.cpp:76:27:76:34 | buf | semmle.label | buf |
| test.cpp:76:32:76:34 | buf | semmle.label | buf |
| test.cpp:77:26:77:44 | & ... | semmle.label | & ... |
| test.cpp:77:27:77:34 | buf | semmle.label | buf |
| test.cpp:77:27:77:44 | access to array | semmle.label | access to array |
| 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:11 | charBuf | semmle.label | charBuf |
| test.cpp:88:5:88:11 | charBuf | semmle.label | charBuf |
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 | 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: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 | 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:35:5:35:22 | 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 |
| test.cpp:36:5:36:24 | access to array | test.cpp:36:10:36:12 | buf | 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:14:43:16 | buf | 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:10:49:12 | buf | 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:10:50:12 | buf | 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:14:57:16 | buf | 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: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 | 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 | 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:77:27:77:44 | 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:77:27:77:44 | access to array | test.cpp:77:32:77:34 | buf | 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:32:77:34 | buf | test.cpp:67:6:67:6 | 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 |
9 changes: 9 additions & 0 deletions cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/constant-size/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,3 +78,12 @@ void testInterproc(BigArray *arr) {

addToPointerAndAssign(arr->buf);
}

#define MAX_SIZE_BYTES 4096

void testCharIndex(BigArray *arr) {
char *charBuf = (char*) arr->buf;

charBuf[MAX_SIZE_BYTES - 1] = 0; // GOOD
charBuf[MAX_SIZE_BYTES] = 0; // BAD [FALSE NEGATIVE]
}