Skip to content

Commit 27043a0

Browse files
luchua-bcluchua-bc
committed
File path injection with the JFinal framework
1 parent 8e40899 commit 27043a0

File tree

12 files changed

+855
-0
lines changed

12 files changed

+855
-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 that a file path
9+
being accessed is composed using data from outside the application (such as the HTTP request, the database, or
10+
the filesystem). It allows 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 the file path. 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 (e.g., must come within <code>user_content/</code>) or disallowed
17+
(e.g. must not come within <code>/internal</code>) paths, ensuring that neither path traversal using <code>../</code>
18+
or URL encoding are 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, 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: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
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 JFinalController
17+
import PathSanitizer
18+
import DataFlow::PathGraph
19+
20+
/**
21+
* A sink that represents a file read operation.
22+
*/
23+
private class ReadFileSinkModels extends SinkModelCsv {
24+
override predicate row(string row) {
25+
row =
26+
[
27+
"java.io;FileInputStream;false;FileInputStream;;;Argument[0];read-file",
28+
"java.io;File;false;File;;;Argument[0];read-file"
29+
]
30+
}
31+
}
32+
33+
/**
34+
* A sink that represents a file creation or access, such as a file read, write, copy or move operation.
35+
*/
36+
private class FileAccessSink extends DataFlow::Node {
37+
FileAccessSink() { sinkNode(this, "create-file") or sinkNode(this, "read-file") }
38+
}
39+
40+
class InjectFilePathConfig extends TaintTracking::Configuration {
41+
InjectFilePathConfig() { this = "InjectFilePathConfig" }
42+
43+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
44+
45+
override predicate isSink(DataFlow::Node sink) { sink instanceof FileAccessSink }
46+
47+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
48+
guard instanceof PathTraversalBarrierGuard
49+
}
50+
}
51+
52+
from DataFlow::PathNode source, DataFlow::PathNode sink, InjectFilePathConfig conf
53+
where conf.hasFlowPath(source, sink)
54+
select sink.getNode(), source, sink, "External control of file name or path due to $@.",
55+
source.getNode(), "user-provided value"

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

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
import java
2+
import semmle.code.java.dataflow.FlowSources
3+
4+
/** The class `com.jfinal.config.Routes`. */
5+
class JFinalRoutes extends RefType {
6+
JFinalRoutes() { this.hasQualifiedName("com.jfinal.config", "Routes") }
7+
}
8+
9+
/** The method `add` of the class `Routes`. */
10+
class AddJFinalRoutes extends Method {
11+
AddJFinalRoutes() {
12+
this.getDeclaringType() instanceof JFinalRoutes and
13+
this.getName() = "add"
14+
}
15+
}
16+
17+
/** The class `com.jfinal.core.Controller`. */
18+
class JFinalController extends RefType {
19+
JFinalController() { this.hasQualifiedName("com.jfinal.core", "Controller") }
20+
}
21+
22+
/** Source model of remote flow source with `JFinal`. */
23+
private class JFinalControllerSource extends SourceModelCsv {
24+
override predicate row(string row) {
25+
row =
26+
[
27+
"com.jfinal.core;Controller;true;getAttr" + ["", "ForInt", "ForStr"] +
28+
";;;ReturnValue;remote",
29+
"com.jfinal.core;Controller;true;getCookie" + ["", "Object", "Objects", "ToInt", "ToLong"] +
30+
";;;ReturnValue;remote",
31+
"com.jfinal.core;Controller;true;getFile" + ["", "s"] + ";;;ReturnValue;remote",
32+
"com.jfinal.core;Controller;true;getHeader;;;ReturnValue;remote",
33+
"com.jfinal.core;Controller;true;getKv;;;ReturnValue;remote",
34+
"com.jfinal.core;Controller;true;getPara" +
35+
[
36+
"", "Map", "ToBoolean", "ToDate", "ToInt", "ToLong", "Values", "ValuesToInt",
37+
"ValuesToLong"
38+
] + ";;;ReturnValue;remote",
39+
"com.jfinal.core;Controller;true;getSession" + ["", "Attr"] + ";;;ReturnValue;remote",
40+
"com.jfinal.core;Controller;true;get" + ["", "Int", "Long", "Boolean", "Date"] +
41+
";;;ReturnValue;remote"
42+
]
43+
}
44+
}

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

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
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+
}

‎java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
edges
2+
| FilePathInjection.java:20:21:20:34 | getPara(...) : String | FilePathInjection.java:25:47:25:59 | finalFilePath |
3+
| FilePathInjection.java:61:50:61:58 | file : File | FilePathInjection.java:66:30:66:33 | file |
4+
| FilePathInjection.java:88:17:88:44 | getParameter(...) : String | FilePathInjection.java:92:24:92:31 | filePath |
5+
| FilePathInjection.java:88:17:88:44 | getParameter(...) : String | FilePathInjection.java:92:24:92:31 | filePath : String |
6+
| FilePathInjection.java:92:15:92:32 | new File(...) : File | FilePathInjection.java:100:19:100:22 | file : File |
7+
| FilePathInjection.java:92:24:92:31 | filePath : String | FilePathInjection.java:92:15:92:32 | new File(...) : File |
8+
| FilePathInjection.java:100:19:100:22 | file : File | FilePathInjection.java:61:50:61:58 | file : File |
9+
nodes
10+
| FilePathInjection.java:20:21:20:34 | getPara(...) : String | semmle.label | getPara(...) : String |
11+
| FilePathInjection.java:25:47:25:59 | finalFilePath | semmle.label | finalFilePath |
12+
| FilePathInjection.java:61:50:61:58 | file : File | semmle.label | file : File |
13+
| FilePathInjection.java:66:30:66:33 | file | semmle.label | file |
14+
| FilePathInjection.java:88:17:88:44 | getParameter(...) : String | semmle.label | getParameter(...) : String |
15+
| FilePathInjection.java:92:15:92:32 | new File(...) : File | semmle.label | new File(...) : File |
16+
| FilePathInjection.java:92:24:92:31 | filePath | semmle.label | filePath |
17+
| FilePathInjection.java:92:24:92:31 | filePath : String | semmle.label | filePath : String |
18+
| FilePathInjection.java:100:19:100:22 | file : File | semmle.label | file : File |
19+
subpaths
20+
#select
21+
| FilePathInjection.java:25:47:25:59 | finalFilePath | FilePathInjection.java:20:21:20:34 | getPara(...) : String | FilePathInjection.java:25:47:25:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:20:21:20:34 | getPara(...) | user-provided value |
22+
| FilePathInjection.java:66:30:66:33 | file | FilePathInjection.java:88:17:88:44 | getParameter(...) : String | FilePathInjection.java:66:30:66:33 | file | External control of file name or path due to $@. | FilePathInjection.java:88:17:88:44 | getParameter(...) | user-provided value |
23+
| FilePathInjection.java:92:24:92:31 | filePath | FilePathInjection.java:88:17:88:44 | getParameter(...) : String | FilePathInjection.java:92:24:92:31 | filePath | External control of file name or path due to $@. | FilePathInjection.java:88:17:88:44 | getParameter(...) | user-provided value |

0 commit comments

Comments
 (0)