Skip to content

Update Go Path Injection Sanitizer and Sink #20064

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
owen-mc merged 4 commits into from
Sep 3, 2025
Merged

Update Go Path Injection Sanitizer and Sink #20064

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
504ae0f
Update go path sanitizers and sinks
Kwstubbs Jul 16, 2025
f86152d
Add sanitizer changes and fix test
Kwstubbs Jul 16, 2025
b4b848a
Fix tests and simplify sanitizer
Kwstubbs Jul 21, 2025
e2f3c9d
Reword change note
owen-mc Jul 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions go/ql/lib/change-notes/2025-07-15-path-injection-sanitizers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
1 change: 0 additions & 1 deletion go/ql/lib/ext/os.model.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Expand Down
9 changes: 8 additions & 1 deletion go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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*()
)
Copy link
Contributor

@owen-mc owen-mc Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use getStringValue on any reference of os.PathSeparator then we'll get its actual string value (the compiler knows what value a constant has). On Windows of course this is "\". So we can simplify this to:

Suggested change
concatNode.getOperand(0).asExpr().(StringLit).getValue() = "/"
or
exists(DeclaredConstant dc |
dc.hasQualifiedName("os", "PathSeparator") and
dc.getAReference() = concatNode.getOperand(0).asExpr().getAChildExpr*()
)
concatNode.getOperand(0).getStringValue() = ["/", "\"]

The above code allows using the string literal "\" as well, which seems sensible for any code which is written to only be run on Windows. What do you think?

Copy link
Contributor Author

@Kwstubbs Kwstubbs Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added concatNode.getOperand(0).getStringValue().prefix(1) to account for cases where there is additional text but the first element is still appropriate.

) and
this = cleanCall.getResult()
)
}
Expand Down
32 changes: 16 additions & 16 deletions go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
6 changes: 6 additions & 0 deletions go/ql/test/query-tests/Security/CWE-022/TaintedPath.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"io/ioutil"
"mime/multipart"
"net/http"
"os"
"path"
"path/filepath"
"regexp"
Expand Down Expand Up @@ -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) + 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))
Expand Down