Skip to content

Commit 5b67ba2

Browse files
authored
Merge pull request #10177 from atorralba/atorralba/path-sanitizer
Java: Promote `PathSanitizer.qll` from experimental
2 parents cbeff4e + 6db0db4 commit 5b67ba2

File tree

20 files changed

+908
-415
lines changed

20 files changed

+908
-415
lines changed

java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -361,19 +361,7 @@ private class SummaryModelCsvBase extends SummaryModelCsv {
361361
"java.net;URI;false;toURL;;;Argument[-1];ReturnValue;taint;manual",
362362
"java.net;URI;false;toString;;;Argument[-1];ReturnValue;taint;manual",
363363
"java.net;URI;false;toAsciiString;;;Argument[-1];ReturnValue;taint;manual",
364-
"java.io;File;true;toURI;;;Argument[-1];ReturnValue;taint;manual",
365-
"java.io;File;true;toPath;;;Argument[-1];ReturnValue;taint;manual",
366-
"java.io;File;true;getAbsoluteFile;;;Argument[-1];ReturnValue;taint;manual",
367-
"java.io;File;true;getCanonicalFile;;;Argument[-1];ReturnValue;taint;manual",
368-
"java.io;File;true;getAbsolutePath;;;Argument[-1];ReturnValue;taint;manual",
369-
"java.io;File;true;getCanonicalPath;;;Argument[-1];ReturnValue;taint;manual",
370364
"java.nio;ByteBuffer;false;array;();;Argument[-1];ReturnValue;taint;manual",
371-
"java.nio.file;Path;true;normalize;;;Argument[-1];ReturnValue;taint;manual",
372-
"java.nio.file;Path;true;resolve;;;Argument[-1..0];ReturnValue;taint;manual",
373-
"java.nio.file;Path;false;toFile;;;Argument[-1];ReturnValue;taint;manual",
374-
"java.nio.file;Path;true;toString;;;Argument[-1];ReturnValue;taint;manual",
375-
"java.nio.file;Path;true;toUri;;;Argument[-1];ReturnValue;taint;manual",
376-
"java.nio.file;Paths;true;get;;;Argument[0..1];ReturnValue;taint;manual",
377365
"java.io;BufferedReader;true;readLine;;;Argument[-1];ReturnValue;taint;manual",
378366
"java.io;Reader;true;read;();;Argument[-1];ReturnValue;taint;manual",
379367
// arg to return
@@ -400,8 +388,6 @@ private class SummaryModelCsvBase extends SummaryModelCsv {
400388
// arg to arg
401389
"java.lang;System;false;arraycopy;;;Argument[0];Argument[2];taint;manual",
402390
// constructor flow
403-
"java.io;File;false;File;;;Argument[0];Argument[-1];taint;manual",
404-
"java.io;File;false;File;;;Argument[1];Argument[-1];taint;manual",
405391
"java.net;URI;false;URI;(String);;Argument[0];Argument[-1];taint;manual",
406392
"java.net;URL;false;URL;(String);;Argument[0];Argument[-1];taint;manual",
407393
"javax.xml.transform.stream;StreamSource;false;StreamSource;;;Argument[0];Argument[-1];taint;manual",

java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,57 @@ predicate localExprTaint(Expr src, Expr sink) {
3333
localTaint(DataFlow::exprNode(src), DataFlow::exprNode(sink))
3434
}
3535

36+
/** Holds if `node` is an endpoint for local taint flow. */
37+
signature predicate nodeSig(DataFlow::Node node);
38+
39+
/** Provides local taint flow restricted to a given set of sources and sinks. */
40+
module LocalTaintFlow<nodeSig/1 source, nodeSig/1 sink> {
41+
private predicate reachRev(DataFlow::Node n) {
42+
sink(n)
43+
or
44+
exists(DataFlow::Node mid |
45+
localTaintStep(n, mid) and
46+
reachRev(mid)
47+
)
48+
}
49+
50+
private predicate reachFwd(DataFlow::Node n) {
51+
reachRev(n) and
52+
(
53+
source(n)
54+
or
55+
exists(DataFlow::Node mid |
56+
localTaintStep(mid, n) and
57+
reachFwd(mid)
58+
)
59+
)
60+
}
61+
62+
private predicate step(DataFlow::Node n1, DataFlow::Node n2) {
63+
localTaintStep(n1, n2) and
64+
reachFwd(n1) and
65+
reachFwd(n2)
66+
}
67+
68+
/**
69+
* Holds if taint can flow from `n1` to `n2` in zero or more local
70+
* (intra-procedural) steps that are restricted to be part of a path between
71+
* `source` and `sink`.
72+
*/
73+
pragma[inline]
74+
predicate hasFlow(DataFlow::Node n1, DataFlow::Node n2) { step*(n1, n2) }
75+
76+
/**
77+
* Holds if taint can flow from `n1` to `n2` in zero or more local
78+
* (intra-procedural) steps that are restricted to be part of a path between
79+
* `source` and `sink`.
80+
*/
81+
pragma[inline]
82+
predicate hasExprFlow(Expr n1, Expr n2) {
83+
hasFlow(DataFlow::exprNode(n1), DataFlow::exprNode(n2))
84+
}
85+
}
86+
3687
cached
3788
private module Cached {
3889
private import DataFlowImplCommon as DataFlowImplCommon

java/ql/lib/semmle/code/java/security/Files.qll

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,3 +70,29 @@ private class WriteFileSinkModels extends SinkModelCsv {
7070
]
7171
}
7272
}
73+
74+
private class FileSummaryModels extends SummaryModelCsv {
75+
override predicate row(string row) {
76+
row =
77+
[
78+
"java.io;File;false;File;;;Argument[0];Argument[-1];taint;manual",
79+
"java.io;File;false;File;;;Argument[1];Argument[-1];taint;manual",
80+
"java.io;File;true;getAbsoluteFile;;;Argument[-1];ReturnValue;taint;manual",
81+
"java.io;File;true;getAbsolutePath;;;Argument[-1];ReturnValue;taint;manual",
82+
"java.io;File;true;getCanonicalFile;;;Argument[-1];ReturnValue;taint;manual",
83+
"java.io;File;true;getCanonicalPath;;;Argument[-1];ReturnValue;taint;manual",
84+
"java.io;File;true;toPath;;;Argument[-1];ReturnValue;taint;manual",
85+
"java.io;File;true;toString;;;Argument[-1];ReturnValue;taint;manual",
86+
"java.io;File;true;toURI;;;Argument[-1];ReturnValue;taint;manual",
87+
"java.nio.file;Path;true;normalize;;;Argument[-1];ReturnValue;taint;manual",
88+
"java.nio.file;Path;true;resolve;;;Argument[-1..0];ReturnValue;taint;manual",
89+
"java.nio.file;Path;true;toAbsolutePath;;;Argument[-1];ReturnValue;taint;manual",
90+
"java.nio.file;Path;false;toFile;;;Argument[-1];ReturnValue;taint;manual",
91+
"java.nio.file;Path;true;toString;;;Argument[-1];ReturnValue;taint;manual",
92+
"java.nio.file;Path;true;toUri;;;Argument[-1];ReturnValue;taint;manual",
93+
"java.nio.file;Paths;true;get;;;Argument[0..1];ReturnValue;taint;manual",
94+
"java.nio.file;FileSystem;true;getPath;;;Argument[0];ReturnValue;taint;manual",
95+
"java.nio.file;FileSystem;true;getRootDirectories;;;Argument[0];ReturnValue;taint;manual"
96+
]
97+
}
98+
}
Lines changed: 277 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,277 @@
1+
/** Provides classes and predicates to reason about sanitization of path injection vulnerabilities. */
2+
3+
import java
4+
private import semmle.code.java.controlflow.Guards
5+
private import semmle.code.java.dataflow.ExternalFlow
6+
private import semmle.code.java.dataflow.FlowSources
7+
private import semmle.code.java.dataflow.SSA
8+
9+
/** A sanitizer that protects against path injection vulnerabilities. */
10+
abstract class PathInjectionSanitizer extends DataFlow::Node { }
11+
12+
/**
13+
* Provides a set of nodes validated by a method that uses a validation guard.
14+
*/
15+
private module ValidationMethod<DataFlow::guardChecksSig/3 validationGuard> {
16+
/** Gets a node that is safely guarded by a method that uses the given guard check. */
17+
DataFlow::Node getAValidatedNode() {
18+
exists(MethodAccess ma, int pos, RValue rv |
19+
validationMethod(ma.getMethod(), pos) and
20+
ma.getArgument(pos) = rv and
21+
adjacentUseUseSameVar(rv, result.asExpr()) and
22+
ma.getBasicBlock().bbDominates(result.asExpr().getBasicBlock())
23+
)
24+
}
25+
26+
/**
27+
* Holds if `m` validates its `arg`th parameter by using `validationGuard`.
28+
*/
29+
private predicate validationMethod(Method m, int arg) {
30+
exists(
31+
Guard g, SsaImplicitInit var, ControlFlowNode exit, ControlFlowNode normexit, boolean branch
32+
|
33+
validationGuard(g, var.getAUse(), branch) and
34+
var.isParameterDefinition(m.getParameter(arg)) and
35+
exit = m and
36+
normexit.getANormalSuccessor() = exit and
37+
1 = strictcount(ControlFlowNode n | n.getANormalSuccessor() = exit)
38+
|
39+
g.(ConditionNode).getABranchSuccessor(branch) = exit or
40+
g.controls(normexit.getBasicBlock(), branch)
41+
)
42+
}
43+
}
44+
45+
/**
46+
* Holds if `g` is guard that compares a path to a trusted value.
47+
*/
48+
private predicate exactPathMatchGuard(Guard g, Expr e, boolean branch) {
49+
exists(MethodAccess ma, RefType t |
50+
t instanceof TypeString or
51+
t instanceof TypeUri or
52+
t instanceof TypePath or
53+
t instanceof TypeFile or
54+
t.hasQualifiedName("android.net", "Uri")
55+
|
56+
ma.getMethod().getDeclaringType() = t and
57+
ma = g and
58+
ma.getMethod().getName() = ["equals", "equalsIgnoreCase"] and
59+
e = ma.getQualifier() and
60+
branch = true
61+
)
62+
}
63+
64+
private class ExactPathMatchSanitizer extends PathInjectionSanitizer {
65+
ExactPathMatchSanitizer() {
66+
this = DataFlow::BarrierGuard<exactPathMatchGuard/3>::getABarrierNode()
67+
or
68+
this = ValidationMethod<exactPathMatchGuard/3>::getAValidatedNode()
69+
}
70+
}
71+
72+
abstract private class PathGuard extends Guard {
73+
abstract Expr getCheckedExpr();
74+
}
75+
76+
private predicate anyNode(DataFlow::Node n) { any() }
77+
78+
private predicate pathGuardNode(DataFlow::Node n) { n.asExpr() = any(PathGuard g).getCheckedExpr() }
79+
80+
private predicate localTaintFlowToPathGuard(Expr e, PathGuard g) {
81+
TaintTracking::LocalTaintFlow<anyNode/1, pathGuardNode/1>::hasExprFlow(e, g.getCheckedExpr())
82+
}
83+
84+
private class AllowedPrefixGuard extends PathGuard instanceof MethodAccess {
85+
AllowedPrefixGuard() {
86+
(isStringPrefixMatch(this) or isPathPrefixMatch(this)) and
87+
not isDisallowedWord(super.getAnArgument())
88+
}
89+
90+
override Expr getCheckedExpr() { result = super.getQualifier() }
91+
}
92+
93+
/**
94+
* Holds if `g` is a guard that considers a path safe because it is checked against trusted prefixes.
95+
* This requires additional protection against path traversal, either another guard (`PathTraversalGuard`)
96+
* or a sanitizer (`PathNormalizeSanitizer`), to ensure any internal `..` components are removed from the path.
97+
*/
98+
private predicate allowedPrefixGuard(Guard g, Expr e, boolean branch) {
99+
branch = true and
100+
// Local taint-flow is used here to handle cases where the validated expression comes from the
101+
// expression reaching the sink, but it's not the same one, e.g.:
102+
// File file = source();
103+
// String strPath = file.getCanonicalPath();
104+
// if (strPath.startsWith("/safe/dir"))
105+
// sink(file);
106+
g instanceof AllowedPrefixGuard and
107+
localTaintFlowToPathGuard(e, g) and
108+
exists(Expr previousGuard |
109+
localTaintFlowToPathGuard(previousGuard.(PathNormalizeSanitizer), g)
110+
or
111+
previousGuard
112+
.(PathTraversalGuard)
113+
.controls(g.getBasicBlock(), previousGuard.(PathTraversalGuard).getBranch())
114+
)
115+
}
116+
117+
private class AllowedPrefixSanitizer extends PathInjectionSanitizer {
118+
AllowedPrefixSanitizer() {
119+
this = DataFlow::BarrierGuard<allowedPrefixGuard/3>::getABarrierNode() or
120+
this = ValidationMethod<allowedPrefixGuard/3>::getAValidatedNode()
121+
}
122+
}
123+
124+
/**
125+
* Holds if `g` is a guard that considers a path safe because it is checked for `..` components, having previously
126+
* been checked for a trusted prefix.
127+
*/
128+
private predicate dotDotCheckGuard(Guard g, Expr e, boolean branch) {
129+
// Local taint-flow is used here to handle cases where the validated expression comes from the
130+
// expression reaching the sink, but it's not the same one, e.g.:
131+
// Path path = source();
132+
// String strPath = path.toString();
133+
// if (!strPath.contains("..") && strPath.startsWith("/safe/dir"))
134+
// sink(path);
135+
branch = g.(PathTraversalGuard).getBranch() and
136+
localTaintFlowToPathGuard(e, g) and
137+
exists(Guard previousGuard |
138+
previousGuard.(AllowedPrefixGuard).controls(g.getBasicBlock(), true)
139+
or
140+
previousGuard.(BlockListGuard).controls(g.getBasicBlock(), false)
141+
)
142+
}
143+
144+
private class DotDotCheckSanitizer extends PathInjectionSanitizer {
145+
DotDotCheckSanitizer() {
146+
this = DataFlow::BarrierGuard<dotDotCheckGuard/3>::getABarrierNode() or
147+
this = ValidationMethod<dotDotCheckGuard/3>::getAValidatedNode()
148+
}
149+
}
150+
151+
private class BlockListGuard extends PathGuard instanceof MethodAccess {
152+
BlockListGuard() {
153+
(isStringPartialMatch(this) or isPathPrefixMatch(this)) and
154+
isDisallowedWord(super.getAnArgument())
155+
}
156+
157+
override Expr getCheckedExpr() { result = super.getQualifier() }
158+
}
159+
160+
/**
161+
* Holds if `g` is a guard that considers a string safe because it is checked against a blocklist of known dangerous values.
162+
* This requires additional protection against path traversal, either another guard (`PathTraversalGuard`)
163+
* or a sanitizer (`PathNormalizeSanitizer`), to ensure any internal `..` components are removed from the path.
164+
*/
165+
private predicate blockListGuard(Guard g, Expr e, boolean branch) {
166+
branch = false and
167+
// Local taint-flow is used here to handle cases where the validated expression comes from the
168+
// expression reaching the sink, but it's not the same one, e.g.:
169+
// File file = source();
170+
// String strPath = file.getCanonicalPath();
171+
// if (!strPath.contains("..") && !strPath.startsWith("/dangerous/dir"))
172+
// sink(file);
173+
g instanceof BlockListGuard and
174+
localTaintFlowToPathGuard(e, g) and
175+
exists(Expr previousGuard |
176+
localTaintFlowToPathGuard(previousGuard.(PathNormalizeSanitizer), g)
177+
or
178+
previousGuard
179+
.(PathTraversalGuard)
180+
.controls(g.getBasicBlock(), previousGuard.(PathTraversalGuard).getBranch())
181+
)
182+
}
183+
184+
private class BlockListSanitizer extends PathInjectionSanitizer {
185+
BlockListSanitizer() {
186+
this = DataFlow::BarrierGuard<blockListGuard/3>::getABarrierNode() or
187+
this = ValidationMethod<blockListGuard/3>::getAValidatedNode()
188+
}
189+
}
190+
191+
private predicate isStringPrefixMatch(MethodAccess ma) {
192+
exists(Method m | m = ma.getMethod() and m.getDeclaringType() instanceof TypeString |
193+
m.hasName("startsWith")
194+
or
195+
m.hasName("regionMatches") and
196+
ma.getArgument(0).(CompileTimeConstantExpr).getIntValue() = 0
197+
or
198+
m.hasName("matches") and
199+
not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().matches(".*%")
200+
)
201+
}
202+
203+
/**
204+
* Holds if `ma` is a call to a method that checks a partial string match.
205+
*/
206+
private predicate isStringPartialMatch(MethodAccess ma) {
207+
isStringPrefixMatch(ma)
208+
or
209+
ma.getMethod().getDeclaringType() instanceof TypeString and
210+
ma.getMethod().hasName(["contains", "matches", "regionMatches", "indexOf", "lastIndexOf"])
211+
}
212+
213+
/**
214+
* Holds if `ma` is a call to a method that checks whether a path starts with a prefix.
215+
*/
216+
private predicate isPathPrefixMatch(MethodAccess ma) {
217+
exists(RefType t |
218+
t instanceof TypePath
219+
or
220+
t.hasQualifiedName("kotlin.io", "FilesKt")
221+
|
222+
t = ma.getMethod().getDeclaringType() and
223+
ma.getMethod().hasName("startsWith")
224+
)
225+
}
226+
227+
private predicate isDisallowedWord(CompileTimeConstantExpr word) {
228+
word.getStringValue().matches(["/", "\\", "%WEB-INF%", "%/data%"])
229+
}
230+
231+
/** A complementary guard that protects against path traversal, by looking for the literal `..`. */
232+
private class PathTraversalGuard extends PathGuard {
233+
PathTraversalGuard() {
234+
exists(MethodAccess ma |
235+
ma.getMethod().getDeclaringType() instanceof TypeString and
236+
ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".."
237+
|
238+
this = ma and
239+
ma.getMethod().hasName("contains")
240+
or
241+
exists(EqualityTest eq |
242+
this = eq and
243+
ma.getMethod().hasName(["indexOf", "lastIndexOf"]) and
244+
eq.getAnOperand() = ma and
245+
eq.getAnOperand().(CompileTimeConstantExpr).getIntValue() = -1
246+
)
247+
)
248+
}
249+
250+
override Expr getCheckedExpr() {
251+
exists(MethodAccess ma | ma = this.(EqualityTest).getAnOperand() or ma = this |
252+
result = ma.getQualifier()
253+
)
254+
}
255+
256+
boolean getBranch() {
257+
this instanceof MethodAccess and result = false
258+
or
259+
result = this.(EqualityTest).polarity()
260+
}
261+
}
262+
263+
/** A complementary sanitizer that protects against path traversal using path normalization. */
264+
private class PathNormalizeSanitizer extends MethodAccess {
265+
PathNormalizeSanitizer() {
266+
exists(RefType t |
267+
t instanceof TypePath or
268+
t.hasQualifiedName("kotlin.io", "FilesKt")
269+
|
270+
this.getMethod().getDeclaringType() = t and
271+
this.getMethod().hasName("normalize")
272+
)
273+
or
274+
this.getMethod().getDeclaringType() instanceof TypeFile and
275+
this.getMethod().hasName(["getCanonicalPath", "getCanonicalFile"])
276+
}
277+
}

0 commit comments

Comments
 (0)