Skip to content

C++: Only one alert message per alert on cpp/overrun-write #13041

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
MathiasVP merged 4 commits into from
May 9, 2023
Merged

C++: Only one alert message per alert on cpp/overrun-write #13041

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a09a8db
C++: Add testcase with repeated TP alerts.
MathiasVP May 4, 2023
2587f8e
C++: Only alert on the largest possible overflow.
MathiasVP May 4, 2023
de08ada
C++: Accept test changes.
MathiasVP May 4, 2023
cad0244
Merge branch 'main' into overrun-write-only-one-alert
MathiasVP May 5, 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
30 changes: 24 additions & 6 deletions cpp/ql/src/experimental/Likely Bugs/OverrunWriteProductFlow.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,16 +134,34 @@ module StringSizeConfig implements ProductFlow::StateConfigSig {

module StringSizeFlow = ProductFlow::GlobalWithState<StringSizeConfig>;

int getOverflow(
DataFlow::Node source1, DataFlow::Node source2, DataFlow::Node sink1, DataFlow::Node sink2,
Comment thread
jketema marked this conversation as resolved.
CallInstruction c, Expr buffer
) {
result > 0 and
exists(
StringSizeFlow::PathNode1 pathSource1, StringSizeFlow::PathNode2 pathSource2,
StringSizeFlow::PathNode1 pathSink1, StringSizeFlow::PathNode2 pathSink2
|
StringSizeFlow::flowPath(pathSource1, pathSource2, pathSink1, pathSink2) and
source1 = pathSource1.getNode() and
source2 = pathSource2.getNode() and
sink1 = pathSink1.getNode() and
sink2 = pathSink2.getNode() and
isSinkPairImpl(c, sink1, sink2, result + pathSink2.getState(), buffer)
)
}

from
StringSizeFlow::PathNode1 source1, StringSizeFlow::PathNode2 source2,
StringSizeFlow::PathNode1 sink1, StringSizeFlow::PathNode2 sink2, int overflow, int sinkState,
CallInstruction c, DataFlow::Node sourceNode, Expr buffer, string element
StringSizeFlow::PathNode1 sink1, StringSizeFlow::PathNode2 sink2, int overflow, CallInstruction c,
Expr buffer, string element
where
StringSizeFlow::flowPath(source1, source2, sink1, sink2) and
sinkState = sink2.getState() and
isSinkPairImpl(c, sink1.getNode(), sink2.getNode(), overflow + sinkState, buffer) and
overflow > 0 and
sourceNode = source1.getNode() and
overflow =
max(getOverflow(source1.getNode(), source2.getNode(), sink1.getNode(), sink2.getNode(), c,
buffer)
) and
if overflow = 1 then element = " element." else element = " elements."
select c.getUnconvertedResultExpression(), source1, sink1,
"This write may overflow $@ by " + overflow + element, buffer, buffer.toString()
44 changes: 24 additions & 20 deletions cpp/ql/test/experimental/query-tests/Security/CWE/CWE-119/OverrunWriteProductFlow.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,15 +212,16 @@ edges
| test.cpp:214:24:214:24 | p | test.cpp:216:10:216:10 | p |
| test.cpp:220:43:220:48 | call to malloc | test.cpp:222:15:222:20 | buffer |
| test.cpp:222:15:222:20 | buffer | test.cpp:214:24:214:24 | p |
| test.cpp:225:40:225:45 | buffer | test.cpp:226:5:226:26 | ... = ... |
| test.cpp:226:5:226:26 | ... = ... | test.cpp:226:12:226:17 | p_str indirection [post update] [string] |
| test.cpp:231:27:231:32 | call to malloc | test.cpp:232:22:232:27 | buffer |
| test.cpp:232:16:232:19 | set_string output argument [string] | test.cpp:233:12:233:14 | str indirection [string] |
| test.cpp:232:22:232:27 | buffer | test.cpp:225:40:225:45 | buffer |
| test.cpp:232:22:232:27 | buffer | test.cpp:232:16:232:19 | set_string output argument [string] |
| test.cpp:233:12:233:14 | str indirection [string] | test.cpp:233:12:233:21 | string |
| test.cpp:233:12:233:14 | str indirection [string] | test.cpp:233:16:233:21 | string indirection |
| test.cpp:233:16:233:21 | string indirection | test.cpp:233:12:233:21 | string |
| test.cpp:228:43:228:48 | call to malloc | test.cpp:232:10:232:15 | buffer |
| test.cpp:235:40:235:45 | buffer | test.cpp:236:5:236:26 | ... = ... |
| test.cpp:236:5:236:26 | ... = ... | test.cpp:236:12:236:17 | p_str indirection [post update] [string] |
| test.cpp:241:27:241:32 | call to malloc | test.cpp:242:22:242:27 | buffer |
| test.cpp:242:16:242:19 | set_string output argument [string] | test.cpp:243:12:243:14 | str indirection [string] |
| test.cpp:242:22:242:27 | buffer | test.cpp:235:40:235:45 | buffer |
| test.cpp:242:22:242:27 | buffer | test.cpp:242:16:242:19 | set_string output argument [string] |
| test.cpp:243:12:243:14 | str indirection [string] | test.cpp:243:12:243:21 | string |
| test.cpp:243:12:243:14 | str indirection [string] | test.cpp:243:16:243:21 | string indirection |
| test.cpp:243:16:243:21 | string indirection | test.cpp:243:12:243:21 | string |
nodes
| test.cpp:16:11:16:21 | mk_string_t indirection [string] | semmle.label | mk_string_t indirection [string] |
| test.cpp:18:5:18:30 | ... = ... | semmle.label | ... = ... |
Expand Down Expand Up @@ -390,17 +391,19 @@ nodes
| test.cpp:216:10:216:10 | p | semmle.label | p |
| test.cpp:220:43:220:48 | call to malloc | semmle.label | call to malloc |
| test.cpp:222:15:222:20 | buffer | semmle.label | buffer |
| test.cpp:225:40:225:45 | buffer | semmle.label | buffer |
| test.cpp:226:5:226:26 | ... = ... | semmle.label | ... = ... |
| test.cpp:226:12:226:17 | p_str indirection [post update] [string] | semmle.label | p_str indirection [post update] [string] |
| test.cpp:231:27:231:32 | call to malloc | semmle.label | call to malloc |
| test.cpp:232:16:232:19 | set_string output argument [string] | semmle.label | set_string output argument [string] |
| test.cpp:232:22:232:27 | buffer | semmle.label | buffer |
| test.cpp:233:12:233:14 | str indirection [string] | semmle.label | str indirection [string] |
| test.cpp:233:12:233:21 | string | semmle.label | string |
| test.cpp:233:16:233:21 | string indirection | semmle.label | string indirection |
| test.cpp:228:43:228:48 | call to malloc | semmle.label | call to malloc |
| test.cpp:232:10:232:15 | buffer | semmle.label | buffer |
| test.cpp:235:40:235:45 | buffer | semmle.label | buffer |
| test.cpp:236:5:236:26 | ... = ... | semmle.label | ... = ... |
| test.cpp:236:12:236:17 | p_str indirection [post update] [string] | semmle.label | p_str indirection [post update] [string] |
| test.cpp:241:27:241:32 | call to malloc | semmle.label | call to malloc |
| test.cpp:242:16:242:19 | set_string output argument [string] | semmle.label | set_string output argument [string] |
| test.cpp:242:22:242:27 | buffer | semmle.label | buffer |
| test.cpp:243:12:243:14 | str indirection [string] | semmle.label | str indirection [string] |
| test.cpp:243:12:243:21 | string | semmle.label | string |
| test.cpp:243:16:243:21 | string indirection | semmle.label | string indirection |
subpaths
| 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] |
| 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] |
#select
| 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 |
| 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 |
Expand All @@ -417,4 +420,5 @@ subpaths
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
11 changes: 11 additions & 0 deletions cpp/ql/test/experimental/query-tests/Security/CWE/CWE-119/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,16 @@ void test_missing_call_context(unsigned char *unrelated_buffer, unsigned size) {
call_memset(buffer, size);
}

bool unknown();

void repeated_alerts(unsigned size, unsigned offset) {
unsigned char* buffer = (unsigned char*)malloc(size);
while(unknown()) {
++size;
}
memset(buffer, 0, size); // BAD
}

void set_string(string_t* p_str, char* buffer) {
p_str->string = buffer;
}
Expand All @@ -232,3 +242,4 @@ void test_flow_through_setter(unsigned size) {
set_string(&str, buffer);
memset(str.string, 0, size + 1); // BAD
}