Skip to content

Commit ff4826d

Browse files
luchua-bcluchua-bc
committed
Correct the data model and update qldoc
1 parent 27043a0 commit ff4826d

File tree

6 files changed

+132
-58
lines changed

6 files changed

+132
-58
lines changed

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

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,24 +5,24 @@
55

66

77
<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
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, the database, or
1010
the filesystem). It allows an attacker to traverse through the filesystem and access arbitrary files.</p>
1111
</overview>
1212

1313
<recommendation>
14-
<p>Unsanitized user provided data must not be used to construct the file path. In order to prevent File
14+
<p>Unsanitized user-provided data must not be used to construct the file path. In order to prevent File
1515
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.
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.
1919
</p>
2020
</recommendation>
2121

2222
<example>
2323
<p>The following examples show the bad case and the good case respectively.
2424
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
25+
without validating the input, which may cause file leakage. In the <code>GOOD</code> method, the file path
2626
is validated.
2727
</p>
2828
<sample src="FilePathInjection.java" />

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

Lines changed: 12 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -13,36 +13,26 @@
1313

1414
import java
1515
import semmle.code.java.dataflow.FlowSources
16+
import semmle.code.java.security.PathCreation
1617
import JFinalController
1718
import PathSanitizer
1819
import DataFlow::PathGraph
1920

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-
4021
class InjectFilePathConfig extends TaintTracking::Configuration {
4122
InjectFilePathConfig() { this = "InjectFilePathConfig" }
4223

43-
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
24+
override predicate isSource(DataFlow::Node source) {
25+
source instanceof RemoteFlowSource or
26+
isGetAttributeFromRemoteSource(source.asExpr())
27+
}
28+
29+
override predicate isSink(DataFlow::Node sink) {
30+
sink.asExpr() = any(PathCreation p).getAnInput()
31+
}
4432

45-
override predicate isSink(DataFlow::Node sink) { sink instanceof FileAccessSink }
33+
override predicate isSanitizer(DataFlow::Node node) {
34+
exists(Type t | t = node.getType() | t instanceof BoxedType or t instanceof PrimitiveType)
35+
}
4636

4737
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
4838
guard instanceof PathTraversalBarrierGuard

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

Lines changed: 62 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,31 +1,80 @@
11
import java
2+
import semmle.code.java.dataflow.TaintTracking2
23
import semmle.code.java.dataflow.FlowSources
34

4-
/** The class `com.jfinal.config.Routes`. */
5-
class JFinalRoutes extends RefType {
6-
JFinalRoutes() { this.hasQualifiedName("com.jfinal.config", "Routes") }
5+
/** The class `com.jfinal.core.Controller`. */
6+
class JFinalController extends RefType {
7+
JFinalController() { this.hasQualifiedName("com.jfinal.core", "Controller") }
78
}
89

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"
10+
/** The method `getSessionAttr` of `JFinalController`. */
11+
class GetSessionAttributeMethod extends Method {
12+
GetSessionAttributeMethod() {
13+
this.getName() = "getSessionAttr" and
14+
this.getDeclaringType().getASupertype*() instanceof JFinalController
1415
}
1516
}
1617

17-
/** The class `com.jfinal.core.Controller`. */
18-
class JFinalController extends RefType {
19-
JFinalController() { this.hasQualifiedName("com.jfinal.core", "Controller") }
18+
/** The method `setSessionAttr` of `JFinalController`. */
19+
class SetSessionAttributeMethod extends Method {
20+
SetSessionAttributeMethod() {
21+
this.getName() = "setSessionAttr" and
22+
this.getDeclaringType().getASupertype*() instanceof JFinalController
23+
}
24+
}
25+
26+
/** The request attribute getter method of `JFinalController`. */
27+
class GetRequestAttributeMethod extends Method {
28+
GetRequestAttributeMethod() {
29+
this.getName().matches("getAttr%") and
30+
this.getDeclaringType().getASupertype*() instanceof JFinalController
31+
}
32+
}
33+
34+
/** The request attribute setter method of `JFinalController`. */
35+
class SetRequestAttributeMethod extends Method {
36+
SetRequestAttributeMethod() {
37+
this.getName() = ["set", "setAttr", "setAttrs"] and
38+
this.getDeclaringType().getASupertype*() instanceof JFinalController
39+
}
40+
}
41+
42+
/** Taint configuration of flow from remote source to attribute setter methods. */
43+
class SetRemoteAttributeConfig extends TaintTracking2::Configuration {
44+
SetRemoteAttributeConfig() { this = "SetRemoteAttributeConfig" }
45+
46+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
47+
48+
override predicate isSink(DataFlow::Node sink) {
49+
exists(MethodAccess ma |
50+
ma.getMethod() instanceof SetSessionAttributeMethod and
51+
sink.asExpr() = ma.getArgument(1)
52+
)
53+
}
54+
}
55+
56+
/** Holds if the result of an attribute getter call is from a method invocation of remote attribute setter. */
57+
predicate isGetAttributeFromRemoteSource(Expr expr) {
58+
exists(MethodAccess gma, MethodAccess sma |
59+
(
60+
gma.getMethod() instanceof GetSessionAttributeMethod and
61+
sma.getMethod() instanceof SetSessionAttributeMethod
62+
or
63+
gma.getMethod() instanceof GetRequestAttributeMethod and
64+
sma.getMethod() instanceof SetRequestAttributeMethod
65+
) and
66+
expr = gma and
67+
gma.getArgument(0).(CompileTimeConstantExpr).getStringValue() =
68+
sma.getArgument(0).(CompileTimeConstantExpr).getStringValue() and
69+
exists(SetRemoteAttributeConfig cc | cc.hasFlowToExpr(sma.getArgument(1)))
70+
)
2071
}
2172

2273
/** Source model of remote flow source with `JFinal`. */
2374
private class JFinalControllerSource extends SourceModelCsv {
2475
override predicate row(string row) {
2576
row =
2677
[
27-
"com.jfinal.core;Controller;true;getAttr" + ["", "ForInt", "ForStr"] +
28-
";;;ReturnValue;remote",
2978
"com.jfinal.core;Controller;true;getCookie" + ["", "Object", "Objects", "ToInt", "ToLong"] +
3079
";;;ReturnValue;remote",
3180
"com.jfinal.core;Controller;true;getFile" + ["", "s"] + ";;;ReturnValue;remote",
@@ -36,7 +85,6 @@ private class JFinalControllerSource extends SourceModelCsv {
3685
"", "Map", "ToBoolean", "ToDate", "ToInt", "ToLong", "Values", "ValuesToInt",
3786
"ValuesToLong"
3887
] + ";;;ReturnValue;remote",
39-
"com.jfinal.core;Controller;true;getSession" + ["", "Attr"] + ";;;ReturnValue;remote",
4088
"com.jfinal.core;Controller;true;get" + ["", "Int", "Long", "Boolean", "Date"] +
4189
";;;ReturnValue;remote"
4290
]

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

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,20 @@
11
edges
22
| 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 |
3+
| FilePathInjection.java:40:21:40:34 | getPara(...) : String | FilePathInjection.java:43:25:43:37 | finalFilePath |
4+
| FilePathInjection.java:65:29:65:55 | getSessionAttr(...) : String | FilePathInjection.java:71:47:71:59 | finalFilePath |
5+
| FilePathInjection.java:111:17:111:44 | getParameter(...) : String | FilePathInjection.java:115:24:115:31 | filePath |
96
nodes
107
| FilePathInjection.java:20:21:20:34 | getPara(...) : String | semmle.label | getPara(...) : String |
118
| 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 |
9+
| FilePathInjection.java:40:21:40:34 | getPara(...) : String | semmle.label | getPara(...) : String |
10+
| FilePathInjection.java:43:25:43:37 | finalFilePath | semmle.label | finalFilePath |
11+
| FilePathInjection.java:65:29:65:55 | getSessionAttr(...) : String | semmle.label | getSessionAttr(...) : String |
12+
| FilePathInjection.java:71:47:71:59 | finalFilePath | semmle.label | finalFilePath |
13+
| FilePathInjection.java:111:17:111:44 | getParameter(...) : String | semmle.label | getParameter(...) : String |
14+
| FilePathInjection.java:115:24:115:31 | filePath | semmle.label | filePath |
1915
subpaths
2016
#select
2117
| 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 |
18+
| FilePathInjection.java:43:25:43:37 | finalFilePath | FilePathInjection.java:40:21:40:34 | getPara(...) : String | FilePathInjection.java:43:25:43:37 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:40:21:40:34 | getPara(...) | user-provided value |
19+
| FilePathInjection.java:71:47:71:59 | finalFilePath | FilePathInjection.java:65:29:65:55 | getSessionAttr(...) : String | FilePathInjection.java:71:47:71:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:65:29:65:55 | getSessionAttr(...) | user-provided value |
20+
| FilePathInjection.java:115:24:115:31 | filePath | FilePathInjection.java:111:17:111:44 | getParameter(...) : String | FilePathInjection.java:115:24:115:31 | filePath | External control of file name or path due to $@. | FilePathInjection.java:111:17:111:44 | getParameter(...) | user-provided value |

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,29 @@ public void uploadFile2() throws IOException {
5858
}
5959
}
6060

61+
// BAD: Upload file to user specified path without validation
62+
public void uploadFile3() throws IOException {
63+
String savePath = getPara("dir");
64+
setSessionAttr("uploadDir", savePath);
65+
String sessionUploadDir = getSessionAttr("uploadDir");
66+
67+
File file = getFile("fileParam").getFile();
68+
String finalFilePath = BASE_PATH + sessionUploadDir;
69+
70+
FileInputStream fis = new FileInputStream(file);
71+
FileOutputStream fos = new FileOutputStream(finalFilePath);
72+
int i = 0;
73+
74+
do {
75+
byte[] buf = new byte[1024];
76+
i = fis.read(buf);
77+
fos.write(buf);
78+
} while (i != -1);
79+
80+
fis.close();
81+
fos.close();
82+
}
83+
6184
private void readFile(HttpServletResponse resp, File file) {
6285
OutputStream os = null;
6386
FileInputStream fis = null;

‎java/ql/test/stubs/jfinal-4.9.15/com/jfinal/core/Controller.java

Lines changed: 16 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)