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: Tweak go tainted path additions #16180

Merged
merged 3 commits into from
Apr 11, 2024
+70 −32
Merged

Go: Tweak go tainted path additions #16180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
21189af
Improve QLDoc and refactor QL
owen-mc Apr 10, 2024
c3fefa8
Add extra sanitizer Part.FileName()
owen-mc Apr 10, 2024
a7c5e84
Expand `ReplaceAll` in sanitizer
owen-mc Apr 10, 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
53 changes: 37 additions & 16 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 @@ -88,13 +88,41 @@ module TaintedPath {
}
}

/**An call to ParseMultipartForm creates multipart.Form and cleans multipart.Form.FileHeader.Filename using path.Base() */
class MultipartClean extends Sanitizer {
MultipartClean() {
exists(DataFlow::FieldReadNode frn |
frn.getField().hasQualifiedName("mime/multipart", "FileHeader", "Filename") and
this = frn
)
/**
* A read from the field `Filename` of the type `mime/multipart.FileHeader`,
* considered as a sanitizer for path traversal.
*
* The only way to create a `mime/multipart.FileHeader` is to create a
* `mime/multipart.Form`, which creates the `Filename` field of each
* `mime/multipart.FileHeader` by calling `Part.FileName`, which calls
* `path/filepath.Base` on its return value. In general `path/filepath.Base`
* is not a sanitizer for path traversal, but in this specific case where the
* output is going to be used as a filename rather than a directory name, it
* is adequate.
*/
class MimeMultipartFileHeaderFilenameSanitizer extends Sanitizer {
MimeMultipartFileHeaderFilenameSanitizer() {
this.(DataFlow::FieldReadNode)
.getField()
.hasQualifiedName("mime/multipart", "FileHeader", "Filename")
}
}

/**
* A call to `mime/multipart.Part.FileName`, considered as a sanitizer
* against path traversal.
*
* `Part.FileName` calls `path/filepath.Base` on its return value. In
* general `path/filepath.Base` is not a sanitizer for path traversal, but in
* this specific case where the output is going to be used as a filename
* rather than a directory name, it is adequate.
*/
class MimeMultipartPartFileNameSanitizer extends Sanitizer {
MimeMultipartPartFileNameSanitizer() {
this =
any(Method m | m.hasQualifiedName("mime/multipart", "Part", "FileName"))
.getACall()
.getResult()
}
}

Expand All @@ -120,15 +148,8 @@ module TaintedPath {
* A replacement of the form `!strings.ReplaceAll(nd, "..")` or `!strings.ReplaceAll(nd, ".")`, considered as a sanitizer for
* path traversal.
*/
class DotDotReplace extends Sanitizer {
DotDotReplace() {
exists(DataFlow::CallNode cleanCall, DataFlow::Node valueNode |
cleanCall = any(Function f | f.hasQualifiedName("strings", "ReplaceAll")).getACall() and
valueNode = cleanCall.getArgument(1) and
valueNode.asExpr().(StringLit).getValue() = ["..", "."] and
this = cleanCall.getResult()
)
}
class DotDotReplaceAll extends StringOps::ReplaceAll, Sanitizer {
DotDotReplaceAll() { this.getReplacedString() = ["..", "."] }
}

/**
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,20 +1,20 @@
edges
| TaintedPath.go:13:18:13:22 | selection of URL | TaintedPath.go:13:18:13:30 | call to Query | provenance | |
| TaintedPath.go:13:18:13:30 | call to Query | TaintedPath.go:16:29:16:40 | tainted_path | provenance | |
| TaintedPath.go:13:18:13:30 | call to Query | TaintedPath.go:20:57:20:68 | tainted_path | provenance | |
| TaintedPath.go:13:18:13:30 | call to Query | TaintedPath.go:67:39:67:56 | ...+... | provenance | |
| TaintedPath.go:20:57:20:68 | tainted_path | TaintedPath.go:20:28:20:69 | call to Join | provenance | |
| TaintedPath.go:67:39:67:56 | ...+... | TaintedPath.go:67:28:67:57 | call to Clean | provenance | |
| TaintedPath.go:14:18:14:22 | selection of URL | TaintedPath.go:14:18:14:30 | call to Query | provenance | |
| TaintedPath.go:14:18:14:30 | call to Query | TaintedPath.go:17:29:17:40 | tainted_path | provenance | |
| 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 | |
| TaintedPath.go:68:39:68:56 | ...+... | TaintedPath.go:68:28:68:57 | call to Clean | provenance | |
nodes
| TaintedPath.go:13:18:13:22 | selection of URL | semmle.label | selection of URL |
| TaintedPath.go:13:18:13:30 | call to Query | semmle.label | call to Query |
| TaintedPath.go:16:29:16:40 | tainted_path | semmle.label | tainted_path |
| TaintedPath.go:20:28:20:69 | call to Join | semmle.label | call to Join |
| TaintedPath.go:20:57:20:68 | tainted_path | semmle.label | tainted_path |
| TaintedPath.go:67:28:67:57 | call to Clean | semmle.label | call to Clean |
| TaintedPath.go:67:39:67:56 | ...+... | semmle.label | ...+... |
| 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 | ...+... |
subpaths
#select
| TaintedPath.go:16:29:16:40 | tainted_path | TaintedPath.go:13:18:13:22 | selection of URL | TaintedPath.go:16:29:16:40 | tainted_path | This path depends on a $@. | TaintedPath.go:13:18:13:22 | selection of URL | user-provided value |
| TaintedPath.go:20:28:20:69 | call to Join | TaintedPath.go:13:18:13:22 | selection of URL | TaintedPath.go:20:28:20:69 | call to Join | This path depends on a $@. | TaintedPath.go:13:18:13:22 | selection of URL | user-provided value |
| TaintedPath.go:67:28:67:57 | call to Clean | TaintedPath.go:13:18:13:22 | selection of URL | TaintedPath.go:67:28:67:57 | call to Clean | This path depends on a $@. | TaintedPath.go:13:18:13:22 | selection of URL | user-provided value |
| 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 |
17 changes: 17 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 @@ -2,6 +2,7 @@ package main

import (
"io/ioutil"
"mime/multipart"
"net/http"
"path"
"path/filepath"
Expand Down Expand Up @@ -76,4 +77,20 @@ func handler(w http.ResponseWriter, r *http.Request) {
}
data, _ = ioutil.ReadFile(files[0].Filename)
w.Write(data)

// GOOD: Part.FileName sanitized by filepath.Base
r.ParseMultipartForm(32 << 20)
file, _, err := r.FormFile("uploadfile")
if err != nil {
return
}
reader := multipart.NewReader(file, "foo")

// Read the first part
part, err := reader.NextPart()
if err != nil {
return
}

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