Rust: Add models for actix-web #20543
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds models for the actix-web framework to improve CodeQL analysis coverage. The changes enable proper detection of remote taint sources and data flow in actix-web applications by adding source and summary models.
Key Changes
- Added models to identify actix-web route handlers as remote taint sources
- Added taint flow model for
Path::into_inner()method - Updated test expectations to reflect newly detected taint flows
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| rust/ql/lib/codeql/rust/frameworks/actix-web.model.yml | New model file defining taint sources and summaries for actix-web framework |
| rust/ql/test/library-tests/dataflow/sources/web_frameworks.rs | Updated test annotations to reflect expected taint flow detection |
| rust/ql/test/library-tests/dataflow/sources/TaintSources.expected | Updated expected test results with new taint source detections |
| - addsTo: | ||
| pack: codeql/rust-all | ||
| extensible: sourceModel | ||
| data: |
There was a problem hiding this comment.
The parameter range 0..7 appears to be a magic number. Consider adding a comment explaining why parameters 0-7 are considered remote sources, or if this should be a more specific range based on actual actix-web handler parameter patterns.
| data: | |
| data: | |
| # The parameter range 0..7 is used to conservatively cover typical actix-web handler functions, | |
| # which usually accept up to 7 parameters (e.g., HttpRequest, Path, Query, Data, etc.). | |
| # If actix-web changes its handler signature conventions, this range should be updated accordingly. |
Copilot uses AI. Check for mistakes.
| web::Query(a): web::Query<String>, // $ MISSING: Alert[rust/summary/taint-sources] | ||
| web::Query(a): web::Query<String>, | ||
| ) -> String { | ||
| sink(a); // $ MISSING: hasTaintFlow |
There was a problem hiding this comment.
Again it's a bit of a shame we don't get flow to the sink, but in this case I'm not sure what model / approach we'd need to get this working.
There was a problem hiding this comment.
I think it's due to the pattern in the parameter. We probably want to say that if web::Query(a) is tainted then the a inside the parameter is also tainted?
There was a problem hiding this comment.
web::Queryis an enum constructor (I think), I'm not 100% sure if that's close enough to a normal function that a MaD model is going to work at the moment.- it's also "reverse" flow, in the sense that you would usually model flow from the argument
ainweb::Query(a)to the return value, but in this case we want the other way around. I'm sure this sort of thing is quite common with patterns but I'm not sure how sophisticated our handling is at the moment (e.g. in Swift we handle reverse flow through non-trivial LHS expressions in assignments automatically given the forwards models; we may potentially do something similar for patterns in Rust). - try it (both ways round) and see???
| path: web::Path<(String, String)>, // $ MISSING: Alert[rust/summary/taint-sources] | ||
| path: web::Path<(String, String)>, | ||
| ) -> String { | ||
| let (a, b) = path.into_inner(); |
There was a problem hiding this comment.
Could we / would it be reasonable to get flow through this line with additional models of Path.into_inner? I'd justify this by claiming that a Path is often used much like a tuple as in this code.
- ["<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"]
...
There was a problem hiding this comment.
Like in the other case, I was thinking that if path.into_inner() is tainted, then that taint should flow into both a and b? I think we also want a.1 to be tainted if a is tainted (and maybe we already have that)?
There was a problem hiding this comment.
Yeah, we could possibly add a general case for flow through stuff like let (a, b) = c and c.1 (where c is a tuple). I'm a bit nervous about it hurting accuracy (and/or performance) elsewhere though - especially if we come to the same conclusion for other types of contents like struct and array elements.
@hvitved might be able to be more specific about what might happen if we do this.
My suggestion above was more specific - it would be specific to Path.into_inner, tainting the tuple elements in addition to the tuple as a whole.
There was a problem hiding this comment.
I actually thought that if any sort of "collection" was tainted, then accessing it's content would also give taint.
Note that the Path here is struct Path<T>(T) under the hood. So it can in principle store anything. Though maybe the tuple case is most common.
There was a problem hiding this comment.
That's true, I suspect tuples are common but there is in fact an example of Path on a Struct (that implements Deserialize) in the docs for actix-web here: https://docs.rs/actix-web/latest/actix_web/web/struct.Path.html
Adds models for actix-web with a focus on making the existing tests pass.