Skip to content

Commit 6057b4b

Browse files
owen-mcowen-mc
committed
Fix path sanitizer
1 parent 747aa45 commit 6057b4b

5 files changed

Lines changed: 68 additions & 15 deletions

File tree

‎java/ql/lib/change-notes/2026-07-11-file-getname-path-sanitizer.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* `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.

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

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -162,8 +162,3 @@ extensions:
162162
extensible: sourceModel
163163
data:
164164
- ["java.io", "FileInputStream", True, "FileInputStream", "", "", "Argument[this]", "file", "manual"]
165-
- addsTo:
166-
pack: codeql/java-all
167-
extensible: barrierModel
168-
data:
169-
- ["java.io", "File", True, "getName", "()", "", "ReturnValue", "path-injection", "manual"]

‎java/ql/lib/semmle/code/java/security/PathSanitizer.qll‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -97,16 +97,28 @@ private class AllowedPrefixSanitizer extends PathInjectionSanitizer {
9797
}
9898
}
9999

100+
/** A call to `java.io.File.getName`, which returns the final component of a path. */
101+
private class FileGetNameCall extends MethodCall {
102+
FileGetNameCall() { this.getMethod().hasQualifiedName("java.io", "File", "getName") }
103+
}
104+
100105
/**
101106
* Holds if `g` is a guard that considers a path safe because it is checked for `..` components, having previously
102-
* been checked for a trusted prefix.
107+
* been checked for a trusted prefix or been reduced to its final path component by `File.getName`.
103108
*/
104109
private predicate dotDotCheckGuard(Guard g, Expr e, boolean branch) {
105110
pathTraversalGuard(g, e, branch) and
106-
exists(Guard previousGuard |
107-
previousGuard.(AllowedPrefixGuard).controls(g.getBasicBlock(), true)
111+
(
112+
exists(Guard previousGuard |
113+
previousGuard.(AllowedPrefixGuard).controls(g.getBasicBlock(), true)
114+
or
115+
previousGuard.(BlockListGuard).controls(g.getBasicBlock(), false)
116+
)
108117
or
109-
previousGuard.(BlockListGuard).controls(g.getBasicBlock(), false)
118+
// `File.getName` strips any directory prefix, returning only the final path
119+
// component. The only remaining path traversal risk is when that component is
120+
// itself `..`, so a check for `..` components completes the sanitization.
121+
exists(FileGetNameCall getName | localTaintFlowToPathGuard(getName, g))
110122
)
111123
}
112124

‎java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.expected‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
#select
22
| 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 |
33
| 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 |
4+
| 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 |
45
| 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 |
56
| 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 |
67
| 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
8182
| SanitizationTests2.java:14:59:14:91 | path : String | SanitizationTests2.java:16:59:16:62 | path | provenance | Sink:MaD:23 |
8283
| TaintedPath.java:13:17:13:89 | new BufferedReader(...) : BufferedReader | TaintedPath.java:14:27:14:40 | filenameReader : BufferedReader | provenance | |
8384
| TaintedPath.java:13:36:13:88 | new InputStreamReader(...) : InputStreamReader | TaintedPath.java:13:17:13:89 | new BufferedReader(...) : BufferedReader | provenance | MaD:74 |
84-
| TaintedPath.java:13:58:13:78 | getInputStream(...) : InputStream | TaintedPath.java:13:36:13:88 | new InputStreamReader(...) : InputStreamReader | provenance | Src:MaD:72 MaD:76 |
85+
| TaintedPath.java:13:58:13:78 | getInputStream(...) : InputStream | TaintedPath.java:13:36:13:88 | new InputStreamReader(...) : InputStreamReader | provenance | Src:MaD:72 MaD:78 |
8586
| TaintedPath.java:14:27:14:40 | filenameReader : BufferedReader | TaintedPath.java:14:27:14:51 | readLine(...) : String | provenance | MaD:75 |
8687
| TaintedPath.java:14:27:14:51 | readLine(...) : String | TaintedPath.java:16:71:16:78 | filename | provenance | Sink:MaD:27 |
88+
| TaintedPath.java:98:17:98:89 | new BufferedReader(...) : BufferedReader | TaintedPath.java:99:27:99:40 | filenameReader : BufferedReader | provenance | |
89+
| TaintedPath.java:98:36:98:88 | new InputStreamReader(...) : InputStreamReader | TaintedPath.java:98:17:98:89 | new BufferedReader(...) : BufferedReader | provenance | MaD:74 |
90+
| TaintedPath.java:98:58:98:78 | getInputStream(...) : InputStream | TaintedPath.java:98:36:98:88 | new InputStreamReader(...) : InputStreamReader | provenance | Src:MaD:72 MaD:78 |
91+
| TaintedPath.java:99:27:99:40 | filenameReader : BufferedReader | TaintedPath.java:99:27:99:51 | readLine(...) : String | provenance | MaD:75 |
92+
| TaintedPath.java:99:27:99:51 | readLine(...) : String | TaintedPath.java:100:30:100:37 | filename : String | provenance | |
93+
| TaintedPath.java:100:21:100:38 | new File(...) : File | TaintedPath.java:101:27:101:30 | file : File | provenance | |
94+
| TaintedPath.java:100:30:100:37 | filename : String | TaintedPath.java:100:21:100:38 | new File(...) : File | provenance | MaD:76 |
95+
| TaintedPath.java:101:27:101:30 | file : File | TaintedPath.java:101:27:101:40 | getName(...) : String | provenance | MaD:77 |
96+
| TaintedPath.java:101:27:101:40 | getName(...) : String | TaintedPath.java:105:71:105:78 | baseName | provenance | Sink:MaD:27 |
8797
| Test.java:32:16:32:45 | getParameter(...) : String | Test.java:37:61:37:68 | source(...) : String | provenance | Src:MaD:73 |
8898
| Test.java:32:16:32:45 | getParameter(...) : String | Test.java:39:41:39:48 | source(...) : String | provenance | Src:MaD:73 |
8999
| 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
312322
| 73 | Source: javax.servlet; ServletRequest; false; getParameter; (String); ; ReturnValue; remote; manual |
313323
| 74 | Summary: java.io; BufferedReader; false; BufferedReader; ; ; Argument[0]; Argument[this]; taint; manual |
314324
| 75 | Summary: java.io; BufferedReader; true; readLine; ; ; Argument[this]; ReturnValue; taint; manual |
315-
| 76 | Summary: java.io; InputStreamReader; false; InputStreamReader; ; ; Argument[0]; Argument[this]; taint; manual |
325+
| 76 | Summary: java.io; File; false; File; ; ; Argument[0]; Argument[this]; taint; manual |
326+
| 77 | Summary: java.io; File; true; getName; (); ; Argument[this]; ReturnValue; taint; manual |
327+
| 78 | Summary: java.io; InputStreamReader; false; InputStreamReader; ; ; Argument[0]; Argument[this]; taint; manual |
316328
nodes
317329
| SanitizationTests2.java:14:59:14:91 | path : String | semmle.label | path : String |
318330
| SanitizationTests2.java:16:59:16:62 | path | semmle.label | path |
@@ -322,6 +334,16 @@ nodes
322334
| TaintedPath.java:14:27:14:40 | filenameReader : BufferedReader | semmle.label | filenameReader : BufferedReader |
323335
| TaintedPath.java:14:27:14:51 | readLine(...) : String | semmle.label | readLine(...) : String |
324336
| TaintedPath.java:16:71:16:78 | filename | semmle.label | filename |
337+
| TaintedPath.java:98:17:98:89 | new BufferedReader(...) : BufferedReader | semmle.label | new BufferedReader(...) : BufferedReader |
338+
| TaintedPath.java:98:36:98:88 | new InputStreamReader(...) : InputStreamReader | semmle.label | new InputStreamReader(...) : InputStreamReader |
339+
| TaintedPath.java:98:58:98:78 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
340+
| TaintedPath.java:99:27:99:40 | filenameReader : BufferedReader | semmle.label | filenameReader : BufferedReader |
341+
| TaintedPath.java:99:27:99:51 | readLine(...) : String | semmle.label | readLine(...) : String |
342+
| TaintedPath.java:100:21:100:38 | new File(...) : File | semmle.label | new File(...) : File |
343+
| TaintedPath.java:100:30:100:37 | filename : String | semmle.label | filename : String |
344+
| TaintedPath.java:101:27:101:30 | file : File | semmle.label | file : File |
345+
| TaintedPath.java:101:27:101:40 | getName(...) : String | semmle.label | getName(...) : String |
346+
| TaintedPath.java:105:71:105:78 | baseName | semmle.label | baseName |
325347
| Test.java:32:16:32:45 | getParameter(...) : String | semmle.label | getParameter(...) : String |
326348
| Test.java:37:52:37:68 | (...)... | semmle.label | (...)... |
327349
| Test.java:37:61:37:68 | source(...) : String | semmle.label | source(...) : String |

‎java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -93,18 +93,38 @@ public void sendUserFileGood5(Socket sock, String user) throws Exception {
9393
}
9494
}
9595

96-
public void sendUserFileGood4(Socket sock, String user) throws IOException {
96+
public void sendUserFileBad2(Socket sock, String user) throws IOException {
9797
BufferedReader filenameReader =
98-
new BufferedReader(new InputStreamReader(sock.getInputStream(), "UTF-8"));
98+
new BufferedReader(new InputStreamReader(sock.getInputStream(), "UTF-8")); // $ Source[java/path-injection]
9999
String filename = filenameReader.readLine();
100100
File file = new File(filename);
101101
String baseName = file.getName();
102-
// GOOD: only use the final component of the user provided path
103-
BufferedReader fileReader = new BufferedReader(new FileReader(baseName));
102+
// BAD: `getName()` strips directory separators but does not remove a `..`
103+
// component (`new File("..").getName()` returns ".."), so it is not a
104+
// complete path injection sanitizer.
105+
BufferedReader fileReader = new BufferedReader(new FileReader(baseName)); // $ Alert[java/path-injection]
104106
String fileLine = fileReader.readLine();
105107
while (fileLine != null) {
106108
sock.getOutputStream().write(fileLine.getBytes());
107109
fileLine = fileReader.readLine();
108110
}
109111
}
112+
113+
public void sendUserFileGood4(Socket sock, String user) throws IOException {
114+
BufferedReader filenameReader =
115+
new BufferedReader(new InputStreamReader(sock.getInputStream(), "UTF-8"));
116+
String filename = filenameReader.readLine();
117+
File file = new File(filename);
118+
String baseName = file.getName();
119+
// GOOD: `getName()` strips directory separators and the `..` check removes
120+
// the only remaining traversal possibility.
121+
if (!baseName.contains("..")) {
122+
BufferedReader fileReader = new BufferedReader(new FileReader(baseName));
123+
String fileLine = fileReader.readLine();
124+
while (fileLine != null) {
125+
sock.getOutputStream().write(fileLine.getBytes());
126+
fileLine = fileReader.readLine();
127+
}
128+
}
129+
}
110130
}

0 commit comments

Comments
 (0)