Go: filepath.Clean with a prepended path separator is modeled as a path-injection sanitizer and causes false negatives
- Lenguaje dominante
- CodeQL
- Estrellas
- 10.1k
- Forks
- 2.1k
- Merge medio
- 2 d 15 h
- PR fusionados (30 d)
- 141
Descripción
**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
Guía de contribución
Línea de trabajo
Ejecuta go/ql/test/query-tests/Security/CWE-022/TaintedPath.qlref e inspecciona FilepathCleanSanitizer en go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll junto con las expectativas de TaintedPath.go. Verifica que las dos llamadas a filepath.Clean en las líneas 64 y 69 conserven la taint y generen alertas que lleguen a ioutil.ReadFile; después, vuelve a ejecutar la prueba para confirmar los resultados esperados.
Escrito por el modelo de indexación a partir del texto del issue.
Evaluación
- Stack tecnológico
- go
- Área
- security, testing
- Tipo de issue
- Error
- Dificultad
- 3/5
- Tiempo estimado
- 1-2 días
- Estado de actividad
- Tranquilo
- Claridad
- Bien especificado
- Aptitud para principiantes
- 68/100