Skip to content

Commit b5c1024

Browse files
MathiasVPMathiasVP
authored
Merge pull request #13041 from MathiasVP/overrun-write-only-one-alert
C++: Only one alert message per alert on `cpp/overrun-write`
2 parents e996eae + cad0244 commit b5c1024

File tree

3 files changed

+59
-26
lines changed

3 files changed

+59
-26
lines changed

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

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -134,16 +134,34 @@ module StringSizeConfig implements ProductFlow::StateConfigSig {
134134

135135
module StringSizeFlow = ProductFlow::GlobalWithState<StringSizeConfig>;
136136

137+
int getOverflow(
138+
DataFlow::Node source1, DataFlow::Node source2, DataFlow::Node sink1, DataFlow::Node sink2,
139+
CallInstruction c, Expr buffer
140+
) {
141+
result > 0 and
142+
exists(
143+
StringSizeFlow::PathNode1 pathSource1, StringSizeFlow::PathNode2 pathSource2,
144+
StringSizeFlow::PathNode1 pathSink1, StringSizeFlow::PathNode2 pathSink2
145+
|
146+
StringSizeFlow::flowPath(pathSource1, pathSource2, pathSink1, pathSink2) and
147+
source1 = pathSource1.getNode() and
148+
source2 = pathSource2.getNode() and
149+
sink1 = pathSink1.getNode() and
150+
sink2 = pathSink2.getNode() and
151+
isSinkPairImpl(c, sink1, sink2, result + pathSink2.getState(), buffer)
152+
)
153+
}
154+
137155
from
138156
StringSizeFlow::PathNode1 source1, StringSizeFlow::PathNode2 source2,
139-
StringSizeFlow::PathNode1 sink1, StringSizeFlow::PathNode2 sink2, int overflow, int sinkState,
140-
CallInstruction c, DataFlow::Node sourceNode, Expr buffer, string element
157+
StringSizeFlow::PathNode1 sink1, StringSizeFlow::PathNode2 sink2, int overflow, CallInstruction c,
158+
Expr buffer, string element
141159
where
142160
StringSizeFlow::flowPath(source1, source2, sink1, sink2) and
143-
sinkState = sink2.getState() and
144-
isSinkPairImpl(c, sink1.getNode(), sink2.getNode(), overflow + sinkState, buffer) and
145-
overflow > 0 and
146-
sourceNode = source1.getNode() and
161+
overflow =
162+
max(getOverflow(source1.getNode(), source2.getNode(), sink1.getNode(), sink2.getNode(), c,
163+
buffer)
164+
) and
147165
if overflow = 1 then element = " element." else element = " elements."
148166
select c.getUnconvertedResultExpression(), source1, sink1,
149167
"This write may overflow $@ by " + overflow + element, buffer, buffer.toString()

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

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -212,15 +212,16 @@ edges
212212
| test.cpp:214:24:214:24 | p | test.cpp:216:10:216:10 | p |
213213
| test.cpp:220:43:220:48 | call to malloc | test.cpp:222:15:222:20 | buffer |
214214
| test.cpp:222:15:222:20 | buffer | test.cpp:214:24:214:24 | p |
215-
| test.cpp:225:40:225:45 | buffer | test.cpp:226:5:226:26 | ... = ... |
216-
| test.cpp:226:5:226:26 | ... = ... | test.cpp:226:12:226:17 | p_str indirection [post update] [string] |
217-
| test.cpp:231:27:231:32 | call to malloc | test.cpp:232:22:232:27 | buffer |
218-
| test.cpp:232:16:232:19 | set_string output argument [string] | test.cpp:233:12:233:14 | str indirection [string] |
219-
| test.cpp:232:22:232:27 | buffer | test.cpp:225:40:225:45 | buffer |
220-
| test.cpp:232:22:232:27 | buffer | test.cpp:232:16:232:19 | set_string output argument [string] |
221-
| test.cpp:233:12:233:14 | str indirection [string] | test.cpp:233:12:233:21 | string |
222-
| test.cpp:233:12:233:14 | str indirection [string] | test.cpp:233:16:233:21 | string indirection |
223-
| test.cpp:233:16:233:21 | string indirection | test.cpp:233:12:233:21 | string |
215+
| test.cpp:228:43:228:48 | call to malloc | test.cpp:232:10:232:15 | buffer |
216+
| test.cpp:235:40:235:45 | buffer | test.cpp:236:5:236:26 | ... = ... |
217+
| test.cpp:236:5:236:26 | ... = ... | test.cpp:236:12:236:17 | p_str indirection [post update] [string] |
218+
| test.cpp:241:27:241:32 | call to malloc | test.cpp:242:22:242:27 | buffer |
219+
| test.cpp:242:16:242:19 | set_string output argument [string] | test.cpp:243:12:243:14 | str indirection [string] |
220+
| test.cpp:242:22:242:27 | buffer | test.cpp:235:40:235:45 | buffer |
221+
| test.cpp:242:22:242:27 | buffer | test.cpp:242:16:242:19 | set_string output argument [string] |
222+
| test.cpp:243:12:243:14 | str indirection [string] | test.cpp:243:12:243:21 | string |
223+
| test.cpp:243:12:243:14 | str indirection [string] | test.cpp:243:16:243:21 | string indirection |
224+
| test.cpp:243:16:243:21 | string indirection | test.cpp:243:12:243:21 | string |
224225
nodes
225226
| test.cpp:16:11:16:21 | mk_string_t indirection [string] | semmle.label | mk_string_t indirection [string] |
226227
| test.cpp:18:5:18:30 | ... = ... | semmle.label | ... = ... |
@@ -390,17 +391,19 @@ nodes
390391
| test.cpp:216:10:216:10 | p | semmle.label | p |
391392
| test.cpp:220:43:220:48 | call to malloc | semmle.label | call to malloc |
392393
| test.cpp:222:15:222:20 | buffer | semmle.label | buffer |
393-
| test.cpp:225:40:225:45 | buffer | semmle.label | buffer |
394-
| test.cpp:226:5:226:26 | ... = ... | semmle.label | ... = ... |
395-
| test.cpp:226:12:226:17 | p_str indirection [post update] [string] | semmle.label | p_str indirection [post update] [string] |
396-
| test.cpp:231:27:231:32 | call to malloc | semmle.label | call to malloc |
397-
| test.cpp:232:16:232:19 | set_string output argument [string] | semmle.label | set_string output argument [string] |
398-
| test.cpp:232:22:232:27 | buffer | semmle.label | buffer |
399-
| test.cpp:233:12:233:14 | str indirection [string] | semmle.label | str indirection [string] |
400-
| test.cpp:233:12:233:21 | string | semmle.label | string |
401-
| test.cpp:233:16:233:21 | string indirection | semmle.label | string indirection |
394+
| test.cpp:228:43:228:48 | call to malloc | semmle.label | call to malloc |
395+
| test.cpp:232:10:232:15 | buffer | semmle.label | buffer |
396+
| test.cpp:235:40:235:45 | buffer | semmle.label | buffer |
397+
| test.cpp:236:5:236:26 | ... = ... | semmle.label | ... = ... |
398+
| test.cpp:236:12:236:17 | p_str indirection [post update] [string] | semmle.label | p_str indirection [post update] [string] |
399+
| test.cpp:241:27:241:32 | call to malloc | semmle.label | call to malloc |
400+
| test.cpp:242:16:242:19 | set_string output argument [string] | semmle.label | set_string output argument [string] |
401+
| test.cpp:242:22:242:27 | buffer | semmle.label | buffer |
402+
| test.cpp:243:12:243:14 | str indirection [string] | semmle.label | str indirection [string] |
403+
| test.cpp:243:12:243:21 | string | semmle.label | string |
404+
| test.cpp:243:16:243:21 | string indirection | semmle.label | string indirection |
402405
subpaths
403-
| test.cpp:232:22:232:27 | buffer | test.cpp:225:40:225:45 | buffer | test.cpp:226:12:226:17 | p_str indirection [post update] [string] | test.cpp:232:16:232:19 | set_string output argument [string] |
406+
| test.cpp:242:22:242:27 | buffer | test.cpp:235:40:235:45 | buffer | test.cpp:236:12:236:17 | p_str indirection [post update] [string] | test.cpp:242:16:242:19 | set_string output argument [string] |
404407
#select
405408
| test.cpp:42:5:42:11 | call to strncpy | test.cpp:18:19:18:24 | call to malloc | test.cpp:42:18:42:23 | string | This write may overflow $@ by 1 element. | test.cpp:42:18:42:23 | string | string |
406409
| test.cpp:72:9:72:15 | call to strncpy | test.cpp:18:19:18:24 | call to malloc | test.cpp:72:22:72:27 | string | This write may overflow $@ by 1 element. | test.cpp:72:22:72:27 | string | string |
@@ -417,4 +420,5 @@ subpaths
417420
| test.cpp:199:9:199:15 | call to strncpy | test.cpp:147:19:147:24 | call to malloc | test.cpp:199:22:199:27 | string | This write may overflow $@ by 2 elements. | test.cpp:199:22:199:27 | string | string |
418421
| test.cpp:203:9:203:15 | call to strncpy | test.cpp:147:19:147:24 | call to malloc | test.cpp:203:22:203:27 | string | This write may overflow $@ by 2 elements. | test.cpp:203:22:203:27 | string | string |
419422
| test.cpp:207:9:207:15 | call to strncpy | test.cpp:147:19:147:24 | call to malloc | test.cpp:207:22:207:27 | string | This write may overflow $@ by 3 elements. | test.cpp:207:22:207:27 | string | string |
420-
| test.cpp:233:5:233:10 | call to memset | test.cpp:231:27:231:32 | call to malloc | test.cpp:233:12:233:21 | string | This write may overflow $@ by 1 element. | test.cpp:233:16:233:21 | string | string |
423+
| test.cpp:232:3:232:8 | call to memset | test.cpp:228:43:228:48 | call to malloc | test.cpp:232:10:232:15 | buffer | This write may overflow $@ by 32 elements. | test.cpp:232:10:232:15 | buffer | buffer |
424+
| test.cpp:243:5:243:10 | call to memset | test.cpp:241:27:241:32 | call to malloc | test.cpp:243:12:243:21 | string | This write may overflow $@ by 1 element. | test.cpp:243:16:243:21 | string | string |

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,16 @@ void test_missing_call_context(unsigned char *unrelated_buffer, unsigned size) {
222222
call_memset(buffer, size);
223223
}
224224

225+
bool unknown();
226+
227+
void repeated_alerts(unsigned size, unsigned offset) {
228+
unsigned char* buffer = (unsigned char*)malloc(size);
229+
while(unknown()) {
230+
++size;
231+
}
232+
memset(buffer, 0, size); // BAD
233+
}
234+
225235
void set_string(string_t* p_str, char* buffer) {
226236
p_str->string = buffer;
227237
}
@@ -232,3 +242,4 @@ void test_flow_through_setter(unsigned size) {
232242
set_string(&str, buffer);
233243
memset(str.string, 0, size + 1); // BAD
234244
}
245+

0 commit comments

Comments
 (0)