Skip to content

Commit 6b5d6bd

Browse files
paldepindpaldepind
committed
Shared: Pass SummaryComponentStack to isSource and getSourceType
1 parent 44472c3 commit 6b5d6bd

File tree

9 files changed

+660
-493
lines changed

9 files changed

+660
-493
lines changed

‎rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -508,7 +508,8 @@ module RustDataFlow implements InputSig<Location> {
508508
*/
509509
predicate jumpStep(Node node1, Node node2) {
510510
FlowSummaryImpl::Private::Steps::summaryJumpStep(node1.(FlowSummaryNode).getSummaryNode(),
511-
node2.(FlowSummaryNode).getSummaryNode())
511+
node2.(FlowSummaryNode).getSummaryNode()) or
512+
FlowSummaryImpl::Private::Steps::sourceJumpStep(node1.(FlowSummaryNode).getSummaryNode(), node2)
512513
}
513514

514515
pragma[nomagic]

‎rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll‎

Lines changed: 38 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,10 @@ private import rust
66
private import codeql.dataflow.internal.FlowSummaryImpl
77
private import codeql.dataflow.internal.AccessPathSyntax as AccessPath
88
private import codeql.rust.dataflow.internal.DataFlowImpl
9+
private import codeql.rust.internal.PathResolution
910
private import codeql.rust.dataflow.FlowSummary
11+
private import codeql.rust.dataflow.Ssa
12+
private import codeql.rust.controlflow.CfgNodes
1013
private import Content
1114

1215
module Input implements InputSig<Location, RustDataFlow> {
@@ -133,16 +136,44 @@ private module StepsInput implements Impl::Private::StepsInputSig {
133136
result.asCallCfgNode().getCall().getStaticTarget() = sc
134137
}
135138

136-
RustDataFlow::Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponent sc) {
137-
sc = Impl::Private::SummaryComponent::return(_) and
139+
/** Gets the argument of `source` described by `sc`, if any. */
140+
private Expr getSourceNodeArgument(Input::SourceBase source, Impl::Private::SummaryComponent sc) {
141+
exists(ArgumentPosition pos |
142+
sc = Impl::Private::SummaryComponent::argument(pos) and
143+
result = pos.getArgument(source.getCall())
144+
)
145+
}
146+
147+
/** Get the callable that `expr` refers to. */
148+
private Callable getCallable(Expr expr) {
149+
result = resolvePath(expr.(PathExpr).getPath()).(Function)
150+
or
151+
result = expr.(ClosureExpr)
152+
or
153+
// The expression is an SSA read of an assignment of a closure
154+
exists(Ssa::Definition def, ExprCfgNode value |
155+
def.getARead().getAstNode() = expr and
156+
def.getAnUltimateDefinition().(Ssa::WriteDefinition).assigns(value) and
157+
result = value.getExpr().(ClosureExpr)
158+
)
159+
}
160+
161+
RustDataFlow::DataFlowCallable getSourceNodeEnclosingCallable(Input::SourceBase source) {
162+
result.asCfgScope() = source.getEnclosingCfgScope()
163+
}
164+
165+
RustDataFlow::Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponentStack s) {
166+
s.head() = Impl::Private::SummaryComponent::return(_) and
138167
result.asExpr().getExpr() = source.getCall()
139168
or
140-
exists(CallExprBase call, Expr arg, ArgumentPosition pos |
141-
result.(RustDataFlow::PostUpdateNode).getPreUpdateNode().asExpr().getExpr() = arg and
142-
sc = Impl::Private::SummaryComponent::argument(pos) and
143-
call = source.getCall() and
144-
arg = pos.getArgument(call)
169+
exists(ArgumentPosition pos, Expr arg |
170+
s.head() = Impl::Private::SummaryComponent::parameter(pos) and
171+
arg = getSourceNodeArgument(source, s.tail().head()) and
172+
result.asParameter() = getCallable(arg).getParam(pos.getPosition())
145173
)
174+
or
175+
result.(RustDataFlow::PostUpdateNode).getPreUpdateNode().asExpr().getExpr() =
176+
getSourceNodeArgument(source, s.head())
146177
}
147178

148179
RustDataFlow::Node getSinkNode(Input::SinkBase sink, Impl::Private::SummaryComponent sc) {

‎rust/ql/test/library-tests/dataflow/models/main.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -258,20 +258,20 @@ mod source_into_function {
258258
}
259259

260260
fn test_source_into_function() {
261-
let a = |a| sink(a); // $ MISSING: hasValueFlow=1
261+
let a = |a| sink(a); // $ hasValueFlow=1
262262
pass_source(1, a);
263263

264264
pass_source(2, |a| {
265-
sink(a); // $ MISSING: hasValueFlow=2
265+
sink(a); // $ hasValueFlow=2
266266
});
267267

268268
fn f(a: i64) {
269-
sink(a) // $ MISSING: hasValueFlow=3
269+
sink(a) // $ hasValueFlow=3
270270
}
271271
pass_source(3, f);
272272

273273
pass_source(4, async move |a| {
274-
sink(a); // $ MISSING: hasValueFlow=4
274+
sink(a); // $ hasValueFlow=4
275275
});
276276
}
277277
}

‎rust/ql/test/library-tests/dataflow/models/models.expected‎

Lines changed: 106 additions & 57 deletions
Large diffs are not rendered by default.

‎rust/ql/test/library-tests/dataflow/sources/InlineFlow.expected‎

Lines changed: 448 additions & 397 deletions
Large diffs are not rendered by default.

‎rust/ql/test/library-tests/dataflow/sources/web_frameworks.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -241,15 +241,15 @@ mod warp_test {
241241
let map_route =
242242
warp::path::param().map(|a: String| // $ Alert[rust/summary/taint-sources]
243243
{
244-
sink(a); // $ MISSING: hasTaintFlow
244+
sink(a); // $ hasTaintFlow
245245

246246
"".to_string()
247247
});
248248

249249
// A route with parameter and `then`
250250
let then_route = warp::path::param().then( // $ Alert[rust/summary/taint-sources]
251251
async move |a: String| {
252-
sink(a); // $ MISSING: hasTaintFlow
252+
sink(a); // $ hasTaintFlow
253253

254254
"".to_string()
255255
},
@@ -260,7 +260,7 @@ mod warp_test {
260260
async move | id: u64 |
261261
{
262262
if id != 0 {
263-
sink(id); // $ MISSING: hasTaintFlow
263+
sink(id); // $ hasTaintFlow
264264
Ok("".to_string())
265265
} else {
266266
Err(warp::reject::not_found())
@@ -272,7 +272,7 @@ mod warp_test {
272272
let path_and_map_route = warp::path("1").and(warp::path::param()).map( // $ Alert[rust/summary/taint-sources]
273273
| a: String |
274274
{
275-
sink(a); // $ MISSING: hasTaintFlow
275+
sink(a); // $ hasTaintFlow
276276

277277
"".to_string()
278278
},

‎rust/ql/test/query-tests/security/CWE-918/RequestForgery.expected‎

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88
| request_forgery_tests.rs:31:29:31:40 | ...::get | request_forgery_tests.rs:5:29:5:36 | user_url | request_forgery_tests.rs:31:29:31:40 | ...::get | The URL of this request depends on a $@. | request_forgery_tests.rs:5:29:5:36 | user_url | user-provided value |
99
| request_forgery_tests.rs:37:37:37:48 | ...::get | request_forgery_tests.rs:5:29:5:36 | user_url | request_forgery_tests.rs:37:37:37:48 | ...::get | The URL of this request depends on a $@. | request_forgery_tests.rs:5:29:5:36 | user_url | user-provided value |
1010
| request_forgery_tests.rs:37:37:37:48 | ...::get | request_forgery_tests.rs:5:29:5:36 | user_url | request_forgery_tests.rs:37:37:37:48 | ...::get | The URL of this request depends on a $@. | request_forgery_tests.rs:5:29:5:36 | user_url | user-provided value |
11+
| request_forgery_tests.rs:68:28:68:39 | ...::get | request_forgery_tests.rs:65:33:65:40 | and_then | request_forgery_tests.rs:68:28:68:39 | ...::get | The URL of this request depends on a $@. | request_forgery_tests.rs:65:33:65:40 | and_then | user-provided value |
12+
| request_forgery_tests.rs:68:28:68:39 | ...::get | request_forgery_tests.rs:65:33:65:40 | and_then | request_forgery_tests.rs:68:28:68:39 | ...::get | The URL of this request depends on a $@. | request_forgery_tests.rs:65:33:65:40 | and_then | user-provided value |
1113
edges
1214
| request_forgery_tests.rs:4:5:4:14 | res | request_forgery_tests.rs:16:27:16:49 | { ... } | provenance | |
1315
| request_forgery_tests.rs:4:5:4:14 | res | request_forgery_tests.rs:20:27:20:57 | { ... } | provenance | |
@@ -28,22 +30,22 @@ edges
2830
| request_forgery_tests.rs:16:13:16:15 | url | request_forgery_tests.rs:17:39:17:41 | url | provenance | |
2931
| request_forgery_tests.rs:16:27:16:49 | ...::format(...) | request_forgery_tests.rs:4:5:4:14 | res | provenance | |
3032
| request_forgery_tests.rs:16:27:16:49 | ...::must_use(...) | request_forgery_tests.rs:16:13:16:15 | url | provenance | |
31-
| request_forgery_tests.rs:16:27:16:49 | MacroExpr | request_forgery_tests.rs:16:27:16:49 | ...::format(...) | provenance | MaD:2 |
32-
| request_forgery_tests.rs:16:27:16:49 | { ... } | request_forgery_tests.rs:16:27:16:49 | ...::must_use(...) | provenance | MaD:3 |
33+
| request_forgery_tests.rs:16:27:16:49 | MacroExpr | request_forgery_tests.rs:16:27:16:49 | ...::format(...) | provenance | MaD:3 |
34+
| request_forgery_tests.rs:16:27:16:49 | { ... } | request_forgery_tests.rs:16:27:16:49 | ...::must_use(...) | provenance | MaD:4 |
3335
| request_forgery_tests.rs:17:38:17:41 | &url [&ref] | request_forgery_tests.rs:17:25:17:36 | ...::get | provenance | MaD:1 Sink:MaD:1 |
3436
| request_forgery_tests.rs:17:39:17:41 | url | request_forgery_tests.rs:17:38:17:41 | &url [&ref] | provenance | |
3537
| request_forgery_tests.rs:20:13:20:15 | url | request_forgery_tests.rs:21:39:21:41 | url | provenance | |
3638
| request_forgery_tests.rs:20:27:20:57 | ...::format(...) | request_forgery_tests.rs:4:5:4:14 | res | provenance | |
3739
| request_forgery_tests.rs:20:27:20:57 | ...::must_use(...) | request_forgery_tests.rs:20:13:20:15 | url | provenance | |
38-
| request_forgery_tests.rs:20:27:20:57 | MacroExpr | request_forgery_tests.rs:20:27:20:57 | ...::format(...) | provenance | MaD:2 |
39-
| request_forgery_tests.rs:20:27:20:57 | { ... } | request_forgery_tests.rs:20:27:20:57 | ...::must_use(...) | provenance | MaD:3 |
40+
| request_forgery_tests.rs:20:27:20:57 | MacroExpr | request_forgery_tests.rs:20:27:20:57 | ...::format(...) | provenance | MaD:3 |
41+
| request_forgery_tests.rs:20:27:20:57 | { ... } | request_forgery_tests.rs:20:27:20:57 | ...::must_use(...) | provenance | MaD:4 |
4042
| request_forgery_tests.rs:21:38:21:41 | &url [&ref] | request_forgery_tests.rs:21:25:21:36 | ...::get | provenance | MaD:1 Sink:MaD:1 |
4143
| request_forgery_tests.rs:21:39:21:41 | url | request_forgery_tests.rs:21:38:21:41 | &url [&ref] | provenance | |
4244
| request_forgery_tests.rs:24:13:24:15 | url | request_forgery_tests.rs:25:39:25:41 | url | provenance | |
4345
| request_forgery_tests.rs:24:27:24:70 | ...::format(...) | request_forgery_tests.rs:4:5:4:14 | res | provenance | |
4446
| request_forgery_tests.rs:24:27:24:70 | ...::must_use(...) | request_forgery_tests.rs:24:13:24:15 | url | provenance | |
45-
| request_forgery_tests.rs:24:27:24:70 | MacroExpr | request_forgery_tests.rs:24:27:24:70 | ...::format(...) | provenance | MaD:2 |
46-
| request_forgery_tests.rs:24:27:24:70 | { ... } | request_forgery_tests.rs:24:27:24:70 | ...::must_use(...) | provenance | MaD:3 |
47+
| request_forgery_tests.rs:24:27:24:70 | MacroExpr | request_forgery_tests.rs:24:27:24:70 | ...::format(...) | provenance | MaD:3 |
48+
| request_forgery_tests.rs:24:27:24:70 | { ... } | request_forgery_tests.rs:24:27:24:70 | ...::must_use(...) | provenance | MaD:4 |
4749
| request_forgery_tests.rs:25:38:25:41 | &url [&ref] | request_forgery_tests.rs:25:25:25:36 | ...::get | provenance | MaD:1 Sink:MaD:1 |
4850
| request_forgery_tests.rs:25:39:25:41 | url | request_forgery_tests.rs:25:38:25:41 | &url [&ref] | provenance | |
4951
| request_forgery_tests.rs:31:42:31:50 | &user_url [&ref] | request_forgery_tests.rs:31:29:31:40 | ...::get | provenance | MaD:1 Sink:MaD:1 |
@@ -54,10 +56,19 @@ edges
5456
| request_forgery_tests.rs:37:50:37:58 | &user_url [&ref] | request_forgery_tests.rs:37:37:37:48 | ...::get | provenance | MaD:1 Sink:MaD:1 |
5557
| request_forgery_tests.rs:37:51:37:58 | user_url | request_forgery_tests.rs:37:50:37:58 | &user_url [&ref] | provenance | |
5658
| request_forgery_tests.rs:37:51:37:58 | user_url | request_forgery_tests.rs:37:50:37:58 | &user_url [&ref] | provenance | |
59+
| request_forgery_tests.rs:65:33:65:40 | and_then | request_forgery_tests.rs:65:49:65:57 | ...: String | provenance | Src:MaD:2 |
60+
| request_forgery_tests.rs:65:33:65:40 | and_then | request_forgery_tests.rs:65:49:65:57 | ...: String | provenance | Src:MaD:2 |
61+
| request_forgery_tests.rs:65:49:65:57 | ...: String | request_forgery_tests.rs:68:42:68:42 | a | provenance | |
62+
| request_forgery_tests.rs:65:49:65:57 | ...: String | request_forgery_tests.rs:68:42:68:42 | a | provenance | |
63+
| request_forgery_tests.rs:68:41:68:42 | &a [&ref] | request_forgery_tests.rs:68:28:68:39 | ...::get | provenance | MaD:1 Sink:MaD:1 |
64+
| request_forgery_tests.rs:68:41:68:42 | &a [&ref] | request_forgery_tests.rs:68:28:68:39 | ...::get | provenance | MaD:1 Sink:MaD:1 |
65+
| request_forgery_tests.rs:68:42:68:42 | a | request_forgery_tests.rs:68:41:68:42 | &a [&ref] | provenance | |
66+
| request_forgery_tests.rs:68:42:68:42 | a | request_forgery_tests.rs:68:41:68:42 | &a [&ref] | provenance | |
5767
models
5868
| 1 | Sink: reqwest::get; Argument[0]; request-url |
59-
| 2 | Summary: alloc::fmt::format; Argument[0]; ReturnValue; taint |
60-
| 3 | Summary: core::hint::must_use; Argument[0]; ReturnValue; value |
69+
| 2 | Source: <_ as warp::filter::Filter>::and_then; Argument[0].Parameter[0..7]; remote |
70+
| 3 | Summary: alloc::fmt::format; Argument[0]; ReturnValue; taint |
71+
| 4 | Summary: core::hint::must_use; Argument[0]; ReturnValue; value |
6172
nodes
6273
| request_forgery_tests.rs:4:5:4:14 | res | semmle.label | res |
6374
| request_forgery_tests.rs:4:5:4:14 | res | semmle.label | res |
@@ -106,4 +117,14 @@ nodes
106117
| request_forgery_tests.rs:37:50:37:58 | &user_url [&ref] | semmle.label | &user_url [&ref] |
107118
| request_forgery_tests.rs:37:51:37:58 | user_url | semmle.label | user_url |
108119
| request_forgery_tests.rs:37:51:37:58 | user_url | semmle.label | user_url |
120+
| request_forgery_tests.rs:65:33:65:40 | and_then | semmle.label | and_then |
121+
| request_forgery_tests.rs:65:33:65:40 | and_then | semmle.label | and_then |
122+
| request_forgery_tests.rs:65:49:65:57 | ...: String | semmle.label | ...: String |
123+
| request_forgery_tests.rs:65:49:65:57 | ...: String | semmle.label | ...: String |
124+
| request_forgery_tests.rs:68:28:68:39 | ...::get | semmle.label | ...::get |
125+
| request_forgery_tests.rs:68:28:68:39 | ...::get | semmle.label | ...::get |
126+
| request_forgery_tests.rs:68:41:68:42 | &a [&ref] | semmle.label | &a [&ref] |
127+
| request_forgery_tests.rs:68:41:68:42 | &a [&ref] | semmle.label | &a [&ref] |
128+
| request_forgery_tests.rs:68:42:68:42 | a | semmle.label | a |
129+
| request_forgery_tests.rs:68:42:68:42 | a | semmle.label | a |
109130
subpaths

‎rust/ql/test/query-tests/security/CWE-918/request_forgery_tests.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,10 +62,10 @@ mod warp_test {
6262
async fn test_warp() {
6363
// A route with parameter and `and_then`
6464
let map_route =
65-
warp::path::param().and_then(async |a: String| // $ MISSING: Source=a
65+
warp::path::param().and_then(async |a: String| // $ Source=a
6666
{
6767

68-
let response = reqwest::get(&a).await; // $ MISSING: Alert[rust/request-forgery]=a
68+
let response = reqwest::get(&a).await; // $ Alert[rust/request-forgery]=a
6969
match response {
7070
Ok(resp) => Ok(resp.text().await.unwrap_or_default()),
7171
Err(_err) => Err(warp::reject::not_found()),

‎shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll‎

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1206,12 +1206,12 @@ module Make<
12061206
* Holds if this node is an exit node, i.e. after all stores have been performed.
12071207
*
12081208
* A local flow step should be added from this node to a data flow node representing
1209-
* `sc` inside `source`.
1209+
* `s` inside `source`.
12101210
*/
1211-
predicate isExit(SourceElement source, SummaryComponent sc, string model) {
1211+
predicate isExit(SourceElement source, SummaryComponentStack s, string model) {
12121212
source = source_ and
12131213
model = model_ and
1214-
state_.isSourceOutputState(source, TSingletonSummaryComponentStack(sc), _, model)
1214+
state_.isSourceOutputState(source, s, _, model)
12151215
}
12161216

12171217
override predicate isHidden() { not this.isEntry(_, _) }
@@ -1460,7 +1460,7 @@ module Make<
14601460

14611461
DataFlowType getSyntheticGlobalType(SyntheticGlobal sg);
14621462

1463-
DataFlowType getSourceType(SourceBase source, SummaryComponent sc);
1463+
DataFlowType getSourceType(SourceBase source, SummaryComponentStack sc);
14641464

14651465
DataFlowType getSinkType(SinkBase sink, SummaryComponent sc);
14661466
}
@@ -1543,9 +1543,9 @@ module Make<
15431543
)
15441544
or
15451545
exists(SourceElement source |
1546-
exists(SummaryComponent sc |
1547-
n.(SourceOutputNode).isExit(source, sc, _) and
1548-
result = getSourceType(source, sc)
1546+
exists(SummaryComponentStack s |
1547+
n.(SourceOutputNode).isExit(source, s, _) and
1548+
result = getSourceType(source, s)
15491549
)
15501550
or
15511551
exists(SummaryComponentStack s, ContentSet cont |
@@ -1574,13 +1574,16 @@ module Make<
15741574
/** Gets a call that targets summarized callable `sc`. */
15751575
DataFlowCall getACall(SummarizedCallable sc);
15761576

1577+
/** Gets the enclosing callable of `source`. */
1578+
DataFlowCallable getSourceNodeEnclosingCallable(SourceBase source);
1579+
15771580
/**
1578-
* Gets a data flow node corresponding to the `sc` part of `source`.
1581+
* Gets a data flow node corresponding to the `s` part of `source`.
15791582
*
1580-
* `sc` is typically `ReturnValue` and the result is the node that
1583+
* `s` is typically `ReturnValue` and the result is the node that
15811584
* represents the return value of `source`.
15821585
*/
1583-
Node getSourceNode(SourceBase source, SummaryComponent sc);
1586+
Node getSourceNode(SourceBase source, SummaryComponentStack s);
15841587

15851588
/**
15861589
* Gets a data flow node corresponding to the `sc` part of `sink`.
@@ -1622,13 +1625,20 @@ module Make<
16221625
)
16231626
}
16241627

1625-
predicate sourceLocalStep(SourceOutputNode nodeFrom, Node nodeTo, string model) {
1626-
exists(SummaryComponent sc, SourceElement source |
1628+
predicate sourceStep(SourceOutputNode nodeFrom, Node nodeTo, string model, boolean local) {
1629+
exists(SummaryComponentStack sc, SourceElement source |
16271630
nodeFrom.isExit(source, sc, model) and
1628-
nodeTo = StepsInput::getSourceNode(source, sc)
1631+
nodeTo = StepsInput::getSourceNode(source, sc) and
1632+
if StepsInput::getSourceNodeEnclosingCallable(source) = getNodeEnclosingCallable(nodeTo)
1633+
then local = true
1634+
else local = false
16291635
)
16301636
}
16311637

1638+
predicate sourceLocalStep(SourceOutputNode nodeFrom, Node nodeTo, string model) {
1639+
sourceStep(nodeFrom, nodeTo, model, true)
1640+
}
1641+
16321642
predicate sinkLocalStep(Node nodeFrom, SinkInputNode nodeTo, string model) {
16331643
exists(SummaryComponent sc, SinkElement sink |
16341644
nodeFrom = StepsInput::getSinkNode(sink, sc) and
@@ -1689,6 +1699,10 @@ module Make<
16891699
)
16901700
}
16911701

1702+
predicate sourceJumpStep(SourceOutputNode nodeFrom, Node nodeTo) {
1703+
sourceStep(nodeFrom, nodeTo, _, false)
1704+
}
1705+
16921706
/**
16931707
* Holds if values stored inside content `c` are cleared at `n`. `n` is a
16941708
* synthesized summary node, so in order for values to be cleared at calls

0 commit comments

Comments
 (0)