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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+34 −18
Open

Update Go Path Injection Sanitizer and Sink #20064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 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
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*()
)
Comment on lines +91 to +96
Copy link
Contributor

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?

) 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