Skip to content

Commit 78630f2

Browse files
committed
Match attribute name to reduce FP
1 parent e3d0e9f commit 78630f2

File tree

3 files changed

+80
-40
lines changed

3 files changed

+80
-40
lines changed

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

Lines changed: 28 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,34 @@ class SetRequestAttributeMethod extends Method {
3838
}
3939
}
4040

41+
/**
42+
* Holds if the result of an attribute getter call is from a method invocation of remote attribute setter.
43+
* Only values received from remote flow source is to be checked by the query.
44+
*/
45+
predicate isGetAttributeFromRemoteSource(Expr expr) {
46+
exists(MethodAccess gma, MethodAccess sma |
47+
(
48+
gma.getMethod() instanceof GetSessionAttributeMethod and
49+
sma.getMethod() instanceof SetSessionAttributeMethod
50+
or
51+
gma.getMethod() instanceof GetRequestAttributeMethod and
52+
sma.getMethod() instanceof SetRequestAttributeMethod
53+
) and
54+
expr = gma and
55+
gma.getArgument(0).(CompileTimeConstantExpr).getStringValue() =
56+
sma.getArgument(0).(CompileTimeConstantExpr).getStringValue() and
57+
gma.getEnclosingCallable() = sma.getEnclosingCallable() and
58+
TaintTracking::localExprTaint(any(RemoteFlowSource rs).asExpr(), sma.getArgument(1))
59+
)
60+
}
61+
62+
/** Remote flow source of JFinal request or session attribute getters. */
63+
private class JFinalRequestSource extends RemoteFlowSource {
64+
JFinalRequestSource() { isGetAttributeFromRemoteSource(this.asExpr()) }
65+
66+
override string getSourceType() { result = "JFinal session or request attribute source" }
67+
}
68+
4169
/** Source model of remote flow source with `JFinal`. */
4270
private class JFinalControllerSource extends SourceModelCsv {
4371
override predicate row(string row) {
@@ -58,21 +86,3 @@ private class JFinalControllerSource extends SourceModelCsv {
5886
]
5987
}
6088
}
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-
}
Lines changed: 6 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,36 +1,20 @@
11
edges
22
| FilePathInjection.java:21:21:21:34 | getPara(...) : String | FilePathInjection.java:26:47:26:59 | finalFilePath |
3-
| FilePathInjection.java:64:21:64:34 | getPara(...) : String | FilePathInjection.java:65:31:65:38 | savePath : String |
4-
| 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 |
5-
| 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 |
63
| FilePathInjection.java:66:29:66:55 | getSessionAttr(...) : String | FilePathInjection.java:72:47:72:59 | finalFilePath |
7-
| FilePathInjection.java:66:29:66:55 | this <.method> [com.jfinal.core.Controller.session, <map.value>] : String | FilePathInjection.java:66:29:66:55 | getSessionAttr(...) : String |
8-
| FilePathInjection.java:87:21:87:34 | getPara(...) : String | FilePathInjection.java:88:24:88:31 | savePath : String |
9-
| 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 |
10-
| 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 |
114
| FilePathInjection.java:89:29:89:48 | getAttr(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath |
12-
| FilePathInjection.java:89:29:89:48 | this <.method> [com.jfinal.core.Controller.request, <map.value>] : String | FilePathInjection.java:89:29:89:48 | getAttr(...) : String |
13-
| FilePathInjection.java:159:17:159:44 | getParameter(...) : String | FilePathInjection.java:163:24:163:31 | filePath |
5+
| FilePathInjection.java:205:17:205:44 | getParameter(...) : String | FilePathInjection.java:209:24:209:31 | filePath |
146
nodes
157
| FilePathInjection.java:21:21:21:34 | getPara(...) : String | semmle.label | getPara(...) : String |
168
| FilePathInjection.java:26:47:26:59 | finalFilePath | semmle.label | finalFilePath |
17-
| FilePathInjection.java:64:21:64:34 | getPara(...) : String | semmle.label | getPara(...) : String |
18-
| 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 |
19-
| FilePathInjection.java:65:31:65:38 | savePath : String | semmle.label | savePath : String |
209
| FilePathInjection.java:66:29:66:55 | getSessionAttr(...) : String | semmle.label | getSessionAttr(...) : String |
21-
| 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 |
2210
| FilePathInjection.java:72:47:72:59 | finalFilePath | semmle.label | finalFilePath |
23-
| FilePathInjection.java:87:21:87:34 | getPara(...) : String | semmle.label | getPara(...) : String |
24-
| 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 |
25-
| FilePathInjection.java:88:24:88:31 | savePath : String | semmle.label | savePath : String |
2611
| FilePathInjection.java:89:29:89:48 | getAttr(...) : String | semmle.label | getAttr(...) : String |
27-
| 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 |
2812
| FilePathInjection.java:95:47:95:59 | finalFilePath | semmle.label | finalFilePath |
29-
| FilePathInjection.java:159:17:159:44 | getParameter(...) : String | semmle.label | getParameter(...) : String |
30-
| FilePathInjection.java:163:24:163:31 | filePath | semmle.label | filePath |
13+
| FilePathInjection.java:205:17:205:44 | getParameter(...) : String | semmle.label | getParameter(...) : String |
14+
| FilePathInjection.java:209:24:209:31 | filePath | semmle.label | filePath |
3115
subpaths
3216
#select
3317
| 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 |
34-
| 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 |
35-
| 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 |
36-
| FilePathInjection.java:163:24:163:31 | filePath | FilePathInjection.java:159:17:159:44 | getParameter(...) : String | FilePathInjection.java:163:24:163:31 | filePath | External control of file name or path due to $@. | FilePathInjection.java:159:17:159:44 | getParameter(...) | user-provided value |
18+
| FilePathInjection.java:72:47:72:59 | finalFilePath | FilePathInjection.java:66:29:66:55 | getSessionAttr(...) : String | FilePathInjection.java:72:47:72:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:66:29:66:55 | getSessionAttr(...) | user-provided value |
19+
| FilePathInjection.java:95:47:95:59 | finalFilePath | FilePathInjection.java:89:29:89:48 | getAttr(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:89:29:89:48 | getAttr(...) | user-provided value |
20+
| FilePathInjection.java:209:24:209:31 | filePath | FilePathInjection.java:205:17:205:44 | getParameter(...) : String | FilePathInjection.java:209:24:209:31 | filePath | External control of file name or path due to $@. | FilePathInjection.java:205:17:205:44 | getParameter(...) | user-provided value |

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

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,52 @@ public void uploadFile5() throws IOException {
128128
fos.close();
129129
}
130130

131+
// GOOD: Upload file to a system path from a request object
132+
public void uploadFile6() throws IOException {
133+
setAttr("uploadDir", "/data/upload_dir/");
134+
String requestUploadDir = getAttr("uploadDir");
135+
136+
File file = getFile("fileParam").getFile();
137+
String finalFilePath = BASE_PATH + requestUploadDir;
138+
139+
FileInputStream fis = new FileInputStream(file);
140+
FileOutputStream fos = new FileOutputStream(finalFilePath);
141+
int i = 0;
142+
143+
do {
144+
byte[] buf = new byte[1024];
145+
i = fis.read(buf);
146+
fos.write(buf);
147+
} while (i != -1);
148+
149+
fis.close();
150+
fos.close();
151+
}
152+
153+
// GOOD: Upload file to a system path from a request object
154+
public void uploadFile7() throws IOException {
155+
String savePath = getPara("dir");
156+
setAttr("uploadDir", savePath);
157+
setAttr("realUploadDir", "/data/upload_dir/");
158+
String requestUploadDir = getAttr("realUploadDir");
159+
160+
File file = getFile("fileParam").getFile();
161+
String finalFilePath = BASE_PATH + requestUploadDir;
162+
163+
FileInputStream fis = new FileInputStream(file);
164+
FileOutputStream fos = new FileOutputStream(finalFilePath);
165+
int i = 0;
166+
167+
do {
168+
byte[] buf = new byte[1024];
169+
i = fis.read(buf);
170+
fos.write(buf);
171+
} while (i != -1);
172+
173+
fis.close();
174+
fos.close();
175+
}
176+
131177
private void readFile(HttpServletResponse resp, File file) {
132178
OutputStream os = null;
133179
FileInputStream fis = null;

0 commit comments

Comments
 (0)