Rust, shared: Support Parameter in source MaD models
#20452
Conversation
github-actions
bot
added
Rust
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
paldepind
force-pushed
the
rust/mad-source-parameter
branch
from
7bbf6f7 to
e3c9f79
Compare
paldepind
force-pushed
the
rust/mad-source-parameter
branch
from
e3c9f79 to
e6bd0ed
Compare
| exists(ArgumentPosition pos, Expr arg | | ||
| s.head() = Impl::Private::SummaryComponent::parameter(pos) and | ||
| arg = getSourceNodeArgument(source, s.tail().head()) and | ||
| result.asParameter() = getCallable(arg).getParam(pos.getPosition()) |
There was a problem hiding this comment.
So if I'm understanding correctly, this is pretty specific to the pattern .Argument[x].Parameter[y]. Could we plausibly want to support other flow path variants like (without having thought too hard about whether these examples in particular make sense) .Argument[x].Reference.Parameter[y], .Argument[x].Parameter[y].Parameter[z] or .ReturnValue.Parameter[x] or something?
There was a problem hiding this comment.
Yes, Parameter is only supported right after Argument. It seems plausible that other combinations could make sense, though it's not immediately clear to me what they would be.
I think it would make sense to be on the lookout for other sensible things, but I don't think we should generalize the current implementation right now.
There was a problem hiding this comment.
OK, something to keep an eye on then.
paldepind
force-pushed
the
rust/mad-source-parameter
branch
from
e6bd0ed to
c83a9f9
Compare
paldepind
force-pushed
the
rust/mad-source-parameter
branch
from
bb712f0 to
99e88c5
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for MaD (Model-as-Data) sources with Argument[_].Parameter[_] access paths in Rust. This enables modeling of sources that pass tainted data to callbacks or function parameters.
Key changes:
- Updated the shared dataflow framework to support
Parametercomponents in source access paths - Modified flow summary implementations across multiple languages to use
SummaryComponentStackinstead ofSummaryComponent - Added jump steps for sources that cross callable boundaries
Reviewed Changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll |
Core implementation supporting Parameter in source models with jump steps |
rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll |
Rust-specific implementation for handling callback parameter sources |
rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll |
Integration of source jump steps in Rust dataflow |
rust/ql/test/library-tests/dataflow/models/models.ext.yml |
Added test model for pass_source function |
| Multiple language-specific files | Updated signatures to use SummaryComponentStack |
| DataFlowCall getACall(SummarizedCallable sc); | ||
|
|
||
| /** Gets the enclosing callable of `source`. */ | ||
| DataFlowCallable getSourceNodeEnclosingCallable(SourceBase source); |
There was a problem hiding this comment.
We need to know wether the node returned by getSourceNode is in the same callable as SourceBase, and I don't think we can get that information without adding a new predicate.
| --- | ||
| * The models-as-data format for sources now supports access paths of the form | ||
| `Argument[i].Parameter[j]`. This denotes that the source passes tainted data to | ||
| the `j`th parameter of it's `i`th argument (which must be a function or a |
|
|
||
| /** Get the callable that `expr` refers to. */ | ||
| private Callable getCallable(Expr expr) { | ||
| result = resolvePath(expr.(PathExpr).getPath()).(Function) |
There was a problem hiding this comment.
I wasn't aware this was possible in Rust; we don't currently handle it in normal data flow.
| arg = pos.getArgument(call) | ||
| exists(ArgumentPosition pos, Expr arg | | ||
| s.head() = Impl::Private::SummaryComponent::parameter(pos) and | ||
| arg = getSourceNodeArgument(source, s.tail().head()) and |
There was a problem hiding this comment.
We should check that s.tail() is a singleton.
| ) | ||
| or | ||
| result.(RustDataFlow::PostUpdateNode).getPreUpdateNode().asExpr().getExpr() = | ||
| getSourceNodeArgument(source, s.head()) |
There was a problem hiding this comment.
We should check that s is a singleton.
This PR adds support for MaD sources with an
Argument[_].Parameter[_]access path. This corresponds to sources that pass tainted data to a callback/function.The implementation is my interpretation of a comment on Slack by @hvitved. My understanding is that this is not necessarily the long-term ideal way to implement this, but it is a quick way to give us something that works and that lets us write the models that we want to write for Rust.
The DCA report looks good. Taint reach triples on
rust-vulnerable-appswhich makes sense as it uses Warp for which we now have working models.