diff --git a/java/ql/lib/change-notes/2026-07-11-file-getname-path-sanitizer.md b/java/ql/lib/change-notes/2026-07-11-file-getname-path-sanitizer.md new file mode 100644 index 000000000000..dddcae521ff3 --- /dev/null +++ b/java/ql/lib/change-notes/2026-07-11-file-getname-path-sanitizer.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* `java.io.File.getName()` is no longer treated as a complete sanitizer for `java/path-injection`, since it does not remove a `..` path component (for example `new File("..").getName()` returns `".."`). It is now only recognized as a sanitizer when combined with a subsequent check for `..` components, which may result in new alerts. diff --git a/java/ql/lib/ext/java.io.model.yml b/java/ql/lib/ext/java.io.model.yml index dd47342d590a..f7c236de4eeb 100644 --- a/java/ql/lib/ext/java.io.model.yml +++ b/java/ql/lib/ext/java.io.model.yml @@ -162,8 +162,3 @@ extensions: extensible: sourceModel data: - ["java.io", "FileInputStream", True, "FileInputStream", "", "", "Argument[this]", "file", "manual"] - - addsTo: - pack: codeql/java-all - extensible: barrierModel - data: - - ["java.io", "File", True, "getName", "()", "", "ReturnValue", "path-injection", "manual"] diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index 42304bb72dc9..7c428a6691cd 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -97,16 +97,28 @@ private class AllowedPrefixSanitizer extends PathInjectionSanitizer { } } +/** A call to `java.io.File.getName`, which returns the final component of a path. */ +private class FileGetNameCall extends MethodCall { + FileGetNameCall() { this.getMethod().hasQualifiedName("java.io", "File", "getName") } +} + /** * Holds if `g` is a guard that considers a path safe because it is checked for `..` components, having previously - * been checked for a trusted prefix. + * been checked for a trusted prefix or been reduced to its final path component by `File.getName`. */ private predicate dotDotCheckGuard(Guard g, Expr e, boolean branch) { pathTraversalGuard(g, e, branch) and - exists(Guard previousGuard | - previousGuard.(AllowedPrefixGuard).controls(g.getBasicBlock(), true) + ( + exists(Guard previousGuard | + previousGuard.(AllowedPrefixGuard).controls(g.getBasicBlock(), true) + or + previousGuard.(BlockListGuard).controls(g.getBasicBlock(), false) + ) or - previousGuard.(BlockListGuard).controls(g.getBasicBlock(), false) + // `File.getName` strips any directory prefix, returning only the final path + // component. The only remaining path traversal risk is when that component is + // itself `..`, so a check for `..` components completes the sanitization. + exists(FileGetNameCall getName | localTaintFlowToPathGuard(getName, g)) ) } diff --git a/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.expected b/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.expected index bfeb4d730c25..b44b294edc47 100644 --- a/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.expected +++ b/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.expected @@ -1,6 +1,7 @@ #select | SanitizationTests2.java:16:59:16:62 | path | SanitizationTests2.java:14:59:14:91 | path : String | SanitizationTests2.java:16:59:16:62 | path | This path depends on a $@. | SanitizationTests2.java:14:59:14:91 | path | user-provided value | | TaintedPath.java:16:71:16:78 | filename | TaintedPath.java:13:58:13:78 | getInputStream(...) : InputStream | TaintedPath.java:16:71:16:78 | filename | This path depends on a $@. | TaintedPath.java:13:58:13:78 | getInputStream(...) | user-provided value | +| TaintedPath.java:105:71:105:78 | baseName | TaintedPath.java:98:58:98:78 | getInputStream(...) : InputStream | TaintedPath.java:105:71:105:78 | baseName | This path depends on a $@. | TaintedPath.java:98:58:98:78 | getInputStream(...) | user-provided value | | Test.java:37:52:37:68 | (...)... | Test.java:32:16:32:45 | getParameter(...) : String | Test.java:37:52:37:68 | (...)... | This path depends on a $@. | Test.java:32:16:32:45 | getParameter(...) | user-provided value | | Test.java:39:32:39:48 | (...)... | Test.java:32:16:32:45 | getParameter(...) : String | Test.java:39:32:39:48 | (...)... | This path depends on a $@. | Test.java:32:16:32:45 | getParameter(...) | user-provided value | | Test.java:41:47:41:63 | (...)... | Test.java:32:16:32:45 | getParameter(...) : String | Test.java:41:47:41:63 | (...)... | This path depends on a $@. | Test.java:32:16:32:45 | getParameter(...) | user-provided value | @@ -81,9 +82,18 @@ edges | SanitizationTests2.java:14:59:14:91 | path : String | SanitizationTests2.java:16:59:16:62 | path | provenance | Sink:MaD:23 | | TaintedPath.java:13:17:13:89 | new BufferedReader(...) : BufferedReader | TaintedPath.java:14:27:14:40 | filenameReader : BufferedReader | provenance | | | TaintedPath.java:13:36:13:88 | new InputStreamReader(...) : InputStreamReader | TaintedPath.java:13:17:13:89 | new BufferedReader(...) : BufferedReader | provenance | MaD:74 | -| TaintedPath.java:13:58:13:78 | getInputStream(...) : InputStream | TaintedPath.java:13:36:13:88 | new InputStreamReader(...) : InputStreamReader | provenance | Src:MaD:72 MaD:76 | +| TaintedPath.java:13:58:13:78 | getInputStream(...) : InputStream | TaintedPath.java:13:36:13:88 | new InputStreamReader(...) : InputStreamReader | provenance | Src:MaD:72 MaD:78 | | TaintedPath.java:14:27:14:40 | filenameReader : BufferedReader | TaintedPath.java:14:27:14:51 | readLine(...) : String | provenance | MaD:75 | | TaintedPath.java:14:27:14:51 | readLine(...) : String | TaintedPath.java:16:71:16:78 | filename | provenance | Sink:MaD:27 | +| TaintedPath.java:98:17:98:89 | new BufferedReader(...) : BufferedReader | TaintedPath.java:99:27:99:40 | filenameReader : BufferedReader | provenance | | +| TaintedPath.java:98:36:98:88 | new InputStreamReader(...) : InputStreamReader | TaintedPath.java:98:17:98:89 | new BufferedReader(...) : BufferedReader | provenance | MaD:74 | +| TaintedPath.java:98:58:98:78 | getInputStream(...) : InputStream | TaintedPath.java:98:36:98:88 | new InputStreamReader(...) : InputStreamReader | provenance | Src:MaD:72 MaD:78 | +| TaintedPath.java:99:27:99:40 | filenameReader : BufferedReader | TaintedPath.java:99:27:99:51 | readLine(...) : String | provenance | MaD:75 | +| TaintedPath.java:99:27:99:51 | readLine(...) : String | TaintedPath.java:100:30:100:37 | filename : String | provenance | | +| TaintedPath.java:100:21:100:38 | new File(...) : File | TaintedPath.java:101:27:101:30 | file : File | provenance | | +| TaintedPath.java:100:30:100:37 | filename : String | TaintedPath.java:100:21:100:38 | new File(...) : File | provenance | MaD:76 | +| TaintedPath.java:101:27:101:30 | file : File | TaintedPath.java:101:27:101:40 | getName(...) : String | provenance | MaD:77 | +| TaintedPath.java:101:27:101:40 | getName(...) : String | TaintedPath.java:105:71:105:78 | baseName | provenance | Sink:MaD:27 | | Test.java:32:16:32:45 | getParameter(...) : String | Test.java:37:61:37:68 | source(...) : String | provenance | Src:MaD:73 | | Test.java:32:16:32:45 | getParameter(...) : String | Test.java:39:41:39:48 | source(...) : String | provenance | Src:MaD:73 | | Test.java:32:16:32:45 | getParameter(...) : String | Test.java:41:56:41:63 | source(...) : String | provenance | Src:MaD:73 | @@ -312,7 +322,9 @@ models | 73 | Source: javax.servlet; ServletRequest; false; getParameter; (String); ; ReturnValue; remote; manual | | 74 | Summary: java.io; BufferedReader; false; BufferedReader; ; ; Argument[0]; Argument[this]; taint; manual | | 75 | Summary: java.io; BufferedReader; true; readLine; ; ; Argument[this]; ReturnValue; taint; manual | -| 76 | Summary: java.io; InputStreamReader; false; InputStreamReader; ; ; Argument[0]; Argument[this]; taint; manual | +| 76 | Summary: java.io; File; false; File; ; ; Argument[0]; Argument[this]; taint; manual | +| 77 | Summary: java.io; File; true; getName; (); ; Argument[this]; ReturnValue; taint; manual | +| 78 | Summary: java.io; InputStreamReader; false; InputStreamReader; ; ; Argument[0]; Argument[this]; taint; manual | nodes | SanitizationTests2.java:14:59:14:91 | path : String | semmle.label | path : String | | SanitizationTests2.java:16:59:16:62 | path | semmle.label | path | @@ -322,6 +334,16 @@ nodes | TaintedPath.java:14:27:14:40 | filenameReader : BufferedReader | semmle.label | filenameReader : BufferedReader | | TaintedPath.java:14:27:14:51 | readLine(...) : String | semmle.label | readLine(...) : String | | TaintedPath.java:16:71:16:78 | filename | semmle.label | filename | +| TaintedPath.java:98:17:98:89 | new BufferedReader(...) : BufferedReader | semmle.label | new BufferedReader(...) : BufferedReader | +| TaintedPath.java:98:36:98:88 | new InputStreamReader(...) : InputStreamReader | semmle.label | new InputStreamReader(...) : InputStreamReader | +| TaintedPath.java:98:58:98:78 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream | +| TaintedPath.java:99:27:99:40 | filenameReader : BufferedReader | semmle.label | filenameReader : BufferedReader | +| TaintedPath.java:99:27:99:51 | readLine(...) : String | semmle.label | readLine(...) : String | +| TaintedPath.java:100:21:100:38 | new File(...) : File | semmle.label | new File(...) : File | +| TaintedPath.java:100:30:100:37 | filename : String | semmle.label | filename : String | +| TaintedPath.java:101:27:101:30 | file : File | semmle.label | file : File | +| TaintedPath.java:101:27:101:40 | getName(...) : String | semmle.label | getName(...) : String | +| TaintedPath.java:105:71:105:78 | baseName | semmle.label | baseName | | Test.java:32:16:32:45 | getParameter(...) : String | semmle.label | getParameter(...) : String | | Test.java:37:52:37:68 | (...)... | semmle.label | (...)... | | Test.java:37:61:37:68 | source(...) : String | semmle.label | source(...) : String | diff --git a/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java b/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java index fffb93c62916..fc01e1d23568 100644 --- a/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java +++ b/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java @@ -93,18 +93,38 @@ public void sendUserFileGood5(Socket sock, String user) throws Exception { } } - public void sendUserFileGood4(Socket sock, String user) throws IOException { + public void sendUserFileBad2(Socket sock, String user) throws IOException { BufferedReader filenameReader = - new BufferedReader(new InputStreamReader(sock.getInputStream(), "UTF-8")); + new BufferedReader(new InputStreamReader(sock.getInputStream(), "UTF-8")); // $ Source[java/path-injection] String filename = filenameReader.readLine(); File file = new File(filename); String baseName = file.getName(); - // GOOD: only use the final component of the user provided path - BufferedReader fileReader = new BufferedReader(new FileReader(baseName)); + // BAD: `getName()` strips directory separators but does not remove a `..` + // component (`new File("..").getName()` returns ".."), so it is not a + // complete path injection sanitizer. + BufferedReader fileReader = new BufferedReader(new FileReader(baseName)); // $ Alert[java/path-injection] String fileLine = fileReader.readLine(); while (fileLine != null) { sock.getOutputStream().write(fileLine.getBytes()); fileLine = fileReader.readLine(); } } + + public void sendUserFileGood4(Socket sock, String user) throws IOException { + BufferedReader filenameReader = + new BufferedReader(new InputStreamReader(sock.getInputStream(), "UTF-8")); + String filename = filenameReader.readLine(); + File file = new File(filename); + String baseName = file.getName(); + // GOOD: `getName()` strips directory separators and the `..` check removes + // the only remaining traversal possibility. + if (!baseName.contains("..")) { + BufferedReader fileReader = new BufferedReader(new FileReader(baseName)); + String fileLine = fileReader.readLine(); + while (fileLine != null) { + sock.getOutputStream().write(fileLine.getBytes()); + fileLine = fileReader.readLine(); + } + } + } }