Skip to content

Rust: Add reverse post-update flow steps #19106

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 26, 2025
Merged

Rust: Add reverse post-update flow steps #19106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fcb1d94
Rust: Add more data flow tests
hvitved Mar 25, 2025
72028c0
Rust: Add reverse post-update flow steps
hvitved Mar 25, 2025
f45eca7
Address review comments
hvitved Mar 26, 2025
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
2 changes: 2 additions & 0 deletions rust/ql/lib/codeql/rust/dataflow/internal/DataFlowConsistency.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ private module Input implements InputSig<Location, RustDataFlow> {
// We allow flow into post-update node for receiver expressions (from the
// synthetic post receiever node).
n.(Node::PostUpdateNode).getPreUpdateNode().asExpr() = any(Node::ReceiverNode r).getReceiver()
or
n.(Node::PostUpdateNode).getPreUpdateNode().asExpr() = getPostUpdateReverseStep(_, _)
}

predicate missingLocationExclude(RustDataFlow::Node n) { not exists(n.asExpr().getLocation()) }
Expand Down
25 changes: 25 additions & 0 deletions rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,28 @@
result.(BreakExprCfgNode).getTarget() = e
}

/**
* Holds if a reverse local flow step should be added from the post-update node
* for `e` to the post-update node for the result.
*
* This is needed to allow for side-effects on compound expressions to propagate
* to sub components. For example, in
*
* ```rust
* ({ foo(); &mut a}).set_data(taint);
* ```
*
* we add a reverse flow step from `[post] { foo(); &mut a}` to `[post] &mut a`,
* in order for the side-effect of `set_data` to reach `&mut a`.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, this comment really helps add context 🤩

ExprCfgNode getPostUpdateReverseStep(ExprCfgNode e, boolean preservesValue) {

Check warning

Code scanning / CodeQL

Missing QLDoc for parameter Warning

The QLDoc has no documentation for preservesValue, but the QLDoc mentions set_data
result = getALastEvalNode(e) and
preservesValue = true
or
result = e.(CastExprCfgNode).getExpr() and
preservesValue = false
}

module LocalFlow {
predicate flowSummaryLocalStep(Node nodeFrom, Node nodeTo, string model) {
exists(FlowSummaryImpl::Public::SummarizedCallable c |
Expand Down Expand Up @@ -274,6 +296,9 @@
// The dual step of the above, for the post-update nodes.
nodeFrom.(PostUpdateNode).getPreUpdateNode().(ReceiverNode).getReceiver() =
nodeTo.(PostUpdateNode).getPreUpdateNode().asExpr()
or
nodeTo.(PostUpdateNode).getPreUpdateNode().asExpr() =
getPostUpdateReverseStep(nodeFrom.(PostUpdateNode).getPreUpdateNode().asExpr(), true)
}
}

Expand Down
18 changes: 8 additions & 10 deletions rust/ql/lib/codeql/rust/dataflow/internal/Node.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ abstract class NodePublic extends TNode {
/**
* Gets the expression that corresponds to this node, if any.
*/
ExprCfgNode asExpr() { none() }
final ExprCfgNode asExpr() { this = TExprNode(result) }

/**
* Gets the parameter that corresponds to this node, if any.
Expand All @@ -39,7 +39,7 @@ abstract class NodePublic extends TNode {
/**
* Gets the pattern that corresponds to this node, if any.
*/
PatCfgNode asPat() { none() }
final PatCfgNode asPat() { this = TPatNode(result) }
}

abstract class Node extends NodePublic {
Expand Down Expand Up @@ -144,16 +144,12 @@ class ExprNode extends AstCfgFlowNode, TExprNode {
override ExprCfgNode n;

ExprNode() { this = TExprNode(n) }

override ExprCfgNode asExpr() { result = n }
}

final class PatNode extends AstCfgFlowNode, TPatNode {
override PatCfgNode n;

PatNode() { this = TPatNode(n) }

override PatCfgNode asPat() { result = n }
}

/** A data flow node that corresponds to a name node in the CFG. */
Expand Down Expand Up @@ -467,10 +463,12 @@ newtype TNode =
or
e =
[
any(IndexExprCfgNode i).getBase(), any(FieldExprCfgNode access).getExpr(),
any(TryExprCfgNode try).getExpr(),
any(PrefixExprCfgNode pe | pe.getOperatorName() = "*").getExpr(),
any(AwaitExprCfgNode a).getExpr(), any(MethodCallExprCfgNode mc).getReceiver()
any(IndexExprCfgNode i).getBase(), //
any(FieldExprCfgNode access).getExpr(), //
any(TryExprCfgNode try).getExpr(), //
any(PrefixExprCfgNode pe | pe.getOperatorName() = "*").getExpr(), //
any(AwaitExprCfgNode a).getExpr(), any(MethodCallExprCfgNode mc).getReceiver(), //
getPostUpdateReverseStep(any(PostUpdateNode n).getPreUpdateNode().asExpr(), _)
]
} or
TReceiverNode(MethodCallExprCfgNode mc, Boolean isPost) or
Expand Down
20 changes: 15 additions & 5 deletions rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,8 @@
private import codeql.rust.dataflow.Ssa

private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInputSig {
private import codeql.rust.dataflow.internal.DataFlowImpl as DataFlowImpl

class Expr extends CfgNodes::AstCfgNode {
predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) }
}
Expand All @@ -343,14 +345,22 @@
none() // handled in `DataFlowImpl.qll` instead
}

private predicate isArg(CfgNodes::CallExprBaseCfgNode call, CfgNodes::ExprCfgNode e) {
call.getArgument(_) = e
or
call.(CfgNodes::MethodCallExprCfgNode).getReceiver() = e
or
exists(CfgNodes::ExprCfgNode mid |
isArg(call, mid) and
e = DataFlowImpl::getPostUpdateReverseStep(mid, _)
)
}

predicate allowFlowIntoUncertainDef(UncertainWriteDefinition def) {
exists(CfgNodes::CallExprBaseCfgNode call, Variable v, BasicBlock bb, int i |

Check warning

Code scanning / CodeQL

Omittable 'exists' variable Warning

This exists variable can be omitted by using a don't-care expression
in this argument
Loading
.
def.definesAt(v, bb, i) and
mutablyBorrows(bb.getNode(i).getAstNode(), v)
|
call.getArgument(_) = bb.getNode(i)
or
call.(CfgNodes::MethodCallExprCfgNode).getReceiver() = bb.getNode(i)
mutablyBorrows(bb.getNode(i).getAstNode(), v) and
isArg(call, bb.getNode(i))
)
}

Expand Down
3 changes: 3 additions & 0 deletions rust/ql/lib/codeql/rust/dataflow/internal/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ module RustTaintTracking implements InputSig<Location, RustDataFlow> {
exists(FormatArgsExprCfgNode format | succ.asExpr() = format |
pred.asExpr() = [format.getArgumentExpr(_), format.getFormatTemplateVariableAccess(_)]
)
or
succ.(Node::PostUpdateNode).getPreUpdateNode().asExpr() =
getPostUpdateReverseStep(pred.(Node::PostUpdateNode).getPreUpdateNode().asExpr(), false)
)
or
FlowSummaryImpl::Private::Steps::summaryLocalStep(pred.(Node::FlowSummaryNode).getSummaryNode(),
Expand Down
Loading