Go: DotDotReplaceAll ignores the replacement value and causes false-negative path-injection alerts

[wildoranges]

Description of the issue

The Go go/path-injection query treats expressions modeled by
StringOps::ReplaceAll as sanitized when the replaced string is "." or
"..". The sanitizer does not check the replacement string or establish that
the resulting path is relative or contained within a trusted directory.

This causes false negatives for at least two independent reasons:

1. The replacement can make the path more dangerous, because the model checks
   only the string being replaced and not the replacement value.
2. Even replacing ".." with the empty string can leave or create an
   attacker-controlled absolute path.

Affected sanitizer

DotDotReplaceAll in TaintedPathCustomizations.qll:

/**
 * A replacement of the form `!strings.ReplaceAll(nd, "..")` or
 * `!strings.ReplaceAll(nd, ".")`, considered as a sanitizer for path traversal.
 */
class DotDotReplaceAll extends StringOps::ReplaceAll, Sanitizer {
  DotDotReplaceAll() { this.getReplacedString() = ["..", "."] }
}

StringOps::ReplaceAll.getReplacedString() represents the old argument. For
strings.ReplaceAll(s, old, new), the model exposes and checks old, but not
new:

StringOps.qll:

class ReplaceAll extends DataFlow::Node instanceof ReplaceAll::Range {
  /** Gets the `old` in `strings.ReplaceAll(s, old, new)`. */
  string getReplacedString() { result = super.getReplacedString() }
}

The standard-library implementation of this range
similarly obtains only argument 1 (old):

override string getReplacedString() { result = this.getArgument(1).getStringValue() }

Consequently, any replacement value is accepted by DotDotReplaceAll.

False negative when the replacement increases traversal

For example:

func handler(w http.ResponseWriter, r *http.Request) {
    taintedPath := r.URL.Query().Get("path")
    path := strings.ReplaceAll(taintedPath, "..", "../..")
    data, _ := os.ReadFile(path) // expected: go/path-injection
    w.Write(data)
}

For taintedPath = "../secret", the replacement produces
"../../secret". It increases the number of parent-directory components, but
CodeQL treats the return value as sanitized because the old argument is
"..".

I also verified this directly in the existing TaintedPath.go test by changing
the replacement at line 37 to "../.." and adding an expected
go/path-injection alert. With DotDotReplaceAll enabled, the test reports:

| TaintedPath.go:37:77:37:105 | comment | Missing result: Alert[go/path-injection] |

False negative with an empty replacement

The existing test uses an empty replacement:

TaintedPath.go lines 36-38:

// GOOD: Sanitized by strings.ReplaceAll and replaces all .. with empty string
data, _ = ioutil.ReadFile(strings.ReplaceAll(tainted_path, "..", ""))
w.Write(data)

Removing ".." does not ensure that the path is safe:

strings.ReplaceAll("..../etc/passwd", "..", "") // "/etc/passwd"
strings.ReplaceAll("/etc/passwd", "..", "")     // "/etc/passwd"

In both cases the result is an attacker-controlled absolute path. This is
consistent with the query help, which says that absolute paths can point
anywhere on the file system and similarly warns that naive removal of traversal
sequences can be insufficient:

• TaintedPath.qhelp: absolute paths can point anywhere on the file system
• TaintedPath.qhelp: warning about naive traversal-sequence removal

Steps to reproduce using the existing test

1. Run the existing test:

   codeql test run go/ql/test/query-tests/Security/CWE-022/TaintedPath.qlref
   

   The test passes and no alert is reported for the ReplaceAll result at
   TaintedPath.go line 37.

2. Remove only the DotDotReplaceAll class from
   go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll and rerun the
   same test.

3. The test fails with the new source-to-sink result:

   | TaintedPath.go:37:28:37:69 | call to ReplaceAll | ... | user-provided value |
   | TaintedPath.go:37:28:37:69 | call to ReplaceAll | Unexpected result: Alert |
   

The individual ablation changes no other alert locations in this test. This
confirms that DotDotReplaceAll suppresses the flow from the URL-derived path
through strings.ReplaceAll to ioutil.ReadFile.

Expected behavior

Replacing "." or ".." should not be treated as an unconditional
path-injection sanitizer. In particular, a replacement operation must not
stop taint without considering the replacement value and the properties of the
resulting path.

Since even an empty replacement does not prove that the result is relative or
contained within a safe directory, a possible fix is to remove
DotDotReplaceAll and update the affected test expectation. Sound validation
can instead rely on checks that establish locality or containment.

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. This works as intended: We assume that if you are replacing ".." with something else, then you know what you're doing. This is clearly an approximation, but it helps us to reduce the number of false positives this query might otherwise produce.

We do hope to improve these sanitizers in the future.