Skip to content
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.

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

Rust: Value flow and taint flow through formatting strings #18394

Merged
merged 7 commits into from
Jan 6, 2025
Merged
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
14 changes: 7 additions & 7 deletions rust/ql/.generated.list

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

47 changes: 47 additions & 0 deletions rust/ql/lib/codeql/rust/controlflow/CfgNodes.qll
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,53 @@ final class MethodCallExprCfgNode extends CallExprBaseCfgNode, Nodes::MethodCall
*/
final class CallExprCfgNode extends CallExprBaseCfgNode, Nodes::CallExprCfgNode { }

/**
* A FormatArgsExpr. For example:
* ```rust
* format_args!("no args");
* format_args!("{} foo {:?}", 1, 2);
* format_args!("{b} foo {a:?}", a=1, b=2);
* let (x, y) = (1, 42);
* format_args!("{x}, {y}");
* ```
*/
final class FormatArgsExprCfgNode extends Nodes::FormatArgsExprCfgNode {
Fixed Show fixed Hide fixed
private FormatArgsExprChildMapping node;

FormatArgsExprCfgNode() { node = this.getAstNode() }

/** Gets the `i`th argument of this format arguments expression (0-based). */
ExprCfgNode getArgumentExpr(int i) {
any(ChildMapping mapping).hasCfgChild(node, node.getArg(i).getExpr(), this, result)
}

/** Gets a format argument of the `i`th format of this format arguments expression (0-based). */
FormatTemplateVariableAccessCfgNode getFormatTemplateVariableAccess(int i) {
exists(FormatTemplateVariableAccess v |
v.getArgument() = node.getFormat(i).getArgument() and
result.getFormatTemplateVariableAccess() = v and
any(ChildMapping mapping).hasCfgChild(node, v, this, result)
)
}
}

/**
* A MacroCall. For example:
* ```rust
* todo!()
* ```
*/
final class MacroCallCfgNode extends Nodes::MacroCallCfgNode {
private MacroCallChildMapping node;

MacroCallCfgNode() { node = this.getAstNode() }

/** Gets the CFG node for the expansion of this macro call, if it exists. */
CfgNode getExpandedNode() {
any(ChildMapping mapping).hasCfgChild(node, node.getExpanded(), this, result)
}
}

/**
* A record expression. For example:
* ```rust
Expand Down
4 changes: 4 additions & 0 deletions rust/ql/lib/codeql/rust/controlflow/internal/CfgNodes.qll
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ class RecordPatChildMapping extends ParentAstNode, RecordPat {
}
}

class MacroCallChildMapping extends ParentAstNode, MacroCall {
override predicate relevantChild(AstNode child) { child = this.getExpanded() }
}

class FormatArgsExprChildMapping extends ParentAstNode, CfgImpl::ExprTrees::FormatArgsExprTree {
override predicate relevantChild(AstNode child) { child = this.getChildNode(_) }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,24 +131,8 @@ class LetStmtTree extends PreOrderTree, LetStmt {
}
}

class MacroCallTree extends ControlFlowTree, MacroCall {
override predicate first(AstNode first) {
first(this.getExpanded(), first)
or
not exists(this.getExpanded()) and first = this
}

override predicate last(AstNode last, Completion c) {
last(this.getExpanded(), last, c)
or
not exists(this.getExpanded()) and
last = this and
completionIsValidFor(c, last)
}

override predicate succ(AstNode pred, AstNode succ, Completion c) { none() }

override predicate propagatesAbnormal(AstNode child) { child = this.getExpanded() }
class MacroCallTree extends StandardPostOrderTree, MacroCall {
override AstNode getChildNode(int i) { i = 0 and result = this.getExpanded() }
}

class MacroStmtsTree extends StandardPreOrderTree, MacroStmts {
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,7 @@ private ExprCfgNode getALastEvalNode(ExprCfgNode e) {
result = e.(BreakExprCfgNode).getExpr() or
result = e.(BlockExprCfgNode).getTailExpr() or
result = e.(MatchExprCfgNode).getArmExpr(_) or
result = e.(MacroExprCfgNode).getMacroCall().(MacroCallCfgNode).getExpandedNode() or
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this rather be

result = e.(MacroExprCfgNode).getMacroCall() or
result = e.(MacroCallCfgNode).getExpandedNode() or

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conceptually yes, but MacroCall is not an expression and doesn't have a node in the data-flow graph, so it won't work. To do that I think we'd have to change the type of getALastEvalNode and add a new kind of data-flow node for MacroCall.

ThaSo just adding a step over the MacroCall seems simpler and is also what we do for the other kind of nodes that have their expression nested further down.

We could add a method on MacroExprCfgNode to get the expanded node directly?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, let's leave it as-is then.

result.(BreakExprCfgNode).getTarget() = e
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ module RustTaintTracking implements InputSig<Location, RustDataFlow> {
RustDataFlow::readStep(pred, cs, succ) and
cs.getContent() instanceof ArrayElementContent
)
or
exists(FormatArgsExprCfgNode format | succ.asExpr() = format |
pred.asExpr() = [format.getArgumentExpr(_), format.getFormatTemplateVariableAccess(_)]
)
)
or
FlowSummaryImpl::Private::Steps::summaryLocalStep(pred.(Node::FlowSummaryNode).getSummaryNode(),
Expand Down
4 changes: 4 additions & 0 deletions rust/ql/lib/codeql/rust/elements/Format.qll

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions rust/ql/lib/codeql/rust/elements/FormatArgsArg.qll

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions rust/ql/lib/codeql/rust/elements/internal/FormatImpl.qll
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ module Impl {
* ```rust
* println!("Hello {}", "world");
* ```
* or the `{value:#width$.precision$}` in:
* ```rust
* println!("Value {value:#width$.precision$}");
* ```
*/
class Format extends Generated::Format {
private Raw::FormatArgsExpr parent;
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions rust/ql/lib/codeql/rust/elements/internal/generated/Raw.qll

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions rust/ql/lib/codeql/rust/frameworks/stdlib/lang-core.model.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,7 @@ extensions:
- ["lang:core", "<crate::result::Result>::unwrap_or", "Argument[0]", "ReturnValue", "value", "manual"]
# String
- ["lang:alloc", "<crate::string::String>::as_str", "Argument[self]", "ReturnValue", "taint", "manual"]
# Hint
- ["lang:core", "crate::hint::must_use", "Argument[0]", "ReturnValue", "value", "manual"]
# Fmt
- ["lang:alloc", "crate::fmt::format", "Argument[0]", "ReturnValue", "taint", "manual"]
4 changes: 2 additions & 2 deletions rust/ql/test/extractor-tests/generated/.generated_tests.list

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading