hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-25 00:35:20 +02:00
Code Issues Packages Projects Releases Wiki Activity

Refactor using OptionalStep

Browse Source
This commit is contained in:
Arthur Baars
2025-03-14 17:30:30 +01:00
parent d3e28772ae
commit 5a91b94395
14 changed files with 135 additions and 43 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
rust/ql/integration-tests/hello-project/summary.expected
View File

@@ -15,7 +15,7 @@
| Macro calls - resolved | 2 |
| Macro calls - total | 2 |
| Macro calls - unresolved | 0 |
| Taint edges - number of edges | 1675 |
| Taint edges - number of edges | 1674 |
| Taint reach - nodes tainted | 0 |
| Taint reach - per million nodes | 0 |
| Taint sinks - cryptographic operations | 0 |

2
rust/ql/integration-tests/hello-workspace/summary.cargo.expected
View File

@@ -15,7 +15,7 @@
| Macro calls - resolved | 2 |
| Macro calls - total | 2 |
| Macro calls - unresolved | 0 |
| Taint edges - number of edges | 1675 |
| Taint edges - number of edges | 1674 |
| Taint reach - nodes tainted | 0 |
| Taint reach - per million nodes | 0 |
| Taint sinks - cryptographic operations | 0 |

2
rust/ql/integration-tests/hello-workspace/summary.rust-project.expected
View File

@@ -15,7 +15,7 @@
| Macro calls - resolved | 2 |
| Macro calls - total | 2 |
| Macro calls - unresolved | 0 |
| Taint edges - number of edges | 1675 |
| Taint edges - number of edges | 1674 |
| Taint reach - nodes tainted | 0 |
| Taint reach - per million nodes | 0 |
| Taint sinks - cryptographic operations | 0 |

35
rust/ql/lib/codeql/rust/dataflow/internal/Content.qll
View File

@@ -214,6 +214,41 @@ final class SingletonContentSet extends ContentSet, TSingletonContentSet {
override Content getAReadContent() { result = c }
}
/**
* A step in a flow summary defined using `OptionalStep[name]`. An `OptionalStep` is "opt-in", which means
* that by default the step is not present in the flow summary and needs to be explicitly enabled by defining
* an additional flow step.
*/
final class OptionalStep extends ContentSet, TOptionalStep {
override string toString() {
exists(string name |
this = TOptionalStep(name) and
result = "OptionalStep[" + name + "]"
)
}
override Content getAStoreContent() { none() }
override Content getAReadContent() { none() }
}
/**
* A step in a flow summary defined using `OptionalBarrier[name]`. An `OptionalBarrier` is "opt-out", by default
* data can flow freely through the step. Flow through the step can be explicity blocked by defining its node as a barrier.
*/
final class OptionalBarrier extends ContentSet, TOptionalBarrier {
override string toString() {
exists(string name |
this = TOptionalBarrier(name) and
result = "OptionalBarrier[" + name + "]"
)
}
override Content getAStoreContent() { none() }
override Content getAReadContent() { none() }
}
private import codeql.rust.internal.CachedStages
cached

59
rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll
View File

@@ -581,6 +581,12 @@ module RustDataFlow implements InputSig<Location> {
model = ""
or
LocalFlow::flowSummaryLocalStep(nodeFrom, nodeTo, model)
or
// Add flow through optional barriers. This step is then blocked by the barrier for queries that choose to use the barrier.
FlowSummaryImpl::Private::Steps::summaryReadStep(nodeFrom
.(Node::FlowSummaryNode)
.getSummaryNode(), TOptionalBarrier(_), nodeTo.(Node::FlowSummaryNode).getSummaryNode()) and
model = ""
}
/**
@@ -710,7 +716,8 @@ module RustDataFlow implements InputSig<Location> {
)
or
FlowSummaryImpl::Private::Steps::summaryReadStep(node1.(FlowSummaryNode).getSummaryNode(), cs,
node2.(FlowSummaryNode).getSummaryNode())
node2.(FlowSummaryNode).getSummaryNode()) and
not isSpecialContentSet(cs)
}
pragma[nomagic]
@@ -807,7 +814,8 @@ module RustDataFlow implements InputSig<Location> {
storeContentStep(node1, cs.(SingletonContentSet).getContent(), node2)
or
FlowSummaryImpl::Private::Steps::summaryStoreStep(node1.(FlowSummaryNode).getSummaryNode(), cs,
node2.(FlowSummaryNode).getSummaryNode())
node2.(FlowSummaryNode).getSummaryNode()) and
not isSpecialContentSet(cs)
}
/**
@@ -1093,7 +1101,24 @@ private module Cached {
newtype TReturnKind = TNormalReturnKind()
cached
newtype TContentSet = TSingletonContentSet(Content c)
newtype TContentSet =
TSingletonContentSet(Content c) or
TOptionalStep(string name) {
name = any(FlowSummaryImpl::Private::AccessPathToken tok).getAnArgument("OptionalStep")
} or
TOptionalBarrier(string name) {
name = any(FlowSummaryImpl::Private::AccessPathToken tok).getAnArgument("OptionalBarrier")
}
/**
* Holds if `cs` is used to encode a special operation as a content component, but should not
* be treated as an ordinary content component.
*/
cached
predicate isSpecialContentSet(ContentSet cs) {
cs instanceof TOptionalStep or
cs instanceof TOptionalBarrier
}
/** Holds if `n` is a flow source of kind `kind`. */
cached
@@ -1105,3 +1130,31 @@ private module Cached {
}
import Cached
cached
private module OptionalSteps {
/**
* A step in a flow summary defined using `OptionalStep[name]`. An `OptionalStep` is "opt-in", which means
* that by default the step is not present in the flow summary and needs to be explicitly enabled by defining
* an additional flow step.
*/
cached
predicate optionalStep(Node node1, string name, Node node2) {
FlowSummaryImpl::Private::Steps::summaryReadStep(node1.(FlowSummaryNode).getSummaryNode(),
TOptionalStep(name), node2.(FlowSummaryNode).getSummaryNode()) or
FlowSummaryImpl::Private::Steps::summaryStoreStep(node1.(FlowSummaryNode).getSummaryNode(),
TOptionalStep(name), node2.(FlowSummaryNode).getSummaryNode())
}
/**
* A step in a flow summary defined using `OptionalBarrier[name]`. An `OptionalBarrier` is "opt-out", by default
* data can flow freely through the step. Flow through the step can be explicity blocked by defining its node as a barrier.
*/
cached
predicate optionalBarrier(Node node, string name) {
FlowSummaryImpl::Private::Steps::summaryReadStep(_, TOptionalBarrier(name),
node.(FlowSummaryNode).getSummaryNode())
}
}
import OptionalSteps

15
rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll
View File

@@ -54,7 +54,7 @@ module Input implements InputSig<Location, RustDataFlow> {
RustDataFlow::ArgumentPosition callbackSelfParameterPosition() { result.isClosureSelf() }
ReturnKind getStandardReturnValueKind() { result = TNormalReturnKind() }
ReturnKind getStandardReturnValueKind() { result = TNormalReturnKind() and Stage::ref() }
string encodeParameterPosition(ParameterPosition pos) { result = pos.toString() }
@@ -105,6 +105,10 @@ module Input implements InputSig<Location, RustDataFlow> {
c = TFutureContent() and
arg = ""
)
or
cs = TOptionalStep(arg) and result = "OptionalStep"
or
cs = TOptionalBarrier(arg) and result = "OptionalBarrier"
}
string encodeReturn(ReturnKind rk, string arg) { none() }
@@ -193,3 +197,12 @@ module ParsePositions {
i = AccessPath::parseInt(c)
}
}
cached
module Stage {
cached
predicate ref() { 1 = 1 }
cached
predicate backref() { optionalStep(_, _, _) }
}

4
rust/ql/lib/codeql/rust/dataflow/internal/TaintTrackingImpl.qll
View File

@@ -69,7 +69,9 @@ module RustTaintTracking implements InputSig<Location, RustDataFlow> {
exists(Content c | c = cs.(SingletonContentSet).getContent() |
c instanceof ElementContent or
c instanceof ReferenceContent
)
) and
// Optional steps are added through isAdditionalFlowStep but we don't want the implicit reads
not optionalStep(node, _, _)
}
/**

15
rust/ql/lib/codeql/rust/frameworks/stdlib/Stdlib.qll
View File

@@ -21,18 +21,3 @@ private class StartswithCall extends Path::SafeAccessCheck::Range, CfgNodes::Met
branch = true
}
}
/**
* A call to `Path.canonicalize`.
* See https://doc.rust-lang.org/std/path/struct.Path.html#method.canonicalize
*/
private class PathCanonicalizeCall extends Path::PathNormalization::Range {
CfgNodes::MethodCallExprCfgNode call;
PathCanonicalizeCall() {
call = this.asExpr() and
call.getAstNode().(Resolvable).getResolvedPath() = "<crate::path::Path>::canonicalize"
}
override DataFlow::Node getPathArg() { result.asExpr() = call.getReceiver() }
}

1
rust/ql/lib/codeql/rust/frameworks/stdlib/fs.model.yml
View File

@@ -16,4 +16,5 @@ extensions:
- ["lang:std", "<crate::path::PathBuf as crate::convert::From>::from", "Argument[0]", "ReturnValue", "taint", "manual"]
- ["lang:std", "<crate::path::Path>::join", "Argument[self]", "ReturnValue", "taint", "manual"]
- ["lang:std", "<crate::path::Path>::join", "Argument[0]", "ReturnValue", "taint", "manual"]
- ["lang:std", "<crate::path::Path>::canonicalize", "Argument[self]", "ReturnValue.Field[crate::result::Result::Ok(0)].OptionalStep[normalize-path]", "taint", "manual"]
- ["lang:std", "<crate::path::Path>::canonicalize", "Argument[self]", "ReturnValue.Field[crate::result::Result::Ok(0)]", "taint", "manual"]

3
rust/ql/lib/codeql/rust/frameworks/stdlib/lang-core.model.yml
View File

@@ -32,6 +32,3 @@ extensions:
- ["lang:alloc", "<crate::string::String>::as_str", "Argument[self]", "ReturnValue", "taint", "manual"]
- ["lang:alloc", "<crate::string::String>::as_bytes", "Argument[self]", "ReturnValue", "taint", "manual"]
- ["lang:alloc", "<_ as crate::string::ToString>::to_string", "Argument[self]", "ReturnValue", "taint", "manual"]
# Result
- ["lang:core", "<crate::result::Result>::unwrap", "Argument[self]", "ReturnValue", "taint", "manual"]

11
rust/ql/src/queries/security/CWE-022/TaintedPath.ql
View File

@@ -16,6 +16,7 @@
import rust
import codeql.rust.dataflow.DataFlow
import codeql.rust.dataflow.internal.DataFlowImpl as DataflowImpl
import codeql.rust.dataflow.TaintTracking
import codeql.rust.security.TaintedPathExtensions
import TaintedPathFlow::PathGraph
@@ -68,7 +69,10 @@ module TaintedPathConfig implements DataFlow::StateConfigSig {
predicate isBarrier(DataFlow::Node node, FlowState state) {
// Block `NotNormalized` paths here, since they change state to `NormalizedUnchecked`
node instanceof Path::PathNormalization and
(
node instanceof Path::PathNormalization or
DataflowImpl::optionalStep(_, "normalize-path", node)
) and
state instanceof NotNormalized
or
node instanceof Path::SafeAccessCheck and
@@ -78,7 +82,10 @@ module TaintedPathConfig implements DataFlow::StateConfigSig {
predicate isAdditionalFlowStep(
DataFlow::Node nodeFrom, FlowState stateFrom, DataFlow::Node nodeTo, FlowState stateTo
) {
nodeFrom = nodeTo.(Path::PathNormalization).getPathArg() and
(
nodeFrom = nodeTo.(Path::PathNormalization).getPathArg() or
DataflowImpl::optionalStep(nodeFrom, "normalize-path", nodeTo)
) and
stateFrom instanceof NotNormalized and
stateTo instanceof NormalizedUnchecked
}

2
rust/ql/test/query-tests/diagnostics/SummaryStats.expected
View File

@@ -15,7 +15,7 @@
| Macro calls - resolved | 8 |
| Macro calls - total | 9 |
| Macro calls - unresolved | 1 |
| Taint edges - number of edges | 1675 |
| Taint edges - number of edges | 1674 |
| Taint reach - nodes tainted | 0 |
| Taint reach - per million nodes | 0 |
| Taint sinks - cryptographic operations | 0 |

2
rust/ql/test/query-tests/security/CWE-020/RegexInjection.expected
View File

@@ -9,7 +9,7 @@ edges
| main.rs:5:17:5:45 | res | main.rs:5:25:5:44 | { ... } | provenance | |
| main.rs:5:25:5:44 | ...::format(...) | main.rs:5:17:5:45 | res | provenance | |
| main.rs:5:25:5:44 | ...::must_use(...) | main.rs:5:9:5:13 | regex | provenance | |
| main.rs:5:25:5:44 | MacroExpr | main.rs:5:25:5:44 | ...::format(...) | provenance | MaD:71 |
| main.rs:5:25:5:44 | MacroExpr | main.rs:5:25:5:44 | ...::format(...) | provenance | MaD:72 |
| main.rs:5:25:5:44 | { ... } | main.rs:5:25:5:44 | ...::must_use(...) | provenance | MaD:3022 |
| main.rs:6:26:6:30 | regex | main.rs:6:25:6:30 | &regex | provenance | |
nodes

25
rust/ql/test/query-tests/security/CWE-022/TaintedPath.expected
View File

@@ -6,29 +6,29 @@ edges
| src/main.rs:6:11:6:19 | file_name | src/main.rs:8:35:8:43 | file_name | provenance | |
| src/main.rs:8:9:8:17 | file_path | src/main.rs:10:24:10:32 | file_path | provenance | |
| src/main.rs:8:21:8:44 | ...::from(...) | src/main.rs:8:9:8:17 | file_path | provenance | |
| src/main.rs:8:35:8:43 | file_name | src/main.rs:8:21:8:44 | ...::from(...) | provenance | MaD:4 |
| src/main.rs:8:35:8:43 | file_name | src/main.rs:8:21:8:44 | ...::from(...) | provenance | MaD:5 |
| 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 |
| src/main.rs:37:11:37:19 | file_path | src/main.rs:40:52:40:60 | file_path | provenance | |
| src/main.rs:40:9:40:17 | file_path | src/main.rs:45:24:45:32 | file_path | provenance | |
| src/main.rs:40:21:40:62 | public_path.join(...) | src/main.rs:40:9:40:17 | file_path | provenance | |
| src/main.rs:40:38:40:61 | ...::from(...) | src/main.rs:40:21:40:62 | public_path.join(...) | provenance | MaD:3 |
| src/main.rs:40:52:40:60 | file_path | src/main.rs:40:38:40:61 | ...::from(...) | provenance | MaD:4 |
| src/main.rs:40:38:40:61 | ...::from(...) | src/main.rs:40:21:40:62 | public_path.join(...) | provenance | MaD:4 |
| src/main.rs:40:52:40:60 | file_path | src/main.rs:40:38:40:61 | ...::from(...) | provenance | MaD:5 |
| 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 |
| src/main.rs:50:11:50:19 | file_path | src/main.rs:53:52:53:60 | file_path | provenance | |
| src/main.rs:53:9:53:17 | file_path | src/main.rs:54:21:54:29 | file_path | provenance | |
| src/main.rs:53:9:53:17 | file_path | src/main.rs:54:21:54:44 | file_path.canonicalize(...) [Ok] | provenance | MaD:3 |
| src/main.rs:53:21:53:62 | public_path.join(...) | src/main.rs:53:9:53:17 | file_path | provenance | |
| src/main.rs:53:38:53:61 | ...::from(...) | src/main.rs:53:21:53:62 | public_path.join(...) | provenance | MaD:3 |
| src/main.rs:53:52:53:60 | file_path | src/main.rs:53:38:53:61 | ...::from(...) | provenance | MaD:4 |
| src/main.rs:53:38:53:61 | ...::from(...) | src/main.rs:53:21:53:62 | public_path.join(...) | provenance | MaD:4 |
| src/main.rs:53:52:53:60 | file_path | src/main.rs:53:38:53:61 | ...::from(...) | provenance | MaD:5 |
| src/main.rs:54:9:54:17 | file_path | src/main.rs:59:24:59:32 | file_path | provenance | |
| src/main.rs:54:21:54:29 | file_path | src/main.rs:54:21:54:44 | file_path.canonicalize(...) | provenance | Config |
| src/main.rs:54:21:54:44 | file_path.canonicalize(...) | src/main.rs:54:21:54:53 | ... .unwrap(...) | provenance | MaD:2 |
| src/main.rs:54:21:54:44 | file_path.canonicalize(...) [Ok] | src/main.rs:54:21:54:53 | ... .unwrap(...) | provenance | MaD:2 |
| src/main.rs:54:21:54:53 | ... .unwrap(...) | src/main.rs:54:9:54:17 | file_path | provenance | |
| 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 |
models
| 1 | Sink: lang:std; crate::fs::read_to_string; path-injection; Argument[0] |
| 2 | Summary: lang:core; <crate::result::Result>::unwrap; Argument[self]; ReturnValue; taint |
| 3 | Summary: lang:std; <crate::path::Path>::join; Argument[0]; ReturnValue; taint |
| 4 | Summary: lang:std; <crate::path::PathBuf as crate::convert::From>::from; Argument[0]; ReturnValue; taint |
| 2 | Summary: lang:core; <crate::result::Result>::unwrap; Argument[self].Field[crate::result::Result::Ok(0)]; ReturnValue; value |
| 3 | Summary: lang:std; <crate::path::Path>::canonicalize; Argument[self]; ReturnValue.Field[crate::result::Result::Ok(0)].OptionalStep[normalize-path]; taint |
| 4 | Summary: lang:std; <crate::path::Path>::join; Argument[0]; ReturnValue; taint |
| 5 | Summary: lang:std; <crate::path::PathBuf as crate::convert::From>::from; Argument[0]; ReturnValue; taint |
nodes
| src/main.rs:6:11:6:19 | file_name | semmle.label | file_name |
| src/main.rs:8:9:8:17 | file_path | semmle.label | file_path |
@@ -49,8 +49,7 @@ nodes
| src/main.rs:53:38:53:61 | ...::from(...) | semmle.label | ...::from(...) |
| src/main.rs:53:52:53:60 | file_path | semmle.label | file_path |
| src/main.rs:54:9:54:17 | file_path | semmle.label | file_path |
| src/main.rs:54:21:54:29 | file_path | semmle.label | file_path |
| src/main.rs:54:21:54:44 | file_path.canonicalize(...) | semmle.label | file_path.canonicalize(...) |
| src/main.rs:54:21:54:44 | file_path.canonicalize(...) [Ok] | semmle.label | file_path.canonicalize(...) [Ok] |
| src/main.rs:54:21:54:53 | ... .unwrap(...) | semmle.label | ... .unwrap(...) |
| src/main.rs:59:5:59:22 | ...::read_to_string | semmle.label | ...::read_to_string |
| src/main.rs:59:24:59:32 | file_path | semmle.label | file_path |