Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ private module StepsInput implements Impl::Private::StepsInputSig {
result.getStaticCallTarget().getUnderlyingCallable() = sc
}

Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponent sc) { none() }
DataFlowCallable getSourceNodeEnclosingCallable(Input::SourceBase source) { none() }

Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponentStack s) { none() }

Node getSinkNode(Input::SinkBase sink, Impl::Private::SummaryComponent sc) { none() }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ private module TypesInput implements Impl::Private::TypesInputSig {
)
}

DataFlowType getSourceType(Input::SourceBase source, Impl::Private::SummaryComponent sc) {
DataFlowType getSourceType(Input::SourceBase source, Impl::Private::SummaryComponentStack s) {
none()
}

Expand All @@ -195,7 +195,9 @@ private module StepsInput implements Impl::Private::StepsInputSig {
sc = viableCallable(result).asSummarizedCallable()
}

Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponent sc) { none() }
DataFlowCallable getSourceNodeEnclosingCallable(Input::SourceBase source) { none() }

Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponentStack s) { none() }

Node getSinkNode(Input::SinkBase sink, Impl::Private::SummaryComponent sc) { none() }
}
Expand Down
4 changes: 3 additions & 1 deletion go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,9 @@ private module StepsInput implements Impl::Private::StepsInputSig {
)
}

Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponent sc) { none() }
DataFlowCallable getSourceNodeEnclosingCallable(Input::SourceBase source) { none() }

Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponentStack s) { none() }

Node getSinkNode(Input::SinkBase sink, Impl::Private::SummaryComponent sc) { none() }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ private module TypesInput implements Impl::Private::TypesInputSig {
exists(rk)
}

DataFlowType getSourceType(Input::SourceBase source, Impl::Private::SummaryComponent sc) {
DataFlowType getSourceType(Input::SourceBase source, Impl::Private::SummaryComponentStack s) {
none()
}

Expand All @@ -144,7 +144,9 @@ private module StepsInput implements Impl::Private::StepsInputSig {
sc = viableCallable(result).asSummarizedCallable()
}

Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponent sc) { none() }
DataFlowCallable getSourceNodeEnclosingCallable(Input::SourceBase source) { none() }

Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponentStack s) { none() }

Node getSinkNode(Input::SinkBase sink, Impl::Private::SummaryComponent sc) { none() }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,9 @@ private module FlowSummaryStepInput implements Private::StepsInputSig {
)
}

DataFlow::Node getSourceNode(SourceBase source, Private::SummaryComponent sc) { none() }
DataFlowCallable getSourceNodeEnclosingCallable(SourceBase source) { none() }

DataFlow::Node getSourceNode(SourceBase source, Private::SummaryComponentStack s) { none() }

DataFlow::Node getSinkNode(SinkBase sink, Private::SummaryComponent sc) { none() }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,9 @@ private module StepsInput implements Impl::Private::StepsInputSig {
])
}

Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponent sc) { none() }
DataFlowCallable getSourceNodeEnclosingCallable(Input::SourceBase source) { none() }

Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponentStack s) { none() }

Node getSinkNode(Input::SinkBase sink, Impl::Private::SummaryComponent sc) { none() }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,9 @@ private module StepsInput implements Impl::Private::StepsInputSig {
result.asCall().getAstNode() = sc.(LibraryCallable).getACallSimple()
}

Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponent sc) { none() }
DataFlowCallable getSourceNodeEnclosingCallable(Input::SourceBase source) { none() }

Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponentStack s) { none() }

Node getSinkNode(Input::SinkBase sink, Impl::Private::SummaryComponent sc) { none() }
}
Expand Down
7 changes: 7 additions & 0 deletions rust/ql/lib/change-notes/2025-09-19-parameter-mad.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
category: feature
---
* 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: its

closure).
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,8 @@ module RustDataFlow implements InputSig<Location> {
*/
predicate jumpStep(Node node1, Node node2) {
FlowSummaryImpl::Private::Steps::summaryJumpStep(node1.(FlowSummaryNode).getSummaryNode(),
node2.(FlowSummaryNode).getSummaryNode())
node2.(FlowSummaryNode).getSummaryNode()) or
FlowSummaryImpl::Private::Steps::sourceJumpStep(node1.(FlowSummaryNode).getSummaryNode(), node2)
}

pragma[nomagic]
Expand Down
45 changes: 38 additions & 7 deletions rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ private import rust
private import codeql.dataflow.internal.FlowSummaryImpl
private import codeql.dataflow.internal.AccessPathSyntax as AccessPath
private import codeql.rust.dataflow.internal.DataFlowImpl
private import codeql.rust.internal.PathResolution
private import codeql.rust.dataflow.FlowSummary
private import codeql.rust.dataflow.Ssa
private import codeql.rust.controlflow.CfgNodes
private import Content

module Input implements InputSig<Location, RustDataFlow> {
Expand Down Expand Up @@ -133,16 +136,44 @@ private module StepsInput implements Impl::Private::StepsInputSig {
result.asCallCfgNode().getCall().getStaticTarget() = sc
}

RustDataFlow::Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponent sc) {
sc = Impl::Private::SummaryComponent::return(_) and
/** Gets the argument of `source` described by `sc`, if any. */
private Expr getSourceNodeArgument(Input::SourceBase source, Impl::Private::SummaryComponent sc) {
exists(ArgumentPosition pos |
sc = Impl::Private::SummaryComponent::argument(pos) and
result = pos.getArgument(source.getCall())
)
}

/** Get the callable that `expr` refers to. */
private Callable getCallable(Expr expr) {
result = resolvePath(expr.(PathExpr).getPath()).(Function)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware this was possible in Rust; we don't currently handle it in normal data flow.

or
result = expr.(ClosureExpr)
or
// The expression is an SSA read of an assignment of a closure
exists(Ssa::Definition def, ExprCfgNode value |
def.getARead().getAstNode() = expr and
def.getAnUltimateDefinition().(Ssa::WriteDefinition).assigns(value) and
result = value.getExpr().(ClosureExpr)
)
}

RustDataFlow::DataFlowCallable getSourceNodeEnclosingCallable(Input::SourceBase source) {
result.asCfgScope() = source.getEnclosingCfgScope()
}

RustDataFlow::Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponentStack s) {
s.head() = Impl::Private::SummaryComponent::return(_) and
result.asExpr().getExpr() = source.getCall()
or
exists(CallExprBase call, Expr arg, ArgumentPosition pos |
result.(RustDataFlow::PostUpdateNode).getPreUpdateNode().asExpr().getExpr() = arg and
sc = Impl::Private::SummaryComponent::argument(pos) and
call = source.getCall() and
arg = pos.getArgument(call)
exists(ArgumentPosition pos, Expr arg |
s.head() = Impl::Private::SummaryComponent::parameter(pos) and
arg = getSourceNodeArgument(source, s.tail().head()) and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check that s.tail() is a singleton.

result.asParameter() = getCallable(arg).getParam(pos.getPosition())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, something to keep an eye on then.

)
or
result.(RustDataFlow::PostUpdateNode).getPreUpdateNode().asExpr().getExpr() =
getSourceNodeArgument(source, s.head())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check that s is a singleton.

}

RustDataFlow::Node getSinkNode(Input::SinkBase sink, Impl::Private::SummaryComponent sc) {
Expand Down
7 changes: 3 additions & 4 deletions rust/ql/lib/utils/test/InlineFlowTest.qll
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,9 @@ private module FlowTestImpl implements InputSig<Location, RustDataFlow> {
result = src.asExpr().(CallExprCfgNode).getArgument(0).toString()
or
sourceNode(src, _) and
exists(CallExprBase call |
call = src.(Node::FlowSummaryNode).getSourceElement().getCall() and
result = call.getArgList().getArg(0).toString()
)
result = src.(Node::FlowSummaryNode).getSourceElement().getCall().getArg(0).toString() and
// Don't use the result if it contains spaces
not result.matches("% %")
}

bindingset[src, sink]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
multipleCallTargets
| main.rs:362:14:362:30 | ... .lt(...) |
| main.rs:389:14:389:30 | ... .lt(...) |
27 changes: 27 additions & 0 deletions rust/ql/test/library-tests/dataflow/models/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,33 @@ fn test_enum_method_source() {
}
}

mod source_into_function {
use super::sink;

// has a source model
fn pass_source<A>(_i: i64, f: impl FnOnce(i64) -> A) -> A {
f(42)
}

fn test_source_into_function() {
let a = |a| sink(a); // $ hasValueFlow=1
pass_source(1, a);

pass_source(2, |a| {
sink(a); // $ hasValueFlow=2
});

fn f(a: i64) {
sink(a) // $ hasValueFlow=3
}
pass_source(3, f);

pass_source(4, async move |a| {
sink(a); // $ hasValueFlow=4
});
}
}

// has a sink model
fn enum_sink(e: MyFieldEnum) {}

Expand Down
Loading
Loading