From 504ae0f35ab9d19265cbbdd8547e814519f1dd5a Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Wed, 16 Jul 2025 06:12:45 +0000 Subject: [PATCH 1/2] Update go path sanitizers and sinks --- .../2025-07-15-path-injection-sanitizers.md | 4 ++++ go/ql/lib/ext/os.model.yml | 1 - .../query-tests/Security/CWE-022/TaintedPath.expected | 10 +++++----- go/ql/test/query-tests/Security/CWE-022/TaintedPath.go | 5 +++-- 4 files changed, 12 insertions(+), 8 deletions(-) create mode 100644 go/ql/lib/change-notes/2025-07-15-path-injection-sanitizers.md diff --git a/go/ql/lib/change-notes/2025-07-15-path-injection-sanitizers.md b/go/ql/lib/change-notes/2025-07-15-path-injection-sanitizers.md new file mode 100644 index 000000000000..e4ff7224ad2f --- /dev/null +++ b/go/ql/lib/change-notes/2025-07-15-path-injection-sanitizers.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Remove model`CreateTemp` function, from the `os` package, as a path-injection sink due to proper sanitization by Go. Add check for `os.PathSeparator` in sanitizers for path-injection query. \ No newline at end of file diff --git a/go/ql/lib/ext/os.model.yml b/go/ql/lib/ext/os.model.yml index b4f074146b79..66316b4ff35b 100644 --- a/go/ql/lib/ext/os.model.yml +++ b/go/ql/lib/ext/os.model.yml @@ -28,7 +28,6 @@ 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, "WriteFile", "", "", "Argument[0]", "path-injection", "manual"] # command-injection - ["os", "", False, "StartProcess", "", "", "Argument[0]", "command-injection", "manual"] diff --git a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected index 839d35f663ce..fc6e39f697de 100644 --- a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected +++ b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected @@ -1,14 +1,14 @@ #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:69:28:69:57 | call to Clean | TaintedPath.go:14:18:14:22 | selection of URL | TaintedPath.go:69:28:69:57 | call to Clean | This path depends on a $@. | TaintedPath.go:14:18:14: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:14:18:14:30 | call to Query | TaintedPath.go:69:39:69: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:69:39:69:56 | ...+... | TaintedPath.go:69:28:69: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 | @@ -20,6 +20,6 @@ nodes | 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:69:28:69:57 | call to Clean | semmle.label | call to Clean | +| TaintedPath.go:69:39:69:56 | ...+... | semmle.label | ...+... | subpaths diff --git a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go index e6a1c49f4c5b..99b3a29741b4 100644 --- a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go +++ b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go @@ -8,8 +8,8 @@ import ( "path/filepath" "regexp" "strings" + "os" ) - func handler(w http.ResponseWriter, r *http.Request) { tainted_path := r.URL.Query()["path"][0] @@ -58,9 +58,10 @@ func handler(w http.ResponseWriter, r *http.Request) { w.Write(data) } - // GOOD: Sanitized by filepath.Clean with a prepended '/' forcing interpretation + // GOOD: Sanitized by filepath.Clean with a prepended '/' or os.PathSeparator forcing interpretation // as an absolute path, so that Clean will throw away any leading `..` components. data, _ = ioutil.ReadFile(filepath.Clean("/" + tainted_path)) + data, _ = ioutil.ReadFile(filepath.Clean(string(os.PathSeparator) + tainted_path)) w.Write(data) // BAD: Sanitized by path.Clean with a prepended '/' forcing interpretation From f86152d3bd45e57cd2b64aacd7bd28e3c3317ebb Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Wed, 16 Jul 2025 21:27:33 +0000 Subject: [PATCH 2/2] Add sanitizer changes and fix test --- .../2025-07-15-path-injection-sanitizers.md | 2 +- .../go/security/TaintedPathCustomizations.qll | 9 +++++- .../Security/CWE-022/TaintedPath.expected | 32 +++++++++---------- .../Security/CWE-022/TaintedPath.go | 9 ++++-- 4 files changed, 32 insertions(+), 20 deletions(-) diff --git a/go/ql/lib/change-notes/2025-07-15-path-injection-sanitizers.md b/go/ql/lib/change-notes/2025-07-15-path-injection-sanitizers.md index e4ff7224ad2f..69596cf98d92 100644 --- a/go/ql/lib/change-notes/2025-07-15-path-injection-sanitizers.md +++ b/go/ql/lib/change-notes/2025-07-15-path-injection-sanitizers.md @@ -1,4 +1,4 @@ --- category: minorAnalysis --- -* Remove model`CreateTemp` function, from the `os` package, as a path-injection sink due to proper sanitization by Go. Add check for `os.PathSeparator` in sanitizers for path-injection query. \ No newline at end of file +* Remove model `CreateTemp` function, from the `os` package, as a path-injection sink due to proper sanitization by Go. Add check for `os.PathSeparator` in sanitizers for path-injection query. \ No newline at end of file diff --git a/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll b/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll index df601ce1eb84..760de2d9c546 100644 --- a/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll +++ b/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll @@ -87,7 +87,14 @@ 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).asExpr().(StringLit).getValue() = "/" + or + exists(DeclaredConstant dc | + dc.hasQualifiedName("os", "PathSeparator") and + dc.getAReference() = concatNode.getOperand(0).asExpr().getAChildExpr*() + ) + ) and this = cleanCall.getResult() ) } diff --git a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected index fc6e39f697de..f5d86e68dbc6 100644 --- a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected +++ b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected @@ -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:69:28:69:57 | call to Clean | TaintedPath.go:14:18:14:22 | selection of URL | TaintedPath.go:69:28:69: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:69:39:69: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:69:39:69:56 | ...+... | TaintedPath.go:69:28:69: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:69:28:69:57 | call to Clean | semmle.label | call to Clean | -| TaintedPath.go:69:39:69: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 diff --git a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go index 99b3a29741b4..3949d8408a1c 100644 --- a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go +++ b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go @@ -4,12 +4,13 @@ import ( "io/ioutil" "mime/multipart" "net/http" + "os" "path" "path/filepath" "regexp" "strings" - "os" ) + func handler(w http.ResponseWriter, r *http.Request) { tainted_path := r.URL.Query()["path"][0] @@ -58,9 +59,13 @@ func handler(w http.ResponseWriter, r *http.Request) { w.Write(data) } - // GOOD: Sanitized by filepath.Clean with a prepended '/' or os.PathSeparator forcing interpretation + // GOOD: Sanitized by filepath.Clean with a prepended '/' forcing interpretation // as an absolute path, so that Clean will throw away any leading `..` components. 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) + tainted_path)) w.Write(data)