Skip to content

Commit d14497f

Browse files
KwstubbsKwstubbs
committed
Add New Sanitizers and Modify Old Ones
1 parent 8e653d0 commit d14497f

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
@@ -91,19 +91,46 @@ module TaintedPath {
9191
}
9292

9393
/**
94-
* A call to `[file]path.Clean("/" + e)`, considered to sanitize `e` against path traversal.
94+
* A call to `filepath.Clean("/" + e)`, considered to sanitize `e` against path traversal.
9595
*/
9696
class FilepathCleanSanitizer extends Sanitizer {
9797
FilepathCleanSanitizer() {
9898
exists(DataFlow::CallNode cleanCall, StringOps::Concatenation concatNode |
9999
cleanCall =
100-
any(Function f | f.hasQualifiedName(["path", "path/filepath"], "Clean")).getACall() and
100+
any(Function f | f.hasQualifiedName("path/filepath", "Clean")).getACall() and
101101
concatNode = cleanCall.getArgument(0) and
102102
concatNode.getOperand(0).asExpr().(StringLit).getValue() = "/" and
103103
this = cleanCall.getResult()
104104
)
105105
}
106106
}
107+
/**
108+
* A call to `filepath.Base(e)`, considered to sanitize `e` against path traversal.
109+
*/
110+
class FilepathBaseSanitizer extends Sanitizer {
111+
FilepathBaseSanitizer() {
112+
exists(Function f, FunctionOutput outp |
113+
f.hasQualifiedName("path/filepath", "Base") and
114+
outp.isResult(0) and
115+
this = outp.getNode(f.getACall())
116+
)
117+
}
118+
}
119+
120+
/** An call to ParseMultipartForm creates multipart.Form and cleans mutlpart.Form.FileHeader.Filename using path.Base() */
121+
class MultipartClean extends Sanitizer {
122+
MultipartClean() {
123+
exists(DataFlow::FieldReadNode frn, ControlFlow::Node node, DataFlow::CallNode cleanCall, Method get |
124+
get.hasQualifiedName("net/http","Request", "ParseMultipartForm") and
125+
cleanCall = get.getACall() and
126+
cleanCall.asInstruction() = node and
127+
frn.getField().hasQualifiedName("mime/multipart", "FileHeader", "Filename") and
128+
node.getASuccessor*() = frn.asInstruction()
129+
|
130+
this = frn.getBase()
131+
)
132+
}
133+
}
107134

108135
/**
109136
* A check of the form `!strings.Contains(nd, "..")`, considered as a sanitizer guard for
@@ -122,6 +149,21 @@ module TaintedPath {
122149
branch = false
123150
}
124151
}
152+
/**
153+
* A replacement of the form `!strings.ReplaceAll(nd, "..")` or `!strings.ReplaceAll(nd, ".")`, considered as a sanitizer guard for
154+
* path traversal.
155+
*/
156+
class DotDotReplace extends Sanitizer {
157+
DotDotReplace() {
158+
exists(DataFlow::CallNode cleanCall, DataFlow::Node valueNode |
159+
cleanCall =
160+
any(Function f | f.hasQualifiedName("strings", "ReplaceAll")).getACall() and
161+
valueNode = cleanCall.getArgument(1) and
162+
(valueNode.asExpr().(StringLit).getValue() = ".." or valueNode.asExpr().(StringLit).getValue() = ".") and
163+
this = cleanCall.getResult()
164+
)
165+
}
166+
}
125167

126168
/**
127169
* 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 |
34
| TaintedPath.go:13:18:13:30 | call to Query | TaintedPath.go:16:29:16:40 | tainted_path |
45
| TaintedPath.go:13:18:13:30 | call to Query | TaintedPath.go:20:57:20:68 | tainted_path |
56
| TaintedPath.go:20:57:20:68 | tainted_path | TaintedPath.go:20:28:20:69 | call to Join |
67
| tst.go:14:2:14:39 | ... := ...[1] | tst.go:17:41:17:56 | selection of Filename |
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)