Rust: Model futures-io, rustls, futures-rustls #19626
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…g (reference). We shouldn't need these.
There was a problem hiding this comment.
Pull Request Overview
This PR adds CodeQL models and tests to improve taint-tracking for asynchronous I/O in Rust, covering futures-io, rustls, and futures-rustls. It also fixes test sink recognition in the web_frameworks suite.
- Introduce
test_futures_io.rsandtest_rustlsintest.rsto exerciseAsyncRead/AsyncBufReadpatterns over TLS streams. - Add dependency entries for
rustls,futures-rustls, andasync-stdinoptions.yml. - Provide new model YAMLs (
rustls.model.yml,futures.model.yml,async-rs.model.yml) to define taint sources and summaries for the relevant crates.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/ql/test/library-tests/dataflow/sources/web_frameworks.rs | Update expected sink annotations from $ MISSING to $ hasTaintFlow. |
| rust/ql/test/library-tests/dataflow/sources/test_futures_io.rs | New tests covering AsyncRead/AsyncBufRead over TLS streams. |
| rust/ql/test/library-tests/dataflow/sources/test.rs | Add test_rustls function and invoke it from main. |
| rust/ql/test/library-tests/dataflow/sources/options.yml | Add rustls, futures-rustls, and async-std dependencies. |
| rust/ql/test/library-tests/dataflow/sources/TaintSources.expected | Update expected taint alerts for new connection and args sources. |
| rust/ql/test/library-tests/dataflow/sources/InlineFlow.ql | Broaden sink matching predicate to %::sink. |
| rust/ql/lib/codeql/rust/frameworks/rustls.model.yml | Add source/summary model entries for rustls and futures-rustls. |
| rust/ql/lib/codeql/rust/frameworks/futures.model.yml | Add summary model entries for futures-util I/O and streams. |
| rust/ql/lib/codeql/rust/frameworks/async-rs.model.yml | Add source model entry for async-std TCP connect. |
| extensible: summaryModel | ||
| data: | ||
| - ["repo:https://github.com/quininer/futures-rustls:futures-rustls", "<crate::TlsConnector>::connect", "Argument[1]", "ReturnValue.Future.Field[crate::result::Result::Ok(0)]", "taint", "manual"] | ||
| - ["repo:https://github.com/quininer/futures-rustls:futures-rustls", "<crate::client::TlsStream as crate::if_std::AsyncRead>::poll_read", "Argument[self].Reference", "Argument[1].Reference", "taint", "manual"] |
There was a problem hiding this comment.
Add a summaryModel entry for <crate::client::TlsStream as crate::if_std::AsyncBufRead>::poll_fill_buf so that taint is correctly propagated through buffered reads, mirroring the existing poll_read rule.
Suggested change
| - ["repo:https://github.com/quininer/futures-rustls:futures-rustls", "<crate::client::TlsStream as crate::if_std::AsyncRead>::poll_read", "Argument[self].Reference", "Argument[1].Reference", "taint", "manual"] | |
| - ["repo:https://github.com/quininer/futures-rustls:futures-rustls", "<crate::client::TlsStream as crate::if_std::AsyncRead>::poll_read", "Argument[self].Reference", "Argument[1].Reference", "taint", "manual"] | |
| - ["repo:https://github.com/quininer/futures-rustls:futures-rustls", "<crate::client::TlsStream as crate::if_std::AsyncBufRead>::poll_fill_buf", "Argument[self].Reference", "ReturnValue.Reference", "taint", "manual"] |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Nice try, but it doesn't help. Hopefully when we have support for MaD trait models, we can clean this stuff up.
Comment on lines
+8
to
+15
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncReadExt::read", "Argument[self]", "Argument[0].Reference", "taint", "manual"] | ||
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncReadExt::read", "Argument[self].Reference", "Argument[0].Reference", "taint", "manual"] | ||
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncReadExt::read_to_end", "Argument[self]", "Argument[0].Reference", "taint", "manual"] | ||
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncReadExt::read_to_end", "Argument[self].Reference", "Argument[0].Reference", "taint", "manual"] | ||
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncBufReadExt::read_line", "Argument[self]", "Argument[0].Reference", "taint", "manual"] | ||
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncBufReadExt::read_line", "Argument[self].Reference", "Argument[0].Reference", "taint", "manual"] | ||
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncBufReadExt::read_until", "Argument[self]", "Argument[1].Reference", "taint", "manual"] | ||
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncBufReadExt::read_until", "Argument[self].Reference", "Argument[1].Reference", "taint", "manual"] |
There was a problem hiding this comment.
[nitpick] These two read rules only differ by .Reference. Consider consolidating into a single pattern (e.g., match Argument[self]) to reduce duplication and maintenance overhead.
Suggested change
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncReadExt::read", "Argument[self]", "Argument[0].Reference", "taint", "manual"] | |
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncReadExt::read", "Argument[self].Reference", "Argument[0].Reference", "taint", "manual"] | |
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncReadExt::read_to_end", "Argument[self]", "Argument[0].Reference", "taint", "manual"] | |
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncReadExt::read_to_end", "Argument[self].Reference", "Argument[0].Reference", "taint", "manual"] | |
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncBufReadExt::read_line", "Argument[self]", "Argument[0].Reference", "taint", "manual"] | |
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncBufReadExt::read_line", "Argument[self].Reference", "Argument[0].Reference", "taint", "manual"] | |
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncBufReadExt::read_until", "Argument[self]", "Argument[1].Reference", "taint", "manual"] | |
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncBufReadExt::read_until", "Argument[self].Reference", "Argument[1].Reference", "taint", "manual"] | |
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncReadExt::read", "Argument[self].*", "Argument[0].Reference", "taint", "manual"] | |
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncReadExt::read_to_end", "Argument[self].*", "Argument[0].Reference", "taint", "manual"] | |
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncBufReadExt::read_line", "Argument[self].*", "Argument[0].Reference", "taint", "manual"] | |
| - ["repo:https://github.com/rust-lang/futures-rs:futures-util", "crate::io::AsyncBufReadExt::read_until", "Argument[self].*", "Argument[1].Reference", "taint", "manual"] |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
There's missing data flow through certain implicit dereferences, when that's fixed we can get rid of the duplicate models.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add models for
futures-io(traits, part offutures-rs), and forrustls+futures-rustls(which uses the traits and includes what seem like high value sources).Most of these work well, but the models for
poll_readandpoll_fill_bufaren't working reliably. I haven't been able to figure out what's wrong and I suspect it isn't an issue with the models themselves.There's also a fix of the test sink recognition logic that allows the test to find sinks in
sources/web_frameworks.rs, which also gives us a few results insources/web_frameworks.rsthat we should have been finding all along.