From cd6975f7b743b19e533bf97de09f54ad06e454ca Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 17 Jun 2025 16:18:47 +0100 Subject: [PATCH 1/2] Rust: Update DotDotCheck from getResolvedPath -> getCanonicalPath. --- .../codeql/rust/security/TaintedPathExtensions.qll | 3 ++- .../security/CWE-022/TaintedPath.expected | 13 +++++++++++++ .../test/query-tests/security/CWE-022/src/main.rs | 4 ++-- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/rust/ql/lib/codeql/rust/security/TaintedPathExtensions.qll b/rust/ql/lib/codeql/rust/security/TaintedPathExtensions.qll index 5f8d8b77ee82..016d79e840fc 100644 --- a/rust/ql/lib/codeql/rust/security/TaintedPathExtensions.qll +++ b/rust/ql/lib/codeql/rust/security/TaintedPathExtensions.qll @@ -69,7 +69,8 @@ module SanitizerGuard { */ private class DotDotCheck extends SanitizerGuard::Range, CfgNodes::MethodCallExprCfgNode { DotDotCheck() { - this.getAstNode().(Resolvable).getResolvedPath() = "::contains" and + this.getAstNode().(CallExprBase).getStaticTarget().(Addressable).getCanonicalPath() = + "alloc::string::String::contains" and this.getArgument(0).getAstNode().(LiteralExpr).getTextValue() = ["\"..\"", "\"../\"", "\"..\\\""] } diff --git a/rust/ql/test/query-tests/security/CWE-022/TaintedPath.expected b/rust/ql/test/query-tests/security/CWE-022/TaintedPath.expected index 7d8bb23d4c59..d2d38c18ec06 100644 --- a/rust/ql/test/query-tests/security/CWE-022/TaintedPath.expected +++ b/rust/ql/test/query-tests/security/CWE-022/TaintedPath.expected @@ -1,5 +1,6 @@ #select | src/main.rs:10:5:10:22 | ...::read_to_string | src/main.rs:6:11:6:19 | file_name | src/main.rs:10:5:10:22 | ...::read_to_string | This path depends on a $@. | src/main.rs:6:11:6:19 | file_name | user-provided value | +| src/main.rs:20:5:20:22 | ...::read_to_string | src/main.rs:14:36:14:44 | file_name | src/main.rs:20:5:20:22 | ...::read_to_string | This path depends on a $@. | src/main.rs:14:36:14:44 | file_name | user-provided value | | src/main.rs:45:5:45:22 | ...::read_to_string | src/main.rs:37:11:37:19 | file_path | src/main.rs:45:5:45:22 | ...::read_to_string | This path depends on a $@. | src/main.rs:37:11:37:19 | file_path | user-provided value | | src/main.rs:59:5:59:22 | ...::read_to_string | src/main.rs:50:11:50:19 | file_path | src/main.rs:59:5:59:22 | ...::read_to_string | This path depends on a $@. | src/main.rs:50:11:50:19 | file_path | user-provided value | edges @@ -9,6 +10,12 @@ edges | 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:4 | | 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:14:36:14:44 | file_name | src/main.rs:19:35:19:43 | file_name | provenance | | +| src/main.rs:19:9:19:17 | file_path | src/main.rs:20:24:20:32 | file_path | provenance | | +| src/main.rs:19:21:19:44 | ...::from(...) | src/main.rs:19:9:19:17 | file_path | provenance | | +| src/main.rs:19:35:19:43 | file_name | src/main.rs:19:21:19:44 | ...::from(...) | provenance | MaD:4 | +| src/main.rs:19:35:19:43 | file_name | src/main.rs:19:21:19:44 | ...::from(...) | provenance | MaD:4 | +| src/main.rs:20:24:20:32 | file_path | src/main.rs:20:5:20: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 | | @@ -38,6 +45,12 @@ nodes | src/main.rs:8:35:8:43 | file_name | semmle.label | file_name | | src/main.rs:10:5:10:22 | ...::read_to_string | semmle.label | ...::read_to_string | | src/main.rs:10:24:10:32 | file_path | semmle.label | file_path | +| src/main.rs:14:36:14:44 | file_name | semmle.label | file_name | +| src/main.rs:19:9:19:17 | file_path | semmle.label | file_path | +| src/main.rs:19:21:19:44 | ...::from(...) | semmle.label | ...::from(...) | +| src/main.rs:19:35:19:43 | file_name | semmle.label | file_name | +| src/main.rs:20:5:20:22 | ...::read_to_string | semmle.label | ...::read_to_string | +| src/main.rs:20:24:20:32 | file_path | semmle.label | file_path | | src/main.rs:37:11:37:19 | file_path | semmle.label | file_path | | src/main.rs:40:9:40:17 | file_path | semmle.label | file_path | | src/main.rs:40:21:40:62 | public_path.join(...) | semmle.label | public_path.join(...) | diff --git a/rust/ql/test/query-tests/security/CWE-022/src/main.rs b/rust/ql/test/query-tests/security/CWE-022/src/main.rs index 7c13da08db50..7882060230df 100644 --- a/rust/ql/test/query-tests/security/CWE-022/src/main.rs +++ b/rust/ql/test/query-tests/security/CWE-022/src/main.rs @@ -11,13 +11,13 @@ fn tainted_path_handler_bad( } //#[handler] -fn tainted_path_handler_good(Query(file_name): Query) -> Result { +fn tainted_path_handler_good(Query(file_name): Query) -> Result { // $ SPURIOUS: Source=remote2 // GOOD: ensure that the filename has no path separators or parent directory references if file_name.contains("..") || file_name.contains("/") || file_name.contains("\\") { return Err(Error::from_status(StatusCode::BAD_REQUEST)); } let file_path = PathBuf::from(file_name); - fs::read_to_string(file_path).map_err(InternalServerError) // $ path-injection-sink + fs::read_to_string(file_path).map_err(InternalServerError) // $ path-injection-sink SPURIOUS: Alert[rust/path-injection]=remote2 } //#[handler] From 7f659804e44913a4b7fd701de587a66571b0fb39 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 24 Jul 2025 17:20:25 +0100 Subject: [PATCH 2/2] Rust: Fix the canonical path. --- .../codeql/rust/security/TaintedPathExtensions.qll | 2 +- .../security/CWE-022/TaintedPath.expected | 13 ------------- .../test/query-tests/security/CWE-022/src/main.rs | 4 ++-- 3 files changed, 3 insertions(+), 16 deletions(-) diff --git a/rust/ql/lib/codeql/rust/security/TaintedPathExtensions.qll b/rust/ql/lib/codeql/rust/security/TaintedPathExtensions.qll index 016d79e840fc..9310999bd3dd 100644 --- a/rust/ql/lib/codeql/rust/security/TaintedPathExtensions.qll +++ b/rust/ql/lib/codeql/rust/security/TaintedPathExtensions.qll @@ -70,7 +70,7 @@ module SanitizerGuard { private class DotDotCheck extends SanitizerGuard::Range, CfgNodes::MethodCallExprCfgNode { DotDotCheck() { this.getAstNode().(CallExprBase).getStaticTarget().(Addressable).getCanonicalPath() = - "alloc::string::String::contains" and + ["::contains", "::contains"] and this.getArgument(0).getAstNode().(LiteralExpr).getTextValue() = ["\"..\"", "\"../\"", "\"..\\\""] } diff --git a/rust/ql/test/query-tests/security/CWE-022/TaintedPath.expected b/rust/ql/test/query-tests/security/CWE-022/TaintedPath.expected index d83305ff1346..60847b71b798 100644 --- a/rust/ql/test/query-tests/security/CWE-022/TaintedPath.expected +++ b/rust/ql/test/query-tests/security/CWE-022/TaintedPath.expected @@ -1,6 +1,5 @@ #select | src/main.rs:10:5:10:22 | ...::read_to_string | src/main.rs:6:11:6:19 | file_name | src/main.rs:10:5:10:22 | ...::read_to_string | This path depends on a $@. | src/main.rs:6:11:6:19 | file_name | user-provided value | -| src/main.rs:20:5:20:22 | ...::read_to_string | src/main.rs:14:36:14:44 | file_name | src/main.rs:20:5:20:22 | ...::read_to_string | This path depends on a $@. | src/main.rs:14:36:14:44 | file_name | user-provided value | 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 | | @@ -8,12 +7,6 @@ edges | src/main.rs:8:35:8:43 | file_name | src/main.rs:8:21:8:44 | ...::from(...) | provenance | MaD:2 | | src/main.rs:8:35:8:43 | file_name | src/main.rs:8:21:8:44 | ...::from(...) | provenance | MaD:2 | | 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:14:36:14:44 | file_name | src/main.rs:19:35:19:43 | file_name | provenance | | -| src/main.rs:19:9:19:17 | file_path | src/main.rs:20:24:20:32 | file_path | provenance | | -| src/main.rs:19:21:19:44 | ...::from(...) | src/main.rs:19:9:19:17 | file_path | provenance | | -| src/main.rs:19:35:19:43 | file_name | src/main.rs:19:21:19:44 | ...::from(...) | provenance | MaD:2 | -| src/main.rs:19:35:19:43 | file_name | src/main.rs:19:21:19:44 | ...::from(...) | provenance | MaD:2 | -| src/main.rs:20:24:20:32 | file_path | src/main.rs:20:5:20:22 | ...::read_to_string | provenance | MaD:1 Sink:MaD:1 | models | 1 | Sink: std::fs::read_to_string; Argument[0]; path-injection | | 2 | Summary: ::from; Argument[0]; ReturnValue; taint | @@ -24,10 +17,4 @@ nodes | src/main.rs:8:35:8:43 | file_name | semmle.label | file_name | | src/main.rs:10:5:10:22 | ...::read_to_string | semmle.label | ...::read_to_string | | src/main.rs:10:24:10:32 | file_path | semmle.label | file_path | -| src/main.rs:14:36:14:44 | file_name | semmle.label | file_name | -| src/main.rs:19:9:19:17 | file_path | semmle.label | file_path | -| src/main.rs:19:21:19:44 | ...::from(...) | semmle.label | ...::from(...) | -| src/main.rs:19:35:19:43 | file_name | semmle.label | file_name | -| src/main.rs:20:5:20:22 | ...::read_to_string | semmle.label | ...::read_to_string | -| src/main.rs:20:24:20:32 | file_path | semmle.label | file_path | subpaths diff --git a/rust/ql/test/query-tests/security/CWE-022/src/main.rs b/rust/ql/test/query-tests/security/CWE-022/src/main.rs index 80076230c864..972ac8e7b6a0 100644 --- a/rust/ql/test/query-tests/security/CWE-022/src/main.rs +++ b/rust/ql/test/query-tests/security/CWE-022/src/main.rs @@ -11,13 +11,13 @@ fn tainted_path_handler_bad( } //#[handler] -fn tainted_path_handler_good(Query(file_name): Query) -> Result { // $ SPURIOUS: Source=remote2 +fn tainted_path_handler_good(Query(file_name): Query) -> Result { // GOOD: ensure that the filename has no path separators or parent directory references if file_name.contains("..") || file_name.contains("/") || file_name.contains("\\") { return Err(Error::from_status(StatusCode::BAD_REQUEST)); } let file_path = PathBuf::from(file_name); - fs::read_to_string(file_path).map_err(InternalServerError) // $ path-injection-sink SPURIOUS: Alert[rust/path-injection]=remote2 + fs::read_to_string(file_path).map_err(InternalServerError) // $ path-injection-sink } //#[handler]