github / github/codeql

Go: filepath.Clean with a prepended path separator is modeled as a path-injection sanitizer and causes false negatives

未關閉
#22,185 1 則留言 0 個 reaction 已指派 0 人 在 GitHub 檢視
question
主要語言
CodeQL
星號
10.1k
分支
2.1k
平均合併
2 天 15 小時
30 天內合併 PR
141

描述

**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

貢獻指南

開啟貢獻指南

研究方向

執行 go/ql/test/query-tests/Security/CWE-022/TaintedPath.qlref,並對照 TaintedPath.go 中的預期結果檢查 go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll 裡的 FilepathCleanSanitizer。驗證第 64 行和第 69 行的兩個 filepath.Clean 呼叫仍保留 taint,並回報到達 ioutil.ReadFile 的警示,然後重新執行測試以確認預期結果。

由索引模型根據 Issue 內容生成。

評估

技術堆疊
go
領域
security, testing
Issue 類型
缺陷
難度
3/5
預估耗時
1-2 天
活躍度
冷清
描述清晰度
描述清楚
新手友好度
68/100

把新 issue 寄到你的電子郵件信箱

精選適合新手參與的 GitHub issue 摘要。