java.io.File.getName() (Java) modeled as path-injection barrier causes false-negative path-traversal alerts

[wildoranges]

Description of the issue

java.io.File.getName() is modeled as a barrierModel for path-injection in java/ql/lib/ext/java.io.model.yml, but getName() is not a complete path-injection sanitizer — it returns ".." for input "..", and ".." can escape an intended directory when used as a path component. This causes CodeQL to miss path-traversal alerts (false negatives) on code that uses getName() to "sanitize" user-controlled paths.

CodeQL's own test suite (TaintedPath.java) marks such code as GOOD (non-vulnerable). I confirmed the false negative by removing the barrierModel row and re-running the test — the missing alert appears. The barrierModel is unsound because getName() does not eliminate ".."; the exploitability of the specific test case is limited (explained below), but the barrier incorrectly suppresses the taint-flow alert and getName("..") does escape a base directory in the common new File(baseDir, getName(...)) pattern.

The model

java/ql/lib/ext/java.io.model.yml:

Line 99 (summaryModel — taint propagation, correct):

- ["java.io", "File", True, "getName", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]

Line 169 (barrierModel — path-injection sanitizer, the defect):

- ["java.io", "File", True, "getName", "()", "", "ReturnValue", "path-injection", "manual"]

The barrierModel means CodeQL considers the return value of getName() to be safe from path-injection (path-injection taint is stopped at the barrier).

Why File.getName() is not a complete path-injection sanitizer

Per the Java docs, getName() "Returns the name of the file or directory denoted by this abstract pathname." The docs do not specify any special handling of the ".." parent-directory component.

CodeQL's own sanitizer rationale and its gap. getName() was added as a path-injection sanitizer in commit 121780c55a ("Java: add File.getName as a path injection sanitizer"), with this comment on the sanitizer class (in PathSanitizer.qll, before it was converted to MaD):

/**
 * A sanitizer that protects against path injection vulnerabilities
 * by extracting the final component of the user provided path.
 *
 * TODO: convert this class to models-as-data if sanitizer support is added
 */
private class FileGetNameSanitizer extends PathInjectionSanitizer { ... }

The same rationale appears in the test case sendUserFileGood4 added in that commit: // GOOD: only use the final component of the user provided path. The rationale is that extracting the final component strips separator-bearing traversal (e.g. getName("../../etc/passwd") = "passwd"), which is correct. But the rationale does not account for the ".." input: ".." is the final component of "..", and getName("..") returns ".." unchanged — a final component that is itself a traversal vector. The "extracting the final component" rationale is therefore unsound for the ".." edge case, and the barrierModel (added later by commit f6e3c77145, "Convert path injection barrier to MaD", which dropped the comment) inherits this gap. (Note: unlike mime/multipart.FileHeader.Filename, whose model file carries an explicit "adequate" tradeoff comment, the getName barrierModel row has no comment — the rationale lives only in the pre-MaD QL class and the test comment.)

In particular, for the input ".." (which is itself a single name in the pathname's name sequence), getName() returns it unchanged. Verified by running JDK 21.0.11:

new File("..").getName()                    = ".."        // not eliminated — ".." is the last (only) name
new File(".").getName()                     = "."
new File("/etc/passwd").getName()           = "passwd"    // separators stripped, safe
new File("/var/../etc/passwd").getName()    = "passwd"    // separators stripped, safe
new File("").getName()                      = ""

getName() does strip directory separators (so "/etc/passwd" → "passwd" is safe), but it does not eliminate ".." — because ".." is a single name, not a separator. So the return value is unsafe when used as a path component:

String baseDir = "/home/user/safepath";
File f = new File(baseDir, new File(userInput).getName());   // userInput = ".."  =>  baseDir/..  =  parent of baseDir

Verified by running JDK 21.0.11:

new File(baseDir, new File("..").getName())  = /home/user/safepath/..
  -> canonical path: /home/user             // escapes baseDir

The barrierModel would suppress the path-injection alert on any sink fed getName()'s return value (including the new File(baseDir, ...) pattern and the FileReader(baseName) test case below), because taint of kind path-injection is stopped at getName().

Confirmed false negatives (with ablation)

java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java lines 96-109 (sendUserFileGood4):

public void sendUserFileGood4(Socket sock, String user) throws IOException {
    BufferedReader filenameReader =
            new BufferedReader(new InputStreamReader(sock.getInputStream(), "UTF-8"));
    String filename = filenameReader.readLine();   // tainted (from socket)
    File file = new File(filename);
    String baseName = file.getName();              // GOOD comment: "only use the final component"
    // GOOD: only use the final component of the user provided path
    BufferedReader fileReader = new BufferedReader(new FileReader(baseName));
    ...
}

• filename is tainted (from sock.getInputStream() via readLine)
• FileReader is a path-injection sink
• TaintedPath.expected reports path-injection alerts for TaintedPath.java lines 13, 14, 16, but not for line 103 (new FileReader(baseName) where baseName = file.getName())

If File.getName() were not a barrierModel, path-injection taint would flow filename → new File(filename) → file.getName() → baseName → new FileReader(baseName) and trigger an alert. The barrier suppresses this alert.

Exploitability of the test case (important caveat). baseName can be ".." when filename = ".." (verified by running JDK 21.0.11: new File("..").getName() returns ".."):

filename = "..";
File file = new File("..");
String baseName = file.getName();  // ".."  — getName() does not eliminate ".."
new FileReader(baseName);          // baseName = "..", tainted path reaches the sink

However, in this specific test case the sink is new FileReader(baseName), and new FileReader("..") throws FileNotFoundException: .. (Is a directory) (verified on JDK 21.0.11) — ".." is a directory, not a readable file. So this specific FileReader use is not directly exploitable for reading an arbitrary file. The taint-flow alert is still legitimately raised by the query (the taint reaches the sink unsanitized, which is what path-injection detection flags), but the test case as written is a weak demonstration of exploitable harm.

Where getName("..") does cause real harm: the new File(baseDir, getName(...)) pattern. The common, intended use of getName() is to extract a filename and combine it with a safe base directory. In that pattern ".." escapes the base directory:

String baseDir = "/home/user/uploads";
File f = new File(baseDir, new File(userInput).getName());   // userInput = ".."  =>  baseDir/..  =  parent of baseDir

Verified by running JDK 21.0.11:

new File("/home/user/uploads", new File("..").getName()).getCanonicalPath()  = /home/user   // escapes baseDir

getName() strips separator-bearing traversal (e.g. getName("../../etc/passwd") = "passwd", which stays inside baseDir), so this is limited to escaping one level (to baseDir's parent) — it is not arbitrary path traversal. The impact is bounded (e.g. new File(baseDir, getName("..")).createNewFile() resolves to baseDir's parent, an existing directory, and returns false; delete() on a non-empty parent returns false), but new File(baseDir, getName("..")) does resolve to a path outside baseDir, so any path-injection sink acting on it (delete/mkdir/createNewFile/deleteOnExit) operates outside the intended directory — which is exactly what path-injection detection is meant to flag. The barrierModel suppresses the alert for these sinks too, because it marks getName()'s return value as safe for all path-injection sinks, not just FileReader.

So the barrierModel is unsound: getName() does not sanitize "..", the taint reaches path-injection sinks unsanitized (confirmed by ablation), and in the new File(baseDir, getName(...)) pattern it escapes the intended directory.

Ablation confirmation: I removed the File.getName() barrierModel row from java/ql/lib/ext/java.io.model.yml and re-ran the test. The .actual output now includes line 103:

| TaintedPath.java:103:71:103:78 | baseName | TaintedPath.java:98:58:98:78 | getInputStream(...) : InputStream | TaintedPath.java:103:71:103:78 | baseName | This path depends on a $@. | TaintedPath.java:98:58:98:78 | getInputStream(...) | user-provided value |

This confirms the barrierModel is suppressing a legitimate path-injection alert. The taint flows getInputStream (line 98, Source) → readLine (line 99) → filename (line 100) → new File (line 100) → file.getName() (line 101) → baseName (line 103) → new FileReader(baseName) (Sink), and baseName can be ".." (a traversal path).

Re-adding the barrierModel row restores the baseline (line 103 is no longer flagged, test passes) — the alert appears and disappears exactly with the barrier, confirming it is the cause of the false negative.

Steps to reproduce

1. Run the TaintedPath query on the existing test:

   codeql test run java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.qlref
   

   Observe that line 103 (new FileReader(baseName) where baseName = file.getName()) is not in the alerts (only lines 13, 14, 16 of TaintedPath.java are flagged). The test passes (.actual matches .expected).

2. Remove the File.getName() barrierModel row from java/ql/lib/ext/java.io.model.yml:

   # Remove this line (the entire barrierModel block):
   - ["java.io", "File", True, "getName", "()", "", "ReturnValue", "path-injection", "manual"]

   and re-run. The test now fails with an unexpected alert on line 103 (Unexpected result: Alert on baseName), with the full taint trace getInputStream → readLine → filename → new File → getName → baseName → FileReader.

3. Re-add the barrierModel row and re-run; the alert disappears and the test passes again (baseline restored).

Suggested fix

Remove the barrierModel row for File.getName():

# Remove this line — File.getName() is not a complete path-injection sanitizer (returns ".." for ".." input)
- ["java.io", "File", True, "getName", "()", "", "ReturnValue", "path-injection", "manual"]

Keep the summaryModel row (taint propagation is correct):

- ["java.io", "File", True, "getName", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]

Additionally, the TaintedPath.java test case sendUserFileGood4 (lines 96-109) should be re-evaluated. It is currently marked GOOD with the comment "only use the final component of the user provided path". That reasoning holds for separator-bearing inputs (getName("../../etc/passwd") = "passwd", which cannot traverse), but it does not hold for filename = "..", where getName("..") = ".." and the taint reaches the FileReader sink unsanitized. Note that this specific test case uses new FileReader(baseName), and FileReader("..") throws (it is a directory, not a file), so the test case is a weak demonstration of exploitable harm — but the getName() barrier is unsound in general (the new File(baseDir, getName(...)) pattern with directory-operation sinks does escape baseDir, as shown above).

Context

• Tested with CodeQL CLI 2.25.6, repo HEAD a24d222d96 (codeql-cli/latest-577-ga24d222d96).
• The barrierModel for getName() was added in commit f6e3c77145 ("Convert path injection barrier to MaD").
• The summaryModel row for getName() (taint propagation, Argument[this] → ReturnValue, kind taint) is correct and is not affected by this fix — only the path-injection barrierModel is unsound.

Notes

• getName() does strip directory separators (returns only the last component), so it prevents traversal in most cases (e.g. getName() on "/etc/passwd" returns "passwd", which cannot traverse, and getName("../../etc/passwd") returns "passwd"). The defect is specifically the ".." (and ".", and empty-string) edge case, where the input is a single name rather than a path with separators. Verified by running JDK 21.0.11: new File("..").getName() → "..".
• Impact is limited to escaping one level. Because getName() strips separator-bearing traversal, only the ".." case escapes, and ".." resolves to the parent of the base directory (not an arbitrary path). This is less severe than path/filepath.Rel (which preserves .. and can reach arbitrary paths), but it is still a real path-injection gap: getName() is not a sanitizer, the barrier suppresses a legitimate taint-flow alert, and directory-operation sinks acting on new File(baseDir, getName("..")) operate outside the intended directory.
• The specific test case (FileReader(baseName)) is a weak demonstration. new FileReader("..") throws (directory, not file), so that exact line is not directly exploitable for reading an arbitrary file. The ablation still confirms the barrier suppresses a taint-flow alert, and the new File(baseDir, getName(...)) pattern is the case where ".." escapes baseDir (impact bounded to one level, as noted above).
• A conditional barrier (exclude ".."/"."/empty) would be sound, but CodeQL MaD barrierModel does not support conditions, so removal is the simpler fix.