-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
base: main
Are you sure you want to change the base?
Conversation
fbf1477 to
b5453a5
Compare
b5453a5 to
5635afc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances Rust taint tracking by lifting content reads as taint steps, enabling automatic taint propagation when reading from tainted values (e.g., foo.bar inherits taint from foo). The implementation filters out small primitive types (numerics, booleans, characters) to avoid spurious results in injection queries.
Key changes:
- Added logic to propagate taint through
readContentStepoperations with type-based filtering - Simplified actix-web models by removing redundant field-specific taint summaries
- Updated test expectations to reflect newly detected taint flows
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
rust/ql/lib/codeql/rust/dataflow/internal/TaintTrackingImpl.qll |
Implements taint propagation through read operations with primitive type filtering |
rust/ql/lib/codeql/rust/frameworks/actix-web.model.yml |
Removes redundant field-specific taint models that are now handled automatically |
rust/ql/test/library-tests/dataflow/sources/web_frameworks/test.rs |
Updates test annotations to reflect newly detected taint flows in web framework handlers |
rust/ql/test/library-tests/dataflow/sources/web_frameworks/InlineFlow.expected |
Updates expected flow analysis results with new taint propagation edges |
rust/ql/test/library-tests/dataflow/sources/net/test.rs |
Updates network test annotations for newly detected flows |
rust/ql/test/library-tests/dataflow/sources/net/InlineFlow.expected |
Updates expected network flow results |
rust/ql/test/library-tests/dataflow/sources/file/InlineFlow.expected |
Updates expected file I/O flow results |
rust/ql/test/library-tests/dataflow/sources/env/test.rs |
Updates environment variable test annotations |
rust/ql/test/library-tests/dataflow/sources/env/InlineFlow.expected |
Updates expected environment flow results |
rust/ql/test/library-tests/dataflow/sources/database/test.rs |
Updates database test annotations |
rust/ql/test/library-tests/dataflow/sources/database/InlineFlow.expected |
Updates expected database flow results |
rust/ql/test/query-tests/security/CWE-825/AccessAfterLifetime.expected |
Updates lifetime analysis test expectations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
hvitved
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance doesn't look good on rust.
Let read steps give rise to taint steps. This has the effect that if
foois tainted and an operation reads fromfoo(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).
This PR lifts
readContentStepinstead ofreadStep. The latter subsumes the former and additionally includes reads from flow summaries. Doing the type based restriction for these wasn't completely trivial and including them without such a restriction caused spurious results in some of the tests. If anyone has an idea on how to do the type restriction for those, then we can do that in follow up work.