Skip to content

Rust: Lift content reads as taint steps #20879

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Rust: Lift content reads as taint steps #20879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7d47a48
Rust: Lift content reads as taint steps
paldepind Nov 20, 2025
7479309
Rust: Remov unneeded model
paldepind Nov 21, 2025
530a898
Rust: Update tests and expected files
paldepind Nov 21, 2025
a8d580f
Rust: Refactor flow summary implementation
paldepind Nov 26, 2025
22d39b4
Rust: Add extensible predicate to exclude fields and block fieldless …
paldepind Nov 26, 2025
5d6e52a
Rust: Exclude range start and end from field taint steps
paldepind Nov 27, 2025
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
33 changes: 17 additions & 16 deletions rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,20 @@ private import codeql.rust.dataflow.FlowSummary
private import codeql.rust.dataflow.Ssa
private import Content

predicate encodeContentTupleField(TupleFieldContent c, string arg) {
exists(Addressable a, int pos, string prefix |
arg = prefix + "(" + pos + ")" and prefix = a.getCanonicalPath()
|
c.isStructField(a, pos) or c.isVariantField(a, pos)
)
}

predicate encodeContentStructField(StructFieldContent c, string arg) {
exists(Addressable a, string field | arg = a.getCanonicalPath() + "::" + field |
c.isStructField(a, field) or c.isVariantField(a, field)
)
}

module Input implements InputSig<Location, RustDataFlow> {
private import codeql.rust.elements.internal.CallExprBaseImpl::Impl as CallExprBaseImpl
private import codeql.rust.frameworks.stdlib.Stdlib
Expand Down Expand Up @@ -61,24 +75,11 @@ module Input implements InputSig<Location, RustDataFlow> {
exists(Content c | cs = TSingletonContentSet(c) |
result = "Field" and
(
exists(Addressable a, int pos, string prefix |
arg = prefix + "(" + pos + ")" and prefix = a.getCanonicalPath()
|
c.(TupleFieldContent).isStructField(a, pos)
or
c.(TupleFieldContent).isVariantField(a, pos)
)
encodeContentTupleField(c, arg)
or
exists(Addressable a, string field | arg = a.getCanonicalPath() + "::" + field |
c.(StructFieldContent).isStructField(a, field)
or
c.(StructFieldContent).isVariantField(a, field)
)
encodeContentStructField(c, arg)
or
exists(int pos |
c = TTuplePositionContent(pos) and
arg = pos.toString()
)
exists(int pos | c = TTuplePositionContent(pos) and arg = pos.toString())
)
or
result = "Reference" and
Expand Down
42 changes: 37 additions & 5 deletions rust/ql/lib/codeql/rust/dataflow/internal/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,23 @@ private import Node as Node
private import Content
private import FlowSummaryImpl as FlowSummaryImpl
private import codeql.rust.internal.CachedStages
private import codeql.rust.internal.TypeInference as TypeInference
private import codeql.rust.internal.Type as Type
private import codeql.rust.frameworks.stdlib.Builtins as Builtins

/**
* Holds if the field `field` should, by default, be excluded from taint steps.
* The syntax used to denote the field is the same as for `Field` in
* models-as-data.
Comment on lines +15 to +17
Copy link
Contributor

@geoffw0 geoffw0 Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be slightly more specific about what we mean here, perhaps something like:

Suggested change
* Holds if the field `field` should, by default, be excluded from taint steps.
* The syntax used to denote the field is the same as for `Field` in
* models-as-data.
* Holds if the field `field` should, by default, be excluded from taint steps
* from the containing type to reads of the field. The models-as-data syntax
* used to denote the field is the same as for `Field[]` access path elements.

*/
extensible predicate excludeFieldTaintStep(string field);

private predicate excludedTaintStepContent(Content c) {
exists(string arg | excludeFieldTaintStep(arg) |
FlowSummaryImpl::encodeContentStructField(c, arg) or
FlowSummaryImpl::encodeContentTupleField(c, arg)
)
}

module RustTaintTracking implements InputSig<Location, RustDataFlow> {
predicate defaultTaintSanitizer(DataFlow::Node node) { none() }
Expand Down Expand Up @@ -40,11 +57,26 @@ module RustTaintTracking implements InputSig<Location, RustDataFlow> {
succ.asExpr() = index
)
or
// Although data flow through collections and references is modeled using
// stores/reads, we also allow taint to flow out of a tainted collection
// or reference.
// This is needed in order to support taint-tracking configurations where
// the source is a collection or reference.
// Read steps give rise to taint steps. This has the effect that if `foo`
// is tainted and an operation reads from `foo` (e.g., `foo.bar`) then
// taint is propagated. We limit this to not apply if the type of the
// operation is a small primitive type as these are often uninteresting
// (for instance in the case of an injection query).
Copy link
Contributor

@geoffw0 geoffw0 Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not completely convinced we should apply this limitation here, rather than adding barriers for these types to all injection queries. I haven't seen the kinds of results you're talking about though, other than in the pseudocode example you gave on this PR discussion. Perhaps we could plan to look into this as follow-up, since doing this would mostly add even more flow to what we're already doing here.

exists(Content c |
RustDataFlow::readContentStep(pred, c, succ) and
forex(Type::Type t | t = TypeInference::inferType(succ.asExpr()) |
not exists(Struct s | s = t.(Type::StructType).getStruct() |
s instanceof Builtins::NumericType or
s instanceof Builtins::Bool or
s instanceof Builtins::Char
)
) and
Copy link
Contributor

@geoffw0 geoffw0 Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The forex means that we will only get a result if type inference succeeds on succ. Is this on purpose?

not excludedTaintStepContent(c) and
not TypeInference::inferType(succ.asExpr()).(Type::EnumType).getEnum().isFieldless()
Copy link
Contributor

@geoffw0 geoffw0 Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could move this line into the forex and reduce it to:

and
not t.(Type::EnumType).getEnum().isFieldless()

)
or
// Let all read steps (including those from flow summaries and those that
// result in small primitive types) give rise to taint steps.
Copy link
Contributor

@geoffw0 geoffw0 Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've lost some context from the original comment here, I think.

exists(SingletonContentSet cs | RustDataFlow::readStep(pred, cs, succ) |
cs.getContent() instanceof ElementContent
or
Expand Down
7 changes: 1 addition & 6 deletions rust/ql/lib/codeql/rust/frameworks/actix-web.model.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,4 @@ extensions:
pack: codeql/rust-all
extensible: summaryModel
data:
- ["<actix_web::types::path::Path>::into_inner", "Argument[self]", "ReturnValue", "taint", "manual"]
- ["<actix_web::types::path::Path>::into_inner", "Argument[self]", "ReturnValue.Field[0]", "taint", "manual"]
- ["<actix_web::types::path::Path>::into_inner", "Argument[self]", "ReturnValue.Field[1]", "taint", "manual"]
- ["<actix_web::types::path::Path>::into_inner", "Argument[self]", "ReturnValue.Field[2]", "taint", "manual"]
- ["<actix_web::types::path::Path>::into_inner", "Argument[self]", "ReturnValue.Field[3]", "taint", "manual"]
- ["<actix_web::types::path::Path>::into_inner", "Argument[self]", "ReturnValue.Field[4]", "taint", "manual"]
- ["<actix_web::types::path::Path>::into_inner", "Argument[self]", "ReturnValue", "taint", "manual"]
6 changes: 6 additions & 0 deletions rust/ql/lib/codeql/rust/frameworks/stdlib/core.model.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,3 +74,9 @@ extensions:
- ["core::ptr::write_bytes", "Argument[0]", "pointer-access", "manual"]
- ["core::ptr::write_unaligned", "Argument[0]", "pointer-access", "manual"]
- ["core::ptr::write_volatile", "Argument[0]", "pointer-access", "manual"]
- addsTo:
pack: codeql/rust-all
extensible: excludeFieldTaintStep
data:
- ["core::ops::range::RangeInclusive::start"]
- ["core::ops::range::RangeInclusive::end"]
28 changes: 28 additions & 0 deletions rust/ql/test/library-tests/dataflow/sources/database/InlineFlow.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ edges
| test.rs:42:20:42:21 | t1 [element] | test.rs:42:13:42:15 | row | provenance | |
| test.rs:48:22:48:30 | query_map | test.rs:50:14:50:24 | ...: i64 | provenance | Src:MaD:3 |
| test.rs:50:14:50:24 | ...: i64 | test.rs:51:22:51:27 | values | provenance | |
| test.rs:55:22:55:30 | query_map | test.rs:57:14:57:39 | ...: ... | provenance | Src:MaD:3 |
| test.rs:57:14:57:39 | ...: ... | test.rs:59:22:59:29 | values.1 | provenance | |
| test.rs:64:13:64:17 | total | test.rs:68:14:68:18 | total | provenance | |
| test.rs:64:21:67:10 | conn.query_fold(...) [Ok] | test.rs:64:21:67:11 | TryExpr | provenance | |
| test.rs:64:21:67:11 | TryExpr | test.rs:64:13:64:17 | total | provenance | |
Expand All @@ -62,6 +64,9 @@ edges
| test.rs:66:19:66:21 | row | test.rs:66:13:66:21 | ... + ... | provenance | MaD:11 |
| test.rs:66:19:66:21 | row | test.rs:66:13:66:21 | ... + ... | provenance | MaD:12 |
| test.rs:66:19:66:21 | row | test.rs:66:13:66:21 | ... + ... | provenance | MaD:15 |
| test.rs:70:22:70:31 | query_fold | test.rs:70:83:70:105 | ...: ... | provenance | Src:MaD:2 |
| test.rs:70:83:70:105 | ...: ... | test.rs:72:17:72:20 | name | provenance | |
| test.rs:72:17:72:20 | name | test.rs:75:18:75:21 | name | provenance | |
| test.rs:105:13:105:14 | v1 | test.rs:106:14:106:15 | v1 | provenance | |
| test.rs:105:24:105:33 | row.get(...) [Some] | test.rs:105:24:105:42 | ... .unwrap() | provenance | MaD:16 |
| test.rs:105:24:105:42 | ... .unwrap() | test.rs:105:13:105:14 | v1 | provenance | |
Expand All @@ -82,6 +87,8 @@ edges
| test.rs:114:28:114:35 | take_opt | test.rs:114:24:114:38 | row.take_opt(...) [Some, Ok] | provenance | Src:MaD:10 |
| test.rs:135:22:135:30 | query_map | test.rs:137:14:137:24 | ...: i64 | provenance | Src:MaD:5 |
| test.rs:137:14:137:24 | ...: i64 | test.rs:138:22:138:27 | values | provenance | |
| test.rs:142:22:142:30 | query_map | test.rs:144:14:144:39 | ...: ... | provenance | Src:MaD:5 |
| test.rs:144:14:144:39 | ...: ... | test.rs:146:22:146:29 | values.1 | provenance | |
| test.rs:151:13:151:17 | total | test.rs:155:14:155:18 | total | provenance | |
| test.rs:151:21:154:10 | conn.query_fold(...) [future, Ok] | test.rs:151:21:154:16 | await ... [Ok] | provenance | |
| test.rs:151:21:154:16 | await ... [Ok] | test.rs:151:21:154:17 | TryExpr | provenance | |
Expand All @@ -95,6 +102,9 @@ edges
| test.rs:153:19:153:21 | row | test.rs:153:13:153:21 | ... + ... | provenance | MaD:11 |
| test.rs:153:19:153:21 | row | test.rs:153:13:153:21 | ... + ... | provenance | MaD:12 |
| test.rs:153:19:153:21 | row | test.rs:153:13:153:21 | ... + ... | provenance | MaD:15 |
| test.rs:157:22:157:31 | query_fold | test.rs:157:83:157:105 | ...: ... | provenance | Src:MaD:4 |
| test.rs:157:83:157:105 | ...: ... | test.rs:159:17:159:20 | name | provenance | |
| test.rs:159:17:159:20 | name | test.rs:162:18:162:21 | name | provenance | |
nodes
| test.rs:18:13:18:14 | v1 | semmle.label | v1 |
| test.rs:18:24:18:33 | row.get(...) [Some] | semmle.label | row.get(...) [Some] |
Expand Down Expand Up @@ -137,6 +147,9 @@ nodes
| test.rs:48:22:48:30 | query_map | semmle.label | query_map |
| test.rs:50:14:50:24 | ...: i64 | semmle.label | ...: i64 |
| test.rs:51:22:51:27 | values | semmle.label | values |
| test.rs:55:22:55:30 | query_map | semmle.label | query_map |
| test.rs:57:14:57:39 | ...: ... | semmle.label | ...: ... |
| test.rs:59:22:59:29 | values.1 | semmle.label | values.1 |
| test.rs:64:13:64:17 | total | semmle.label | total |
| test.rs:64:21:67:10 | conn.query_fold(...) [Ok] | semmle.label | conn.query_fold(...) [Ok] |
| test.rs:64:21:67:11 | TryExpr | semmle.label | TryExpr |
Expand All @@ -147,6 +160,10 @@ nodes
| test.rs:66:13:66:21 | ... + ... | semmle.label | ... + ... |
| test.rs:66:19:66:21 | row | semmle.label | row |
| test.rs:68:14:68:18 | total | semmle.label | total |
| test.rs:70:22:70:31 | query_fold | semmle.label | query_fold |
| test.rs:70:83:70:105 | ...: ... | semmle.label | ...: ... |
| test.rs:72:17:72:20 | name | semmle.label | name |
| test.rs:75:18:75:21 | name | semmle.label | name |
| test.rs:105:13:105:14 | v1 | semmle.label | v1 |
| test.rs:105:24:105:33 | row.get(...) [Some] | semmle.label | row.get(...) [Some] |
| test.rs:105:24:105:42 | ... .unwrap() | semmle.label | ... .unwrap() |
Expand All @@ -172,6 +189,9 @@ nodes
| test.rs:135:22:135:30 | query_map | semmle.label | query_map |
| test.rs:137:14:137:24 | ...: i64 | semmle.label | ...: i64 |
| test.rs:138:22:138:27 | values | semmle.label | values |
| test.rs:142:22:142:30 | query_map | semmle.label | query_map |
| test.rs:144:14:144:39 | ...: ... | semmle.label | ...: ... |
| test.rs:146:22:146:29 | values.1 | semmle.label | values.1 |
| test.rs:151:13:151:17 | total | semmle.label | total |
| test.rs:151:21:154:10 | conn.query_fold(...) [future, Ok] | semmle.label | conn.query_fold(...) [future, Ok] |
| test.rs:151:21:154:16 | await ... [Ok] | semmle.label | await ... [Ok] |
Expand All @@ -183,6 +203,10 @@ nodes
| test.rs:153:13:153:21 | ... + ... | semmle.label | ... + ... |
| test.rs:153:19:153:21 | row | semmle.label | row |
| test.rs:155:14:155:18 | total | semmle.label | total |
| test.rs:157:22:157:31 | query_fold | semmle.label | query_fold |
| test.rs:157:83:157:105 | ...: ... | semmle.label | ...: ... |
| test.rs:159:17:159:20 | name | semmle.label | name |
| test.rs:162:18:162:21 | name | semmle.label | name |
subpaths
testFailures
#select
Expand All @@ -194,12 +218,16 @@ testFailures
| test.rs:41:14:41:70 | ... .unwrap() | test.rs:41:42:41:44 | get | test.rs:41:14:41:70 | ... .unwrap() | $@ | test.rs:41:42:41:44 | get | get |
| test.rs:44:22:44:22 | v | test.rs:40:27:40:35 | exec_iter | test.rs:44:22:44:22 | v | $@ | test.rs:40:27:40:35 | exec_iter | exec_iter |
| test.rs:51:22:51:27 | values | test.rs:48:22:48:30 | query_map | test.rs:51:22:51:27 | values | $@ | test.rs:48:22:48:30 | query_map | query_map |
| test.rs:59:22:59:29 | values.1 | test.rs:55:22:55:30 | query_map | test.rs:59:22:59:29 | values.1 | $@ | test.rs:55:22:55:30 | query_map | query_map |
| test.rs:65:18:65:20 | row | test.rs:64:26:64:35 | query_fold | test.rs:65:18:65:20 | row | $@ | test.rs:64:26:64:35 | query_fold | query_fold |
| test.rs:68:14:68:18 | total | test.rs:64:26:64:35 | query_fold | test.rs:68:14:68:18 | total | $@ | test.rs:64:26:64:35 | query_fold | query_fold |
| test.rs:75:18:75:21 | name | test.rs:70:22:70:31 | query_fold | test.rs:75:18:75:21 | name | $@ | test.rs:70:22:70:31 | query_fold | query_fold |
| test.rs:106:14:106:15 | v1 | test.rs:105:28:105:30 | get | test.rs:106:14:106:15 | v1 | $@ | test.rs:105:28:105:30 | get | get |
| test.rs:109:14:109:15 | v2 | test.rs:108:28:108:34 | get_opt | test.rs:109:14:109:15 | v2 | $@ | test.rs:108:28:108:34 | get_opt | get_opt |
| test.rs:112:14:112:15 | v3 | test.rs:111:28:111:31 | take | test.rs:112:14:112:15 | v3 | $@ | test.rs:111:28:111:31 | take | take |
| test.rs:115:14:115:15 | v4 | test.rs:114:28:114:35 | take_opt | test.rs:115:14:115:15 | v4 | $@ | test.rs:114:28:114:35 | take_opt | take_opt |
| test.rs:138:22:138:27 | values | test.rs:135:22:135:30 | query_map | test.rs:138:22:138:27 | values | $@ | test.rs:135:22:135:30 | query_map | query_map |
| test.rs:146:22:146:29 | values.1 | test.rs:142:22:142:30 | query_map | test.rs:146:22:146:29 | values.1 | $@ | test.rs:142:22:142:30 | query_map | query_map |
| test.rs:152:18:152:20 | row | test.rs:151:26:151:35 | query_fold | test.rs:152:18:152:20 | row | $@ | test.rs:151:26:151:35 | query_fold | query_fold |
| test.rs:155:14:155:18 | total | test.rs:151:26:151:35 | query_fold | test.rs:155:14:155:18 | total | $@ | test.rs:151:26:151:35 | query_fold | query_fold |
| test.rs:162:18:162:21 | name | test.rs:157:22:157:31 | query_fold | test.rs:162:18:162:21 | name | $@ | test.rs:157:22:157:31 | query_fold | query_fold |
8 changes: 4 additions & 4 deletions rust/ql/test/library-tests/dataflow/sources/database/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ mod test_mysql {
"SELECT id, name, age FROM person",
|values: (i64, String, i32)| -> () {
sink(values.0); // $ MISSING: hasTaintFlow
sink(values.1); // $ MISSING: hasTaintFlow
sink(values.1); // $ hasTaintFlow
sink(values.2); // $ MISSING: hasTaintFlow
}
)?;
Expand All @@ -72,7 +72,7 @@ mod test_mysql {
let name: String = row.1;
let age: i32 = row.2;
sink(id); // $ MISSING: hasTaintFlow
sink(name); // $ MISSING: hasTaintFlow
sink(name); // $ hasTaintFlow
sink(age); // $ MISSING: hasTaintFlow
acc + 1
})?;
Expand Down Expand Up @@ -143,7 +143,7 @@ mod test_mysql_async {
"SELECT id, name, age FROM person",
|values: (i64, String, i32)| -> () {
sink(values.0); // $ MISSING: hasTaintFlow
sink(values.1); // $ MISSING: hasTaintFlow
sink(values.1); // $ hasTaintFlow
sink(values.2); // $ MISSING: hasTaintFlow
}
).await?;
Expand All @@ -159,7 +159,7 @@ mod test_mysql_async {
let name: String = row.1;
let age: i32 = row.2;
sink(id); // $ MISSING: hasTaintFlow
sink(name); // $ MISSING: hasTaintFlow
sink(name); // $ hasTaintFlow
sink(age); // $ MISSING: hasTaintFlow
acc + 1
}).await?;
Expand Down
Loading
Loading