Skip to content

Commit f08d1d1

Browse files
aibaarsaibaars
committed
Rust: tainted path implement basic sanitizers
1 parent ecca805 commit f08d1d1

File tree

11 files changed

+210
-32
lines changed

11 files changed

+210
-32
lines changed

‎rust/ql/lib/codeql/rust/Concepts.qll

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ private import codeql.rust.dataflow.DataFlow
88
private import codeql.threatmodels.ThreatModels
99
private import codeql.rust.Frameworks
1010
private import codeql.rust.dataflow.FlowSource
11+
private import codeql.rust.controlflow.ControlFlowGraph as Cfg
12+
private import codeql.rust.controlflow.CfgNodes as CfgNodes
1113

1214
/**
1315
* A data flow source for a specific threat-model.
@@ -264,3 +266,44 @@ module Cryptography {
264266

265267
class CryptographicAlgorithm = SC::CryptographicAlgorithm;
266268
}
269+
270+
/** Provides classes for modeling path-related APIs. */
271+
module Path {
272+
/**
273+
* A data-flow node that performs path normalization. This is often needed in order
274+
* to safely access paths.
275+
*/
276+
class PathNormalization extends DataFlow::Node instanceof PathNormalization::Range {
277+
/** Gets an argument to this path normalization that is interpreted as a path. */
278+
DataFlow::Node getPathArg() { result = super.getPathArg() }
279+
}
280+
281+
/** Provides a class for modeling new path normalization APIs. */
282+
module PathNormalization {
283+
/**
284+
* A data-flow node that performs path normalization. This is often needed in order
285+
* to safely access paths.
286+
*/
287+
abstract class Range extends DataFlow::Node {
288+
/** Gets an argument to this path normalization that is interpreted as a path. */
289+
abstract DataFlow::Node getPathArg();
290+
}
291+
}
292+
293+
class SafeAccessCheck extends DataFlow::ExprNode {
294+
SafeAccessCheck() { this = DataFlow::BarrierGuard<safeAccessCheck/3>::getABarrierNode() }
295+
}
296+
297+
private predicate safeAccessCheck(CfgNodes::AstCfgNode g, Cfg::CfgNode node, boolean branch) {
298+
g.(SafeAccessCheck::Range).checks(node, branch)
299+
}
300+
301+
/** Provides a class for modeling new path safety checks. */
302+
module SafeAccessCheck {
303+
/** A data-flow node that checks that a path is safe to access. */
304+
abstract class Range extends CfgNodes::AstCfgNode {
305+
/** Holds if this guard validates `node` upon evaluating to `branch`. */
306+
abstract predicate checks(Cfg::CfgNode node, boolean branch);
307+
}
308+
}
309+
}

‎rust/ql/lib/codeql/rust/Frameworks.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,3 +6,4 @@ private import codeql.rust.frameworks.rustcrypto.RustCrypto
66
private import codeql.rust.frameworks.Poem
77
private import codeql.rust.frameworks.Sqlx
88
private import codeql.rust.frameworks.stdlib.Clone
9+
private import codeql.rust.frameworks.stdlib.Stdlib

‎rust/ql/lib/codeql/rust/frameworks/stdlib/Stdlib.qll

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
private import rust
2+
private import codeql.rust.Concepts
3+
private import codeql.rust.controlflow.ControlFlowGraph as Cfg
4+
private import codeql.rust.controlflow.CfgNodes as CfgNodes
5+
private import codeql.rust.dataflow.DataFlow
6+
7+
/**
8+
* A call to the `starts_with` method on a `Path`.
9+
*/
10+
private class StartswithCall extends Path::SafeAccessCheck::Range, CfgNodes::MethodCallExprCfgNode {
11+
StartswithCall() {
12+
this.getAstNode().(Resolvable).getResolvedPath() = "<crate::path::Path>::starts_with"
13+
}
14+
15+
override predicate checks(Cfg::CfgNode e, boolean branch) {
16+
e = this.getReceiver() and
17+
branch = true
18+
}
19+
}
20+
21+
/**
22+
* A call to `Path.canonicalize`.
23+
* See https://doc.rust-lang.org/std/path/struct.Path.html#method.canonicalize
24+
*/
25+
private class PathCanonicalizeCall extends Path::PathNormalization::Range {
26+
CfgNodes::MethodCallExprCfgNode call;
27+
28+
PathCanonicalizeCall() {
29+
call = this.asExpr() and
30+
call.getAstNode().(Resolvable).getResolvedPath() = "<crate::path::Path>::canonicalize"
31+
}
32+
33+
override DataFlow::Node getPathArg() { result.asExpr() = call.getReceiver() }
34+
}

‎rust/ql/lib/codeql/rust/frameworks/stdlib/fs.model.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,3 +16,4 @@ extensions:
1616
- ["lang:std", "<crate::path::PathBuf as crate::convert::From>::from", "Argument[0]", "ReturnValue", "taint", "manual"]
1717
- ["lang:std", "<crate::path::Path>::join", "Argument[self]", "ReturnValue", "taint", "manual"]
1818
- ["lang:std", "<crate::path::Path>::join", "Argument[0]", "ReturnValue", "taint", "manual"]
19+
- ["lang:std", "<crate::path::Path>::canonicalize", "Argument[self]", "ReturnValue.Field[crate::result::Result::Ok(0)]", "taint", "manual"]

‎rust/ql/lib/codeql/rust/frameworks/stdlib/lang-core.model.yml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,3 +32,6 @@ extensions:
3232
- ["lang:alloc", "<crate::string::String>::as_str", "Argument[self]", "ReturnValue", "taint", "manual"]
3333
- ["lang:alloc", "<crate::string::String>::as_bytes", "Argument[self]", "ReturnValue", "taint", "manual"]
3434
- ["lang:alloc", "<_ as crate::string::ToString>::to_string", "Argument[self]", "ReturnValue", "taint", "manual"]
35+
# Result
36+
- ["lang:core", "<crate::result::Result>::unwrap", "Argument[self]", "ReturnValue", "taint", "manual"]
37+

‎rust/ql/lib/codeql/rust/security/TaintedPathExtensions.qll

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ private import codeql.rust.dataflow.DataFlow
77
private import codeql.rust.dataflow.TaintTracking
88
private import codeql.rust.Concepts
99
private import codeql.rust.dataflow.internal.DataFlowImpl
10+
private import codeql.rust.controlflow.ControlFlowGraph as Cfg
11+
private import codeql.rust.controlflow.CfgNodes as CfgNodes
1012

1113
/**
1214
* Provides default sources, sinks and barriers for detecting path injection
@@ -28,6 +30,10 @@ module TaintedPath {
2830
*/
2931
abstract class Barrier extends DataFlow::Node { }
3032

33+
class SanitizerGuard extends DataFlow::Node {
34+
SanitizerGuard() { this = DataFlow::BarrierGuard<sanitizerGuard/3>::getABarrierNode() }
35+
}
36+
3137
/**
3238
* An active threat-model source, considered as a flow source.
3339
*/
@@ -38,3 +44,33 @@ module TaintedPath {
3844
ModelsAsDataSinks() { sinkNode(this, "path-injection") }
3945
}
4046
}
47+
48+
private predicate sanitizerGuard(CfgNodes::AstCfgNode g, Cfg::CfgNode node, boolean branch) {
49+
g.(SanitizerGuard::Range).checks(node, branch)
50+
}
51+
52+
/** Provides a class for modeling new path safety checks. */
53+
module SanitizerGuard {
54+
/** A data-flow node that checks that a path is safe to access. */
55+
abstract class Range extends CfgNodes::AstCfgNode {
56+
/** Holds if this guard validates `node` upon evaluating to `branch`. */
57+
abstract predicate checks(Cfg::CfgNode node, boolean branch);
58+
}
59+
}
60+
61+
/**
62+
* A check of the form `!strings.Contains(nd, "..")`, considered as a sanitizer guard for
63+
* path traversal.
64+
*/
65+
private class DotDotCheck extends SanitizerGuard::Range, CfgNodes::MethodCallExprCfgNode {
66+
DotDotCheck() {
67+
this.getAstNode().(Resolvable).getResolvedPath() = "<str>::contains" and
68+
this.getArgument(0).getAstNode().(LiteralExpr).getTextValue() =
69+
["\"..\"", "\"../\"", "\"..\\\""]
70+
}
71+
72+
override predicate checks(Cfg::CfgNode e, boolean branch) {
73+
e = this.getReceiver() and
74+
branch = false
75+
}
76+
}

‎rust/ql/src/queries/security/CWE-022/TaintedPath.ql

Lines changed: 59 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,19 +19,72 @@ import codeql.rust.dataflow.DataFlow
1919
import codeql.rust.dataflow.TaintTracking
2020
import codeql.rust.security.TaintedPathExtensions
2121
import TaintedPathFlow::PathGraph
22+
private import codeql.rust.Concepts
23+
24+
abstract private class NormalizationState extends string {
25+
bindingset[this]
26+
NormalizationState() { any() }
27+
}
28+
29+
/** A state signifying that the file path has not been normalized. */
30+
class NotNormalized extends NormalizationState {
31+
NotNormalized() { this = "NotNormalized" }
32+
}
33+
34+
/** A state signifying that the file path has been normalized, but not checked. */
35+
class NormalizedUnchecked extends NormalizationState {
36+
NormalizedUnchecked() { this = "NormalizedUnchecked" }
37+
}
2238

2339
/**
24-
* A taint configuration for tainted data that reaches a file access sink.
40+
* This configuration uses two flow states, `NotNormalized` and `NormalizedUnchecked`,
41+
* to track the requirement that a file path must be first normalized and then checked
42+
* before it is safe to use.
43+
*
44+
* At sources, paths are assumed not normalized. At normalization points, they change
45+
* state to `NormalizedUnchecked` after which they can be made safe by an appropriate
46+
* check of the prefix.
47+
*
48+
* Such checks are ineffective in the `NotNormalized` state.
2549
*/
26-
module TaintedPathConfig implements DataFlow::ConfigSig {
27-
predicate isSource(DataFlow::Node node) { node instanceof TaintedPath::Source }
50+
module TaintedPathConfig implements DataFlow::StateConfigSig {
51+
class FlowState = NormalizationState;
52+
53+
predicate isSource(DataFlow::Node source, FlowState state) {
54+
source instanceof TaintedPath::Source and state instanceof NotNormalized
55+
}
56+
57+
predicate isSink(DataFlow::Node sink, FlowState state) {
58+
sink instanceof TaintedPath::Sink and
59+
(
60+
state instanceof NotNormalized or
61+
state instanceof NormalizedUnchecked
62+
)
63+
}
64+
65+
predicate isBarrier(DataFlow::Node node) {
66+
node instanceof TaintedPath::Barrier or node instanceof TaintedPath::SanitizerGuard
67+
}
2868

29-
predicate isSink(DataFlow::Node node) { node instanceof TaintedPath::Sink }
69+
predicate isBarrier(DataFlow::Node node, FlowState state) {
70+
// Block `NotNormalized` paths here, since they change state to `NormalizedUnchecked`
71+
node instanceof Path::PathNormalization and
72+
state instanceof NotNormalized
73+
or
74+
node instanceof Path::SafeAccessCheck and
75+
state instanceof NormalizedUnchecked
76+
}
3077

31-
predicate isBarrier(DataFlow::Node barrier) { barrier instanceof TaintedPath::Barrier }
78+
predicate isAdditionalFlowStep(
79+
DataFlow::Node nodeFrom, FlowState stateFrom, DataFlow::Node nodeTo, FlowState stateTo
80+
) {
81+
nodeFrom = nodeTo.(Path::PathNormalization).getPathArg() and
82+
stateFrom instanceof NotNormalized and
83+
stateTo instanceof NormalizedUnchecked
84+
}
3285
}
3386

34-
module TaintedPathFlow = TaintTracking::Global<TaintedPathConfig>;
87+
module TaintedPathFlow = TaintTracking::GlobalWithState<TaintedPathConfig>;
3588

3689
from TaintedPathFlow::PathNode source, TaintedPathFlow::PathNode sink
3790
where TaintedPathFlow::flowPath(source, sink)

‎rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2165,6 +2165,7 @@ storeStep
21652165
| file://:0:0:0:0 | [summary] to write: ReturnValue.Field[crate::result::Result::Ok(0)] in lang:core::_::<crate::result::Result>::or_else | Ok | file://:0:0:0:0 | [summary] to write: ReturnValue in lang:core::_::<crate::result::Result>::or_else |
21662166
| file://:0:0:0:0 | [summary] to write: ReturnValue.Field[crate::result::Result::Ok(0)] in lang:core::_::<str>::parse | Ok | file://:0:0:0:0 | [summary] to write: ReturnValue in lang:core::_::<str>::parse |
21672167
| file://:0:0:0:0 | [summary] to write: ReturnValue.Field[crate::result::Result::Ok(0)] in lang:std::_::<&[u8] as crate::io::BufRead>::fill_buf | Ok | file://:0:0:0:0 | [summary] to write: ReturnValue in lang:std::_::<&[u8] as crate::io::BufRead>::fill_buf |
2168+
| file://:0:0:0:0 | [summary] to write: ReturnValue.Field[crate::result::Result::Ok(0)] in lang:std::_::<crate::path::Path>::canonicalize | Ok | file://:0:0:0:0 | [summary] to write: ReturnValue in lang:std::_::<crate::path::Path>::canonicalize |
21682169
| file://:0:0:0:0 | [summary] to write: ReturnValue.Field[crate::result::Result::Ok(0)] in lang:std::_::<crate::sync::poison::condvar::Condvar>::wait | Ok | file://:0:0:0:0 | [summary] to write: ReturnValue in lang:std::_::<crate::sync::poison::condvar::Condvar>::wait |
21692170
| file://:0:0:0:0 | [summary] to write: ReturnValue.Field[crate::result::Result::Ok(0)] in lang:std::_::<crate::sync::poison::condvar::Condvar>::wait_timeout | Ok | file://:0:0:0:0 | [summary] to write: ReturnValue in lang:std::_::<crate::sync::poison::condvar::Condvar>::wait_timeout |
21702171
| file://:0:0:0:0 | [summary] to write: ReturnValue.Field[crate::result::Result::Ok(0)] in lang:std::_::<crate::sync::poison::condvar::Condvar>::wait_timeout_ms | Ok | file://:0:0:0:0 | [summary] to write: ReturnValue in lang:std::_::<crate::sync::poison::condvar::Condvar>::wait_timeout_ms |

‎rust/ql/test/query-tests/diagnostics/SummaryStats.expected

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
| Macro calls - resolved | 8 |
1616
| Macro calls - total | 9 |
1717
| Macro calls - unresolved | 1 |
18-
| Taint edges - number of edges | 1674 |
18+
| Taint edges - number of edges | 1675 |
1919
| Taint reach - nodes tainted | 0 |
2020
| Taint reach - per million nodes | 0 |
2121
| Taint sinks - cryptographic operations | 0 |

‎rust/ql/test/query-tests/security/CWE-020/RegexInjection.expected

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,14 @@
33
edges
44
| main.rs:4:9:4:16 | username | main.rs:5:25:5:44 | MacroExpr | provenance | |
55
| main.rs:4:20:4:32 | ...::var | main.rs:4:20:4:40 | ...::var(...) [Ok] | provenance | Src:MaD:62 |
6-
| main.rs:4:20:4:40 | ...::var(...) [Ok] | main.rs:4:20:4:66 | ... .unwrap_or(...) | provenance | MaD:1597 |
6+
| main.rs:4:20:4:40 | ...::var(...) [Ok] | main.rs:4:20:4:66 | ... .unwrap_or(...) | provenance | MaD:1599 |
77
| main.rs:4:20:4:66 | ... .unwrap_or(...) | main.rs:4:9:4:16 | username | provenance | |
88
| main.rs:5:9:5:13 | regex | main.rs:6:26:6:30 | regex | provenance | |
99
| main.rs:5:17:5:45 | res | main.rs:5:25:5:44 | { ... } | provenance | |
1010
| main.rs:5:25:5:44 | ...::format(...) | main.rs:5:17:5:45 | res | provenance | |
1111
| main.rs:5:25:5:44 | ...::must_use(...) | main.rs:5:9:5:13 | regex | provenance | |
12-
| main.rs:5:25:5:44 | MacroExpr | main.rs:5:25:5:44 | ...::format(...) | provenance | MaD:70 |
13-
| main.rs:5:25:5:44 | { ... } | main.rs:5:25:5:44 | ...::must_use(...) | provenance | MaD:3020 |
12+
| main.rs:5:25:5:44 | MacroExpr | main.rs:5:25:5:44 | ...::format(...) | provenance | MaD:71 |
13+
| main.rs:5:25:5:44 | { ... } | main.rs:5:25:5:44 | ...::must_use(...) | provenance | MaD:3022 |
1414
| main.rs:6:26:6:30 | regex | main.rs:6:25:6:30 | &regex | provenance | |
1515
nodes
1616
| main.rs:4:9:4:16 | username | semmle.label | username |

0 commit comments

Comments
 (0)