Skip to content

Go: Add and Modify Sanitizers For TaintedPath #11703

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
merged 5 commits into from
Apr 10, 2024
Merged

Go: Add and Modify Sanitizers For TaintedPath #11703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
530c76c
Add New Sanitizers and Modify Old Ones
Kwstubbs Dec 15, 2022
c9b49d3
resolve feedback
Kwstubbs Oct 8, 2023
c4c0b22
Formattinga and change notes
Kwstubbs Oct 8, 2023
30fe416
Removed filepath.base sanitizer
Kwstubbs Mar 11, 2024
5acc15b
fix grammar
Kwstubbs Mar 12, 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
5 changes: 5 additions & 0 deletions go/ql/lib/change-notes/2024-03-11-addional-gopath-sanitizers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@

* ---
category: minorAnalysis
---
* Added strings.ReplaceAll, http.ParseMultipartForm sanitizers and remove path sanitizer.
31 changes: 28 additions & 3 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 @@ -5,6 +5,7 @@

import go
import semmle.go.dataflow.barrierguardutil.RegexpCheck
import DataFlow

/**
* Provides extension points for customizing the taint tracking configuration for reasoning about
Expand Down Expand Up @@ -74,20 +75,29 @@ module TaintedPath {
}

/**
* A call to `[file]path.Clean("/" + e)`, considered to sanitize `e` against path traversal.
* A call to `filepath.Clean("/" + e)`, considered to sanitize `e` against path traversal.
*/
class FilepathCleanSanitizer extends Sanitizer {
FilepathCleanSanitizer() {
exists(DataFlow::CallNode cleanCall, StringOps::Concatenation concatNode |
cleanCall =
any(Function f | f.hasQualifiedName(["path", "path/filepath"], "Clean")).getACall() and
cleanCall = any(Function f | f.hasQualifiedName("path/filepath", "Clean")).getACall() and
concatNode = cleanCall.getArgument(0) and
concatNode.getOperand(0).asExpr().(StringLit).getValue() = "/" and
this = cleanCall.getResult()
)
}
}

/**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
)
}
}

Comment on lines +91 to +100
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we have decided that filepath.Base is not an adequate sanitizer, I don't think we should include this, as it uses filepath.Base (docs link).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@owen-mc Yup I'm aware, however I would still like to advocate for this sanitizer because it is a filename, not just part of a path. If the .. is treated as a filename, it references a folder which is a failure in Golang. If any path is prepended, then the path also references the previous folder, which again fails for APIs that expect a file. It also makes no sense to add any path after the .. since its a file, not a directory. I have seen this particular FP many times and its never been exploitable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wait, I am confused. Did you mean to sanitize reads of the field Filename in the struct FileHeader (docs)? Or did you mean calls to the method FileName() (docs), which is defined on *Part, i.e. parts of the multipart message. Your code does the first one, but I don't see why that should be sanitized, so I'm guessing maybe you meant to do the second one, because the docs say it has been run through filepath.Base(). I consulted with a colleague and he agreed it was reasonable for the second one to be a sanitizer, so if that's what you meant then please change the code to refer to the right thing and update the test.

Copy link
Contributor Author

@Kwstubbs Kwstubbs Apr 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@owen-mc I added the Filename in FileHeader because it previously led me to a FP when testing it. The only way FileHeader to be instantiated is through readForm, which sanitizes Filename using FileName which uses Base. I understand if you don't wanna add it since the documentation doesn't guarantee anything, but I thought i would include it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that makes sense. I think we can merge this then.

/**
* A check of the form `!strings.Contains(nd, "..")`, considered as a sanitizer guard for
* path traversal.
Expand All @@ -106,6 +116,21 @@ 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()
)
}
}

/**
* A node `nd` guarded by a check that ensures it is contained within some root folder,
* considered as a sanitizer for path traversal.
Expand Down
9 changes: 5 additions & 4 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
Expand Up @@ -2,18 +2,19 @@ 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 | |
| tst.go:14:2:14:39 | ... := ...[1] | tst.go:17:41:17:56 | selection of Filename | provenance | |
| TaintedPath.go:67:39:67:56 | ...+... | TaintedPath.go:67:28:67: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 |
| tst.go:14:2:14:39 | ... := ...[1] | semmle.label | ... := ...[1] |
| tst.go:17:41:17:56 | selection of Filename | semmle.label | selection of Filename |
| TaintedPath.go:67:28:67:57 | call to Clean | semmle.label | call to Clean |
| TaintedPath.go:67:39:67: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 |
| tst.go:17:41:17:56 | selection of Filename | tst.go:14:2:14:39 | ... := ...[1] | tst.go:17:41:17:56 | selection of Filename | This path depends on a $@. | tst.go:14:2:14:39 | ... := ...[1] | 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 |
19 changes: 18 additions & 1 deletion 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 @@ -31,6 +31,10 @@ func handler(w http.ResponseWriter, r *http.Request) {
w.Write(data)
}

// GOOD: Sanitized by strings.ReplaceAll and replaces all .. with empty string
data, _ = ioutil.ReadFile(strings.ReplaceAll(tainted_path, "..", ""))
w.Write(data)

// GOOD: This can only read inside the provided safe path
_, err := filepath.Rel("/home/user/safepath", tainted_path)
if err == nil {
Expand All @@ -53,10 +57,23 @@ func handler(w http.ResponseWriter, r *http.Request) {
w.Write(data)
}

// GOOD: Sanitized by [file]path.Clean with a prepended '/' 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)

// 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))
w.Write(data)

// GOOD: Multipart.Form.FileHeader.Filename sanitized by filepath.Base when calling ParseMultipartForm
r.ParseMultipartForm(32 << 20)
form := r.MultipartForm
files, ok := form.File["files"]
if !ok {
return
}
data, _ = ioutil.ReadFile(files[0].Filename)
w.Write(data)
}