Skip to content

Commit 9db65ea

Browse files
committed
Address review comments
1 parent b8fa943 commit 9db65ea

File tree

2 files changed

+45
-11
lines changed

2 files changed

+45
-11
lines changed

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

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -85,14 +85,20 @@ private class AllowedPrefixGuard extends Guard instanceof MethodAccess {
8585
*/
8686
private predicate allowedPrefixGuard(Guard g, Expr e, boolean branch) {
8787
branch = true and
88+
// Local taint-flow is used here to handle cases where the validated expression comes from the
89+
// expression reaching the sink, but it's not the same one, e.g.:
90+
// File file = source();
91+
// String strPath = file.getCanonicalPath();
92+
// if (strPath.startsWith("/safe/dir"))
93+
// sink(file);
8894
TaintTracking::localExprTaint(e, g.(AllowedPrefixGuard).getCheckedExpr()) and
8995
exists(Expr previousGuard |
9096
TaintTracking::localExprTaint(previousGuard.(PathNormalizeSanitizer),
9197
g.(AllowedPrefixGuard).getCheckedExpr())
9298
or
9399
previousGuard
94100
.(PathTraversalGuard)
95-
.controls(g.getBasicBlock().(ConditionBlock), previousGuard.(PathTraversalGuard).getBranch())
101+
.controls(g.getBasicBlock(), previousGuard.(PathTraversalGuard).getBranch())
96102
)
97103
}
98104

@@ -108,12 +114,18 @@ private class AllowedPrefixSanitizer extends PathInjectionSanitizer {
108114
* been checked for a trusted prefix.
109115
*/
110116
private predicate dotDotCheckGuard(Guard g, Expr e, boolean branch) {
117+
// Local taint-flow is used here to handle cases where the validated expression comes from the
118+
// expression reaching the sink, but it's not the same one, e.g.:
119+
// Path path = source();
120+
// String strPath = path.toString();
121+
// if (!strPath.contains("..") && strPath.startsWith("/safe/dir"))
122+
// sink(path);
111123
branch = g.(PathTraversalGuard).getBranch() and
112124
TaintTracking::localExprTaint(e, g.(PathTraversalGuard).getCheckedExpr()) and
113125
exists(Guard previousGuard |
114-
previousGuard.(AllowedPrefixGuard).controls(g.getBasicBlock().(ConditionBlock), true)
126+
previousGuard.(AllowedPrefixGuard).controls(g.getBasicBlock(), true)
115127
or
116-
previousGuard.(BlockListGuard).controls(g.getBasicBlock().(ConditionBlock), false)
128+
previousGuard.(BlockListGuard).controls(g.getBasicBlock(), false)
117129
)
118130
}
119131

@@ -140,14 +152,20 @@ private class BlockListGuard extends Guard instanceof MethodAccess {
140152
*/
141153
private predicate blockListGuard(Guard g, Expr e, boolean branch) {
142154
branch = false and
155+
// Local taint-flow is used here to handle cases where the validated expression comes from the
156+
// expression reaching the sink, but it's not the same one, e.g.:
157+
// File file = source();
158+
// String strPath = file.getCanonicalPath();
159+
// if (!strPath.contains("..") && !strPath.startsWith("/dangerous/dir"))
160+
// sink(file);
143161
TaintTracking::localExprTaint(e, g.(BlockListGuard).getCheckedExpr()) and
144162
exists(Expr previousGuard |
145163
TaintTracking::localExprTaint(previousGuard.(PathNormalizeSanitizer),
146164
g.(BlockListGuard).getCheckedExpr())
147165
or
148166
previousGuard
149167
.(PathTraversalGuard)
150-
.controls(g.getBasicBlock().(ConditionBlock), previousGuard.(PathTraversalGuard).getBranch())
168+
.controls(g.getBasicBlock(), previousGuard.(PathTraversalGuard).getBranch())
151169
)
152170
}
153171

@@ -245,10 +263,3 @@ private class PathNormalizeSanitizer extends MethodAccess {
245263
this.getMethod().hasName(["getCanonicalPath", "getCanonicalFile"])
246264
}
247265
}
248-
249-
/** A node with path normalization. */
250-
class NormalizedPathNode extends DataFlow::Node {
251-
NormalizedPathNode() {
252-
TaintTracking::localExprTaint(this.asExpr(), any(PathNormalizeSanitizer ma))
253-
}
254-
}

java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,29 @@ import JFinalController
1818
import semmle.code.java.security.PathSanitizer
1919
import DataFlow::PathGraph
2020

21+
/** A complementary sanitizer that protects against path traversal using path normalization. */
22+
class PathNormalizeSanitizer extends MethodAccess {
23+
PathNormalizeSanitizer() {
24+
exists(RefType t |
25+
t instanceof TypePath or
26+
t.hasQualifiedName("kotlin.io", "FilesKt")
27+
|
28+
this.getMethod().getDeclaringType() = t and
29+
this.getMethod().hasName("normalize")
30+
)
31+
or
32+
this.getMethod().getDeclaringType() instanceof TypeFile and
33+
this.getMethod().hasName(["getCanonicalPath", "getCanonicalFile"])
34+
}
35+
}
36+
37+
/** A node with path normalization. */
38+
class NormalizedPathNode extends DataFlow::Node {
39+
NormalizedPathNode() {
40+
TaintTracking::localExprTaint(this.asExpr(), any(PathNormalizeSanitizer ma))
41+
}
42+
}
43+
2144
class InjectFilePathConfig extends TaintTracking::Configuration {
2245
InjectFilePathConfig() { this = "InjectFilePathConfig" }
2346

0 commit comments

Comments
 (0)