Skip to content

Commit 233c3ce

Browse files
owen-mcowen-mc
committed
Fix incorrect sanitizers
1 parent 747aa45 commit 233c3ce

7 files changed

Lines changed: 40 additions & 54 deletions

File tree

‎go/ql/lib/change-notes/2026-07-09-remove-incorrect-path-sanitizer.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The function `Rel` in `path/filepath` was incorrectly considered a sanitizer for `go/path-injection` and `go/zipslip`. This has now been fixed, which may lead to more results for those queries.

‎go/ql/lib/ext/path.filepath.model.yml‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,4 @@
11
extensions:
2-
- addsTo:
3-
pack: codeql/go-all
4-
extensible: barrierModel
5-
data:
6-
- ["path/filepath", "", False, "Rel", "", "", "ReturnValue", "path-injection", "manual"]
72
- addsTo:
83
pack: codeql/go-all
94
extensible: summaryModel

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

Lines changed: 0 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -129,43 +129,6 @@ module TaintedPath {
129129
DotDotReplaceAll() { this.getReplacedString() = ["..", "."] }
130130
}
131131

132-
/**
133-
* A node `nd` guarded by a check that ensures it is contained within some root folder,
134-
* considered as a sanitizer for path traversal.
135-
*
136-
* We currently recognize checks of the following form:
137-
*
138-
* ```
139-
* ..., err := filepath.Rel(base, path)
140-
* if err == nil {
141-
* // path is known to be contained in base
142-
* }
143-
* ```
144-
*/
145-
class PathContainmentCheck extends SanitizerGuard, DataFlow::EqualityTestNode {
146-
DataFlow::Node path;
147-
boolean outcome;
148-
149-
PathContainmentCheck() {
150-
exists(Function f, FunctionInput inp, FunctionOutput outp, DataFlow::Property p |
151-
f.hasQualifiedName("path/filepath", "Rel") and
152-
inp.isParameter(1) and
153-
outp.isResult(1) and
154-
p.isNil()
155-
|
156-
exists(DataFlow::Node call, DataFlow::Node res |
157-
call = f.getACall() and
158-
DataFlow::localFlow(outp.getNode(call), res)
159-
|
160-
p.checkOn(this, outcome, res) and
161-
path = inp.getNode(call)
162-
)
163-
)
164-
}
165-
166-
override predicate checks(Expr e, boolean branch) { e = path.asExpr() and branch = outcome }
167-
}
168-
169132
/**
170133
* A call of the form `strings.HasPrefix(path, ...)` considered as a sanitizer guard
171134
* for `path`.

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

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,36 @@
11
#select
22
| TaintedPath.go:18:29:18:40 | tainted_path | TaintedPath.go:15:18:15:22 | selection of URL | TaintedPath.go:18:29:18:40 | tainted_path | This path depends on a $@. | TaintedPath.go:15:18:15:22 | selection of URL | user-provided value |
33
| TaintedPath.go:22:28:22:69 | call to Join | TaintedPath.go:15:18:15:22 | selection of URL | TaintedPath.go:22:28:22:69 | call to Join | This path depends on a $@. | TaintedPath.go:15:18:15:22 | selection of URL | user-provided value |
4+
| TaintedPath.go:27:28:27:45 | sanitized_filepath | TaintedPath.go:15:18:15:22 | selection of URL | TaintedPath.go:27:28:27:45 | sanitized_filepath | This path depends on a $@. | TaintedPath.go:15:18:15:22 | selection of URL | user-provided value |
5+
| TaintedPath.go:43:29:43:40 | tainted_path | TaintedPath.go:15:18:15:22 | selection of URL | TaintedPath.go:43:29:43:40 | tainted_path | This path depends on a $@. | TaintedPath.go:15:18:15:22 | selection of URL | user-provided value |
46
| TaintedPath.go:74:28:74:57 | call to Clean | TaintedPath.go:15:18:15:22 | selection of URL | TaintedPath.go:74:28:74:57 | call to Clean | This path depends on a $@. | TaintedPath.go:15:18:15:22 | selection of URL | user-provided value |
57
edges
68
| TaintedPath.go:15:18:15:22 | selection of URL | TaintedPath.go:15:18:15:30 | call to Query | provenance | Src:MaD:2 MaD:3 |
79
| TaintedPath.go:15:18:15:30 | call to Query | TaintedPath.go:18:29:18:40 | tainted_path | provenance | Sink:MaD:1 |
810
| TaintedPath.go:15:18:15:30 | call to Query | TaintedPath.go:22:57:22:68 | tainted_path | provenance | |
911
| TaintedPath.go:22:57:22:68 | tainted_path | TaintedPath.go:22:28:22:69 | call to Join | provenance | FunctionModel Sink:MaD:1 |
12+
| TaintedPath.go:22:57:22:68 | tainted_path | TaintedPath.go:26:63:26:74 | tainted_path | provenance | |
13+
| TaintedPath.go:22:57:22:68 | tainted_path | TaintedPath.go:43:29:43:40 | tainted_path | provenance | Sink:MaD:1 |
1014
| TaintedPath.go:22:57:22:68 | tainted_path | TaintedPath.go:74:39:74:56 | ...+... | provenance | |
11-
| TaintedPath.go:74:39:74:56 | ...+... | TaintedPath.go:74:28:74:57 | call to Clean | provenance | MaD:4 Sink:MaD:1 |
15+
| TaintedPath.go:26:2:26:75 | ... := ...[0] | TaintedPath.go:27:28:27:45 | sanitized_filepath | provenance | Sink:MaD:1 |
16+
| TaintedPath.go:26:63:26:74 | tainted_path | TaintedPath.go:26:2:26:75 | ... := ...[0] | provenance | MaD:4 |
17+
| TaintedPath.go:74:39:74:56 | ...+... | TaintedPath.go:74:28:74:57 | call to Clean | provenance | MaD:5 Sink:MaD:1 |
1218
models
1319
| 1 | Sink: io/ioutil; ; false; ReadFile; ; ; Argument[0]; path-injection; manual |
1420
| 2 | Source: net/http; Request; true; URL; ; ; ; remote; manual |
1521
| 3 | Summary: net/url; URL; true; Query; ; ; Argument[receiver]; ReturnValue; taint; manual |
16-
| 4 | Summary: path; ; false; Clean; ; ; Argument[0]; ReturnValue; taint; manual |
22+
| 4 | Summary: path/filepath; ; false; Rel; ; ; Argument[0..1]; ReturnValue[0]; taint; manual |
23+
| 5 | Summary: path; ; false; Clean; ; ; Argument[0]; ReturnValue; taint; manual |
1724
nodes
1825
| TaintedPath.go:15:18:15:22 | selection of URL | semmle.label | selection of URL |
1926
| TaintedPath.go:15:18:15:30 | call to Query | semmle.label | call to Query |
2027
| TaintedPath.go:18:29:18:40 | tainted_path | semmle.label | tainted_path |
2128
| TaintedPath.go:22:28:22:69 | call to Join | semmle.label | call to Join |
2229
| TaintedPath.go:22:57:22:68 | tainted_path | semmle.label | tainted_path |
30+
| TaintedPath.go:26:2:26:75 | ... := ...[0] | semmle.label | ... := ...[0] |
31+
| TaintedPath.go:26:63:26:74 | tainted_path | semmle.label | tainted_path |
32+
| TaintedPath.go:27:28:27:45 | sanitized_filepath | semmle.label | sanitized_filepath |
33+
| TaintedPath.go:43:29:43:40 | tainted_path | semmle.label | tainted_path |
2334
| TaintedPath.go:74:28:74:57 | call to Clean | semmle.label | call to Clean |
2435
| TaintedPath.go:74:39:74:56 | ...+... | semmle.label | ...+... |
2536
subpaths

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,9 @@ func handler(w http.ResponseWriter, r *http.Request) {
2222
data, _ = ioutil.ReadFile(filepath.Join("/home/user/", tainted_path)) // $ Alert[go/path-injection]
2323
w.Write(data)
2424

25-
// GOOD: This can only read inside the provided safe path
25+
// BAD: This could still read any file on the file system
2626
sanitized_filepath, _ := filepath.Rel("/home/user/safepath", tainted_path)
27-
data, _ = ioutil.ReadFile(sanitized_filepath)
27+
data, _ = ioutil.ReadFile(sanitized_filepath) // $ Alert[go/path-injection]
2828
w.Write(data)
2929

3030
// GOOD: This can only read inside the provided safe path
@@ -37,10 +37,10 @@ func handler(w http.ResponseWriter, r *http.Request) {
3737
data, _ = ioutil.ReadFile(strings.ReplaceAll(tainted_path, "..", ""))
3838
w.Write(data)
3939

40-
// GOOD: This can only read inside the provided safe path
40+
// BAD: This could still read any file on the file system
4141
_, err := filepath.Rel("/home/user/safepath", tainted_path)
4242
if err == nil {
43-
data, _ = ioutil.ReadFile(tainted_path)
43+
data, _ = ioutil.ReadFile(tainted_path) // $ Alert[go/path-injection]
4444
w.Write(data)
4545
}
4646

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

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@
22
| UnsafeUnzipSymlinkGood.go:72:3:72:25 | ... := ...[0] | UnsafeUnzipSymlinkGood.go:72:3:72:25 | ... := ...[0] | UnsafeUnzipSymlinkGood.go:61:31:61:62 | call to Join | Unsanitized archive entry, which may contain '..', is used in a $@. | UnsafeUnzipSymlinkGood.go:61:31:61:62 | call to Join | file system operation |
33
| ZipSlip.go:11:2:15:2 | range statement[1] | ZipSlip.go:11:2:15:2 | range statement[1] | ZipSlip.go:14:20:14:20 | p | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.go:14:20:14:20 | p | file system operation |
44
| tarslip.go:15:2:15:30 | ... := ...[0] | tarslip.go:15:2:15:30 | ... := ...[0] | tarslip.go:16:14:16:34 | call to Dir | Unsanitized archive entry, which may contain '..', is used in a $@. | tarslip.go:16:14:16:34 | call to Dir | file system operation |
5+
| tst.go:23:2:43:2 | range statement[1] | tst.go:23:2:43:2 | range statement[1] | tst.go:27:21:27:48 | call to Join | Unsanitized archive entry, which may contain '..', is used in a $@. | tst.go:27:21:27:48 | call to Join | file system operation |
56
| tst.go:23:2:43:2 | range statement[1] | tst.go:23:2:43:2 | range statement[1] | tst.go:29:20:29:23 | path | Unsanitized archive entry, which may contain '..', is used in a $@. | tst.go:29:20:29:23 | path | file system operation |
7+
| tst.go:23:2:43:2 | range statement[1] | tst.go:23:2:43:2 | range statement[1] | tst.go:31:21:31:24 | path | Unsanitized archive entry, which may contain '..', is used in a $@. | tst.go:31:21:31:24 | path | file system operation |
68
edges
79
| UnsafeUnzipSymlinkGood.go:52:24:52:32 | SSA def(candidate) | UnsafeUnzipSymlinkGood.go:61:53:61:61 | candidate | provenance | |
810
| UnsafeUnzipSymlinkGood.go:61:53:61:61 | candidate | UnsafeUnzipSymlinkGood.go:61:31:61:62 | call to Join | provenance | FunctionModel Sink:MaD:3 |
@@ -14,14 +16,20 @@ edges
1416
| ZipSlip.go:12:3:12:30 | ... := ...[0] | ZipSlip.go:14:20:14:20 | p | provenance | Sink:MaD:1 |
1517
| ZipSlip.go:12:24:12:29 | selection of Name | ZipSlip.go:12:3:12:30 | ... := ...[0] | provenance | MaD:4 |
1618
| tarslip.go:15:2:15:30 | ... := ...[0] | tarslip.go:16:23:16:33 | selection of Name | provenance | |
17-
| tarslip.go:16:23:16:33 | selection of Name | tarslip.go:16:14:16:34 | call to Dir | provenance | MaD:5 Sink:MaD:2 |
19+
| tarslip.go:16:23:16:33 | selection of Name | tarslip.go:16:14:16:34 | call to Dir | provenance | MaD:6 Sink:MaD:2 |
20+
| tst.go:23:2:43:2 | range statement[1] | tst.go:25:38:25:41 | path | provenance | |
1821
| tst.go:23:2:43:2 | range statement[1] | tst.go:29:20:29:23 | path | provenance | Sink:MaD:1 |
22+
| tst.go:23:2:43:2 | range statement[1] | tst.go:31:21:31:24 | path | provenance | Sink:MaD:1 |
23+
| tst.go:25:3:25:42 | ... := ...[0] | tst.go:27:41:27:47 | relpath | provenance | |
24+
| tst.go:25:38:25:41 | path | tst.go:25:3:25:42 | ... := ...[0] | provenance | MaD:5 |
25+
| tst.go:27:41:27:47 | relpath | tst.go:27:21:27:48 | call to Join | provenance | FunctionModel Sink:MaD:1 |
1926
models
2027
| 1 | Sink: io/ioutil; ; false; WriteFile; ; ; Argument[0]; path-injection; manual |
2128
| 2 | Sink: os; ; false; MkdirAll; ; ; Argument[0]; path-injection; manual |
2229
| 3 | Sink: os; ; false; Readlink; ; ; Argument[0]; path-injection; manual |
2330
| 4 | Summary: path/filepath; ; false; Abs; ; ; Argument[0]; ReturnValue[0]; taint; manual |
24-
| 5 | Summary: path; ; false; Dir; ; ; Argument[0]; ReturnValue; taint; manual |
31+
| 5 | Summary: path/filepath; ; false; Rel; ; ; Argument[0..1]; ReturnValue[0]; taint; manual |
32+
| 6 | Summary: path; ; false; Dir; ; ; Argument[0]; ReturnValue; taint; manual |
2533
nodes
2634
| UnsafeUnzipSymlinkGood.go:52:24:52:32 | SSA def(candidate) | semmle.label | SSA def(candidate) |
2735
| UnsafeUnzipSymlinkGood.go:61:31:61:62 | call to Join | semmle.label | call to Join |
@@ -37,5 +45,10 @@ nodes
3745
| tarslip.go:16:14:16:34 | call to Dir | semmle.label | call to Dir |
3846
| tarslip.go:16:23:16:33 | selection of Name | semmle.label | selection of Name |
3947
| tst.go:23:2:43:2 | range statement[1] | semmle.label | range statement[1] |
48+
| tst.go:25:3:25:42 | ... := ...[0] | semmle.label | ... := ...[0] |
49+
| tst.go:25:38:25:41 | path | semmle.label | path |
50+
| tst.go:27:21:27:48 | call to Join | semmle.label | call to Join |
51+
| tst.go:27:41:27:47 | relpath | semmle.label | relpath |
4052
| tst.go:29:20:29:23 | path | semmle.label | path |
53+
| tst.go:31:21:31:24 | path | semmle.label | path |
4154
subpaths

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ func uploadFile(w http.ResponseWriter, r *http.Request) {
1414
file, handler, _ := r.FormFile("file")
1515
// err handling
1616
defer file.Close()
17-
tempFile, _ := ioutil.TempFile("/tmp", handler.Filename) // NOT OK
17+
tempFile, _ := ioutil.TempFile("/tmp", handler.Filename) // OK
1818
use(tempFile)
1919
}
2020

@@ -24,11 +24,11 @@ func unzip2(f string, root string) {
2424
path := f.Name
2525
relpath, err := filepath.Rel(root, path)
2626
if err == nil {
27-
ioutil.WriteFile(filepath.Join(root, relpath), []byte("present"), 0666) // OK
27+
ioutil.WriteFile(filepath.Join(root, relpath), []byte("present"), 0666) // $ Sink[go/zipslip]
2828
}
29-
ioutil.WriteFile(path, []byte("present"), 0666) // $ Sink[go/zipslip] // NOT OK
29+
ioutil.WriteFile(path, []byte("present"), 0666) // $ Sink[go/zipslip]
3030
if containedIn(path, root) {
31-
ioutil.WriteFile(path, []byte("present"), 0666) // OK
31+
ioutil.WriteFile(path, []byte("present"), 0666) // $ Sink[go/zipslip]
3232
}
3333
if ok, _ := regexp.MatchString("^[a-z]*$", path); ok {
3434
ioutil.WriteFile(path, []byte("present"), 0666) // OK

0 commit comments

Comments
 (0)