Skip to content

Commit d94dc5a

Browse files
jcogs33jcogs33
authored
Merge pull request #18504 from jcogs33/jcogs33/java/file-constructor-path-sanitizer
Java: `File` constructor path sanitizer
2 parents 53557db + 9bb5fe8 commit d94dc5a

File tree

9 files changed

+345
-193
lines changed

9 files changed

+345
-193
lines changed

‎java/ql/lib/change-notes/2025-01-16-file-constructor-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+
* Added a path injection sanitizer for the `child` argument of a `java.io.File` constructor if that argument does not contain path traversal sequences.

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,10 @@ extensions:
8888
- ["java.io", "DataInput", True, "readUTF", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]
8989
- ["java.io", "DataInputStream", False, "DataInputStream", "", "", "Argument[0]", "Argument[this]", "taint", "manual"]
9090
- ["java.io", "File", False, "File", "", "", "Argument[0]", "Argument[this]", "taint", "manual"]
91-
- ["java.io", "File", False, "File", "", "", "Argument[1]", "Argument[this]", "taint", "manual"]
91+
# We model this taint step in QL as `FileConstructorChildArgumentStep` in the `PathSanitizer` library
92+
# since we need to sanitize the use of this argument but not later uses of the same SSA variable,
93+
# which is not currently possible in Java with a standard sanitizer due to use-use flow.
94+
# - ["java.io", "File", False, "File", "", "", "Argument[1]", "Argument[this]", "taint", "manual"]
9295
- ["java.io", "File", True, "getAbsoluteFile", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
9396
- ["java.io", "File", True, "getAbsolutePath", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
9497
- ["java.io", "File", True, "getCanonicalFile", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]

‎java/ql/lib/semmle/code/java/dataflow/FlowSteps.qll‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ private module Frameworks {
2929
private import semmle.code.java.frameworks.ratpack.RatpackExec
3030
private import semmle.code.java.frameworks.stapler.Stapler
3131
private import semmle.code.java.security.ListOfConstantsSanitizer
32+
private import semmle.code.java.security.PathSanitizer
3233
}
3334

3435
/**

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

Lines changed: 39 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ private import semmle.code.java.dataflow.FlowSources
66
private import semmle.code.java.dataflow.SSA
77
private import semmle.code.java.frameworks.kotlin.IO
88
private import semmle.code.java.frameworks.kotlin.Text
9+
private import semmle.code.java.dataflow.Nullness
910

1011
/** A sanitizer that protects against path injection vulnerabilities. */
1112
abstract class PathInjectionSanitizer extends DataFlow::Node { }
@@ -137,14 +138,7 @@ private class AllowedPrefixSanitizer extends PathInjectionSanitizer {
137138
* been checked for a trusted prefix.
138139
*/
139140
private predicate dotDotCheckGuard(Guard g, Expr e, boolean branch) {
140-
// Local taint-flow is used here to handle cases where the validated expression comes from the
141-
// expression reaching the sink, but it's not the same one, e.g.:
142-
// Path path = source();
143-
// String strPath = path.toString();
144-
// if (!strPath.contains("..") && strPath.startsWith("/safe/dir"))
145-
// sink(path);
146-
branch = g.(PathTraversalGuard).getBranch() and
147-
localTaintFlowToPathGuard(e, g) and
141+
pathTraversalGuard(g, e, branch) and
148142
exists(Guard previousGuard |
149143
previousGuard.(AllowedPrefixGuard).controls(g.getBasicBlock(), true)
150144
or
@@ -352,3 +346,40 @@ private class FileGetNameSanitizer extends PathInjectionSanitizer {
352346
)
353347
}
354348
}
349+
350+
/** Holds if `g` is a guard that checks for `..` components. */
351+
private predicate pathTraversalGuard(Guard g, Expr e, boolean branch) {
352+
// Local taint-flow is used here to handle cases where the validated expression comes from the
353+
// expression reaching the sink, but it's not the same one, e.g.:
354+
// Path path = source();
355+
// String strPath = path.toString();
356+
// if (!strPath.contains("..") && strPath.startsWith("/safe/dir"))
357+
// sink(path);
358+
branch = g.(PathTraversalGuard).getBranch() and
359+
localTaintFlowToPathGuard(e, g)
360+
}
361+
362+
/**
363+
* A taint step from a child argument of a `java.io.File` constructor to the
364+
* constructor call.
365+
*
366+
* This step only applies if the argument is not guarded by a check for `..`
367+
* components (`PathTraversalGuard`), or it does not have any internal `..`
368+
* components removed from it (`PathNormalizeSanitizer`), or if the parent
369+
* argument of the constructor may be null.
370+
*/
371+
private class FileConstructorChildArgumentStep extends AdditionalTaintStep {
372+
override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
373+
exists(ConstructorCall constrCall |
374+
constrCall.getConstructedType() instanceof TypeFile and
375+
n1.asExpr() = constrCall.getArgument(1) and
376+
n2.asExpr() = constrCall
377+
|
378+
not n1 = DataFlow::BarrierGuard<pathTraversalGuard/3>::getABarrierNode() and
379+
not n1 = ValidationMethod<pathTraversalGuard/3>::getAValidatedNode() and
380+
not TaintTracking::localExprTaint(any(PathNormalizeSanitizer p), n1.asExpr())
381+
or
382+
DataFlow::localExprFlow(nullExpr(), constrCall.getArgument(0))
383+
)
384+
}
385+
}

‎java/ql/test/experimental/query-tests/security/CWE-200/InsecureWebResourceResponse.expected‎

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ edges
2222
| InsecureWebResourceResponse.java:65:41:65:43 | url : String | InsecureWebResourceResponse.java:65:31:65:44 | parse(...) : Uri | provenance | MaD:2 |
2323
| InsecureWebResourceResponse.java:66:51:66:84 | new FileInputStream(...) : FileInputStream | InsecureWebResourceResponse.java:68:71:68:81 | inputStream | provenance | |
2424
| InsecureWebResourceResponse.java:66:71:66:73 | uri : Uri | InsecureWebResourceResponse.java:66:71:66:83 | getPath(...) : String | provenance | MaD:4 |
25-
| InsecureWebResourceResponse.java:66:71:66:83 | getPath(...) : String | InsecureWebResourceResponse.java:66:51:66:84 | new FileInputStream(...) : FileInputStream | provenance | MaD:7 |
25+
| InsecureWebResourceResponse.java:66:71:66:83 | getPath(...) : String | InsecureWebResourceResponse.java:66:51:66:84 | new FileInputStream(...) : FileInputStream | provenance | MaD:6 |
2626
| InsecureWebResourceResponse.java:75:20:75:22 | url : String | InsecureWebResourceResponse.java:63:77:63:86 | url : String | provenance | AdditionalTaintStep |
2727
| InsecureWebResourceResponse.java:75:20:75:22 | url : String | InsecureWebResourceResponse.java:84:77:84:86 | url : String | provenance | AdditionalTaintStep |
2828
| InsecureWebResourceResponse.java:75:20:75:22 | url : String | InsecureWebResourceResponse.java:110:77:110:86 | url : String | provenance | AdditionalTaintStep |
@@ -32,11 +32,10 @@ edges
3232
| InsecureWebResourceResponse.java:84:77:84:86 | url : String | InsecureWebResourceResponse.java:86:41:86:43 | url : String | provenance | |
3333
| InsecureWebResourceResponse.java:86:31:86:44 | parse(...) : Uri | InsecureWebResourceResponse.java:88:66:88:68 | uri : Uri | provenance | |
3434
| InsecureWebResourceResponse.java:86:41:86:43 | url : String | InsecureWebResourceResponse.java:86:31:86:44 | parse(...) : Uri | provenance | MaD:2 |
35-
| InsecureWebResourceResponse.java:88:42:88:90 | new File(...) : File | InsecureWebResourceResponse.java:89:75:89:83 | cacheFile : File | provenance | |
3635
| InsecureWebResourceResponse.java:88:66:88:68 | uri : Uri | InsecureWebResourceResponse.java:88:66:88:89 | getLastPathSegment(...) : String | provenance | MaD:3 |
37-
| InsecureWebResourceResponse.java:88:66:88:89 | getLastPathSegment(...) : String | InsecureWebResourceResponse.java:88:42:88:90 | new File(...) : File | provenance | MaD:6 |
36+
| InsecureWebResourceResponse.java:88:66:88:89 | getLastPathSegment(...) : String | InsecureWebResourceResponse.java:89:75:89:83 | cacheFile : File | provenance | AdditionalTaintStep |
3837
| InsecureWebResourceResponse.java:89:55:89:84 | new FileInputStream(...) : FileInputStream | InsecureWebResourceResponse.java:91:75:91:85 | inputStream | provenance | |
39-
| InsecureWebResourceResponse.java:89:75:89:83 | cacheFile : File | InsecureWebResourceResponse.java:89:55:89:84 | new FileInputStream(...) : FileInputStream | provenance | MaD:7 |
38+
| InsecureWebResourceResponse.java:89:75:89:83 | cacheFile : File | InsecureWebResourceResponse.java:89:55:89:84 | new FileInputStream(...) : FileInputStream | provenance | MaD:6 |
4039
| InsecureWebResourceResponse.java:101:20:101:22 | url : String | InsecureWebResourceResponse.java:63:77:63:86 | url : String | provenance | AdditionalTaintStep |
4140
| InsecureWebResourceResponse.java:101:20:101:22 | url : String | InsecureWebResourceResponse.java:84:77:84:86 | url : String | provenance | AdditionalTaintStep |
4241
| InsecureWebResourceResponse.java:101:20:101:22 | url : String | InsecureWebResourceResponse.java:110:77:110:86 | url : String | provenance | AdditionalTaintStep |
@@ -47,11 +46,11 @@ edges
4746
| InsecureWebResourceResponse.java:112:31:112:44 | parse(...) : Uri | InsecureWebResourceResponse.java:113:35:113:37 | uri : Uri | provenance | |
4847
| InsecureWebResourceResponse.java:112:41:112:43 | url : String | InsecureWebResourceResponse.java:112:31:112:44 | parse(...) : Uri | provenance | MaD:2 |
4948
| InsecureWebResourceResponse.java:113:35:113:37 | uri : Uri | InsecureWebResourceResponse.java:113:35:113:47 | getPath(...) : String | provenance | MaD:4 |
50-
| InsecureWebResourceResponse.java:113:35:113:47 | getPath(...) : String | InsecureWebResourceResponse.java:113:35:113:60 | substring(...) : String | provenance | MaD:8 |
49+
| InsecureWebResourceResponse.java:113:35:113:47 | getPath(...) : String | InsecureWebResourceResponse.java:113:35:113:60 | substring(...) : String | provenance | MaD:7 |
5150
| InsecureWebResourceResponse.java:113:35:113:60 | substring(...) : String | InsecureWebResourceResponse.java:115:75:115:78 | path : String | provenance | |
5251
| InsecureWebResourceResponse.java:115:55:115:108 | new FileInputStream(...) : FileInputStream | InsecureWebResourceResponse.java:117:75:117:85 | inputStream | provenance | |
53-
| InsecureWebResourceResponse.java:115:75:115:78 | path : String | InsecureWebResourceResponse.java:115:75:115:107 | substring(...) : String | provenance | MaD:8 |
54-
| InsecureWebResourceResponse.java:115:75:115:107 | substring(...) : String | InsecureWebResourceResponse.java:115:55:115:108 | new FileInputStream(...) : FileInputStream | provenance | MaD:7 |
52+
| InsecureWebResourceResponse.java:115:75:115:78 | path : String | InsecureWebResourceResponse.java:115:75:115:107 | substring(...) : String | provenance | MaD:7 |
53+
| InsecureWebResourceResponse.java:115:75:115:107 | substring(...) : String | InsecureWebResourceResponse.java:115:55:115:108 | new FileInputStream(...) : FileInputStream | provenance | MaD:6 |
5554
| InsecureWebResourceResponse.java:127:20:127:22 | url : String | InsecureWebResourceResponse.java:63:77:63:86 | url : String | provenance | AdditionalTaintStep |
5655
| InsecureWebResourceResponse.java:127:20:127:22 | url : String | InsecureWebResourceResponse.java:84:77:84:86 | url : String | provenance | AdditionalTaintStep |
5756
| InsecureWebResourceResponse.java:127:20:127:22 | url : String | InsecureWebResourceResponse.java:110:77:110:86 | url : String | provenance | AdditionalTaintStep |
@@ -79,11 +78,10 @@ edges
7978
| InsecureWebResourceResponse.java:192:77:192:102 | request : WebResourceRequest | InsecureWebResourceResponse.java:194:31:194:37 | request : WebResourceRequest | provenance | |
8079
| InsecureWebResourceResponse.java:194:31:194:37 | request : WebResourceRequest | InsecureWebResourceResponse.java:194:31:194:46 | getUrl(...) : Uri | provenance | MaD:5 |
8180
| InsecureWebResourceResponse.java:194:31:194:46 | getUrl(...) : Uri | InsecureWebResourceResponse.java:196:66:196:68 | uri : Uri | provenance | |
82-
| InsecureWebResourceResponse.java:196:42:196:90 | new File(...) : File | InsecureWebResourceResponse.java:197:75:197:83 | cacheFile : File | provenance | |
8381
| InsecureWebResourceResponse.java:196:66:196:68 | uri : Uri | InsecureWebResourceResponse.java:196:66:196:89 | getLastPathSegment(...) : String | provenance | MaD:3 |
84-
| InsecureWebResourceResponse.java:196:66:196:89 | getLastPathSegment(...) : String | InsecureWebResourceResponse.java:196:42:196:90 | new File(...) : File | provenance | MaD:6 |
82+
| InsecureWebResourceResponse.java:196:66:196:89 | getLastPathSegment(...) : String | InsecureWebResourceResponse.java:197:75:197:83 | cacheFile : File | provenance | AdditionalTaintStep |
8583
| InsecureWebResourceResponse.java:197:55:197:84 | new FileInputStream(...) : FileInputStream | InsecureWebResourceResponse.java:199:75:199:85 | inputStream | provenance | |
86-
| InsecureWebResourceResponse.java:197:75:197:83 | cacheFile : File | InsecureWebResourceResponse.java:197:55:197:84 | new FileInputStream(...) : FileInputStream | provenance | MaD:7 |
84+
| InsecureWebResourceResponse.java:197:75:197:83 | cacheFile : File | InsecureWebResourceResponse.java:197:55:197:84 | new FileInputStream(...) : FileInputStream | provenance | MaD:6 |
8785
| InsecureWebResourceResponse.java:209:20:209:22 | url : String | InsecureWebResourceResponse.java:63:77:63:86 | url : String | provenance | AdditionalTaintStep |
8886
| InsecureWebResourceResponse.java:209:20:209:22 | url : String | InsecureWebResourceResponse.java:84:77:84:86 | url : String | provenance | AdditionalTaintStep |
8987
| InsecureWebResourceResponse.java:209:20:209:22 | url : String | InsecureWebResourceResponse.java:110:77:110:86 | url : String | provenance | AdditionalTaintStep |
@@ -100,7 +98,7 @@ edges
10098
| InsecureWebResourceResponse.java:234:33:234:35 | url : String | InsecureWebResourceResponse.java:234:23:234:36 | parse(...) : Uri | provenance | MaD:2 |
10199
| InsecureWebResourceResponse.java:235:43:235:76 | new FileInputStream(...) : FileInputStream | InsecureWebResourceResponse.java:237:63:237:73 | inputStream | provenance | |
102100
| InsecureWebResourceResponse.java:235:63:235:65 | uri : Uri | InsecureWebResourceResponse.java:235:63:235:75 | getPath(...) : String | provenance | MaD:4 |
103-
| InsecureWebResourceResponse.java:235:63:235:75 | getPath(...) : String | InsecureWebResourceResponse.java:235:43:235:76 | new FileInputStream(...) : FileInputStream | provenance | MaD:7 |
101+
| InsecureWebResourceResponse.java:235:63:235:75 | getPath(...) : String | InsecureWebResourceResponse.java:235:43:235:76 | new FileInputStream(...) : FileInputStream | provenance | MaD:6 |
104102
| InsecureWebViewActivity.java:27:27:27:37 | getIntent(...) : Intent | InsecureWebViewActivity.java:27:27:27:64 | getStringExtra(...) : String | provenance | MaD:1 |
105103
| InsecureWebViewActivity.java:27:27:27:64 | getStringExtra(...) : String | InsecureWebViewActivity.java:28:20:28:27 | inputUrl : String | provenance | |
106104
| InsecureWebViewActivity.java:28:20:28:27 | inputUrl : String | InsecureWebViewActivity.java:42:28:42:37 | url : String | provenance | |
@@ -111,16 +109,15 @@ edges
111109
| InsecureWebViewActivity.java:55:41:55:43 | url : String | InsecureWebViewActivity.java:55:31:55:44 | parse(...) : Uri | provenance | MaD:2 |
112110
| InsecureWebViewActivity.java:56:51:56:84 | new FileInputStream(...) : FileInputStream | InsecureWebViewActivity.java:58:71:58:81 | inputStream | provenance | |
113111
| InsecureWebViewActivity.java:56:71:56:73 | uri : Uri | InsecureWebViewActivity.java:56:71:56:83 | getPath(...) : String | provenance | MaD:4 |
114-
| InsecureWebViewActivity.java:56:71:56:83 | getPath(...) : String | InsecureWebViewActivity.java:56:51:56:84 | new FileInputStream(...) : FileInputStream | provenance | MaD:7 |
112+
| InsecureWebViewActivity.java:56:71:56:83 | getPath(...) : String | InsecureWebViewActivity.java:56:51:56:84 | new FileInputStream(...) : FileInputStream | provenance | MaD:6 |
115113
models
116114
| 1 | Summary: android.content; Intent; true; getStringExtra; (String); ; Argument[this].SyntheticField[android.content.Intent.extras].MapValue; ReturnValue; value; manual |
117115
| 2 | Summary: android.net; Uri; false; parse; ; ; Argument[0]; ReturnValue; taint; manual |
118116
| 3 | Summary: android.net; Uri; true; getLastPathSegment; ; ; Argument[this]; ReturnValue; taint; manual |
119117
| 4 | Summary: android.net; Uri; true; getPath; ; ; Argument[this]; ReturnValue; taint; manual |
120118
| 5 | Summary: android.webkit; WebResourceRequest; false; getUrl; ; ; Argument[this]; ReturnValue; taint; manual |
121-
| 6 | Summary: java.io; File; false; File; ; ; Argument[1]; Argument[this]; taint; manual |
122-
| 7 | Summary: java.io; FileInputStream; true; FileInputStream; ; ; Argument[0]; Argument[this]; taint; manual |
123-
| 8 | Summary: java.lang; String; false; substring; ; ; Argument[this]; ReturnValue; taint; manual |
119+
| 6 | Summary: java.io; FileInputStream; true; FileInputStream; ; ; Argument[0]; Argument[this]; taint; manual |
120+
| 7 | Summary: java.lang; String; false; substring; ; ; Argument[this]; ReturnValue; taint; manual |
124121
nodes
125122
| InsecureWebResourceResponse.java:28:27:28:37 | getIntent(...) : Intent | semmle.label | getIntent(...) : Intent |
126123
| InsecureWebResourceResponse.java:28:27:28:64 | getStringExtra(...) : String | semmle.label | getStringExtra(...) : String |
@@ -145,7 +142,6 @@ nodes
145142
| InsecureWebResourceResponse.java:84:77:84:86 | url : String | semmle.label | url : String |
146143
| InsecureWebResourceResponse.java:86:31:86:44 | parse(...) : Uri | semmle.label | parse(...) : Uri |
147144
| InsecureWebResourceResponse.java:86:41:86:43 | url : String | semmle.label | url : String |
148-
| InsecureWebResourceResponse.java:88:42:88:90 | new File(...) : File | semmle.label | new File(...) : File |
149145
| InsecureWebResourceResponse.java:88:66:88:68 | uri : Uri | semmle.label | uri : Uri |
150146
| InsecureWebResourceResponse.java:88:66:88:89 | getLastPathSegment(...) : String | semmle.label | getLastPathSegment(...) : String |
151147
| InsecureWebResourceResponse.java:89:55:89:84 | new FileInputStream(...) : FileInputStream | semmle.label | new FileInputStream(...) : FileInputStream |
@@ -174,7 +170,6 @@ nodes
174170
| InsecureWebResourceResponse.java:192:77:192:102 | request : WebResourceRequest | semmle.label | request : WebResourceRequest |
175171
| InsecureWebResourceResponse.java:194:31:194:37 | request : WebResourceRequest | semmle.label | request : WebResourceRequest |
176172
| InsecureWebResourceResponse.java:194:31:194:46 | getUrl(...) : Uri | semmle.label | getUrl(...) : Uri |
177-
| InsecureWebResourceResponse.java:196:42:196:90 | new File(...) : File | semmle.label | new File(...) : File |
178173
| InsecureWebResourceResponse.java:196:66:196:68 | uri : Uri | semmle.label | uri : Uri |
179174
| InsecureWebResourceResponse.java:196:66:196:89 | getLastPathSegment(...) : String | semmle.label | getLastPathSegment(...) : String |
180175
| InsecureWebResourceResponse.java:197:55:197:84 | new FileInputStream(...) : FileInputStream | semmle.label | new FileInputStream(...) : FileInputStream |

0 commit comments

Comments
 (0)