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: Avoid location-based variable analysis #18462

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
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
155 changes: 120 additions & 35 deletions rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ module Impl {

/**
* A path expression that may access a local variable. These are paths that
* only consists of a simple name (i.e., without generic arguments,
* only consist of a simple name (i.e., without generic arguments,
* qualifiers, etc.).
*/
private class VariableAccessCand extends PathExprBase {
Expand Down Expand Up @@ -231,80 +231,169 @@ module Impl {
)
}

/** A subset of `Element`s for which we want to compute pre-order numbers. */
private class RelevantElement extends Element {
RelevantElement() {
this instanceof VariableScope or
this instanceof VariableAccessCand or
this instanceof LetStmt or
getImmediateChildAndAccessor(this, _, _) instanceof RelevantElement
}

pragma[nomagic]
private RelevantElement getChild(int index) {
result = getImmediateChildAndAccessor(this, index, _)
}

pragma[nomagic]
private RelevantElement getImmediateChildMin(int index) {
// A child may have multiple positions for different accessors,
// so always use the first
result = this.getChild(index) and
index = min(int i | result = this.getChild(i) | i)
}

pragma[nomagic]
RelevantElement getImmediateChild(int index) {
result =
rank[index + 1](Element res, int i | res = this.getImmediateChildMin(i) | res order by i)
}

pragma[nomagic]
RelevantElement getImmediateLastChild() {
exists(int last |
result = this.getImmediateChild(last) and
not exists(this.getImmediateChild(last + 1))
)
}
}

/**
* Gets the pre-order numbering of `n`, where the immediately enclosing
* variable scope of `n` is `scope`.
*/
pragma[nomagic]
private int getPreOrderNumbering(VariableScope scope, RelevantElement n) {
n = scope and
result = 0
or
exists(RelevantElement parent |
not parent instanceof VariableScope
or
parent = scope
|
// first child of a previously numbered node
result = getPreOrderNumbering(scope, parent) + 1 and
n = parent.getImmediateChild(0)
or
// non-first child of a previously numbered node
exists(RelevantElement child, int i |
result = getLastPreOrderNumbering(scope, child) + 1 and
child = parent.getImmediateChild(i) and
n = parent.getImmediateChild(i + 1)
)
)
}

/**
* Gets the pre-order numbering of the _last_ node nested under `n`, where the
* immediately enclosing variable scope of `n` (and the last node) is `scope`.
*/
pragma[nomagic]
private int getLastPreOrderNumbering(VariableScope scope, RelevantElement n) {
exists(RelevantElement leaf |
result = getPreOrderNumbering(scope, leaf) and
leaf != scope and
(
not exists(leaf.getImmediateChild(_))
or
leaf instanceof VariableScope
)
|
n = leaf
or
n.getImmediateLastChild() = leaf and
not n instanceof VariableScope
)
or
exists(RelevantElement mid |
mid = n.getImmediateLastChild() and
result = getLastPreOrderNumbering(scope, mid) and
not mid instanceof VariableScope and
not n instanceof VariableScope
)
}

/**
* Holds if `v` is named `name` and is declared inside variable scope
* `scope`, and `v` is bound starting from `(line, column)`.
* `scope`. The pre-order numbering of the binding site of `v`, amongst
* all nodes nester under `scope`, is `ord`.
*/
private predicate variableDeclInScope(
Variable v, VariableScope scope, string name, int line, int column
) {
private predicate variableDeclInScope(Variable v, VariableScope scope, string name, int ord) {
name = v.getName() and
(
parameterDeclInScope(v, scope) and
scope.getLocation().hasLocationFileInfo(_, line, column, _, _)
ord = getPreOrderNumbering(scope, scope)
or
exists(Pat pat | pat = getAVariablePatAncestor(v) |
scope =
any(MatchArmScope arm |
arm.getPat() = pat and
arm.getLocation().hasLocationFileInfo(_, line, column, _, _)
ord = getPreOrderNumbering(scope, arm)
)
or
exists(LetStmt let |
let.getPat() = pat and
scope = getEnclosingScope(let) and
// for `let` statements, variables are bound _after_ the statement, i.e.
// not in the RHS
let.getLocation().hasLocationFileInfo(_, _, _, line, column)
ord = getLastPreOrderNumbering(scope, let) + 1
)
or
exists(IfExpr ie, LetExpr let |
let.getPat() = pat and
ie.getCondition() = let and
scope = ie.getThen() and
scope.getLocation().hasLocationFileInfo(_, line, column, _, _)
ord = getPreOrderNumbering(scope, scope)
)
or
exists(ForExpr fe |
fe.getPat() = pat and
scope = fe.getLoopBody() and
scope.getLocation().hasLocationFileInfo(_, line, column, _, _)
ord = getPreOrderNumbering(scope, scope)
)
or
exists(WhileExpr we, LetExpr let |
let.getPat() = pat and
we.getCondition() = let and
scope = we.getLoopBody() and
scope.getLocation().hasLocationFileInfo(_, line, column, _, _)
ord = getPreOrderNumbering(scope, scope)
)
)
)
}

/**
* Holds if `cand` may access a variable named `name` at
* `(startline, startcolumn, endline, endcolumn)` in the variable scope
* `scope`.
* Holds if `cand` may access a variable named `name` at pre-order number `ord`
* in the variable scope `scope`.
*
* `nestLevel` is the number of nested scopes that need to be traversed
* to reach `scope` from `cand`.
*/
private predicate variableAccessCandInScope(
VariableAccessCand cand, VariableScope scope, string name, int nestLevel, int startline,
int startcolumn, int endline, int endcolumn
VariableAccessCand cand, VariableScope scope, string name, int nestLevel, int ord
) {
name = cand.getName() and
scope = [cand.(VariableScope), getEnclosingScope(cand)] and
cand.getLocation().hasLocationFileInfo(_, startline, startcolumn, endline, endcolumn) and
ord = getPreOrderNumbering(scope, cand) and
nestLevel = 0
or
exists(VariableScope inner |
variableAccessCandInScope(cand, inner, name, nestLevel - 1, _, _, _, _) and
variableAccessCandInScope(cand, inner, name, nestLevel - 1, _) and
scope = getEnclosingScope(inner) and
// Use the location of the inner scope as the location of the access, instead of the
// actual access location. This allows us to collapse multiple accesses in inner
// scopes to a single entity
inner.getLocation().hasLocationFileInfo(_, startline, startcolumn, endline, endcolumn)
// Use the pre-order number of the inner scope as the number of the access. This allows
// us to collapse multiple accesses in inner scopes to a single entity
ord = getPreOrderNumbering(scope, inner)
)
}

Expand Down Expand Up @@ -375,15 +464,12 @@ module Impl {
}

pragma[nomagic]
predicate rankBy(
string name, VariableScope scope, int startline, int startcolumn, int endline, int endcolumn
) {
variableDeclInScope(this.asVariable(), scope, name, startline, startcolumn) and
endline = -1 and
endcolumn = -1
predicate rankBy(string name, VariableScope scope, int ord, int kind) {
variableDeclInScope(this.asVariable(), scope, name, ord) and
kind = 0
or
variableAccessCandInScope(this.asVariableAccessCand(), scope, name, _, startline, startcolumn,
endline, endcolumn)
variableAccessCandInScope(this.asVariableAccessCand(), scope, name, _, ord) and
kind = 1
}
}

Expand All @@ -396,11 +482,10 @@ module Impl {

int getRank(VariableScope scope, string name, VariableOrAccessCand v) {
v =
rank[result](VariableOrAccessCand v0, int startline, int startcolumn, int endline,
int endcolumn |
v0.rankBy(name, scope, startline, startcolumn, endline, endcolumn)
rank[result](VariableOrAccessCand v0, int ord, int kind |
v0.rankBy(name, scope, ord, kind)
|
v0 order by startline, startcolumn, endline, endcolumn
v0 order by ord, kind
)
}
}
Expand Down Expand Up @@ -440,7 +525,7 @@ module Impl {
exists(int rnk |
variableReachesRank(scope, name, v, rnk) and
rnk = rankVariableOrAccess(scope, name, TVariableOrAccessCandVariableAccessCand(cand)) and
variableAccessCandInScope(cand, scope, name, nestLevel, _, _, _, _)
variableAccessCandInScope(cand, scope, name, nestLevel, _)
)
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
models
| 1 | Summary: lang:alloc; <crate::string::String>::as_str; Argument[self]; ReturnValue; taint |
| 2 | Summary: lang:alloc; crate::fmt::format; Argument[0]; ReturnValue; taint |
| 3 | Summary: lang:core; crate::hint::must_use; Argument[0]; ReturnValue; value |
edges
| main.rs:26:9:26:9 | s | main.rs:27:19:27:25 | s[...] | provenance | |
| main.rs:26:13:26:22 | source(...) | main.rs:26:9:26:9 | s | provenance | |
Expand All @@ -27,6 +28,19 @@ edges
| main.rs:77:9:77:18 | formatted3 | main.rs:78:10:78:19 | formatted3 | provenance | |
| main.rs:77:22:77:75 | ...::format(...) | main.rs:77:9:77:18 | formatted3 | provenance | |
| main.rs:77:34:77:74 | MacroExpr | main.rs:77:22:77:75 | ...::format(...) | provenance | MaD:2 |
| main.rs:82:9:82:10 | s1 | main.rs:86:18:86:25 | MacroExpr | provenance | |
| main.rs:82:9:82:10 | s1 | main.rs:87:18:87:32 | MacroExpr | provenance | |
| main.rs:82:14:82:23 | source(...) | main.rs:82:9:82:10 | s1 | provenance | |
| main.rs:86:10:86:26 | res | main.rs:86:18:86:25 | { ... } | provenance | |
| main.rs:86:18:86:25 | ...::format(...) | main.rs:86:10:86:26 | res | provenance | |
| main.rs:86:18:86:25 | ...::must_use(...) | main.rs:86:10:86:26 | MacroExpr | provenance | |
| main.rs:86:18:86:25 | MacroExpr | main.rs:86:18:86:25 | ...::format(...) | provenance | MaD:2 |
| main.rs:86:18:86:25 | { ... } | main.rs:86:18:86:25 | ...::must_use(...) | provenance | MaD:3 |
| main.rs:87:10:87:33 | res | main.rs:87:18:87:32 | { ... } | provenance | |
| main.rs:87:18:87:32 | ...::format(...) | main.rs:87:10:87:33 | res | provenance | |
| main.rs:87:18:87:32 | ...::must_use(...) | main.rs:87:10:87:33 | MacroExpr | provenance | |
| main.rs:87:18:87:32 | MacroExpr | main.rs:87:18:87:32 | ...::format(...) | provenance | MaD:2 |
| main.rs:87:18:87:32 | { ... } | main.rs:87:18:87:32 | ...::must_use(...) | provenance | MaD:3 |
nodes
| main.rs:26:9:26:9 | s | semmle.label | s |
| main.rs:26:13:26:22 | source(...) | semmle.label | source(...) |
Expand Down Expand Up @@ -58,6 +72,20 @@ nodes
| main.rs:77:22:77:75 | ...::format(...) | semmle.label | ...::format(...) |
| main.rs:77:34:77:74 | MacroExpr | semmle.label | MacroExpr |
| main.rs:78:10:78:19 | formatted3 | semmle.label | formatted3 |
| main.rs:82:9:82:10 | s1 | semmle.label | s1 |
| main.rs:82:14:82:23 | source(...) | semmle.label | source(...) |
| main.rs:86:10:86:26 | MacroExpr | semmle.label | MacroExpr |
| main.rs:86:10:86:26 | res | semmle.label | res |
| main.rs:86:18:86:25 | ...::format(...) | semmle.label | ...::format(...) |
| main.rs:86:18:86:25 | ...::must_use(...) | semmle.label | ...::must_use(...) |
| main.rs:86:18:86:25 | MacroExpr | semmle.label | MacroExpr |
| main.rs:86:18:86:25 | { ... } | semmle.label | { ... } |
| main.rs:87:10:87:33 | MacroExpr | semmle.label | MacroExpr |
| main.rs:87:10:87:33 | res | semmle.label | res |
| main.rs:87:18:87:32 | ...::format(...) | semmle.label | ...::format(...) |
| main.rs:87:18:87:32 | ...::must_use(...) | semmle.label | ...::must_use(...) |
| main.rs:87:18:87:32 | MacroExpr | semmle.label | MacroExpr |
| main.rs:87:18:87:32 | { ... } | semmle.label | { ... } |
subpaths
testFailures
#select
Expand All @@ -67,3 +95,5 @@ testFailures
| main.rs:71:10:71:19 | formatted1 | main.rs:68:13:68:22 | source(...) | main.rs:71:10:71:19 | formatted1 | $@ | main.rs:68:13:68:22 | source(...) | source(...) |
| main.rs:74:10:74:19 | formatted2 | main.rs:68:13:68:22 | source(...) | main.rs:74:10:74:19 | formatted2 | $@ | main.rs:68:13:68:22 | source(...) | source(...) |
| main.rs:78:10:78:19 | formatted3 | main.rs:76:17:76:32 | source_usize(...) | main.rs:78:10:78:19 | formatted3 | $@ | main.rs:76:17:76:32 | source_usize(...) | source_usize(...) |
| main.rs:86:10:86:26 | MacroExpr | main.rs:82:14:82:23 | source(...) | main.rs:86:10:86:26 | MacroExpr | $@ | main.rs:82:14:82:23 | source(...) | source(...) |
| main.rs:87:10:87:33 | MacroExpr | main.rs:82:14:82:23 | source(...) | main.rs:87:10:87:33 | MacroExpr | $@ | main.rs:82:14:82:23 | source(...) | source(...) |
4 changes: 2 additions & 2 deletions rust/ql/test/library-tests/dataflow/strings/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ fn format_macro() {
let s2 = "2";
let s3 = "3";

sink(format!("{}", s1)); // $ MISSING: hasTaintFlow=34
sink(format!("{s1} and {s3}")); // $ MISSING: hasTaintFlow=34
sink(format!("{}", s1)); // $ hasTaintFlow=34
sink(format!("{s1} and {s3}")); // $ hasTaintFlow=34
sink(format!("{s2} and {s3}"));
}

Expand Down
Loading
Loading