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

Esegui go/ql/test/query-tests/Security/CWE-022/TaintedPath.qlref e ispeziona FilepathCleanSanitizer in go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll insieme alle aspettative in TaintedPath.go. Verifica che le due chiamate a filepath.Clean alle righe 64 e 69 mantengano la taint e segnalino avvisi che raggiungono ioutil.ReadFile, quindi esegui nuovamente il test per confermare i risultati attesi.

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