Merge pull request #20064 from Kwstubbs/go-path-separator

Update Go Path Injection Sanitizer and Sink

Author: owen-mc

go/ql/lib/change-notes/2025-07-15-path-injection-sanitizers.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* The second argument of the `CreateTemp` function, from the `os` package, is no longer a path-injection sink due to proper sanitization by Go.
+* The query "Uncontrolled data used in path expression" (`go/path-injection`) now detects sanitizing a path by adding `os.PathSeparator` or `\` to the beginning.

go/ql/lib/ext/os.model.yml

@@ -28,7 +28,7 @@ extensions:
       - ["os", "", False, "ReadDir", "", "", "Argument[0]", "path-injection", "manual"]
       - ["os", "", False, "ReadFile", "", "", "Argument[0]", "path-injection", "manual"]
       - ["os", "", False, "MkdirTemp", "", "", "Argument[0..1]", "path-injection", "manual"]
-      - ["os", "", False, "CreateTemp", "", "", "Argument[0..1]", "path-injection", "manual"]
+      - ["os", "", False, "CreateTemp", "", "", "Argument[0]", "path-injection", "manual"]
       - ["os", "", False, "WriteFile", "", "", "Argument[0]", "path-injection", "manual"]
       # command-injection
       - ["os", "", False, "StartProcess", "", "", "Argument[0]", "command-injection", "manual"]

go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll

@@ -87,7 +87,7 @@ module TaintedPath {
       exists(DataFlow::CallNode cleanCall, StringOps::Concatenation concatNode |
         cleanCall = any(Function f | f.hasQualifiedName("path/filepath", "Clean")).getACall() and
         concatNode = cleanCall.getArgument(0) and
-        concatNode.getOperand(0).asExpr().(StringLit).getValue() = "/" and
+        concatNode.getOperand(0).getStringValue().prefix(1) = ["/", "\\"] and
         this = cleanCall.getResult()
       )
     }

go/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/Os.go

@@ -1,25 +1,25 @@
 #select
-| TaintedPath.go:17:29:17:40 | tainted_path | TaintedPath.go:14:18:14:22 | selection of URL | TaintedPath.go:17:29:17:40 | tainted_path | This path depends on a $@. | TaintedPath.go:14:18:14:22 | selection of URL | user-provided value |
-| TaintedPath.go:21:28:21:69 | call to Join | TaintedPath.go:14:18:14:22 | selection of URL | TaintedPath.go:21:28:21:69 | call to Join | This path depends on a $@. | TaintedPath.go:14:18:14:22 | selection of URL | user-provided value |
-| TaintedPath.go:68:28:68:57 | call to Clean | TaintedPath.go:14:18:14:22 | selection of URL | TaintedPath.go:68:28:68:57 | call to Clean | This path depends on a $@. | TaintedPath.go:14:18:14:22 | selection of URL | user-provided value |
+| TaintedPath.go:18:29:18:40 | tainted_path | TaintedPath.go:15:18:15:22 | selection of URL | TaintedPath.go:18:29:18:40 | tainted_path | This path depends on a $@. | TaintedPath.go:15:18:15:22 | selection of URL | user-provided value |
+| TaintedPath.go:22:28:22:69 | call to Join | TaintedPath.go:15:18:15:22 | selection of URL | TaintedPath.go:22:28:22:69 | call to Join | This path depends on a $@. | TaintedPath.go:15:18:15:22 | selection of URL | user-provided value |
+| TaintedPath.go:74:28:74:57 | call to Clean | TaintedPath.go:15:18:15:22 | selection of URL | TaintedPath.go:74:28:74:57 | call to Clean | This path depends on a $@. | TaintedPath.go:15:18:15:22 | selection of URL | user-provided value |
 edges
-| TaintedPath.go:14:18:14:22 | selection of URL | TaintedPath.go:14:18:14:30 | call to Query | provenance | Src:MaD:2 MaD:3 |
-| TaintedPath.go:14:18:14:30 | call to Query | TaintedPath.go:17:29:17:40 | tainted_path | provenance | Sink:MaD:1 |
-| TaintedPath.go:14:18:14:30 | call to Query | TaintedPath.go:21:57:21:68 | tainted_path | provenance |  |
-| TaintedPath.go:14:18:14:30 | call to Query | TaintedPath.go:68:39:68:56 | ...+... | provenance |  |
-| TaintedPath.go:21:57:21:68 | tainted_path | TaintedPath.go:21:28:21:69 | call to Join | provenance | FunctionModel Sink:MaD:1 |
-| TaintedPath.go:68:39:68:56 | ...+... | TaintedPath.go:68:28:68:57 | call to Clean | provenance | MaD:4 Sink:MaD:1 |
+| TaintedPath.go:15:18:15:22 | selection of URL | TaintedPath.go:15:18:15:30 | call to Query | provenance | Src:MaD:2 MaD:3 |
+| TaintedPath.go:15:18:15:30 | call to Query | TaintedPath.go:18:29:18:40 | tainted_path | provenance | Sink:MaD:1 |
+| TaintedPath.go:15:18:15:30 | call to Query | TaintedPath.go:22:57:22:68 | tainted_path | provenance |  |
+| TaintedPath.go:15:18:15:30 | call to Query | TaintedPath.go:74:39:74:56 | ...+... | provenance |  |
+| TaintedPath.go:22:57:22:68 | tainted_path | TaintedPath.go:22:28:22:69 | call to Join | provenance | FunctionModel Sink:MaD:1 |
+| TaintedPath.go:74:39:74:56 | ...+... | TaintedPath.go:74:28:74:57 | call to Clean | provenance | MaD:4 Sink:MaD:1 |
 models
 | 1 | Sink: io/ioutil; ; false; ReadFile; ; ; Argument[0]; path-injection; manual |
 | 2 | Source: net/http; Request; true; URL; ; ; ; remote; manual |
 | 3 | Summary: net/url; URL; true; Query; ; ; Argument[receiver]; ReturnValue; taint; manual |
 | 4 | Summary: path; ; false; Clean; ; ; Argument[0]; ReturnValue; taint; manual |
 nodes
-| TaintedPath.go:14:18:14:22 | selection of URL | semmle.label | selection of URL |
-| TaintedPath.go:14:18:14:30 | call to Query | semmle.label | call to Query |
-| TaintedPath.go:17:29:17:40 | tainted_path | semmle.label | tainted_path |
-| TaintedPath.go:21:28:21:69 | call to Join | semmle.label | call to Join |
-| TaintedPath.go:21:57:21:68 | tainted_path | semmle.label | tainted_path |
-| TaintedPath.go:68:28:68:57 | call to Clean | semmle.label | call to Clean |
-| TaintedPath.go:68:39:68:56 | ...+... | semmle.label | ...+... |
+| TaintedPath.go:15:18:15:22 | selection of URL | semmle.label | selection of URL |
+| TaintedPath.go:15:18:15:30 | call to Query | semmle.label | call to Query |
+| TaintedPath.go:18:29:18:40 | tainted_path | semmle.label | tainted_path |
+| TaintedPath.go:22:28:22:69 | call to Join | semmle.label | call to Join |
+| TaintedPath.go:22:57:22:68 | tainted_path | semmle.label | tainted_path |
+| TaintedPath.go:74:28:74:57 | call to Clean | semmle.label | call to Clean |
+| TaintedPath.go:74:39:74:56 | ...+... | semmle.label | ...+... |
 subpaths

go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected

@@ -4,6 +4,7 @@ import (
 	"io/ioutil"
 	"mime/multipart"
 	"net/http"
+	"os"
 	"path"
 	"path/filepath"
 	"regexp"
@@ -63,6 +64,11 @@ func handler(w http.ResponseWriter, r *http.Request) {
 	data, _ = ioutil.ReadFile(filepath.Clean("/" + tainted_path))
 	w.Write(data)
 
+	// GOOD: Sanitized by filepath.Clean with a prepended os.PathSeparator forcing interpretation
+	// as an absolute path, so that Clean will throw away any leading `..` components.
+	data, _ = ioutil.ReadFile(filepath.Clean(string(os.PathSeparator) + "hardcoded" + tainted_path))
+	w.Write(data)
+
 	// BAD: Sanitized by path.Clean with a prepended '/' forcing interpretation
 	// as an absolute path, however is not sufficient for Windows paths.
 	data, _ = ioutil.ReadFile(path.Clean("/" + tainted_path))