Skip to content

Commit 5f61c21

Browse files
aibaarsaibaars
committed
Refactor using OptionalStep
1 parent 2daef7e commit 5f61c21

File tree

8 files changed

+112
-37
lines changed

8 files changed

+112
-37
lines changed

Diff for: ‎rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

+73-3
Original file line numberDiff line numberDiff line change
@@ -1022,6 +1022,32 @@ final class SingletonContentSet extends ContentSet, TSingletonContentSet {
10221022
override Content getAReadContent() { result = c }
10231023
}
10241024

1025+
final class OptionalStep extends ContentSet, TOptionalStep {
1026+
override string toString() {
1027+
exists(string name |
1028+
this = TOptionalStep(name) and
1029+
result = "OptionalStep[" + name + "]"
1030+
)
1031+
}
1032+
1033+
override Content getAStoreContent() { none() }
1034+
1035+
override Content getAReadContent() { none() }
1036+
}
1037+
1038+
final class OptionalBarrier extends ContentSet, TOptionalBarrier {
1039+
override string toString() {
1040+
exists(string name |
1041+
this = TOptionalBarrier(name) and
1042+
result = "OptionalBarrier[" + name + "]"
1043+
)
1044+
}
1045+
1046+
override Content getAStoreContent() { none() }
1047+
1048+
override Content getAReadContent() { none() }
1049+
}
1050+
10251051
class LambdaCallKind = Unit;
10261052

10271053
/** Holds if `creation` is an expression that creates a lambda of kind `kind`. */
@@ -1222,6 +1248,12 @@ module RustDataFlow implements InputSig<Location> {
12221248
model = ""
12231249
or
12241250
LocalFlow::flowSummaryLocalStep(nodeFrom, nodeTo, model)
1251+
or
1252+
// Add flow through optional barriers. This step is then blocked by the barrier for queries that choose to use the barrier.
1253+
FlowSummaryImpl::Private::Steps::summaryReadStep(nodeFrom
1254+
.(Node::FlowSummaryNode)
1255+
.getSummaryNode(), TOptionalBarrier(_), nodeTo.(Node::FlowSummaryNode).getSummaryNode()) and
1256+
model = ""
12251257
}
12261258

12271259
/**
@@ -1353,7 +1385,8 @@ module RustDataFlow implements InputSig<Location> {
13531385
)
13541386
or
13551387
FlowSummaryImpl::Private::Steps::summaryReadStep(node1.(Node::FlowSummaryNode).getSummaryNode(),
1356-
cs, node2.(Node::FlowSummaryNode).getSummaryNode())
1388+
cs, node2.(Node::FlowSummaryNode).getSummaryNode()) and
1389+
not isSpecialContentSet(cs)
13571390
}
13581391

13591392
pragma[nomagic]
@@ -1450,7 +1483,8 @@ module RustDataFlow implements InputSig<Location> {
14501483
storeContentStep(node1, cs.(SingletonContentSet).getContent(), node2)
14511484
or
14521485
FlowSummaryImpl::Private::Steps::summaryStoreStep(node1.(Node::FlowSummaryNode).getSummaryNode(),
1453-
cs, node2.(Node::FlowSummaryNode).getSummaryNode())
1486+
cs, node2.(Node::FlowSummaryNode).getSummaryNode()) and
1487+
not isSpecialContentSet(cs)
14541488
}
14551489

14561490
/**
@@ -1794,7 +1828,24 @@ private module Cached {
17941828
TReferenceContent()
17951829

17961830
cached
1797-
newtype TContentSet = TSingletonContentSet(Content c)
1831+
newtype TContentSet =
1832+
TSingletonContentSet(Content c) or
1833+
TOptionalStep(string name) {
1834+
name = any(FlowSummaryImpl::Private::AccessPathToken tok).getAnArgument("OptionalStep")
1835+
} or
1836+
TOptionalBarrier(string name) {
1837+
name = any(FlowSummaryImpl::Private::AccessPathToken tok).getAnArgument("OptionalBarrier")
1838+
}
1839+
1840+
/**
1841+
* Holds if `cs` is used to encode a special operation as a content component, but should not
1842+
* be treated as an ordinary content component.
1843+
*/
1844+
cached
1845+
predicate isSpecialContentSet(ContentSet cs) {
1846+
cs instanceof TOptionalStep or
1847+
cs instanceof TOptionalBarrier
1848+
}
17981849

17991850
/** Holds if `n` is a flow source of kind `kind`. */
18001851
cached
@@ -1806,3 +1857,22 @@ private module Cached {
18061857
}
18071858

18081859
import Cached
1860+
1861+
cached
1862+
private module OptionalSteps {
1863+
cached
1864+
predicate optionalStep(Node node1, string name, Node node2) {
1865+
FlowSummaryImpl::Private::Steps::summaryReadStep(node1.(Node::FlowSummaryNode).getSummaryNode(),
1866+
TOptionalStep(name), node2.(Node::FlowSummaryNode).getSummaryNode()) or
1867+
FlowSummaryImpl::Private::Steps::summaryStoreStep(node1.(Node::FlowSummaryNode).getSummaryNode(),
1868+
TOptionalStep(name), node2.(Node::FlowSummaryNode).getSummaryNode())
1869+
}
1870+
1871+
cached
1872+
predicate optionalBarrier(Node node, string name) {
1873+
FlowSummaryImpl::Private::Steps::summaryReadStep(_, TOptionalBarrier(name),
1874+
node.(Node::FlowSummaryNode).getSummaryNode())
1875+
}
1876+
}
1877+
1878+
import OptionalSteps

Diff for: ‎rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll

+14-1
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ module Input implements InputSig<Location, RustDataFlow> {
5353

5454
RustDataFlow::ArgumentPosition callbackSelfParameterPosition() { result.isClosureSelf() }
5555

56-
ReturnKind getStandardReturnValueKind() { result = TNormalReturnKind() }
56+
ReturnKind getStandardReturnValueKind() { result = TNormalReturnKind() and Stage::ref() }
5757

5858
string encodeParameterPosition(ParameterPosition pos) { result = pos.toString() }
5959

@@ -104,6 +104,10 @@ module Input implements InputSig<Location, RustDataFlow> {
104104
c = TFutureContent() and
105105
arg = ""
106106
)
107+
or
108+
cs = TOptionalStep(arg) and result = "OptionalStep"
109+
or
110+
cs = TOptionalBarrier(arg) and result = "OptionalBarrier"
107111
}
108112

109113
string encodeReturn(ReturnKind rk, string arg) { none() }
@@ -192,3 +196,12 @@ module ParsePositions {
192196
i = AccessPath::parseInt(c)
193197
}
194198
}
199+
200+
cached
201+
module Stage {
202+
cached
203+
predicate ref() { 1 = 1 }
204+
205+
cached
206+
predicate backref() { optionalStep(_, _, _) }
207+
}

Diff for: ‎rust/ql/lib/codeql/rust/dataflow/internal/TaintTrackingImpl.qll

+3-1
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,9 @@ module RustTaintTracking implements InputSig<Location, RustDataFlow> {
6767
exists(Content c | c = cs.(SingletonContentSet).getContent() |
6868
c instanceof ElementContent or
6969
c instanceof ReferenceContent
70-
)
70+
) and
71+
// Optional steps are added through isAdditionalFlowStep but we don't want the implicit reads
72+
not optionalStep(node, _, _)
7173
}
7274

7375
/**

Diff for: ‎rust/ql/lib/codeql/rust/frameworks/stdlib/Stdlib.qll

-15
Original file line numberDiff line numberDiff line change
@@ -21,18 +21,3 @@ private class StartswithCall extends Path::SafeAccessCheck::Range, CfgNodes::Met
2121
branch = true
2222
}
2323
}
24-
25-
/**
26-
* A call to `Path.canonicalize`.
27-
* See https://doc.rust-lang.org/std/path/struct.Path.html#method.canonicalize
28-
*/
29-
private class PathCanonicalizeCall extends Path::PathNormalization::Range {
30-
CfgNodes::MethodCallExprCfgNode call;
31-
32-
PathCanonicalizeCall() {
33-
call = this.asExpr() and
34-
call.getAstNode().(Resolvable).getResolvedPath() = "<crate::path::Path>::canonicalize"
35-
}
36-
37-
override DataFlow::Node getPathArg() { result.asExpr() = call.getReceiver() }
38-
}

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

+1
Original file line numberDiff line numberDiff line change
@@ -16,4 +16,5 @@ 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)].OptionalStep[normalize-path]", "taint", "manual"]
1920
- ["lang:std", "<crate::path::Path>::canonicalize", "Argument[self]", "ReturnValue.Field[crate::result::Result::Ok(0)]", "taint", "manual"]

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

-2
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,4 @@ 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"]
3735

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

+9-2
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
import rust
1818
import codeql.rust.dataflow.DataFlow
19+
import codeql.rust.dataflow.internal.DataFlowImpl as DataflowImpl
1920
import codeql.rust.dataflow.TaintTracking
2021
import codeql.rust.security.TaintedPathExtensions
2122
import TaintedPathFlow::PathGraph
@@ -68,7 +69,10 @@ module TaintedPathConfig implements DataFlow::StateConfigSig {
6869

6970
predicate isBarrier(DataFlow::Node node, FlowState state) {
7071
// Block `NotNormalized` paths here, since they change state to `NormalizedUnchecked`
71-
node instanceof Path::PathNormalization and
72+
(
73+
node instanceof Path::PathNormalization or
74+
DataflowImpl::optionalStep(_, "normalize-path", node)
75+
) and
7276
state instanceof NotNormalized
7377
or
7478
node instanceof Path::SafeAccessCheck and
@@ -78,7 +82,10 @@ module TaintedPathConfig implements DataFlow::StateConfigSig {
7882
predicate isAdditionalFlowStep(
7983
DataFlow::Node nodeFrom, FlowState stateFrom, DataFlow::Node nodeTo, FlowState stateTo
8084
) {
81-
nodeFrom = nodeTo.(Path::PathNormalization).getPathArg() and
85+
(
86+
nodeFrom = nodeTo.(Path::PathNormalization).getPathArg() or
87+
DataflowImpl::optionalStep(nodeFrom, "normalize-path", nodeTo)
88+
) and
8289
stateFrom instanceof NotNormalized and
8390
stateTo instanceof NormalizedUnchecked
8491
}

Diff for: ‎rust/ql/test/query-tests/security/CWE-022/TaintedPath.expected

+12-13
Original file line numberDiff line numberDiff line change
@@ -6,29 +6,29 @@ edges
66
| src/main.rs:6:11:6:19 | file_name | src/main.rs:8:35:8:43 | file_name | provenance | |
77
| src/main.rs:8:9:8:17 | file_path | src/main.rs:10:24:10:32 | file_path | provenance | |
88
| src/main.rs:8:21:8:44 | ...::from(...) | src/main.rs:8:9:8:17 | file_path | provenance | |
9-
| src/main.rs:8:35:8:43 | file_name | src/main.rs:8:21:8:44 | ...::from(...) | provenance | MaD:4 |
9+
| src/main.rs:8:35:8:43 | file_name | src/main.rs:8:21:8:44 | ...::from(...) | provenance | MaD:5 |
1010
| src/main.rs:10:24:10:32 | file_path | src/main.rs:10:5:10:22 | ...::read_to_string | provenance | MaD:1 Sink:MaD:1 |
1111
| src/main.rs:37:11:37:19 | file_path | src/main.rs:40:52:40:60 | file_path | provenance | |
1212
| src/main.rs:40:9:40:17 | file_path | src/main.rs:45:24:45:32 | file_path | provenance | |
1313
| src/main.rs:40:21:40:62 | public_path.join(...) | src/main.rs:40:9:40:17 | file_path | provenance | |
14-
| src/main.rs:40:38:40:61 | ...::from(...) | src/main.rs:40:21:40:62 | public_path.join(...) | provenance | MaD:3 |
15-
| src/main.rs:40:52:40:60 | file_path | src/main.rs:40:38:40:61 | ...::from(...) | provenance | MaD:4 |
14+
| src/main.rs:40:38:40:61 | ...::from(...) | src/main.rs:40:21:40:62 | public_path.join(...) | provenance | MaD:4 |
15+
| src/main.rs:40:52:40:60 | file_path | src/main.rs:40:38:40:61 | ...::from(...) | provenance | MaD:5 |
1616
| src/main.rs:45:24:45:32 | file_path | src/main.rs:45:5:45:22 | ...::read_to_string | provenance | MaD:1 Sink:MaD:1 |
1717
| src/main.rs:50:11:50:19 | file_path | src/main.rs:53:52:53:60 | file_path | provenance | |
18-
| src/main.rs:53:9:53:17 | file_path | src/main.rs:54:21:54:29 | file_path | provenance | |
18+
| src/main.rs:53:9:53:17 | file_path | src/main.rs:54:21:54:44 | file_path.canonicalize(...) [Ok] | provenance | MaD:3 |
1919
| src/main.rs:53:21:53:62 | public_path.join(...) | src/main.rs:53:9:53:17 | file_path | provenance | |
20-
| src/main.rs:53:38:53:61 | ...::from(...) | src/main.rs:53:21:53:62 | public_path.join(...) | provenance | MaD:3 |
21-
| src/main.rs:53:52:53:60 | file_path | src/main.rs:53:38:53:61 | ...::from(...) | provenance | MaD:4 |
20+
| src/main.rs:53:38:53:61 | ...::from(...) | src/main.rs:53:21:53:62 | public_path.join(...) | provenance | MaD:4 |
21+
| src/main.rs:53:52:53:60 | file_path | src/main.rs:53:38:53:61 | ...::from(...) | provenance | MaD:5 |
2222
| src/main.rs:54:9:54:17 | file_path | src/main.rs:59:24:59:32 | file_path | provenance | |
23-
| src/main.rs:54:21:54:29 | file_path | src/main.rs:54:21:54:44 | file_path.canonicalize(...) | provenance | Config |
24-
| src/main.rs:54:21:54:44 | file_path.canonicalize(...) | src/main.rs:54:21:54:53 | ... .unwrap(...) | provenance | MaD:2 |
23+
| src/main.rs:54:21:54:44 | file_path.canonicalize(...) [Ok] | src/main.rs:54:21:54:53 | ... .unwrap(...) | provenance | MaD:2 |
2524
| src/main.rs:54:21:54:53 | ... .unwrap(...) | src/main.rs:54:9:54:17 | file_path | provenance | |
2625
| src/main.rs:59:24:59:32 | file_path | src/main.rs:59:5:59:22 | ...::read_to_string | provenance | MaD:1 Sink:MaD:1 |
2726
models
2827
| 1 | Sink: lang:std; crate::fs::read_to_string; path-injection; Argument[0] |
29-
| 2 | Summary: lang:core; <crate::result::Result>::unwrap; Argument[self]; ReturnValue; taint |
30-
| 3 | Summary: lang:std; <crate::path::Path>::join; Argument[0]; ReturnValue; taint |
31-
| 4 | Summary: lang:std; <crate::path::PathBuf as crate::convert::From>::from; Argument[0]; ReturnValue; taint |
28+
| 2 | Summary: lang:core; <crate::result::Result>::unwrap; Argument[self].Field[crate::result::Result::Ok(0)]; ReturnValue; value |
29+
| 3 | Summary: lang:std; <crate::path::Path>::canonicalize; Argument[self]; ReturnValue.Field[crate::result::Result::Ok(0)].OptionalStep[normalize-path]; taint |
30+
| 4 | Summary: lang:std; <crate::path::Path>::join; Argument[0]; ReturnValue; taint |
31+
| 5 | Summary: lang:std; <crate::path::PathBuf as crate::convert::From>::from; Argument[0]; ReturnValue; taint |
3232
nodes
3333
| src/main.rs:6:11:6:19 | file_name | semmle.label | file_name |
3434
| src/main.rs:8:9:8:17 | file_path | semmle.label | file_path |
@@ -49,8 +49,7 @@ nodes
4949
| src/main.rs:53:38:53:61 | ...::from(...) | semmle.label | ...::from(...) |
5050
| src/main.rs:53:52:53:60 | file_path | semmle.label | file_path |
5151
| src/main.rs:54:9:54:17 | file_path | semmle.label | file_path |
52-
| src/main.rs:54:21:54:29 | file_path | semmle.label | file_path |
53-
| src/main.rs:54:21:54:44 | file_path.canonicalize(...) | semmle.label | file_path.canonicalize(...) |
52+
| src/main.rs:54:21:54:44 | file_path.canonicalize(...) [Ok] | semmle.label | file_path.canonicalize(...) [Ok] |
5453
| src/main.rs:54:21:54:53 | ... .unwrap(...) | semmle.label | ... .unwrap(...) |
5554
| src/main.rs:59:5:59:22 | ...::read_to_string | semmle.label | ...::read_to_string |
5655
| src/main.rs:59:24:59:32 | file_path | semmle.label | file_path |

0 commit comments

Comments
 (0)