diff --git a/rust/ql/lib/codeql/rust/frameworks/stdlib/Stdlib.qll b/rust/ql/lib/codeql/rust/frameworks/stdlib/Stdlib.qll index 820c6330c25e..114163efc9cf 100644 --- a/rust/ql/lib/codeql/rust/frameworks/stdlib/Stdlib.qll +++ b/rust/ql/lib/codeql/rust/frameworks/stdlib/Stdlib.qll @@ -14,7 +14,7 @@ private import codeql.rust.internal.PathResolution */ private class StartswithCall extends Path::SafeAccessCheck::Range, CfgNodes::MethodCallExprCfgNode { StartswithCall() { - this.getAstNode().(Resolvable).getResolvedPath() = "::starts_with" + this.getMethodCallExpr().getStaticTarget().getCanonicalPath() = "::starts_with" } override predicate checks(Cfg::CfgNode e, boolean branch) { diff --git a/rust/ql/test/query-tests/security/CWE-022/TaintedPathSinks.ql b/rust/ql/test/query-tests/security/CWE-022/TaintedPathSinks.ql index 66345376de7b..8660d779e2f0 100644 --- a/rust/ql/test/query-tests/security/CWE-022/TaintedPathSinks.ql +++ b/rust/ql/test/query-tests/security/CWE-022/TaintedPathSinks.ql @@ -1,9 +1,18 @@ import rust import codeql.rust.security.TaintedPathExtensions import utils.test.InlineExpectationsTest +import codeql.rust.dataflow.DataFlow +import codeql.rust.dataflow.internal.DataFlowImpl as DataflowImpl +import codeql.rust.Concepts module TaintedPathSinksTest implements TestSig { - string getARelevantTag() { result = "path-injection-sink" } + string getARelevantTag() { + result = + [ + "path-injection-sink", "path-injection-barrier", "path-injection-normalize", + "path-injection-checked" + ] + } predicate hasActualResult(Location location, string element, string tag, string value) { exists(TaintedPath::Sink sink | @@ -13,6 +22,36 @@ module TaintedPathSinksTest implements TestSig { tag = "path-injection-sink" and value = "" ) + or + exists(DataFlow::Node node | + ( + node instanceof TaintedPath::Barrier or + node instanceof TaintedPath::SanitizerGuard // tends to label the node *after* the check + ) and + location = node.getLocation() and + location.getFile().getBaseName() != "" and + element = node.toString() and + tag = "path-injection-barrier" and + value = "" + ) + or + exists(DataFlow::Node node | + DataflowImpl::optionalBarrier(node, "normalize-path") and + location = node.getLocation() and + location.getFile().getBaseName() != "" and + element = node.toString() and + tag = "path-injection-normalize" and + value = "" + ) + or + exists(DataFlow::Node node | + node instanceof Path::SafeAccessCheck and // tends to label the node *after* the check + location = node.getLocation() and + location.getFile().getBaseName() != "" and + element = node.toString() and + tag = "path-injection-checked" and + value = "" + ) } } 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 972ac8e7b6a0..6f8c73654c5e 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 @@ -13,10 +13,10 @@ fn tainted_path_handler_bad( //#[handler] 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("\\") { + if file_name.contains("..") || file_name.contains("/") || file_name.contains("\\") { // $ path-injection-barrier return Err(Error::from_status(StatusCode::BAD_REQUEST)); } - let file_path = PathBuf::from(file_name); + let file_path = PathBuf::from(file_name); // $ path-injection-barrier (following the last `.contains` check) fs::read_to_string(file_path).map_err(InternalServerError) // $ path-injection-sink } @@ -29,12 +29,12 @@ fn tainted_path_handler_folder_good(Query(file_path): Query) -> Result, // $ MISSING: Source=remote4 + Query(file_path): Query, // $ MISSING: Source=remote2 ) -> Result { let public_path = PathBuf::from("/var/www/public_html"); let file_path = public_path.join(PathBuf::from(file_path)); @@ -42,12 +42,37 @@ fn tainted_path_handler_folder_almost_good1( if !file_path.starts_with(public_path) { return Err(Error::from_status(StatusCode::BAD_REQUEST)); } - fs::read_to_string(file_path).map_err(InternalServerError) // $ path-injection-sink MISSING: Alert[rust/path-injection]=remote4 -- we cannot resolve the `join` call above, because it needs a `PathBuf -> Path` `Deref` + fs::read_to_string(file_path).map_err(InternalServerError) // $ path-injection-sink MISSING: path-injection-checked Alert[rust/path-injection]=remote2 -- we cannot resolve the `join` call above, because it needs a `PathBuf -> Path` `Deref` +} + +//#[handler] +fn tainted_path_handler_folder_good_simpler(Query(file_path): Query) -> Result { + let public_path = "/var/www/public_html"; + let file_path = Path::new(&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) // $ path-injection-sink MISSING: path-injection-checked +} + +//#[handler] +fn tainted_path_handler_folder_almost_good1_simpler( + Query(file_path): Query, // $ MISSING: Source=remote3 +) -> Result { + let public_path = "/var/www/public_html"; + let file_path = Path::new(&file_path); + // BAD: the path could still contain `..` and escape 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) // $ path-injection-checked path-injection-sink MISSING: Alert[rust/path-injection]=remote3 } //#[handler] fn tainted_path_handler_folder_almost_good2( - Query(file_path): Query, // $ MISSING: Source=remote5 + Query(file_path): Query, // $ MISSING: Source=remote4 ) -> Result { let public_path = PathBuf::from("/var/www/public_html"); let file_path = public_path.join(PathBuf::from(file_path)); @@ -56,7 +81,21 @@ fn tainted_path_handler_folder_almost_good2( if file_path.starts_with(public_path) { return Err(Error::from_status(StatusCode::BAD_REQUEST)); } - fs::read_to_string(file_path).map_err(InternalServerError) // $ path-injection-sink MISSING: Alert[rust/path-injection]=remote5 -- we cannot resolve the `join` call above, because it needs a `PathBuf -> Path` `Deref` + fs::read_to_string(file_path).map_err(InternalServerError) // $ path-injection-sink MISSING: path-injection-checked Alert[rust/path-injection]=remote4 -- we cannot resolve the `join` call above, because it needs a `PathBuf -> Path` `Deref` +} + +//#[handler] +fn tainted_path_handler_folder_almost_good3( + Query(file_path): Query, // $ MISSING: Source=remote5 +) -> Result { + let public_path = "/var/www/public_html"; + let file_path = Path::new(&file_path); + // BAD: the starts_with check is ineffective before canonicalization, the path could still contain `..` + if !file_path.starts_with(public_path) { + return Err(Error::from_status(StatusCode::BAD_REQUEST)); + } + let file_path = file_path.canonicalize().unwrap(); // $ path-injection-checked + fs::read_to_string(file_path).map_err(InternalServerError) // $ path-injection-sink MISSING: Alert[rust/path-injection]=remote5 } fn sinks(path1: &Path, path2: &Path) {