Skip to content

Commit 111aabb

Browse files
atorralbaatorralba
authored
Merge pull request #7712 from luchua-bc/java/file-path-injection
Java: CWE-073 File path injection with the JFinal framework
2 parents 3170670 + 40bf093 commit 111aabb

File tree

12 files changed

+1031
-0
lines changed

12 files changed

+1031
-0
lines changed

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// BAD: no file download validation
2+
HttpServletRequest request = getRequest();
3+
String path = request.getParameter("path");
4+
String filePath = "/pages/" + path;
5+
HttpServletResponse resp = getResponse();
6+
File file = new File(filePath);
7+
resp.getOutputStream().write(file.readContent());
8+
9+
// BAD: no file upload validation
10+
String savePath = getPara("dir");
11+
File file = getFile("fileParam").getFile();
12+
FileInputStream fis = new FileInputStream(file);
13+
String filePath = "/files/" + savePath;
14+
FileOutputStream fos = new FileOutputStream(filePath);
15+
16+
// GOOD: check for a trusted prefix, ensuring path traversal is not used to erase that prefix:
17+
// (alternatively use `Path.normalize` instead of checking for `..`)
18+
if (!filePath.contains("..") && filePath.hasPrefix("/pages")) { ... }
19+
// Also GOOD: check for a forbidden prefix, ensuring URL-encoding is not used to evade the check:
20+
// (alternatively use `URLDecoder.decode` before `hasPrefix`)
21+
if (filePath.hasPrefix("/files") && !filePath.contains("%")) { ... }

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

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
7+
<overview>
8+
<p>External Control of File Name or Path, also called File Path Injection, is a vulnerability by which
9+
a file path is created using data from outside the application (such as the HTTP request). It allows
10+
an attacker to traverse through the filesystem and access arbitrary files.</p>
11+
</overview>
12+
13+
<recommendation>
14+
<p>Unsanitized user-provided data must not be used to construct file paths. In order to prevent File
15+
Path Injection, it is recommended to avoid concatenating user input directly into the file path. Instead,
16+
user input should be checked against allowed or disallowed paths (for example, the path must be within
17+
<code>/user_content/</code> or must not be within <code>/internal</code>), ensuring that neither path
18+
traversal using <code>../</code> nor URL encoding is used to evade these checks.
19+
</p>
20+
</recommendation>
21+
22+
<example>
23+
<p>The following examples show the bad case and the good case respectively.
24+
The <code>BAD</code> methods show an HTTP request parameter being used directly to construct a file path
25+
without validating the input, which may cause file leakage. In the <code>GOOD</code> method, the file path
26+
is validated.
27+
</p>
28+
<sample src="FilePathInjection.java" />
29+
</example>
30+
31+
<references>
32+
<li>OWASP:
33+
<a href="https://owasp.org/www-community/attacks/Path_Traversal">Path Traversal</a>.
34+
</li>
35+
<li>Veracode:
36+
<a href="https://www.veracode.com/security/dotnet/cwe-73">External Control of File Name or Path Flaw</a>.
37+
</li>
38+
</references>
39+
</qhelp>

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

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
/**
2+
* @name File Path Injection
3+
* @description Loading files based on unvalidated user-input may cause file information disclosure
4+
* and uploading files with unvalidated file types to an arbitrary directory may lead to
5+
* Remote Command Execution (RCE).
6+
* @kind path-problem
7+
* @problem.severity error
8+
* @precision high
9+
* @id java/file-path-injection
10+
* @tags security
11+
* external/cwe-073
12+
*/
13+
14+
import java
15+
import semmle.code.java.dataflow.FlowSources
16+
import semmle.code.java.security.PathCreation
17+
import JFinalController
18+
import experimental.semmle.code.java.PathSanitizer
19+
import DataFlow::PathGraph
20+
21+
class InjectFilePathConfig extends TaintTracking::Configuration {
22+
InjectFilePathConfig() { this = "InjectFilePathConfig" }
23+
24+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
25+
26+
override predicate isSink(DataFlow::Node sink) {
27+
sink.asExpr() = any(PathCreation p).getAnInput() and
28+
not sink instanceof NormalizedPathNode
29+
}
30+
31+
override predicate isSanitizer(DataFlow::Node node) {
32+
exists(Type t | t = node.getType() | t instanceof BoxedType or t instanceof PrimitiveType)
33+
}
34+
35+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
36+
guard instanceof PathTraversalBarrierGuard
37+
}
38+
}
39+
40+
from DataFlow::PathNode source, DataFlow::PathNode sink, InjectFilePathConfig conf
41+
where conf.hasFlowPath(source, sink)
42+
select sink.getNode(), source, sink, "External control of file name or path due to $@.",
43+
source.getNode(), "user-provided value"

‎java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
import java
2+
import semmle.code.java.dataflow.FlowSources
3+
4+
/** The class `com.jfinal.core.Controller`. */
5+
class JFinalController extends RefType {
6+
JFinalController() { this.hasQualifiedName("com.jfinal.core", "Controller") }
7+
}
8+
9+
/** The method `getSessionAttr` of `JFinalController`. */
10+
class GetSessionAttributeMethod extends Method {
11+
GetSessionAttributeMethod() {
12+
this.getName() = "getSessionAttr" and
13+
this.getDeclaringType().getASupertype*() instanceof JFinalController
14+
}
15+
}
16+
17+
/** The method `setSessionAttr` of `JFinalController`. */
18+
class SetSessionAttributeMethod extends Method {
19+
SetSessionAttributeMethod() {
20+
this.getName() = "setSessionAttr" and
21+
this.getDeclaringType().getASupertype*() instanceof JFinalController
22+
}
23+
}
24+
25+
/** A request attribute getter method of `JFinalController`. */
26+
class GetRequestAttributeMethod extends Method {
27+
GetRequestAttributeMethod() {
28+
this.getName().matches("getAttr%") and
29+
this.getDeclaringType().getASupertype*() instanceof JFinalController
30+
}
31+
}
32+
33+
/** A request attribute setter method of `JFinalController`. */
34+
class SetRequestAttributeMethod extends Method {
35+
SetRequestAttributeMethod() {
36+
this.getName() = ["set", "setAttr"] and
37+
this.getDeclaringType().getASupertype*() instanceof JFinalController
38+
}
39+
}
40+
41+
/**
42+
* Value step from a setter call to a corresponding getter call relating to a
43+
* session or request attribute.
44+
*/
45+
private class SetToGetAttributeStep extends AdditionalValueStep {
46+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
47+
exists(MethodAccess gma, MethodAccess sma |
48+
(
49+
gma.getMethod() instanceof GetSessionAttributeMethod and
50+
sma.getMethod() instanceof SetSessionAttributeMethod
51+
or
52+
gma.getMethod() instanceof GetRequestAttributeMethod and
53+
sma.getMethod() instanceof SetRequestAttributeMethod
54+
) and
55+
gma.getArgument(0).(CompileTimeConstantExpr).getStringValue() =
56+
sma.getArgument(0).(CompileTimeConstantExpr).getStringValue()
57+
|
58+
pred.asExpr() = sma.getArgument(1) and
59+
succ.asExpr() = gma
60+
)
61+
}
62+
}
63+
64+
/** Remote flow source models relating to `JFinal`. */
65+
private class JFinalControllerSource extends SourceModelCsv {
66+
override predicate row(string row) {
67+
row =
68+
[
69+
"com.jfinal.core;Controller;true;getCookie" + ["", "Object", "Objects", "ToInt", "ToLong"] +
70+
";;;ReturnValue;remote",
71+
"com.jfinal.core;Controller;true;getFile" + ["", "s"] + ";;;ReturnValue;remote",
72+
"com.jfinal.core;Controller;true;getHeader;;;ReturnValue;remote",
73+
"com.jfinal.core;Controller;true;getKv;;;ReturnValue;remote",
74+
"com.jfinal.core;Controller;true;getPara" +
75+
[
76+
"", "Map", "ToBoolean", "ToDate", "ToInt", "ToLong", "Values", "ValuesToInt",
77+
"ValuesToLong"
78+
] + ";;;ReturnValue;remote",
79+
"com.jfinal.core;Controller;true;get" + ["", "Int", "Long", "Boolean", "Date"] +
80+
";;;ReturnValue;remote"
81+
]
82+
}
83+
}

‎java/ql/src/experimental/semmle/code/java/PathSanitizer.qll

Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,193 @@
1+
import java
2+
import semmle.code.java.controlflow.Guards
3+
import semmle.code.java.dataflow.FlowSources
4+
5+
/** A barrier guard that protects against path traversal vulnerabilities. */
6+
abstract class PathTraversalBarrierGuard extends DataFlow::BarrierGuard { }
7+
8+
/**
9+
* A guard that considers safe a string being exactly compared to a trusted value.
10+
*/
11+
private class ExactStringPathMatchGuard extends PathTraversalBarrierGuard instanceof MethodAccess {
12+
ExactStringPathMatchGuard() {
13+
super.getMethod().getDeclaringType() instanceof TypeString and
14+
super.getMethod().getName() = ["equals", "equalsIgnoreCase"]
15+
}
16+
17+
override predicate checks(Expr e, boolean branch) {
18+
e = super.getQualifier() and
19+
branch = true
20+
}
21+
}
22+
23+
private class AllowListGuard extends Guard instanceof MethodAccess {
24+
AllowListGuard() {
25+
(isStringPartialMatch(this) or isPathPartialMatch(this)) and
26+
not isDisallowedWord(super.getAnArgument())
27+
}
28+
29+
Expr getCheckedExpr() { result = super.getQualifier() }
30+
}
31+
32+
/**
33+
* A guard that considers a path safe because it is checked against an allowlist of partial trusted values.
34+
* This requires additional protection against path traversal, either another guard (`PathTraversalGuard`)
35+
* or a sanitizer (`PathNormalizeSanitizer`), to ensure any internal `..` components are removed from the path.
36+
*/
37+
private class AllowListBarrierGuard extends PathTraversalBarrierGuard instanceof AllowListGuard {
38+
override predicate checks(Expr e, boolean branch) {
39+
e = super.getCheckedExpr() and
40+
branch = true and
41+
(
42+
// Either a path normalization sanitizer comes before the guard,
43+
exists(PathNormalizeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e))
44+
or
45+
// or a check like `!path.contains("..")` comes before the guard
46+
exists(PathTraversalGuard previousGuard |
47+
DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
48+
previousGuard.controls(this.getBasicBlock().(ConditionBlock), false)
49+
)
50+
)
51+
}
52+
}
53+
54+
/**
55+
* A guard that considers a path safe because it is checked for `..` components, having previously
56+
* been checked for a trusted prefix.
57+
*/
58+
private class DotDotCheckBarrierGuard extends PathTraversalBarrierGuard instanceof PathTraversalGuard {
59+
override predicate checks(Expr e, boolean branch) {
60+
e = super.getCheckedExpr() and
61+
branch = false and
62+
// The same value has previously been checked against a list of allowed prefixes:
63+
exists(AllowListGuard previousGuard |
64+
DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
65+
previousGuard.controls(this.getBasicBlock().(ConditionBlock), true)
66+
)
67+
}
68+
}
69+
70+
private class BlockListGuard extends Guard instanceof MethodAccess {
71+
BlockListGuard() {
72+
(isStringPartialMatch(this) or isPathPartialMatch(this)) and
73+
isDisallowedWord(super.getAnArgument())
74+
}
75+
76+
Expr getCheckedExpr() { result = super.getQualifier() }
77+
}
78+
79+
/**
80+
* A guard that considers a string safe because it is checked against a blocklist of known dangerous values.
81+
* This requires a prior check for URL encoding concealing a forbidden value, either a guard (`UrlEncodingGuard`)
82+
* or a sanitizer (`UrlDecodeSanitizer`).
83+
*/
84+
private class BlockListBarrierGuard extends PathTraversalBarrierGuard instanceof BlockListGuard {
85+
override predicate checks(Expr e, boolean branch) {
86+
e = super.getCheckedExpr() and
87+
branch = false and
88+
(
89+
// Either `e` has been URL decoded:
90+
exists(UrlDecodeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e))
91+
or
92+
// or `e` has previously been checked for URL encoding sequences:
93+
exists(UrlEncodingGuard previousGuard |
94+
DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
95+
previousGuard.controls(this.getBasicBlock(), false)
96+
)
97+
)
98+
}
99+
}
100+
101+
/**
102+
* A guard that considers a string safe because it is checked for URL encoding sequences,
103+
* having previously been checked against a block-list of forbidden values.
104+
*/
105+
private class URLEncodingBarrierGuard extends PathTraversalBarrierGuard instanceof UrlEncodingGuard {
106+
override predicate checks(Expr e, boolean branch) {
107+
e = super.getCheckedExpr() and
108+
branch = false and
109+
exists(BlockListGuard previousGuard |
110+
DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
111+
previousGuard.controls(this.getBasicBlock(), false)
112+
)
113+
}
114+
}
115+
116+
/**
117+
* Holds if `ma` is a call to a method that checks a partial string match.
118+
*/
119+
private predicate isStringPartialMatch(MethodAccess ma) {
120+
ma.getMethod().getDeclaringType() instanceof TypeString and
121+
ma.getMethod().getName() =
122+
["contains", "startsWith", "matches", "regionMatches", "indexOf", "lastIndexOf"]
123+
}
124+
125+
/**
126+
* Holds if `ma` is a call to a method of `java.nio.file.Path` that checks a partial path match.
127+
*/
128+
private predicate isPathPartialMatch(MethodAccess ma) {
129+
ma.getMethod().getDeclaringType() instanceof TypePath and
130+
ma.getMethod().getName() = "startsWith"
131+
}
132+
133+
private predicate isDisallowedWord(CompileTimeConstantExpr word) {
134+
word.getStringValue().matches(["%WEB-INF%", "%META-INF%", "%..%"])
135+
}
136+
137+
/** A complementary guard that protects against path traversal, by looking for the literal `..`. */
138+
class PathTraversalGuard extends Guard instanceof MethodAccess {
139+
Expr checked;
140+
141+
PathTraversalGuard() {
142+
super.getMethod().getDeclaringType() instanceof TypeString and
143+
super.getMethod().hasName(["contains", "indexOf"]) and
144+
super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".."
145+
}
146+
147+
Expr getCheckedExpr() { result = super.getQualifier() }
148+
}
149+
150+
/** A complementary sanitizer that protects against path traversal using path normalization. */
151+
private class PathNormalizeSanitizer extends MethodAccess {
152+
PathNormalizeSanitizer() {
153+
this.getMethod().getDeclaringType().hasQualifiedName("java.nio.file", "Path") and
154+
this.getMethod().hasName("normalize")
155+
}
156+
}
157+
158+
/** A complementary guard that protects against double URL encoding, by looking for the literal `%`. */
159+
private class UrlEncodingGuard extends Guard instanceof MethodAccess {
160+
UrlEncodingGuard() {
161+
super.getMethod().getDeclaringType() instanceof TypeString and
162+
super.getMethod().hasName(["contains", "indexOf"]) and
163+
super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%"
164+
}
165+
166+
Expr getCheckedExpr() { result = super.getQualifier() }
167+
}
168+
169+
/** A complementary sanitizer that protects against double URL encoding using URL decoding. */
170+
private class UrlDecodeSanitizer extends MethodAccess {
171+
UrlDecodeSanitizer() {
172+
this.getMethod().getDeclaringType().hasQualifiedName("java.net", "URLDecoder") and
173+
this.getMethod().hasName("decode")
174+
}
175+
}
176+
177+
/** A node with path normalization. */
178+
class NormalizedPathNode extends DataFlow::Node {
179+
NormalizedPathNode() {
180+
TaintTracking::localExprTaint(this.asExpr(), any(PathNormalizeSanitizer ma))
181+
}
182+
}
183+
184+
/** Data model related to `java.nio.file.Path`. */
185+
private class PathDataModel extends SummaryModelCsv {
186+
override predicate row(string row) {
187+
row =
188+
[
189+
"java.nio.file;Paths;true;get;;;Argument[0];ReturnValue;taint",
190+
"java.nio.file;Path;true;normalize;;;Argument[-1];ReturnValue;taint"
191+
]
192+
}
193+
}

0 commit comments

Comments
 (0)