Skip to content

Commit 1e8315d

Browse files
owen-mcowen-mc
authored
Merge pull request #16180 from owen-mc/go/tweak-go-tainted-path-additions
Go: Tweak go tainted path additions
2 parents e6fdc75 + a7c5e84 commit 1e8315d

File tree

3 files changed

+70
-32
lines changed

3 files changed

+70
-32
lines changed

‎go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll

Lines changed: 37 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -88,13 +88,41 @@ module TaintedPath {
8888
}
8989
}
9090

91-
/**An call to ParseMultipartForm creates multipart.Form and cleans multipart.Form.FileHeader.Filename using path.Base() */
92-
class MultipartClean extends Sanitizer {
93-
MultipartClean() {
94-
exists(DataFlow::FieldReadNode frn |
95-
frn.getField().hasQualifiedName("mime/multipart", "FileHeader", "Filename") and
96-
this = frn
97-
)
91+
/**
92+
* A read from the field `Filename` of the type `mime/multipart.FileHeader`,
93+
* considered as a sanitizer for path traversal.
94+
*
95+
* The only way to create a `mime/multipart.FileHeader` is to create a
96+
* `mime/multipart.Form`, which creates the `Filename` field of each
97+
* `mime/multipart.FileHeader` by calling `Part.FileName`, which calls
98+
* `path/filepath.Base` on its return value. In general `path/filepath.Base`
99+
* is not a sanitizer for path traversal, but in this specific case where the
100+
* output is going to be used as a filename rather than a directory name, it
101+
* is adequate.
102+
*/
103+
class MimeMultipartFileHeaderFilenameSanitizer extends Sanitizer {
104+
MimeMultipartFileHeaderFilenameSanitizer() {
105+
this.(DataFlow::FieldReadNode)
106+
.getField()
107+
.hasQualifiedName("mime/multipart", "FileHeader", "Filename")
108+
}
109+
}
110+
111+
/**
112+
* A call to `mime/multipart.Part.FileName`, considered as a sanitizer
113+
* against path traversal.
114+
*
115+
* `Part.FileName` calls `path/filepath.Base` on its return value. In
116+
* general `path/filepath.Base` is not a sanitizer for path traversal, but in
117+
* this specific case where the output is going to be used as a filename
118+
* rather than a directory name, it is adequate.
119+
*/
120+
class MimeMultipartPartFileNameSanitizer extends Sanitizer {
121+
MimeMultipartPartFileNameSanitizer() {
122+
this =
123+
any(Method m | m.hasQualifiedName("mime/multipart", "Part", "FileName"))
124+
.getACall()
125+
.getResult()
98126
}
99127
}
100128

@@ -120,15 +148,8 @@ module TaintedPath {
120148
* A replacement of the form `!strings.ReplaceAll(nd, "..")` or `!strings.ReplaceAll(nd, ".")`, considered as a sanitizer for
121149
* path traversal.
122150
*/
123-
class DotDotReplace extends Sanitizer {
124-
DotDotReplace() {
125-
exists(DataFlow::CallNode cleanCall, DataFlow::Node valueNode |
126-
cleanCall = any(Function f | f.hasQualifiedName("strings", "ReplaceAll")).getACall() and
127-
valueNode = cleanCall.getArgument(1) and
128-
valueNode.asExpr().(StringLit).getValue() = ["..", "."] and
129-
this = cleanCall.getResult()
130-
)
131-
}
151+
class DotDotReplaceAll extends StringOps::ReplaceAll, Sanitizer {
152+
DotDotReplaceAll() { this.getReplacedString() = ["..", "."] }
132153
}
133154

134155
/**

‎go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,20 @@
11
edges
2-
| TaintedPath.go:13:18:13:22 | selection of URL | TaintedPath.go:13:18:13:30 | call to Query | provenance | |
3-
| TaintedPath.go:13:18:13:30 | call to Query | TaintedPath.go:16:29:16:40 | tainted_path | provenance | |
4-
| TaintedPath.go:13:18:13:30 | call to Query | TaintedPath.go:20:57:20:68 | tainted_path | provenance | |
5-
| TaintedPath.go:13:18:13:30 | call to Query | TaintedPath.go:67:39:67:56 | ...+... | provenance | |
6-
| TaintedPath.go:20:57:20:68 | tainted_path | TaintedPath.go:20:28:20:69 | call to Join | provenance | |
7-
| TaintedPath.go:67:39:67:56 | ...+... | TaintedPath.go:67:28:67:57 | call to Clean | provenance | |
2+
| TaintedPath.go:14:18:14:22 | selection of URL | TaintedPath.go:14:18:14:30 | call to Query | provenance | |
3+
| TaintedPath.go:14:18:14:30 | call to Query | TaintedPath.go:17:29:17:40 | tainted_path | provenance | |
4+
| TaintedPath.go:14:18:14:30 | call to Query | TaintedPath.go:21:57:21:68 | tainted_path | provenance | |
5+
| TaintedPath.go:14:18:14:30 | call to Query | TaintedPath.go:68:39:68:56 | ...+... | provenance | |
6+
| TaintedPath.go:21:57:21:68 | tainted_path | TaintedPath.go:21:28:21:69 | call to Join | provenance | |
7+
| TaintedPath.go:68:39:68:56 | ...+... | TaintedPath.go:68:28:68:57 | call to Clean | provenance | |
88
nodes
9-
| TaintedPath.go:13:18:13:22 | selection of URL | semmle.label | selection of URL |
10-
| TaintedPath.go:13:18:13:30 | call to Query | semmle.label | call to Query |
11-
| TaintedPath.go:16:29:16:40 | tainted_path | semmle.label | tainted_path |
12-
| TaintedPath.go:20:28:20:69 | call to Join | semmle.label | call to Join |
13-
| TaintedPath.go:20:57:20:68 | tainted_path | semmle.label | tainted_path |
14-
| TaintedPath.go:67:28:67:57 | call to Clean | semmle.label | call to Clean |
15-
| TaintedPath.go:67:39:67:56 | ...+... | semmle.label | ...+... |
9+
| TaintedPath.go:14:18:14:22 | selection of URL | semmle.label | selection of URL |
10+
| TaintedPath.go:14:18:14:30 | call to Query | semmle.label | call to Query |
11+
| TaintedPath.go:17:29:17:40 | tainted_path | semmle.label | tainted_path |
12+
| TaintedPath.go:21:28:21:69 | call to Join | semmle.label | call to Join |
13+
| TaintedPath.go:21:57:21:68 | tainted_path | semmle.label | tainted_path |
14+
| TaintedPath.go:68:28:68:57 | call to Clean | semmle.label | call to Clean |
15+
| TaintedPath.go:68:39:68:56 | ...+... | semmle.label | ...+... |
1616
subpaths
1717
#select
18-
| 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 |
19-
| 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 |
20-
| 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 |
18+
| 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 |
19+
| 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 |
20+
| 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 |

‎go/ql/test/query-tests/Security/CWE-022/TaintedPath.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package main
22

33
import (
44
"io/ioutil"
5+
"mime/multipart"
56
"net/http"
67
"path"
78
"path/filepath"
@@ -76,4 +77,20 @@ func handler(w http.ResponseWriter, r *http.Request) {
7677
}
7778
data, _ = ioutil.ReadFile(files[0].Filename)
7879
w.Write(data)
80+
81+
// GOOD: Part.FileName sanitized by filepath.Base
82+
r.ParseMultipartForm(32 << 20)
83+
file, _, err := r.FormFile("uploadfile")
84+
if err != nil {
85+
return
86+
}
87+
reader := multipart.NewReader(file, "foo")
88+
89+
// Read the first part
90+
part, err := reader.NextPart()
91+
if err != nil {
92+
return
93+
}
94+
95+
data, _ = ioutil.ReadFile(part.FileName())
7996
}

0 commit comments

Comments
 (0)