Skip to content

Commit 769ff5c

Browse files
MathiasVPMathiasVP
committed
C++: Add 'isAdditionalFlowStep' predicates for both configurations in the product dataflow library and use them to fix missing results in the 'cpp/overrun-write' query.
1 parent ccbbb57 commit 769ff5c

File tree

4 files changed

+94
-17
lines changed

4 files changed

+94
-17
lines changed

‎cpp/ql/lib/experimental/semmle/code/cpp/dataflow/ProductFlow.qll

Lines changed: 73 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,8 @@ module ProductFlow {
2020
* `source1` and `source2` must belong to the same callable.
2121
*/
2222
predicate isSourcePair(
23-
DataFlow::Node source1, string state1, DataFlow::Node source2, string state2
23+
DataFlow::Node source1, DataFlow::FlowState state1, DataFlow::Node source2,
24+
DataFlow::FlowState state2
2425
) {
2526
state1 = "" and
2627
state2 = "" and
@@ -89,6 +90,49 @@ module ProductFlow {
8990
*/
9091
predicate isBarrierOut2(DataFlow::Node node) { none() }
9192

93+
/*
94+
* Holds if data may flow from `node1` to `node2` in addition to the normal data-flow steps in
95+
* the first projection of the product dataflow graph.
96+
*/
97+
98+
predicate isAdditionalFlowStep1(DataFlow::Node node1, DataFlow::Node node2) { none() }
99+
100+
/**
101+
* Holds if data may flow from `node1` to `node2` in addition to the normal data-flow steps in
102+
* the first projection of the product dataflow graph.
103+
*
104+
* This step is only applicable in `state1` and updates the flow state to `state2`.
105+
*/
106+
predicate isAdditionalFlowStep1(
107+
DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
108+
DataFlow::FlowState state2
109+
) {
110+
state1 instanceof DataFlow::FlowStateEmpty and
111+
state2 instanceof DataFlow::FlowStateEmpty and
112+
this.isAdditionalFlowStep1(node1, node2)
113+
}
114+
115+
/**
116+
* Holds if data may flow from `node1` to `node2` in addition to the normal data-flow steps in
117+
* the second projection of the product dataflow graph.
118+
*/
119+
predicate isAdditionalFlowStep2(DataFlow::Node node1, DataFlow::Node node2) { none() }
120+
121+
/**
122+
* Holds if data may flow from `node1` to `node2` in addition to the normal data-flow steps in
123+
* the second projection of the product dataflow graph.
124+
*
125+
* This step is only applicable in `state1` and updates the flow state to `state2`.
126+
*/
127+
predicate isAdditionalFlowStep2(
128+
DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
129+
DataFlow::FlowState state2
130+
) {
131+
state1 instanceof DataFlow::FlowStateEmpty and
132+
state2 instanceof DataFlow::FlowStateEmpty and
133+
this.isAdditionalFlowStep2(node1, node2)
134+
}
135+
92136
predicate hasFlowPath(
93137
DataFlow::PathNode source1, DataFlow2::PathNode source2, DataFlow::PathNode sink1,
94138
DataFlow2::PathNode sink2
@@ -103,54 +147,69 @@ module ProductFlow {
103147
class Conf1 extends DataFlow::Configuration {
104148
Conf1() { this = "Conf1" }
105149

106-
override predicate isSource(DataFlow::Node source, string state) {
150+
override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
107151
exists(Configuration conf | conf.isSourcePair(source, state, _, _))
108152
}
109153

110-
override predicate isSink(DataFlow::Node sink, string state) {
154+
override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
111155
exists(Configuration conf | conf.isSinkPair(sink, state, _, _))
112156
}
113157

114-
override predicate isBarrier(DataFlow::Node node, string state) {
158+
override predicate isBarrier(DataFlow::Node node, DataFlow::FlowState state) {
115159
exists(Configuration conf | conf.isBarrier1(node, state))
116160
}
117161

118162
override predicate isBarrierOut(DataFlow::Node node) {
119163
exists(Configuration conf | conf.isBarrierOut1(node))
120164
}
165+
166+
override predicate isAdditionalFlowStep(
167+
DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
168+
DataFlow::FlowState state2
169+
) {
170+
exists(Configuration conf | conf.isAdditionalFlowStep1(node1, state1, node2, state2))
171+
}
121172
}
122173

123174
class Conf2 extends DataFlow2::Configuration {
124175
Conf2() { this = "Conf2" }
125176

126-
override predicate isSource(DataFlow::Node source, string state) {
127-
exists(Configuration conf, DataFlow::Node source1 |
128-
conf.isSourcePair(source1, _, source, state) and
129-
any(Conf1 c).hasFlow(source1, _)
177+
override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
178+
exists(Configuration conf, DataFlow::PathNode source1 |
179+
conf.isSourcePair(source1.getNode(), source1.getState(), source, state) and
180+
any(Conf1 c).hasFlowPath(source1, _)
130181
)
131182
}
132183

133-
override predicate isSink(DataFlow::Node sink, string state) {
134-
exists(Configuration conf, DataFlow::Node sink1 |
135-
conf.isSinkPair(sink1, _, sink, state) and any(Conf1 c).hasFlow(_, sink1)
184+
override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
185+
exists(Configuration conf, DataFlow::PathNode sink1 |
186+
conf.isSinkPair(sink1.getNode(), sink1.getState(), sink, state) and
187+
any(Conf1 c).hasFlowPath(_, sink1)
136188
)
137189
}
138190

139-
override predicate isBarrier(DataFlow::Node node, string state) {
191+
override predicate isBarrier(DataFlow::Node node, DataFlow::FlowState state) {
140192
exists(Configuration conf | conf.isBarrier2(node, state))
141193
}
142194

143195
override predicate isBarrierOut(DataFlow::Node node) {
144196
exists(Configuration conf | conf.isBarrierOut2(node))
145197
}
198+
199+
override predicate isAdditionalFlowStep(
200+
DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
201+
DataFlow::FlowState state2
202+
) {
203+
exists(Configuration conf | conf.isAdditionalFlowStep2(node1, state1, node2, state2))
204+
}
146205
}
147206
}
148207

149208
private predicate reachableInterprocEntry(
150209
Configuration conf, DataFlow::PathNode source1, DataFlow2::PathNode source2,
151210
DataFlow::PathNode node1, DataFlow2::PathNode node2
152211
) {
153-
conf.isSourcePair(node1.getNode(), _, node2.getNode(), _) and
212+
conf.isSourcePair(node1.getNode(), node1.getState(), node2.getNode(), node2.getState()) and
154213
node1 = source1 and
155214
node2 = source2
156215
or
@@ -213,7 +272,7 @@ module ProductFlow {
213272
) {
214273
exists(DataFlow::PathNode mid1, DataFlow2::PathNode mid2 |
215274
reachableInterprocEntry(conf, source1, source2, mid1, mid2) and
216-
conf.isSinkPair(sink1.getNode(), _, sink2.getNode(), _) and
275+
conf.isSinkPair(sink1.getNode(), sink1.getState(), sink2.getNode(), sink2.getState()) and
217276
localPathStep1*(mid1, sink1) and
218277
localPathStep2*(mid2, sink2)
219278
)

‎cpp/ql/src/experimental/Likely Bugs/OverrunWriteProductFlow.ql

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,21 @@ class StringSizeConfiguration extends ProductFlow::Configuration {
123123
delta > state2.toInt()
124124
)
125125
}
126+
127+
override predicate isAdditionalFlowStep2(
128+
DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
129+
DataFlow::FlowState state2
130+
) {
131+
exists(AddInstruction add, Operand op, int delta, int s1, int s2 |
132+
s1 = [0 .. 32] and // An arbitrary bound because we need to bound `state`
133+
state1 = s1.toString() and
134+
state2 = s2.toString() and
135+
add.hasOperands(node1.asOperand(), op) and
136+
semBounded(op.getDef(), any(SemZeroBound zero), delta, true, _) and
137+
node2.asInstruction() = add and
138+
s1 = s2 + delta
139+
)
140+
}
126141
}
127142

128143
from

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-119/OverrunWriteProductFlow.expected

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,3 +95,6 @@ subpaths
9595
| test.cpp:42:5:42:11 | call to strncpy | test.cpp:18:19:18:24 | call to malloc | test.cpp:42:18:42:23 | Load | This write may overflow $@ by 1 element. | test.cpp:42:18:42:23 | string | string |
9696
| test.cpp:72:9:72:15 | call to strncpy | test.cpp:18:19:18:24 | call to malloc | test.cpp:72:22:72:27 | Load | This write may overflow $@ by 1 element. | test.cpp:72:22:72:27 | string | string |
9797
| test.cpp:80:9:80:15 | call to strncpy | test.cpp:18:19:18:24 | call to malloc | test.cpp:80:22:80:27 | Load | This write may overflow $@ by 2 elements. | test.cpp:80:22:80:27 | string | string |
98+
| test.cpp:99:5:99:11 | call to strncpy | test.cpp:90:19:90:24 | call to malloc | test.cpp:99:18:99:23 | Load | This write may overflow $@ by 1 element. | test.cpp:99:18:99:23 | string | string |
99+
| test.cpp:129:9:129:15 | call to strncpy | test.cpp:90:19:90:24 | call to malloc | test.cpp:129:22:129:27 | Load | This write may overflow $@ by 1 element. | test.cpp:129:22:129:27 | string | string |
100+
| test.cpp:137:9:137:15 | call to strncpy | test.cpp:90:19:90:24 | call to malloc | test.cpp:137:22:137:27 | Load | This write may overflow $@ by 2 elements. | test.cpp:137:22:137:27 | string | string |

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-119/test.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ void test4(unsigned size, char *buf, unsigned anotherSize) {
9696
string_t *str = mk_string_t_plus_one(size);
9797

9898
strncpy(str->string, buf, str->size); // GOOD
99-
strncpy(str->string, buf, str->size + 1); // BAD [NOT DETECTED]
99+
strncpy(str->string, buf, str->size + 1); // BAD
100100

101101
strncpy(str->string, buf, size); // GOOD
102102
strncpy(str->string, buf, size + 1); // GOOD
@@ -126,15 +126,15 @@ void test4(unsigned size, char *buf, unsigned anotherSize) {
126126
}
127127

128128
if(anotherSize <= str->size + 1) {
129-
strncpy(str->string, buf, anotherSize); // BAD [NOT DETECTED]
129+
strncpy(str->string, buf, anotherSize); // BAD
130130
}
131131

132132
if(anotherSize <= size + 1) {
133133
strncpy(str->string, buf, anotherSize); // GOOD
134134
}
135135

136136
if(anotherSize <= str->size + 2) {
137-
strncpy(str->string, buf, anotherSize); // BAD [NOT DETECTED]
137+
strncpy(str->string, buf, anotherSize); // BAD
138138
}
139139

140140
if(anotherSize <= size + 2) {

0 commit comments

Comments
 (0)