Skip to content

Commit 807ef2e

Browse files
owen-mcowen-mc
authored
Merge pull request #700 from smowton/smowton/fix/filepath-clean
Treat path.Clean and filepath.Clean alike re: tainted path sanitization
2 parents 30c8062 + e808423 commit 807ef2e

File tree

3 files changed

+27
-23
lines changed

3 files changed

+27
-23
lines changed

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,12 +77,13 @@ module TaintedPath {
7777
}
7878

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

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

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,19 @@
11
edges
2-
| TaintedPath.go:12:10:12:14 | selection of URL : pointer type | TaintedPath.go:15:29:15:32 | path |
3-
| TaintedPath.go:12:10:12:14 | selection of URL : pointer type | TaintedPath.go:19:28:19:61 | call to Join |
2+
| TaintedPath.go:13:18:13:22 | selection of URL : pointer type | TaintedPath.go:16:29:16:40 | tainted_path |
3+
| TaintedPath.go:13:18:13:22 | selection of URL : pointer type | TaintedPath.go:20:28:20:69 | call to Join |
44
| tst.go:14:2:14:39 | ... := ...[1] : pointer type | tst.go:17:41:17:47 | implicit dereference : FileHeader |
55
| tst.go:14:2:14:39 | ... := ...[1] : pointer type | tst.go:17:41:17:56 | selection of Filename |
66
| tst.go:17:41:17:47 | implicit dereference : FileHeader | tst.go:17:41:17:47 | implicit dereference : FileHeader |
77
| tst.go:17:41:17:47 | implicit dereference : FileHeader | tst.go:17:41:17:56 | selection of Filename |
88
nodes
9-
| TaintedPath.go:12:10:12:14 | selection of URL : pointer type | semmle.label | selection of URL : pointer type |
10-
| TaintedPath.go:15:29:15:32 | path | semmle.label | path |
11-
| TaintedPath.go:19:28:19:61 | call to Join | semmle.label | call to Join |
9+
| TaintedPath.go:13:18:13:22 | selection of URL : pointer type | semmle.label | selection of URL : pointer type |
10+
| TaintedPath.go:16:29:16:40 | tainted_path | semmle.label | tainted_path |
11+
| TaintedPath.go:20:28:20:69 | call to Join | semmle.label | call to Join |
1212
| tst.go:14:2:14:39 | ... := ...[1] : pointer type | semmle.label | ... := ...[1] : pointer type |
1313
| tst.go:17:41:17:47 | implicit dereference : FileHeader | semmle.label | implicit dereference : FileHeader |
1414
| tst.go:17:41:17:56 | selection of Filename | semmle.label | selection of Filename |
1515
subpaths
1616
#select
17-
| TaintedPath.go:15:29:15:32 | path | TaintedPath.go:12:10:12:14 | selection of URL : pointer type | TaintedPath.go:15:29:15:32 | path | This path depends on $@. | TaintedPath.go:12:10:12:14 | selection of URL | a user-provided value |
18-
| TaintedPath.go:19:28:19:61 | call to Join | TaintedPath.go:12:10:12:14 | selection of URL : pointer type | TaintedPath.go:19:28:19:61 | call to Join | This path depends on $@. | TaintedPath.go:12:10:12:14 | selection of URL | a user-provided value |
17+
| 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 $@. | TaintedPath.go:13:18:13:22 | selection of URL | a user-provided value |
18+
| 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 $@. | TaintedPath.go:13:18:13:22 | selection of URL | a user-provided value |
1919
| 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 $@. | tst.go:14:2:14:39 | ... := ...[1] | a user-provided value |

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

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3,57 +3,60 @@ package main
33
import (
44
"io/ioutil"
55
"net/http"
6+
"path"
67
"path/filepath"
78
"regexp"
89
"strings"
910
)
1011

1112
func handler(w http.ResponseWriter, r *http.Request) {
12-
path := r.URL.Query()["path"][0]
13+
tainted_path := r.URL.Query()["path"][0]
1314

1415
// BAD: This could read any file on the file system
15-
data, _ := ioutil.ReadFile(path)
16+
data, _ := ioutil.ReadFile(tainted_path)
1617
w.Write(data)
1718

1819
// BAD: This could still read any file on the file system
19-
data, _ = ioutil.ReadFile(filepath.Join("/home/user/", path))
20+
data, _ = ioutil.ReadFile(filepath.Join("/home/user/", tainted_path))
2021
w.Write(data)
2122

2223
// GOOD: This can only read inside the provided safe path
23-
sanitized_filepath, _ := filepath.Rel("/home/user/safepath", path)
24+
sanitized_filepath, _ := filepath.Rel("/home/user/safepath", tainted_path)
2425
data, _ = ioutil.ReadFile(sanitized_filepath)
2526
w.Write(data)
2627

2728
// GOOD: This can only read inside the provided safe path
28-
if !strings.Contains(path, "..") {
29-
data, _ = ioutil.ReadFile(path)
29+
if !strings.Contains(tainted_path, "..") {
30+
data, _ = ioutil.ReadFile(tainted_path)
3031
w.Write(data)
3132
}
3233

3334
// GOOD: This can only read inside the provided safe path
34-
_, err := filepath.Rel("/home/user/safepath", path)
35+
_, err := filepath.Rel("/home/user/safepath", tainted_path)
3536
if err == nil {
36-
data, _ = ioutil.ReadFile(path)
37+
data, _ = ioutil.ReadFile(tainted_path)
3738
w.Write(data)
3839
}
3940

4041
// GOOD: An attempt has been made to ensure that this can only read inside
4142
// the provided safe path
42-
if strings.HasPrefix(path, "/home/user/safepath/") {
43-
data, _ = ioutil.ReadFile(path)
43+
if strings.HasPrefix(tainted_path, "/home/user/safepath/") {
44+
data, _ = ioutil.ReadFile(tainted_path)
4445
w.Write(data)
4546
}
4647

4748
// GOOD: An attempt has been made to ensure that this can only read inside
4849
// the provided safe path
49-
matched, _ := regexp.MatchString("\\.\\.", path)
50+
matched, _ := regexp.MatchString("\\.\\.", tainted_path)
5051
if !matched {
51-
data, _ = ioutil.ReadFile(filepath.Join("/home/user/", path))
52+
data, _ = ioutil.ReadFile(filepath.Join("/home/user/", tainted_path))
5253
w.Write(data)
5354
}
5455

55-
// GOOD: Sanitized by filepath.Clean with a prepended '/' forcing interpretation
56+
// GOOD: Sanitized by [file]path.Clean with a prepended '/' forcing interpretation
5657
// as an absolute path, so that Clean will throw away any leading `..` components.
57-
data, _ = ioutil.ReadFile(filepath.Clean("/" + path))
58+
data, _ = ioutil.ReadFile(filepath.Clean("/" + tainted_path))
59+
w.Write(data)
60+
data, _ = ioutil.ReadFile(path.Clean("/" + tainted_path))
5861
w.Write(data)
5962
}

0 commit comments

Comments
 (0)