Rust: TaintedPath query #18960

aibaars · 2025-03-10T12:37:01Z

No description provided.

github-actions · 2025-03-10T12:37:57Z

QHelp previews:

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

Uncontrolled data used in path expression

Accessing paths controlled by users can allow an attacker to access unexpected resources. This can result in sensitive information being revealed or deleted, or an attacker being able to influence behavior by modifying unexpected files.

Paths that are naively constructed from data controlled by a user may be absolute paths, or may contain unexpected special characters such as "..". Such a path could point anywhere on the file system.

Recommendation

Validate user input before using it to construct a file path.

Common validation methods include checking that the normalized path is relative and does not contain any ".." components, or checking that the path is contained within a safe folder. The method you should use depends on how the path is used in the application, and whether the path should be a single path component.

If the path should be a single path component (such as a file name), you can check for the existence of any path separators ("/" or "\"), or ".." sequences in the input, and reject the input if any are found.

Note that removing "../" sequences is not sufficient, since the input could still contain a path separator followed by "..". For example, the input ".../...//" would still result in the string "../" if only "../" sequences are removed.

Finally, the simplest (but most restrictive) option is to use an allow list of safe patterns and make sure that the user input matches one of these patterns.

Example

In this example, a user-provided file name is read from a HTTP request and then used to access a file and send it back to the user. However, a malicious user could enter a file name anywhere on the file system, such as "/etc/passwd" or "../../../etc/passwd".

use poem::{error::InternalServerError, handler, web::Query, Result};
use std::{fs, path::PathBuf};

#[handler]
fn tainted_path_handler(Query(file_name): Query<String>) -> Result<String> {
    let file_path = PathBuf::from(file_name);
    // BAD: This could read any file on the filesystem.
    fs::read_to_string(file_path).map_err(InternalServerError)
}

If the input should only be a file name, you can check that it doesn't contain any path separators or ".." sequences.

use poem::{error::InternalServerError, handler, http::StatusCode, web::Query, Error, Result};
use std::{fs, path::PathBuf};

#[handler]
fn tainted_path_handler(Query(file_name): Query<String>) -> Result<String> {
    // 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)
}

If the input should be within a specific directory, you can check that the resolved path is still contained within that directory.

use poem::{error::InternalServerError, handler, http::StatusCode, web::Query, Error, Result};
use std::{env::home_dir, fs, path::PathBuf};

#[handler]
fn tainted_path_handler(Query(file_path): Query<String>) -> Result<String, Error> {
    let public_path = home_dir().unwrap().join("public");
    let file_path = public_path.join(PathBuf::from(file_path));
    let file_path = file_path.canonicalize().unwrap();
    // GOOD: ensure that the path stays within the public folder
    if !file_path.starts_with(public_path) {
        return Err(Error::from_status(StatusCode::BAD_REQUEST));
    }
    fs::read_to_string(file_path).map_err(InternalServerError)
}

References

OWASP: Path Traversal.
Common Weakness Enumeration: CWE-22.
Common Weakness Enumeration: CWE-23.
Common Weakness Enumeration: CWE-36.
Common Weakness Enumeration: CWE-73.
Common Weakness Enumeration: CWE-99.

Pull Request Overview

This pull request adds examples demonstrating both secure and insecure handling of file path tainting in Rust.

Introduces a secure example that validates a file name against path traversal in TaintedPathGoodNormalize.rs.
Implements a secure example that ensures file access remains within a designated public folder in TaintedPathGoodFolder.rs.
Provides an insecure example in TaintedPath.rs to illustrate unsafe file reading.
Updates model files to include new taint propagation rules.

Reviewed Changes

Copilot reviewed 9 out of 23 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
rust/ql/src/queries/security/CWE-022/examples/TaintedPathGoodNormalize.rs	Adds a secure file read example with filename validation.
rust/ql/src/queries/security/CWE-022/examples/TaintedPathGoodFolder.rs	Adds a secure file read example ensuring file stays within a public folder.
rust/ql/src/queries/security/CWE-022/examples/TaintedPath.rs	Adds an insecure file read example to demonstrate tainted file path usage.
rust/ql/lib/codeql/rust/frameworks/stdlib/fs.model.yml	Updates the model to include path-injection taint propagation for fs::read_to_string.
rust/ql/lib/codeql/rust/frameworks/stdlib/lang-core.model.yml	Adds a taint propagation rule for Result::unwrap.

Files not reviewed (14)

rust/ql/integration-tests/hello-project/summary.expected: Language not supported
rust/ql/integration-tests/hello-workspace/summary.cargo.expected: Language not supported
rust/ql/integration-tests/hello-workspace/summary.rust-project.expected: Language not supported
rust/ql/lib/codeql/rust/Concepts.qll: Language not supported
rust/ql/lib/codeql/rust/Frameworks.qll: Language not supported
rust/ql/lib/codeql/rust/dataflow/DataFlow.qll: Language not supported
rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll: Language not supported
rust/ql/lib/codeql/rust/frameworks/Poem.qll: Language not supported
rust/ql/lib/codeql/rust/frameworks/stdlib/Stdlib.qll: Language not supported
rust/ql/lib/codeql/rust/security/TaintedPathExtensions.qll: Language not supported
rust/ql/src/queries/security/CWE-022/TaintedPath.qhelp: Language not supported
rust/ql/src/queries/security/CWE-022/TaintedPath.ql: Language not supported
rust/ql/test/query-tests/security/CWE-022/TaintedPath.expected: Language not supported
rust/ql/test/query-tests/security/CWE-022/TaintedPath.qlref: Language not supported

Comments suppressed due to low confidence (1)

rust/ql/src/queries/security/CWE-022/examples/TaintedPathGoodFolder.rs:7

The variable 'file_path' shadows the function parameter; use a distinct variable name for clarity.

let file_path = public_path.join(PathBuf::from(file_path));

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

hvitved · 2025-03-13T19:13:44Z

rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll

@@ -212,7 +212,8 @@ predicate capturedCallWrite(Expr call, BasicBlock bb, int i, Variable v) {
 /** Holds if `v` may be mutably borrowed in `e`. */
 private predicate mutablyBorrows(Expr e, Variable v) {
  e = any(MethodCallExpr mc).getReceiver() and
-  e.(VariableAccess).getVariable() = v
+  e.(VariableAccess).getVariable() = v and
+  v.isMutable()


I think it would be nice to factor this bit out into a separate PR, with its own DCA run.

geoffw0

I've asked some questions, query and qhelp otherwise LGTM. The examples in the .qhelp are clear and helpful. 👍

Please do a DCA run and request a docs review when you're ready.

rust/ql/lib/codeql/rust/frameworks/Poem.qll

rust/ql/test/query-tests/security/CWE-022/src/main.rs

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

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

rust/ql/test/query-tests/security/CWE-022/src/main.rs

rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll

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

aibaars · 2025-03-18T11:12:23Z

@geoffw0 The QHelp is exactly the same as C# and Java, the only difference are the examples which are meant to resemble the C#/Java variants as closely as possible. See for example: https://codeql.github.com/codeql-query-help/csharp/cs-path-injection/ .

asgerf · 2025-03-18T11:37:34Z

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

@@ -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"]


Suggested change

- ["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].OptionalStep[normalize-path]", "ReturnValue.Field[crate::result::Result::Ok(0)]", "taint", "manual"]

Sorry, I put the optional step in the wrong place when I posted the code. It should be at the end of the input, not at the end of the output.

When I put the step in the "input" then I cannot get the isBarrier predicate to work properly.

Could you try with an OptionalBarrier on the normal summary? Otherwise feel free to use the original form and I'll revisit this when building shared support for optional steps.

Yes that works, see also: 2804c13

asgerf · 2025-03-18T12:00:04Z

rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

+    FlowSummaryImpl::Private::Steps::summaryReadStep(node1.(Node::FlowSummaryNode).getSummaryNode(),
+      TOptionalStep(name), node2.(Node::FlowSummaryNode).getSummaryNode()) or
+    FlowSummaryImpl::Private::Steps::summaryStoreStep(node1.(Node::FlowSummaryNode).getSummaryNode(),
+      TOptionalStep(name), node2.(Node::FlowSummaryNode).getSummaryNode())


Suggested change

FlowSummaryImpl::Private::Steps::summaryReadStep(node1.(Node::FlowSummaryNode).getSummaryNode(),

TOptionalStep(name), node2.(Node::FlowSummaryNode).getSummaryNode()) or

FlowSummaryImpl::Private::Steps::summaryStoreStep(node1.(Node::FlowSummaryNode).getSummaryNode(),

TOptionalStep(name), node2.(Node::FlowSummaryNode).getSummaryNode())

FlowSummaryImpl::Private::Steps::summaryReadStep(node1.(Node::FlowSummaryNode).getSummaryNode(),

TOptionalStep(name), node2.(Node::FlowSummaryNode).getSummaryNode())

With the previous suggestion, the store step should not be necessary.

rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

+  /**
+   * 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.
+   */


rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

+  /**
+   * 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.
+   */


yoff · 2025-03-19T10:40:14Z

Python 👍

hvitved · 2025-03-20T11:56:20Z

rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

@@ -1105,3 +1130,29 @@ private module Cached {
 }

 import Cached
+
+cached
+private module OptionalSteps {


This should be inside the existing Cached module to make sure that they are evaluated together.

geoffw0 · 2025-03-21T14:44:46Z

All of my concerns have been addressed, but I don't speak for other people's comments. I've taken the liberty of adding the ready-for-doc-review tag to get that aspect moving. Let me know if there's anything I can do to help get this merged.

github-actions bot added documentation Rust Pull requests that update Rust code labels Mar 10, 2025

aibaars force-pushed the aibaars/rust-tainted-path branch 5 times, most recently from 8890b12 to 4bf8103 Compare March 13, 2025 17:54

aibaars marked this pull request as ready for review March 13, 2025 17:54

Copilot bot review requested due to automatic review settings March 13, 2025 17:54

Copilot AI reviewed Mar 13, 2025

View reviewed changes

hvitved reviewed Mar 13, 2025

View reviewed changes

aibaars force-pushed the aibaars/rust-tainted-path branch 2 times, most recently from 11510a9 to 2daef7e Compare March 14, 2025 10:29

geoffw0 reviewed Mar 17, 2025

View reviewed changes

asgerf reviewed Mar 18, 2025

View reviewed changes

aibaars added 7 commits March 18, 2025 19:10

Rust: TaintedPath query

8223dde

Add a bit of modelling

4b5883a

Add QL test

0fd69ea

Rust: add Dataflow::BarrierGuard module

ecca805

Rust: tainted path implement basic sanitizers

f08d1d1

Rust: update integration test output

a3cc695

Rust: add missing QLDocs

81f954a

aibaars force-pushed the aibaars/rust-tainted-path branch from 586759f to 0724151 Compare March 18, 2025 18:46

aibaars requested a review from a team as a code owner March 18, 2025 18:46

github-actions bot added the Python label Mar 18, 2025

github-advanced-security bot found potential problems Mar 18, 2025

View reviewed changes

aibaars added the no-change-note-required This PR does not need a change note label Mar 20, 2025

aibaars added 3 commits March 20, 2025 11:16

Rust/Python improve qldoc of SafeAccessCheck

d3e2877

Refactor using OptionalStep

5a91b94

Rust: use optionalBarrier

2804c13

aibaars force-pushed the aibaars/rust-tainted-path branch from 0724151 to 2804c13 Compare March 20, 2025 10:37

Rust: remove Stage::ref() trick

f5fe531

hvitved requested changes Mar 20, 2025

View reviewed changes

aibaars added 2 commits March 20, 2025 13:10

Rust: move optionalStep/Barrier predicates into Cached module

efedfa1

Rust: add more path-injection sinks

b10a296

aibaars force-pushed the aibaars/rust-tainted-path branch from e3761c8 to b10a296 Compare March 20, 2025 15:49

geoffw0 added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Mar 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust: TaintedPath query #18960

Rust: TaintedPath query #18960

aibaars commented Mar 10, 2025

github-actions bot commented Mar 10, 2025 •

edited

Loading

Uncontrolled data used in path expression

Recommendation

Example

References

hvitved Mar 13, 2025

aibaars Mar 14, 2025

geoffw0 left a comment

aibaars commented Mar 18, 2025

asgerf Mar 18, 2025

aibaars Mar 18, 2025

asgerf Mar 18, 2025

aibaars Mar 20, 2025

asgerf Mar 18, 2025

yoff commented Mar 19, 2025

hvitved Mar 20, 2025

geoffw0 commented Mar 21, 2025

	- ["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].OptionalStep[normalize-path]", "ReturnValue.Field[crate::result::Result::Ok(0)]", "taint", "manual"]

Rust: TaintedPath query #18960

Are you sure you want to change the base?

Rust: TaintedPath query #18960

Conversation

aibaars commented Mar 10, 2025

github-actions bot commented Mar 10, 2025 • edited Loading

Uncontrolled data used in path expression

Recommendation

Example

References

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffw0 left a comment

Choose a reason for hiding this comment

aibaars commented Mar 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoff commented Mar 19, 2025

Choose a reason for hiding this comment

geoffw0 commented Mar 21, 2025

github-actions bot commented Mar 10, 2025 •

edited

Loading