Skip to content
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

Go: Add Tainted Path sanitizers #17759

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+662 −0
Open

Go: Add Tainted Path sanitizers #17759

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d195273
Add mux.Vars() and url.Path sanitizers
Kwstubbs Oct 15, 2024
1287f1b
Address feedback
Kwstubbs Oct 15, 2024
374b13e
Remove path sanitizer
Kwstubbs Oct 15, 2024
8744f15
New tests
Kwstubbs Nov 12, 2024
eb3a76d
Merge branch 'github:main' into path-sanitizers
Kwstubbs Nov 13, 2024
460ed30
Fixed tests
Kwstubbs Nov 13, 2024
a94ba25
Apply suggestions from code review
Kwstubbs Nov 13, 2024
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
29 changes: 29 additions & 0 deletions 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 @@ -93,6 +93,35 @@
}
}

// /**
// * A call to `mux.Vars(path)`, considered to sanitize `path` against path traversal.
// * Only enabled when `SkipClean` is not set true.
// */
Kwstubbs marked this conversation as resolved.
Show resolved Hide resolved
class MuxVarsSanitizer extends Sanitizer {

Check warning on line 100 in go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll

View workflow job for this annotation

GitHub Actions / qldoc 

Missing QLdoc for class TaintedPathCustomizations::TaintedPath::MuxVarsSanitizer
MuxVarsSanitizer() {
exists(Function m |
m.hasQualifiedName("github.com/gorilla/mux", "Vars") and
Kwstubbs marked this conversation as resolved.
Show resolved Hide resolved
this = m.getACall().getResult()
) and
not exists(CallExpr f |
f.getTarget().hasQualifiedName("github.com/gorilla/mux", "SkipClean") and
Kwstubbs marked this conversation as resolved.
Show resolved Hide resolved
f.getArgument(0).toString().toLowerCase() = "true"
Kwstubbs marked this conversation as resolved.
Show resolved Hide resolved
)
}
}

// /**
// * A read from `net/url` which is sanitized
// */
Kwstubbs marked this conversation as resolved.
Show resolved Hide resolved
class UrlPathSanitizer extends Sanitizer {

Check warning on line 116 in go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll

View workflow job for this annotation

GitHub Actions / qldoc 

Missing QLdoc for class TaintedPathCustomizations::TaintedPath::UrlPathSanitizer
UrlPathSanitizer() {
exists(DataFlow::Field fld |
this = fld.getARead() and
fld.hasQualifiedName("net/url", "URL", "Path")
)
}
}

/**
* A read from the field `Filename` of the type `mime/multipart.FileHeader`,
* considered as a sanitizer for path traversal.
Expand Down
4 changes: 4 additions & 0 deletions go/ql/src/change-notes/2024-10-14-gopathsanitizer.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Added [github.com/gorilla/mux.Vars](https://pkg.go.dev/github.com/gorilla/mux#Vars) to path sanitizers.
Kwstubbs marked this conversation as resolved.
Show resolved Hide resolved
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:19:29:19:40 | tainted_path | TaintedPath.go:16:18:16:22 | selection of URL | TaintedPath.go:19:29:19:40 | tainted_path | This path depends on a $@. | TaintedPath.go:16:18:16:22 | selection of URL | user-provided value |
| TaintedPath.go:23:28:23:69 | call to Join | TaintedPath.go:16:18:16:22 | selection of URL | TaintedPath.go:23:28:23:69 | call to Join | This path depends on a $@. | TaintedPath.go:16:18:16:22 | selection of URL | user-provided value |
| TaintedPath.go:70:28:70:57 | call to Clean | TaintedPath.go:16:18:16:22 | selection of URL | TaintedPath.go:70:28:70:57 | call to Clean | This path depends on a $@. | TaintedPath.go:16:18:16: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:16:18:16:22 | selection of URL | TaintedPath.go:16:18:16:30 | call to Query | provenance | Src:MaD:2 MaD:3 |
| TaintedPath.go:16:18:16:30 | call to Query | TaintedPath.go:19:29:19:40 | tainted_path | provenance | Sink:MaD:1 |
| TaintedPath.go:16:18:16:30 | call to Query | TaintedPath.go:23:57:23:68 | tainted_path | provenance | |
| TaintedPath.go:16:18:16:30 | call to Query | TaintedPath.go:70:39:70:56 | ...+... | provenance | |
| TaintedPath.go:23:57:23:68 | tainted_path | TaintedPath.go:23:28:23:69 | call to Join | provenance | FunctionModel Sink:MaD:1 |
| TaintedPath.go:70:39:70:56 | ...+... | TaintedPath.go:70:28:70: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:16:18:16:22 | selection of URL | semmle.label | selection of URL |
| TaintedPath.go:16:18:16:30 | call to Query | semmle.label | call to Query |
| TaintedPath.go:19:29:19:40 | tainted_path | semmle.label | tainted_path |
| TaintedPath.go:23:28:23:69 | call to Join | semmle.label | call to Join |
| TaintedPath.go:23:57:23:68 | tainted_path | semmle.label | tainted_path |
| TaintedPath.go:70:28:70:57 | call to Clean | semmle.label | call to Clean |
| TaintedPath.go:70:39:70:56 | ...+... | semmle.label | ...+... |
subpaths
9 changes: 9 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 @@ -8,6 +8,8 @@ import (
"path/filepath"
"regexp"
"strings"

"github.com/gorilla/mux"
)

func handler(w http.ResponseWriter, r *http.Request) {
Expand Down Expand Up @@ -94,3 +96,10 @@ func handler(w http.ResponseWriter, r *http.Request) {

data, _ = ioutil.ReadFile(part.FileName())
}

// GOOD: Sanitized by Gorilla's cleaner
func GorillaHandler(w http.ResponseWriter, r *http.Request) {
not_tainted_path := mux.Vars(r)
Kwstubbs marked this conversation as resolved.
Show resolved Hide resolved
data, _ := ioutil.ReadFile(filepath.Join("/home/user/", not_tainted_path))
w.Write(data)
}
5 changes: 5 additions & 0 deletions go/ql/test/query-tests/Security/CWE-022/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module codeql-go-tests/frameworks/Mux

go 1.14

require github.com/gorilla/mux v1.7.4
27 changes: 27 additions & 0 deletions go/ql/test/query-tests/Security/CWE-022/vendor/github.com/gorilla/mux/LICENSE
View file
Open in desktop
Kwstubbs marked this conversation as resolved.
Show resolved Hide resolved

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

252 changes: 252 additions & 0 deletions go/ql/test/query-tests/Security/CWE-022/vendor/github.com/gorilla/mux/stub.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions go/ql/test/query-tests/Security/CWE-022/vendor/modules.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# github.com/gorilla/mux v1.7.4
## explicit
github.com/gorilla/mux
Loading