Skip to content

Commit 0d6fdc6

Browse files
MathiasVPMathiasVP
committed
C++: Also account for setter-related flow and jump steps.
1 parent 7fa6894 commit 0d6fdc6

File tree

3 files changed

+81
-16
lines changed

3 files changed

+81
-16
lines changed

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

Lines changed: 49 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import semmle.code.cpp.ir.dataflow.DataFlow
22
private import semmle.code.cpp.ir.dataflow.internal.DataFlowPrivate
33
private import semmle.code.cpp.ir.dataflow.internal.DataFlowUtil
4+
private import semmle.code.cpp.ir.dataflow.internal.DataFlowImplCommon
45
private import codeql.util.Unit
56

67
module ProductFlow {
@@ -363,7 +364,40 @@ module ProductFlow {
363364
TOutOf(DataFlowCall call) {
364365
[any(Flow1::PathNode n).getNode(), any(Flow2::PathNode n).getNode()].(OutNode).getCall() =
365366
call
366-
}
367+
} or
368+
TJump()
369+
370+
private predicate into1(Flow1::PathNode pred1, Flow1::PathNode succ1, TKind kind) {
371+
exists(DataFlowCall call |
372+
kind = TInto(call) and
373+
pred1.getNode().(ArgumentNode).getCall() = call and
374+
succ1.getNode() instanceof ParameterNode
375+
)
376+
}
377+
378+
private predicate out1(Flow1::PathNode pred1, Flow1::PathNode succ1, TKind kind) {
379+
exists(ReturnKindExt returnKind, DataFlowCall call |
380+
kind = TOutOf(call) and
381+
succ1.getNode() = returnKind.getAnOutNode(call) and
382+
pred1.getNode().(ReturnNodeExt).getKind() = returnKind
383+
)
384+
}
385+
386+
private predicate into2(Flow2::PathNode pred1, Flow2::PathNode succ1, TKind kind) {
387+
exists(DataFlowCall call |
388+
kind = TInto(call) and
389+
pred1.getNode().(ArgumentNode).getCall() = call and
390+
succ1.getNode() instanceof ParameterNode
391+
)
392+
}
393+
394+
private predicate out2(Flow2::PathNode pred1, Flow2::PathNode succ1, TKind kind) {
395+
exists(ReturnKindExt returnKind, DataFlowCall call |
396+
kind = TOutOf(call) and
397+
succ1.getNode() = returnKind.getAnOutNode(call) and
398+
pred1.getNode().(ReturnNodeExt).getKind() = returnKind
399+
)
400+
}
367401

368402
pragma[nomagic]
369403
private predicate interprocEdge1(
@@ -374,14 +408,14 @@ module ProductFlow {
374408
predDecl != succDecl and
375409
pred1.getNode().getEnclosingCallable() = predDecl and
376410
succ1.getNode().getEnclosingCallable() = succDecl and
377-
exists(DataFlowCall call |
378-
kind = TInto(call) and
379-
pred1.getNode().(ArgumentNode).getCall() = call and
380-
succ1.getNode() instanceof ParameterNode
411+
(
412+
into1(pred1, succ1, kind)
381413
or
382-
kind = TOutOf(call) and
383-
succ1.getNode().(OutNode).getCall() = call and
384-
pred1.getNode() instanceof ReturnNode
414+
out1(pred1, succ1, kind)
415+
or
416+
kind = TJump() and
417+
not into1(pred1, succ1, _) and
418+
not out1(pred1, succ1, _)
385419
)
386420
}
387421

@@ -394,14 +428,14 @@ module ProductFlow {
394428
predDecl != succDecl and
395429
pred2.getNode().getEnclosingCallable() = predDecl and
396430
succ2.getNode().getEnclosingCallable() = succDecl and
397-
exists(DataFlowCall call |
398-
kind = TInto(call) and
399-
pred2.getNode().(ArgumentNode).getCall() = call and
400-
succ2.getNode() instanceof ParameterNode
431+
(
432+
into2(pred2, succ2, kind)
401433
or
402-
kind = TOutOf(call) and
403-
succ2.getNode().(OutNode).getCall() = call and
404-
pred2.getNode() instanceof ReturnNode
434+
out2(pred2, succ2, kind)
435+
or
436+
kind = TJump() and
437+
not into2(pred2, succ2, _) and
438+
not out2(pred2, succ2, _)
405439
)
406440
}
407441

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,15 @@ 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 |
215224
nodes
216225
| test.cpp:16:11:16:21 | mk_string_t indirection [string] | semmle.label | mk_string_t indirection [string] |
217226
| test.cpp:18:5:18:30 | ... = ... | semmle.label | ... = ... |
@@ -381,7 +390,17 @@ nodes
381390
| test.cpp:216:10:216:10 | p | semmle.label | p |
382391
| test.cpp:220:43:220:48 | call to malloc | semmle.label | call to malloc |
383392
| 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 |
384402
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] |
385404
#select
386405
| 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 |
387406
| 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 |
@@ -398,3 +417,4 @@ subpaths
398417
| 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 |
399418
| 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 |
400419
| 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 |

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,4 +220,15 @@ void test_missing_call_context(unsigned char *unrelated_buffer, unsigned size) {
220220
unsigned char* buffer = (unsigned char*)malloc(size);
221221
call_memset(unrelated_buffer, size + 5);
222222
call_memset(buffer, size);
223-
}
223+
}
224+
225+
void set_string(string_t* p_str, char* buffer) {
226+
p_str->string = buffer;
227+
}
228+
229+
void test_flow_through_setter(unsigned size) {
230+
string_t str;
231+
char* buffer = (char*)malloc(size);
232+
set_string(&str, buffer);
233+
memset(str.string, 0, size + 1); // BAD
234+
}

0 commit comments

Comments
 (0)