Skip to content

Commit 4609227

Browse files
luchua-bcluchua-bc
committed
Use data model for request/session attribute operations
1 parent ff4826d commit 4609227

File tree

4 files changed

+104
-54
lines changed

4 files changed

+104
-54
lines changed

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

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,7 @@ import DataFlow::PathGraph
2121
class InjectFilePathConfig extends TaintTracking::Configuration {
2222
InjectFilePathConfig() { this = "InjectFilePathConfig" }
2323

24-
override predicate isSource(DataFlow::Node source) {
25-
source instanceof RemoteFlowSource or
26-
isGetAttributeFromRemoteSource(source.asExpr())
27-
}
24+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
2825

2926
override predicate isSink(DataFlow::Node sink) {
3027
sink.asExpr() = any(PathCreation p).getAnInput()

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

Lines changed: 19 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import java
2-
import semmle.code.java.dataflow.TaintTracking2
32
import semmle.code.java.dataflow.FlowSources
43

54
/** The class `com.jfinal.core.Controller`. */
@@ -34,42 +33,11 @@ class GetRequestAttributeMethod extends Method {
3433
/** The request attribute setter method of `JFinalController`. */
3534
class SetRequestAttributeMethod extends Method {
3635
SetRequestAttributeMethod() {
37-
this.getName() = ["set", "setAttr", "setAttrs"] and
36+
this.getName() = ["set", "setAttr"] and
3837
this.getDeclaringType().getASupertype*() instanceof JFinalController
3938
}
4039
}
4140

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-
)
71-
}
72-
7341
/** Source model of remote flow source with `JFinal`. */
7442
private class JFinalControllerSource extends SourceModelCsv {
7543
override predicate row(string row) {
@@ -90,3 +58,21 @@ private class JFinalControllerSource extends SourceModelCsv {
9058
]
9159
}
9260
}
61+
62+
/** `JFinal` data model related to session and request attribute operations. */
63+
private class JFinalDataModel extends SummaryModelCsv {
64+
override predicate row(string row) {
65+
row =
66+
[
67+
"com.jfinal.core;Controller;true;setSessionAttr;;;Argument[0];MapKey of SyntheticField[com.jfinal.core.Controller.session] of Argument[-1];value",
68+
"com.jfinal.core;Controller;true;setSessionAttr;;;Argument[1];MapValue of SyntheticField[com.jfinal.core.Controller.session] of Argument[-1];value",
69+
"com.jfinal.core;Controller;true;getSessionAttr;;;MapValue of SyntheticField[com.jfinal.core.Controller.session] of Argument[-1];ReturnValue;value",
70+
"com.jfinal.core;Controller;true;set" + ["", "Attr"] +
71+
";;;Argument[0];MapKey of SyntheticField[com.jfinal.core.Controller.request] of Argument[-1];value",
72+
"com.jfinal.core;Controller;true;set" + ["", "Attr"] +
73+
";;;Argument[1];MapValue of SyntheticField[com.jfinal.core.Controller.request] of Argument[-1];value",
74+
"com.jfinal.core;Controller;true;get" + ["Attr", "AttrForStr"] +
75+
";;;MapValue of SyntheticField[com.jfinal.core.Controller.request] of Argument[-1];ReturnValue;value"
76+
]
77+
}
78+
}

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

Lines changed: 36 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,40 @@
11
edges
2-
| FilePathInjection.java:20:21:20:34 | getPara(...) : String | FilePathInjection.java:25:47:25:59 | finalFilePath |
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 |
2+
| FilePathInjection.java:21:21:21:34 | getPara(...) : String | FilePathInjection.java:26:47:26:59 | finalFilePath |
3+
| FilePathInjection.java:41:21:41:34 | getPara(...) : String | FilePathInjection.java:44:25:44:37 | finalFilePath |
4+
| FilePathInjection.java:64:21:64:34 | getPara(...) : String | FilePathInjection.java:65:31:65:38 | savePath : String |
5+
| FilePathInjection.java:65:3:65:39 | this <.method> [post update] [com.jfinal.core.Controller.session, <map.value>] : String | FilePathInjection.java:66:29:66:55 | this <.method> [com.jfinal.core.Controller.session, <map.value>] : String |
6+
| FilePathInjection.java:65:31:65:38 | savePath : String | FilePathInjection.java:65:3:65:39 | this <.method> [post update] [com.jfinal.core.Controller.session, <map.value>] : String |
7+
| FilePathInjection.java:66:29:66:55 | getSessionAttr(...) : String | FilePathInjection.java:72:47:72:59 | finalFilePath |
8+
| FilePathInjection.java:66:29:66:55 | this <.method> [com.jfinal.core.Controller.session, <map.value>] : String | FilePathInjection.java:66:29:66:55 | getSessionAttr(...) : String |
9+
| FilePathInjection.java:87:21:87:34 | getPara(...) : String | FilePathInjection.java:88:24:88:31 | savePath : String |
10+
| FilePathInjection.java:88:3:88:32 | this <.method> [post update] [com.jfinal.core.Controller.request, <map.value>] : String | FilePathInjection.java:89:29:89:48 | this <.method> [com.jfinal.core.Controller.request, <map.value>] : String |
11+
| FilePathInjection.java:88:24:88:31 | savePath : String | FilePathInjection.java:88:3:88:32 | this <.method> [post update] [com.jfinal.core.Controller.request, <map.value>] : String |
12+
| FilePathInjection.java:89:29:89:48 | getAttr(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath |
13+
| FilePathInjection.java:89:29:89:48 | this <.method> [com.jfinal.core.Controller.request, <map.value>] : String | FilePathInjection.java:89:29:89:48 | getAttr(...) : String |
14+
| FilePathInjection.java:158:17:158:44 | getParameter(...) : String | FilePathInjection.java:162:24:162:31 | filePath |
615
nodes
7-
| FilePathInjection.java:20:21:20:34 | getPara(...) : String | semmle.label | getPara(...) : String |
8-
| FilePathInjection.java:25:47:25:59 | finalFilePath | semmle.label | finalFilePath |
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 |
16+
| FilePathInjection.java:21:21:21:34 | getPara(...) : String | semmle.label | getPara(...) : String |
17+
| FilePathInjection.java:26:47:26:59 | finalFilePath | semmle.label | finalFilePath |
18+
| FilePathInjection.java:41:21:41:34 | getPara(...) : String | semmle.label | getPara(...) : String |
19+
| FilePathInjection.java:44:25:44:37 | finalFilePath | semmle.label | finalFilePath |
20+
| FilePathInjection.java:64:21:64:34 | getPara(...) : String | semmle.label | getPara(...) : String |
21+
| FilePathInjection.java:65:3:65:39 | this <.method> [post update] [com.jfinal.core.Controller.session, <map.value>] : String | semmle.label | this <.method> [post update] [com.jfinal.core.Controller.session, <map.value>] : String |
22+
| FilePathInjection.java:65:31:65:38 | savePath : String | semmle.label | savePath : String |
23+
| FilePathInjection.java:66:29:66:55 | getSessionAttr(...) : String | semmle.label | getSessionAttr(...) : String |
24+
| FilePathInjection.java:66:29:66:55 | this <.method> [com.jfinal.core.Controller.session, <map.value>] : String | semmle.label | this <.method> [com.jfinal.core.Controller.session, <map.value>] : String |
25+
| FilePathInjection.java:72:47:72:59 | finalFilePath | semmle.label | finalFilePath |
26+
| FilePathInjection.java:87:21:87:34 | getPara(...) : String | semmle.label | getPara(...) : String |
27+
| FilePathInjection.java:88:3:88:32 | this <.method> [post update] [com.jfinal.core.Controller.request, <map.value>] : String | semmle.label | this <.method> [post update] [com.jfinal.core.Controller.request, <map.value>] : String |
28+
| FilePathInjection.java:88:24:88:31 | savePath : String | semmle.label | savePath : String |
29+
| FilePathInjection.java:89:29:89:48 | getAttr(...) : String | semmle.label | getAttr(...) : String |
30+
| FilePathInjection.java:89:29:89:48 | this <.method> [com.jfinal.core.Controller.request, <map.value>] : String | semmle.label | this <.method> [com.jfinal.core.Controller.request, <map.value>] : String |
31+
| FilePathInjection.java:95:47:95:59 | finalFilePath | semmle.label | finalFilePath |
32+
| FilePathInjection.java:158:17:158:44 | getParameter(...) : String | semmle.label | getParameter(...) : String |
33+
| FilePathInjection.java:162:24:162:31 | filePath | semmle.label | filePath |
1534
subpaths
1635
#select
17-
| 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 |
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 |
36+
| FilePathInjection.java:26:47:26:59 | finalFilePath | FilePathInjection.java:21:21:21:34 | getPara(...) : String | FilePathInjection.java:26:47:26:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:21:21:21:34 | getPara(...) | user-provided value |
37+
| FilePathInjection.java:44:25:44:37 | finalFilePath | FilePathInjection.java:41:21:41:34 | getPara(...) : String | FilePathInjection.java:44:25:44:37 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:41:21:41:34 | getPara(...) | user-provided value |
38+
| FilePathInjection.java:72:47:72:59 | finalFilePath | FilePathInjection.java:64:21:64:34 | getPara(...) : String | FilePathInjection.java:72:47:72:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:64:21:64:34 | getPara(...) | user-provided value |
39+
| FilePathInjection.java:95:47:95:59 | finalFilePath | FilePathInjection.java:87:21:87:34 | getPara(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:87:21:87:34 | getPara(...) | user-provided value |
40+
| FilePathInjection.java:162:24:162:31 | filePath | FilePathInjection.java:158:17:158:44 | getParameter(...) : String | FilePathInjection.java:162:24:162:31 | filePath | External control of file name or path due to $@. | FilePathInjection.java:158:17:158:44 | getParameter(...) | user-provided value |

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

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
import javax.servlet.http.HttpServletRequest;
1111
import javax.servlet.http.HttpServletResponse;
12+
import javax.servlet.http.HttpSession;
1213

1314
import com.jfinal.core.Controller;
1415

@@ -58,7 +59,7 @@ public void uploadFile2() throws IOException {
5859
}
5960
}
6061

61-
// BAD: Upload file to user specified path without validation
62+
// BAD: Upload file to user specified path without validation through session attribute
6263
public void uploadFile3() throws IOException {
6364
String savePath = getPara("dir");
6465
setSessionAttr("uploadDir", savePath);
@@ -81,6 +82,52 @@ public void uploadFile3() throws IOException {
8182
fos.close();
8283
}
8384

85+
// BAD: Upload file to user specified path without validation through request attribute
86+
public void uploadFile4() throws IOException {
87+
String savePath = getPara("dir");
88+
setAttr("uploadDir", savePath);
89+
String requestUploadDir = getAttr("uploadDir");
90+
91+
File file = getFile("fileParam").getFile();
92+
String finalFilePath = BASE_PATH + requestUploadDir;
93+
94+
FileInputStream fis = new FileInputStream(file);
95+
FileOutputStream fos = new FileOutputStream(finalFilePath);
96+
int i = 0;
97+
98+
do {
99+
byte[] buf = new byte[1024];
100+
i = fis.read(buf);
101+
fos.write(buf);
102+
} while (i != -1);
103+
104+
fis.close();
105+
fos.close();
106+
}
107+
108+
// BAD: Upload file to user specified path without validation through session object (not detected)
109+
public void uploadFile5() throws IOException {
110+
String savePath = getPara("dir");
111+
getSession().setAttribute("uploadDir", savePath);
112+
String sessionUploadDir = getSessionAttr("uploadDir");
113+
114+
File file = getFile("fileParam").getFile();
115+
String finalFilePath = BASE_PATH + sessionUploadDir;
116+
117+
FileInputStream fis = new FileInputStream(file);
118+
FileOutputStream fos = new FileOutputStream(finalFilePath);
119+
int i = 0;
120+
121+
do {
122+
byte[] buf = new byte[1024];
123+
i = fis.read(buf);
124+
fos.write(buf);
125+
} while (i != -1);
126+
127+
fis.close();
128+
fos.close();
129+
}
130+
84131
private void readFile(HttpServletResponse resp, File file) {
85132
OutputStream os = null;
86133
FileInputStream fis = null;

0 commit comments

Comments
 (0)