Go: filepath.Clean with a prepended path separator is modeled as a path-injection sanitizer and causes false negatives
- Vorherrschende Sprache
- CodeQL
- Sterne
- 10.1k
- Forks
- 2.1k
- Ø Merge
- 2 T. 15 Std.
- Gemergte PRs (30 T.)
- 141
Beschreibung
**Description of the issue**
The Go `go/path-injection` query treats the result of
`filepath.Clean(constantPrefix + userInput)` as sanitized whenever the first
character of `constantPrefix` is `/` or `\`. This is not sufficient to make a
path safe. `filepath.Clean` normalizes a path, but it does not ensure that the
result is relative or contained within a trusted directory.
As a result, CodeQL suppresses path-injection alerts when attacker-controlled
input is converted into an attacker-controlled absolute path and passed to a
file-system sink. Two instances of this pattern are currently marked as good in
CodeQL's own `TaintedPath.go` test.
**Affected sanitizer**
[`FilepathCleanSanitizer` in `TaintedPathCustomizations.qll`](https://github.com/github/codeql/blob/42843f155e95d25e690aba9ae4620b5a5986a951/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll#L73-L85):
```ql
/**
* A call to `filepath.Clean("/" + e)`, considered to sanitize `e` against path traversal.
*/
class FilepathCleanSanitizer extends Sanitizer {
FilepathCleanSanitizer() {
exists(DataFlow::CallNode cleanCall, StringOps::Concatenation concatNode |
cleanCall = any(Function f | f.hasQualifiedName("path/filepath", "Clean")).getACall() and
concatNode = cleanCall.getArgument(0) and
concatNode.getOperand(0).getStringValue().prefix(1) = ["/", "\\"] and
this = cleanCall.getResult()
)
}
}
```
The sanitizer stops `path-injection` taint at the result of `filepath.Clean`
without checking whether the result is relative or remains under a safe root.
**Why the result is still unsafe**
For example, consider the first existing test case:
```go
taintedPath := "../etc/passwd"
cleaned := filepath.Clean("/" + taintedPath)
fmt.Println(cleaned) // /etc/passwd
```
Prepending `/` prevents `..` from moving above the file-system root, but the
result is still an absolute path controlled by the attacker. An attacker does
not need traversal components at all:
```go
taintedPath := "/etc/passwd"
cleaned := filepath.Clean("/" + taintedPath)
fmt.Println(cleaned) // /etc/passwd
```
The broader sanitizer condition is also unsafe when the constant prefix names
a directory:
```go
taintedPath := "/../../etc/passwd"
cleaned := filepath.Clean("/hardcoded" + taintedPath)
fmt.Println(cleaned) // /etc/passwd
```
`filepath.Clean` only performs lexical normalization. It does not establish a
containment property.
This also conflicts with the query help for `go/path-injection`, which states
that attacker-controlled paths may be absolute and recommends checking that a
normalized path is relative or contained within a safe directory:
- [`TaintedPath.qhelp`: absolute paths can point anywhere on the file system](https://github.com/github/codeql/blob/42843f155e95d25e690aba9ae4620b5a5986a951/go/ql/src/Security/CWE-022/TaintedPath.qhelp#L8-L16)
- [`TaintedPath.qhelp`: normalized paths should be relative or contained within a safe folder](https://github.com/github/codeql/blob/42843f155e95d25e690aba9ae4620b5a5986a951/go/ql/src/Security/CWE-022/TaintedPath.qhelp#L19-L30)
**Existing false-negative test cases**
The existing [`TaintedPath.go` test](https://github.com/github/codeql/blob/42843f155e95d25e690aba9ae4620b5a5986a951/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go#L62-L70)
marks both of these sinks as good:
```go
// GOOD: Sanitized by filepath.Clean with a prepended '/' forcing interpretation
// as an absolute path, so that Clean will throw away any leading `..` components.
data, _ = ioutil.ReadFile(filepath.Clean("/" + tainted_path))
w.Write(data)
// GOOD: Sanitized by filepath.Clean with a prepended os.PathSeparator forcing interpretation
// as an absolute path, so that Clean will throw away any leading `..` components.
data, _ = ioutil.ReadFile(filepath.Clean(string(os.PathSeparator) + "hardcoded" + tainted_path))
w.Write(data)
```
Both calls can resolve to `/etc/passwd` using the inputs shown above. The source
is `r.URL.Query()["path"][0]`, and `ioutil.ReadFile` is a `path-injection` sink,
but no alerts are produced while `FilepathCleanSanitizer` is enabled.
**Steps to reproduce**
1. Run the existing test:
```text
codeql test run go/ql/test/query-tests/Security/CWE-022/TaintedPath.qlref
```
The test passes and no alerts are reported for `TaintedPath.go` lines 64 and
69.
2. Remove only the `FilepathCleanSanitizer` class from
`go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll` and rerun the
same test.
3. The test fails with these two new results:
```text
| TaintedPath.go:64:28:64:61 | call to Clean | ... | user-provided value |
| TaintedPath.go:69:28:69:96 | call to Clean | ... | user-provided value |
| TaintedPath.go:64:28:64:61 | call to Clean | Unexpected result: Alert |
| TaintedPath.go:69:28:69:96 | call to Clean | Unexpected result: Alert |
```
The individual ablation changes no other alert locations in this test. This
confirms that `FilepathCleanSanitizer` is the component suppressing the two
source-to-sink paths.
**Expected behavior**
The result of `filepath.Clean` should not be treated as an unconditional
`path-injection` sanitizer merely because its input has a prepended path
separator. The calls at `TaintedPath.go` lines 64 and 69 should retain taint and
produce alerts when their results reach `ReadFile`.
A possible fix is to remove `FilepathCleanSanitizer` and update the affected
test expectations. Code that separately proves that the normalized path is
relative or contained within a trusted directory can still be handled by the
corresponding validation or guard models.
**Environment**
- CodeQL CLI 2.25.6
- CodeQL repository test baseline: `f6f45d1536`
- Present in `main` at commit `42843f155e95d25e690aba9ae4620b5a5986a951`
- Go 1.22.12 on Linux/amd64
Beitragsleitfaden
Rechercherichtung
Run go/ql/test/query-tests/Security/CWE-022/TaintedPath.qlref and inspect FilepathCleanSanitizer in go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll alongside the expectations in TaintedPath.go. Verify that the two filepath.Clean calls at lines 64 and 69 retain taint and report alerts reaching ioutil.ReadFile, then rerun the test to confirm the expected results.
Vom Indexierungsmodell aus dem Issue-Text verfasst.
Bewertung
- Tech-Stack
- go
- Bereich
- security, testing
- Issue-Typ
- Bug
- Schwierigkeit
- 3/5
- Geschätzter Aufwand
- 1-2 Tage
- Aktivitätsstatus
- Ruhig
- Klarheit
- Klar beschrieben
- Anfängerfreundlichkeit
- 68/100