Skip to content

Commit 530c76c

Browse files
KwstubbsKwstubbs
committed
Add New Sanitizers and Modify Old Ones
1 parent 9c51514 commit 530c76c

File tree

3 files changed

+91
-6
lines changed

3 files changed

+91
-6
lines changed

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

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,19 +74,46 @@ module TaintedPath {
7474
}
7575

7676
/**
77-
* A call to `[file]path.Clean("/" + e)`, considered to sanitize `e` against path traversal.
77+
* A call to `filepath.Clean("/" + e)`, considered to sanitize `e` against path traversal.
7878
*/
7979
class FilepathCleanSanitizer extends Sanitizer {
8080
FilepathCleanSanitizer() {
8181
exists(DataFlow::CallNode cleanCall, StringOps::Concatenation concatNode |
8282
cleanCall =
83-
any(Function f | f.hasQualifiedName(["path", "path/filepath"], "Clean")).getACall() and
83+
any(Function f | f.hasQualifiedName("path/filepath", "Clean")).getACall() and
8484
concatNode = cleanCall.getArgument(0) and
8585
concatNode.getOperand(0).asExpr().(StringLit).getValue() = "/" and
8686
this = cleanCall.getResult()
8787
)
8888
}
8989
}
90+
/**
91+
* A call to `filepath.Base(e)`, considered to sanitize `e` against path traversal.
92+
*/
93+
class FilepathBaseSanitizer extends Sanitizer {
94+
FilepathBaseSanitizer() {
95+
exists(Function f, FunctionOutput outp |
96+
f.hasQualifiedName("path/filepath", "Base") and
97+
outp.isResult(0) and
98+
this = outp.getNode(f.getACall())
99+
)
100+
}
101+
}
102+
103+
/** An call to ParseMultipartForm creates multipart.Form and cleans mutlpart.Form.FileHeader.Filename using path.Base() */
104+
class MultipartClean extends Sanitizer {
105+
MultipartClean() {
106+
exists(DataFlow::FieldReadNode frn, ControlFlow::Node node, DataFlow::CallNode cleanCall, Method get |
107+
get.hasQualifiedName("net/http","Request", "ParseMultipartForm") and
108+
cleanCall = get.getACall() and
109+
cleanCall.asInstruction() = node and
110+
frn.getField().hasQualifiedName("mime/multipart", "FileHeader", "Filename") and
111+
node.getASuccessor*() = frn.asInstruction()
112+
|
113+
this = frn.getBase()
114+
)
115+
}
116+
}
90117

91118
/**
92119
* A check of the form `!strings.Contains(nd, "..")`, considered as a sanitizer guard for
@@ -105,6 +132,21 @@ module TaintedPath {
105132
branch = false
106133
}
107134
}
135+
/**
136+
* A replacement of the form `!strings.ReplaceAll(nd, "..")` or `!strings.ReplaceAll(nd, ".")`, considered as a sanitizer guard for
137+
* path traversal.
138+
*/
139+
class DotDotReplace extends Sanitizer {
140+
DotDotReplace() {
141+
exists(DataFlow::CallNode cleanCall, DataFlow::Node valueNode |
142+
cleanCall =
143+
any(Function f | f.hasQualifiedName("strings", "ReplaceAll")).getACall() and
144+
valueNode = cleanCall.getArgument(1) and
145+
(valueNode.asExpr().(StringLit).getValue() = ".." or valueNode.asExpr().(StringLit).getValue() = ".") and
146+
this = cleanCall.getResult()
147+
)
148+
}
149+
}
108150

109151
/**
110152
* A node `nd` guarded by a check that ensures it is contained within some root folder,

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

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,35 @@
11
edges
2+
<<<<<<< HEAD
23
| TaintedPath.go:13:18:13:22 | selection of URL | TaintedPath.go:13:18:13:30 | call to Query | provenance | |
34
| TaintedPath.go:13:18:13:30 | call to Query | TaintedPath.go:16:29:16:40 | tainted_path | provenance | |
45
| TaintedPath.go:13:18:13:30 | call to Query | TaintedPath.go:20:57:20:68 | tainted_path | provenance | |
56
| TaintedPath.go:20:57:20:68 | tainted_path | TaintedPath.go:20:28:20:69 | call to Join | provenance | |
67
| tst.go:14:2:14:39 | ... := ...[1] | tst.go:17:41:17:56 | selection of Filename | provenance | |
8+
=======
9+
| TaintedPath.go:13:18:13:22 | selection of URL : pointer type | TaintedPath.go:16:29:16:40 | tainted_path |
10+
| TaintedPath.go:13:18:13:22 | selection of URL : pointer type | TaintedPath.go:20:28:20:69 | call to Join |
11+
| TaintedPath.go:13:18:13:22 | selection of URL : pointer type | TaintedPath.go:67:28:67:57 | call to Clean |
12+
| TaintedPath.go:13:18:13:22 | selection of URL : pointer type | TaintedPath.go:77:28:77:56 | call to Base |
13+
| tst.go:14:2:14:39 | ... := ...[1] : pointer type | tst.go:17:41:17:56 | selection of Filename |
14+
>>>>>>> a45343fb6c (Add New Sanitizers and Modify Old Ones)
715
nodes
816
| TaintedPath.go:13:18:13:22 | selection of URL | semmle.label | selection of URL |
917
| TaintedPath.go:13:18:13:30 | call to Query | semmle.label | call to Query |
1018
| TaintedPath.go:16:29:16:40 | tainted_path | semmle.label | tainted_path |
1119
| TaintedPath.go:20:28:20:69 | call to Join | semmle.label | call to Join |
20+
<<<<<<< HEAD
1221
| TaintedPath.go:20:57:20:68 | tainted_path | semmle.label | tainted_path |
1322
| tst.go:14:2:14:39 | ... := ...[1] | semmle.label | ... := ...[1] |
23+
=======
24+
| TaintedPath.go:67:28:67:57 | call to Clean | semmle.label | call to Clean |
25+
| TaintedPath.go:77:28:77:56 | call to Base | semmle.label | call to Base |
26+
| tst.go:14:2:14:39 | ... := ...[1] : pointer type | semmle.label | ... := ...[1] : pointer type |
27+
>>>>>>> a45343fb6c (Add New Sanitizers and Modify Old Ones)
1428
| tst.go:17:41:17:56 | selection of Filename | semmle.label | selection of Filename |
1529
subpaths
1630
#select
17-
| 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 |
18-
| 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 |
19-
| 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 |
31+
| TaintedPath.go:16:29:16:40 | tainted_path | TaintedPath.go:13:18:13:22 | selection of URL : pointer type | 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 |
32+
| TaintedPath.go:20:28:20:69 | call to Join | TaintedPath.go:13:18:13:22 | selection of URL : pointer type | 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 |
33+
| TaintedPath.go:67:28:67:57 | call to Clean | TaintedPath.go:13:18:13:22 | selection of URL : pointer type | 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 |
34+
| TaintedPath.go:77:28:77:56 | call to Base | TaintedPath.go:13:18:13:22 | selection of URL : pointer type | TaintedPath.go:77:28:77:56 | call to Base | This path depends on a $@. | TaintedPath.go:13:18:13:22 | selection of URL | user-provided value |
35+
| tst.go:17:41:17:56 | selection of Filename | tst.go:14:2:14:39 | ... := ...[1] : pointer type | tst.go:17:41:17:56 | selection of Filename | This path depends on a $@. | tst.go:14:2:14:39 | ... := ...[1] | user-provided value |

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

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,10 @@ func handler(w http.ResponseWriter, r *http.Request) {
3131
w.Write(data)
3232
}
3333

34+
// GOOD: Sanitized by strings.ReplaceAll and replaces all .. with empty string
35+
data, _ = ioutil.ReadFile(strings.ReplaceAll(tainted_path, "..", ""))
36+
w.Write(data)
37+
3438
// GOOD: This can only read inside the provided safe path
3539
_, err := filepath.Rel("/home/user/safepath", tainted_path)
3640
if err == nil {
@@ -53,10 +57,33 @@ func handler(w http.ResponseWriter, r *http.Request) {
5357
w.Write(data)
5458
}
5559

56-
// GOOD: Sanitized by [file]path.Clean with a prepended '/' forcing interpretation
60+
// GOOD: Sanitized by filepath.Clean with a prepended '/' forcing interpretation
5761
// as an absolute path, so that Clean will throw away any leading `..` components.
5862
data, _ = ioutil.ReadFile(filepath.Clean("/" + tainted_path))
5963
w.Write(data)
64+
65+
// BAD: Sanitized by path.Clean with a prepended '/' forcing interpretation
66+
// as an absolute path, however is not sufficient for Windows paths.
6067
data, _ = ioutil.ReadFile(path.Clean("/" + tainted_path))
6168
w.Write(data)
69+
70+
// GOOD: Sanitized by filepath.Base with a prepended '/' forcing interpretation
71+
// as an absolute path, so that Base will throw away any leading `..` components.
72+
data, _ = ioutil.ReadFile(filepath.Base("/" + tainted_path))
73+
w.Write(data)
74+
75+
// BAD: Sanitized by path.Base with a prepended '/' forcing interpretation
76+
// as an absolute path, however is not sufficient for Windows paths.
77+
data, _ = ioutil.ReadFile(path.Base("/" + tainted_path))
78+
w.Write(data)
79+
80+
// GOOD: Multipart.Form.FileHeader.Filename sanitized by filepath.Base when calling ParseMultipartForm
81+
r.ParseMultipartForm(32 << 20)
82+
form := r.MultipartForm
83+
files, ok := form.File["files"]
84+
if !ok {
85+
return
86+
}
87+
data, _ = ioutil.ReadFile(files[0].Filename)
88+
w.Write(data)
6289
}

0 commit comments

Comments
 (0)