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

[wildoranges]

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:

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

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:

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:

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
• TaintedPath.qhelp: normalized paths should be relative or contained within a safe folder

Existing false-negative test cases

The existing TaintedPath.go test
marks both of these sinks as good:

// 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:

   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:

   | 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

[jketema]

Hi @wildoranges,

Thanks for your report. You're indeed right that this is a false negative. Contrary to #22184, the specific edge case you're highlighting was not intentionally included. However, as in the case of #22184, the sanitizer helps to reduce the number of false positives this query might otherwise produce.

We do hope to improve these sanitizers in the future, and will make sure to take this issue into account.