Skip to content

Commit b2fb2aa

Browse files
rdmarsh2rdmarsh2
authored
Merge pull request github#13045 from rdmarsh2/rdmarsh2/cpp/improve-constant-off-by-one
C++: stitch paths and ignore cast arrays in constant off-by-one query
2 parents ded98c5 + c3fdc83 commit b2fb2aa

File tree

3 files changed

+112
-57
lines changed

3 files changed

+112
-57
lines changed

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

Lines changed: 72 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import semmle.code.cpp.rangeanalysis.new.internal.semantic.analysis.RangeAnalysi
1414
import semmle.code.cpp.rangeanalysis.new.internal.semantic.SemanticExprSpecific
1515
import semmle.code.cpp.ir.IR
1616
import semmle.code.cpp.ir.dataflow.DataFlow
17-
import PointerArithmeticToDerefFlow::PathGraph
17+
import FieldAddressToDerefFlow::PathGraph
1818

1919
pragma[nomagic]
2020
Instruction getABoundIn(SemBound b, IRFunction func) {
@@ -42,21 +42,6 @@ bindingset[b]
4242
pragma[inline_late]
4343
predicate bounded2(Instruction i, Instruction b, int delta) { boundedImpl(i, b, delta) }
4444

45-
module FieldAddressToPointerArithmeticConfig implements DataFlow::ConfigSig {
46-
predicate isSource(DataFlow::Node source) { isFieldAddressSource(_, source) }
47-
48-
predicate isSink(DataFlow::Node sink) {
49-
exists(PointerAddInstruction pai | pai.getLeft() = sink.asInstruction())
50-
}
51-
}
52-
53-
module FieldAddressToPointerArithmeticFlow =
54-
DataFlow::Global<FieldAddressToPointerArithmeticConfig>;
55-
56-
predicate isFieldAddressSource(Field f, DataFlow::Node source) {
57-
source.asInstruction().(FieldAddressInstruction).getField() = f
58-
}
59-
6045
bindingset[delta]
6146
predicate isInvalidPointerDerefSinkImpl(
6247
int delta, Instruction i, AddressOperand addr, string operation
@@ -93,38 +78,90 @@ predicate isInvalidPointerDerefSink2(DataFlow::Node sink, Instruction i, string
9378
)
9479
}
9580

96-
predicate isConstantSizeOverflowSource(Field f, PointerAddInstruction pai, int delta) {
97-
exists(int size, int bound, DataFlow::Node source, DataFlow::InstructionNode sink |
98-
FieldAddressToPointerArithmeticFlow::flow(source, sink) and
99-
isFieldAddressSource(f, source) and
100-
pai.getLeft() = sink.asInstruction() and
101-
f.getUnspecifiedType().(ArrayType).getArraySize() = size and
102-
semBounded(getSemanticExpr(pai.getRight()), any(SemZeroBound b), bound, true, _) and
103-
delta = bound - size and
104-
delta >= 0 and
105-
size != 0 and
106-
size != 1
107-
)
81+
predicate pointerArithOverflow0(
82+
PointerArithmeticInstruction pai, Field f, int size, int bound, int delta
83+
) {
84+
pai.getElementSize() = f.getUnspecifiedType().(ArrayType).getBaseType().getSize() and
85+
f.getUnspecifiedType().(ArrayType).getArraySize() = size and
86+
semBounded(getSemanticExpr(pai.getRight()), any(SemZeroBound b), bound, true, _) and
87+
delta = bound - size and
88+
delta >= 0 and
89+
size != 0 and
90+
size != 1
10891
}
10992

11093
module PointerArithmeticToDerefConfig implements DataFlow::ConfigSig {
11194
predicate isSource(DataFlow::Node source) {
112-
isConstantSizeOverflowSource(_, source.asInstruction(), _)
95+
pointerArithOverflow0(source.asInstruction(), _, _, _, _)
11396
}
11497

115-
pragma[inline]
98+
predicate isBarrierIn(DataFlow::Node node) { isSource(node) }
99+
100+
predicate isBarrierOut(DataFlow::Node node) { isSink(node) }
101+
116102
predicate isSink(DataFlow::Node sink) { isInvalidPointerDerefSink1(sink, _, _) }
117103
}
118104

119105
module PointerArithmeticToDerefFlow = DataFlow::Global<PointerArithmeticToDerefConfig>;
120106

107+
predicate pointerArithOverflow(
108+
PointerArithmeticInstruction pai, Field f, int size, int bound, int delta
109+
) {
110+
pointerArithOverflow0(pai, f, size, bound, delta) and
111+
PointerArithmeticToDerefFlow::flow(DataFlow::instructionNode(pai), _)
112+
}
113+
114+
module FieldAddressToDerefConfig implements DataFlow::StateConfigSig {
115+
newtype FlowState =
116+
additional TArray(Field f) { pointerArithOverflow(_, f, _, _, _) } or
117+
additional TOverflowArithmetic(PointerArithmeticInstruction pai) {
118+
pointerArithOverflow(pai, _, _, _, _)
119+
}
120+
121+
predicate isSource(DataFlow::Node source, FlowState state) {
122+
exists(Field f |
123+
source.asInstruction().(FieldAddressInstruction).getField() = f and
124+
state = TArray(f)
125+
)
126+
}
127+
128+
predicate isSink(DataFlow::Node sink, FlowState state) {
129+
exists(DataFlow::Node pai |
130+
state = TOverflowArithmetic(pai.asInstruction()) and
131+
PointerArithmeticToDerefFlow::flow(pai, sink)
132+
)
133+
}
134+
135+
predicate isBarrier(DataFlow::Node node, FlowState state) { none() }
136+
137+
predicate isBarrierIn(DataFlow::Node node) { isSource(node, _) }
138+
139+
predicate isBarrierOut(DataFlow::Node node) { isSink(node, _) }
140+
141+
predicate isAdditionalFlowStep(
142+
DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2
143+
) {
144+
exists(PointerArithmeticInstruction pai, Field f |
145+
state1 = TArray(f) and
146+
state2 = TOverflowArithmetic(pai) and
147+
pai.getLeft() = node1.asInstruction() and
148+
node2.asInstruction() = pai and
149+
pointerArithOverflow(pai, f, _, _, _)
150+
)
151+
}
152+
}
153+
154+
module FieldAddressToDerefFlow = DataFlow::GlobalWithState<FieldAddressToDerefConfig>;
155+
121156
from
122-
Field f, PointerArithmeticToDerefFlow::PathNode source,
123-
PointerArithmeticToDerefFlow::PathNode sink, Instruction deref, string operation, int delta
157+
Field f, FieldAddressToDerefFlow::PathNode source, PointerArithmeticInstruction pai,
158+
FieldAddressToDerefFlow::PathNode sink, Instruction deref, string operation, int delta
124159
where
125-
PointerArithmeticToDerefFlow::flowPath(source, sink) and
160+
FieldAddressToDerefFlow::flowPath(source, sink) and
126161
isInvalidPointerDerefSink2(sink.getNode(), deref, operation) and
127-
isConstantSizeOverflowSource(f, source.getNode().asInstruction(), delta)
128-
select source, source, sink,
162+
source.getState() = FieldAddressToDerefConfig::TArray(f) and
163+
sink.getState() = FieldAddressToDerefConfig::TOverflowArithmetic(pai) and
164+
pointerArithOverflow(pai, f, _, _, delta)
165+
select pai, source, sink,
129166
"This pointer arithmetic may have an off-by-" + (delta + 1) +
130167
" 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

Lines changed: 31 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,37 +1,46 @@
11
edges
2-
| test.cpp:66:32:66:32 | p | test.cpp:66:32:66:32 | p |
3-
| test.cpp:66:32:66:32 | p | test.cpp:67:5:67:6 | * ... |
4-
| test.cpp:66:32:66:32 | p | test.cpp:67:6:67:6 | p |
2+
| test.cpp:35:10:35:12 | buf | test.cpp:35:5:35:22 | access to array |
3+
| test.cpp:36:10:36:12 | buf | test.cpp:36:5:36:24 | access to array |
4+
| test.cpp:43:14:43:16 | buf | test.cpp:43:9:43:19 | access to array |
5+
| test.cpp:49:10:49:12 | buf | test.cpp:49:5:49:22 | access to array |
6+
| test.cpp:50:10:50:12 | buf | test.cpp:50:5:50:24 | access to array |
7+
| test.cpp:57:14:57:16 | buf | test.cpp:57:9:57:19 | access to array |
8+
| test.cpp:61:14:61:16 | buf | test.cpp:61:9:61:19 | access to array |
9+
| test.cpp:70:33:70:33 | p | test.cpp:72:5:72:15 | access to array |
510
| test.cpp:77:26:77:44 | & ... | test.cpp:66:32:66:32 | p |
6-
| test.cpp:77:26:77:44 | & ... | test.cpp:66:32:66:32 | p |
7-
| test.cpp:77:27:77:44 | access to array | test.cpp:77:26:77:44 | & ... |
11+
| test.cpp:77:32:77:34 | buf | test.cpp:77:26:77:44 | & ... |
12+
| test.cpp:79:27:79:34 | buf | test.cpp:70:33:70:33 | p |
13+
| test.cpp:79:32:79:34 | buf | test.cpp:79:27:79:34 | buf |
814
nodes
915
| test.cpp:35:5:35:22 | access to array | semmle.label | access to array |
16+
| test.cpp:35:10:35:12 | buf | semmle.label | buf |
1017
| test.cpp:36:5:36:24 | access to array | semmle.label | access to array |
18+
| test.cpp:36:10:36:12 | buf | semmle.label | buf |
1119
| test.cpp:43:9:43:19 | access to array | semmle.label | access to array |
20+
| test.cpp:43:14:43:16 | buf | semmle.label | buf |
1221
| test.cpp:49:5:49:22 | access to array | semmle.label | access to array |
22+
| test.cpp:49:10:49:12 | buf | semmle.label | buf |
1323
| test.cpp:50:5:50:24 | access to array | semmle.label | access to array |
24+
| test.cpp:50:10:50:12 | buf | semmle.label | buf |
1425
| test.cpp:57:9:57:19 | access to array | semmle.label | access to array |
26+
| test.cpp:57:14:57:16 | buf | semmle.label | buf |
1527
| test.cpp:61:9:61:19 | access to array | semmle.label | access to array |
28+
| test.cpp:61:14:61:16 | buf | semmle.label | buf |
1629
| test.cpp:66:32:66:32 | p | semmle.label | p |
17-
| test.cpp:66:32:66:32 | p | semmle.label | p |
18-
| test.cpp:66:32:66:32 | p | semmle.label | p |
19-
| test.cpp:67:5:67:6 | * ... | semmle.label | * ... |
20-
| test.cpp:67:6:67:6 | p | semmle.label | p |
30+
| test.cpp:70:33:70:33 | p | semmle.label | p |
2131
| test.cpp:72:5:72:15 | access to array | semmle.label | access to array |
2232
| test.cpp:77:26:77:44 | & ... | semmle.label | & ... |
23-
| test.cpp:77:27:77:44 | access to array | semmle.label | access to array |
33+
| test.cpp:77:32:77:34 | buf | semmle.label | buf |
34+
| test.cpp:79:27:79:34 | buf | semmle.label | buf |
35+
| test.cpp:79:32:79:34 | buf | semmle.label | buf |
2436
subpaths
2537
#select
26-
| 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 |
27-
| 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 |
28-
| 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 |
29-
| 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 |
30-
| 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 |
31-
| 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 |
32-
| 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 |
33-
| 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 |
34-
| 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 |
35-
| 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 |
36-
| 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 |
37-
| 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 |
38+
| 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 |
39+
| test.cpp:36:5:36:24 | PointerAdd: 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 |
40+
| test.cpp:43:9:43:19 | PointerAdd: 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 |
41+
| test.cpp:49:5:49:22 | PointerAdd: 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 |
42+
| test.cpp:50:5:50:24 | PointerAdd: 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 |
43+
| test.cpp:57:9:57:19 | PointerAdd: 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 |
44+
| 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 |
45+
| 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 |
46+
| 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 |

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,15 @@ void testInterproc(BigArray *arr) {
7979
addToPointerAndAssign(arr->buf);
8080
}
8181

82+
#define MAX_SIZE_BYTES 4096
83+
84+
void testCharIndex(BigArray *arr) {
85+
char *charBuf = (char*) arr->buf;
86+
87+
charBuf[MAX_SIZE_BYTES - 1] = 0; // GOOD
88+
charBuf[MAX_SIZE_BYTES] = 0; // BAD [FALSE NEGATIVE]
89+
}
90+
8291
void testEqRefinement() {
8392
int arr[MAX_SIZE];
8493

0 commit comments

Comments
 (0)