Merge pull request #18960 from github/aibaars/rust-tainted-path

Rust: TaintedPath query

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

@@ -0,0 +1,67 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>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.</p>
+
+<p>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.</p>
+
+</overview>
+<recommendation>
+
+<p>Validate user input before using it to construct a file path.</p>
+
+<p>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.
+</p>
+
+<p>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.
+</p>
+
+<p>
+Note that removing "../" sequences is <i>not</i> 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.
+</p>
+
+<p>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.</p>
+
+</recommendation>
+<example>
+
+<p>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".</p>
+
+<sample src="examples/TaintedPath.rs" />
+
+<p>
+If the input should only be a file name, you can check that it doesn't contain any path separators or ".." sequences.
+</p>
+
+<sample src="examples/TaintedPathGoodNormalize.rs" />
+
+<p>
+If the input should be within a specific directory, you can check that the resolved path 
+is still contained within that directory.
+</p>
+
+<sample src="examples/TaintedPathGoodFolder.rs" />
+
+</example>
+<references>
+
+<li>
+OWASP:
+<a href="https://owasp.org/www-community/attacks/Path_Traversal">Path Traversal</a>.
+</li>
+
+</references>
+</qhelp>

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

@@ -0,0 +1,90 @@
+/**
+ * @name Uncontrolled data used in path expression
+ * @description Accessing paths influenced by users can allow an attacker to access unexpected resources.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 7.5
+ * @precision high
+ * @id rust/path-injection
+ * @tags security
+ *       external/cwe/cwe-022
+ *       external/cwe/cwe-023
+ *       external/cwe/cwe-036
+ *       external/cwe/cwe-073
+ *       external/cwe/cwe-099
+ */
+
+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
+private import codeql.rust.Concepts
+
+newtype NormalizationState =
+  /** A state signifying that the file path has not been normalized. */
+  NotNormalized() or
+  /** A state signifying that the file path has been normalized, but not checked. */
+  NormalizedUnchecked()
+
+/**
+ * This configuration uses two flow states, `NotNormalized` and `NormalizedUnchecked`,
+ * to track the requirement that a file path must be first normalized and then checked
+ * before it is safe to use.
+ *
+ * At sources, paths are assumed not normalized. At normalization points, they change
+ * state to `NormalizedUnchecked` after which they can be made safe by an appropriate
+ * check of the prefix.
+ *
+ * Such checks are ineffective in the `NotNormalized` state.
+ */
+module TaintedPathConfig implements DataFlow::StateConfigSig {
+  class FlowState = NormalizationState;
+
+  predicate isSource(DataFlow::Node source, FlowState state) {
+    source instanceof TaintedPath::Source and state instanceof NotNormalized
+  }
+
+  predicate isSink(DataFlow::Node sink, FlowState state) {
+    sink instanceof TaintedPath::Sink and
+    (
+      state instanceof NotNormalized or
+      state instanceof NormalizedUnchecked
+    )
+  }
+
+  predicate isBarrier(DataFlow::Node node) {
+    node instanceof TaintedPath::Barrier or node instanceof TaintedPath::SanitizerGuard
+  }
+
+  predicate isBarrier(DataFlow::Node node, FlowState state) {
+    // Block `NotNormalized` paths here, since they change state to `NormalizedUnchecked`
+    (
+      node instanceof Path::PathNormalization or
+      DataflowImpl::optionalBarrier(node, "normalize-path")
+    ) and
+    state instanceof NotNormalized
+    or
+    node instanceof Path::SafeAccessCheck and
+    state instanceof NormalizedUnchecked
+  }
+
+  predicate isAdditionalFlowStep(
+    DataFlow::Node nodeFrom, FlowState stateFrom, DataFlow::Node nodeTo, FlowState stateTo
+  ) {
+    (
+      nodeFrom = nodeTo.(Path::PathNormalization).getPathArg() or
+      DataflowImpl::optionalStep(nodeFrom, "normalize-path", nodeTo)
+    ) and
+    stateFrom instanceof NotNormalized and
+    stateTo instanceof NormalizedUnchecked
+  }
+}
+
+module TaintedPathFlow = TaintTracking::GlobalWithState<TaintedPathConfig>;
+
+from TaintedPathFlow::PathNode source, TaintedPathFlow::PathNode sink
+where TaintedPathFlow::flowPath(source, sink)
+select sink.getNode(), source, sink, "This path depends on a $@.", source.getNode(),
+  "user-provided value"

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

@@ -0,0 +1,9 @@
+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)
+}

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

@@ -0,0 +1,14 @@
+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)
+}

rust/ql/src/queries/security/CWE-022/examples/TaintedPathGoodNormalize.rs

@@ -0,0 +1,12 @@
+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)
+}