Rust: Add taint model for add on String#20559
Conversation
| let s1 = source_slice(22); | ||
| let s2 = s1.to_string(); | ||
| sink(s2); // $ MISSING: hasTaintFlow=22 - we are not currently able to resolve the `to_string` call above, which comes from `impl<T: fmt::Display + ?Sized> ToString for T` | ||
| sink(s2); // $ MISSING: hasTaintFlow=22 |
There was a problem hiding this comment.
The explanation here was outdated. We actually resolve the call now, but we can't model the method in a blanket implementation.
There was a problem hiding this comment.
Pull Request Overview
This PR adds a taint model for the add operation on String types in Rust. It enables taint tracking to flow through string concatenation operations where tainted data is added to strings.
- Adds taint flow models for
<alloc::string::String as core::ops::arith::Add>::add - Updates test expectations to reflect new taint tracking capabilities for string concatenation
- Fixes previously missed taint flows in SQL injection and other security tests
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
rust/ql/lib/codeql/rust/frameworks/stdlib/lang-alloc.model.yml |
Adds taint models for String::add operation |
rust/ql/test/query-tests/security/CWE-825/AccessAfterLifetime.expected |
Updates test expectations with new taint model references |
rust/ql/test/query-tests/security/CWE-312/CleartextStorageDatabase.expected |
Updates test expectations with additional taint flows detected |
rust/ql/test/query-tests/security/CWE-312/CleartextLogging.expected |
Updates test expectations with new string concatenation taint flows |
rust/ql/test/query-tests/security/CWE-089/sqlx.rs |
Updates comments from "MISSING" to found alerts for SQL injection cases |
rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected |
Updates test expectations with many new SQL injection flows now detected |
rust/ql/test/library-tests/dataflow/strings/main.rs |
Updates comment to reflect fixed taint flow case |
rust/ql/test/library-tests/dataflow/strings/inline-taint-flow.expected |
Updates test expectations with new taint flows through string concatenation |
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
Looks good, great results in tests, waiting for DCA...
|
DCA looks fine to me. There's a ❌ for stage timings, but it doesn't look like an issue to me. |
| - ["<alloc::string::String as core::ops::arith::Add>::add", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<alloc::string::String as core::ops::arith::Add>::add", "Argument[0].Reference", "ReturnValue", "taint", "manual"] |
There was a problem hiding this comment.
Should we instead lift this to <_ as core::ops::arith::Add>::add?
There was a problem hiding this comment.
If we do this as follow-up, we can include other similar traits (e.g. Sub) as well.
There was a problem hiding this comment.
I've created an issue for this.
3 participants
No description provided.