Skip to content

Commit e56737e

Browse files
luchua-bcluchua-bc
committed
Use value step to optimize the taint step and add a test case for Apache file upload listener
1 parent ed78d39 commit e56737e

File tree

8 files changed

+171
-42
lines changed

8 files changed

+171
-42
lines changed

‎java/ql/src/experimental/Security/CWE/CWE-400/LocalThreadResourceAbuse.ql‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ class ThreadResourceAbuse extends TaintTracking::Configuration {
4949
override predicate isSink(DataFlow::Node sink) { sink instanceof PauseThreadSink }
5050

5151
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
52-
any(ThreadResourceAbuseAdditionalTaintStep r).propagatesTaint(pred, succ)
52+
any(AdditionalValueStep r).step(pred, succ)
5353
}
5454

5555
override predicate isSanitizer(DataFlow::Node node) {

‎java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.ql‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ class ThreadResourceAbuse extends TaintTracking::Configuration {
2323
override predicate isSink(DataFlow::Node sink) { sink instanceof PauseThreadSink }
2424

2525
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
26-
any(ThreadResourceAbuseAdditionalTaintStep r).propagatesTaint(pred, succ)
26+
any(AdditionalValueStep r).step(pred, succ)
2727
}
2828

2929
override predicate isSanitizer(DataFlow::Node node) {

‎java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.qll‎

Lines changed: 25 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import java
44
import semmle.code.java.dataflow.DataFlow
55
import semmle.code.java.dataflow.ExternalFlow
6+
import semmle.code.java.dataflow.FlowSteps
67

78
/** `java.lang.Math` data model for value comparison in the new CSV format. */
89
private class MathCompDataModel extends SummaryModelCsv {
@@ -42,49 +43,36 @@ class LessThanSanitizer extends DataFlow::BarrierGuard instanceof ComparisonExpr
4243
}
4344
}
4445

45-
/**
46-
* A unit class for adding additional taint steps that are specific to thread resource abuse.
47-
*/
48-
class ThreadResourceAbuseAdditionalTaintStep extends Unit {
49-
/**
50-
* Holds if the step from `pred` to `succ` should be considered a taint
51-
* step for thread resource abuse.
52-
*/
53-
abstract predicate propagatesTaint(DataFlow::Node pred, DataFlow::Node succ);
54-
}
55-
56-
private class RunnableAdditionalTaintStep extends ThreadResourceAbuseAdditionalTaintStep {
57-
override predicate propagatesTaint(DataFlow::Node pred, DataFlow::Node succ) {
58-
exists(
59-
Method rm, ClassInstanceExpr ce, Argument arg, Parameter p, FieldAccess fa, int i // thread.start() invokes the run() method of thread implementation
46+
/** Value step from the constructor call of a `Runnable` to the instance parameter (this) of `run`. */
47+
private class RunnableStartToRunStep extends AdditionalValueStep {
48+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
49+
exists(ConstructorCall cc, Method m |
50+
m.getDeclaringType() = cc.getConstructedType().getSourceDeclaration() and
51+
cc.getConstructedType().getASupertype*().hasQualifiedName("java.lang", "Runnable") and
52+
m.hasName("run")
6053
|
61-
rm.hasName("run") and
62-
ce.getConstructedType().getSourceDeclaration() = rm.getSourceDeclaration().getDeclaringType() and
63-
ce.getConstructedType().getASupertype*().hasQualifiedName("java.lang", "Runnable") and
64-
ce.getArgument(i) = arg and
65-
ce.getConstructor().getParameter(i) = p and
66-
fa.getEnclosingCallable() = rm and
67-
DataFlow::localExprFlow(p.getAnAccess(), fa.getField().getAnAssignedValue()) and
68-
pred.asExpr() = arg and
69-
succ.asExpr() = fa
54+
pred.asExpr() = cc and
55+
succ.(DataFlow::InstanceParameterNode).getEnclosingCallable() = m
7056
)
7157
}
7258
}
7359

74-
private class ApacheFileUploadAdditionalTaintStep extends ThreadResourceAbuseAdditionalTaintStep {
75-
override predicate propagatesTaint(DataFlow::Node pred, DataFlow::Node succ) {
76-
exists(Method um, VarAccess va, FieldAccess fa, Constructor ce, AssignExpr ar |
77-
um.getDeclaringType()
60+
/**
61+
* Value step from the constructor call of a `ProgressListener` of Apache File Upload to the
62+
* instance parameter (this) of `update`.
63+
*/
64+
private class ApacheFileUploadProgressUpdateStep extends AdditionalValueStep {
65+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
66+
exists(ConstructorCall cc, Method m |
67+
m.getDeclaringType() = cc.getConstructedType().getSourceDeclaration() and
68+
cc.getConstructedType()
7869
.getASupertype*()
79-
.hasQualifiedName("org.apache.commons.fileupload", "ProgressListener") and
80-
um.hasName("update") and
81-
fa.getEnclosingCallable() = um and
82-
ce.getDeclaringType() = um.getDeclaringType() and
83-
va = ce.getAParameter().getAnAccess() and
84-
pred.asExpr() = va and
85-
succ.asExpr() = fa and
86-
ar.getSource() = va and
87-
ar.getDest() = fa.getField().getAnAccess()
70+
.hasQualifiedName(["org.apache.commons.fileupload", "org.apache.commons.fileupload2"],
71+
"ProgressListener") and
72+
m.hasName("update")
73+
|
74+
pred.asExpr() = cc and
75+
succ.(DataFlow::InstanceParameterNode).getEnclosingCallable() = m
8876
)
8977
}
9078
}

‎java/ql/test/experimental/query-tests/security/CWE-400/ThreadResourceAbuse.expected‎

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,68 @@
11
edges
22
| ThreadResourceAbuse.java:18:25:18:57 | getParameter(...) : String | ThreadResourceAbuse.java:21:28:21:36 | delayTime : Number |
3-
| ThreadResourceAbuse.java:21:28:21:36 | delayTime : Number | ThreadResourceAbuse.java:74:18:74:25 | waitTime |
3+
| ThreadResourceAbuse.java:21:4:21:37 | new UncheckedSyncAction(...) [waitTime] : Number | ThreadResourceAbuse.java:72:15:72:17 | parameter this [waitTime] : Number |
4+
| ThreadResourceAbuse.java:21:28:21:36 | delayTime : Number | ThreadResourceAbuse.java:21:4:21:37 | new UncheckedSyncAction(...) [waitTime] : Number |
5+
| ThreadResourceAbuse.java:21:28:21:36 | delayTime : Number | ThreadResourceAbuse.java:66:30:66:41 | waitTime : Number |
46
| ThreadResourceAbuse.java:29:82:29:114 | getParameter(...) : String | ThreadResourceAbuse.java:30:28:30:36 | delayTime : Number |
5-
| ThreadResourceAbuse.java:30:28:30:36 | delayTime : Number | ThreadResourceAbuse.java:74:18:74:25 | waitTime |
7+
| ThreadResourceAbuse.java:30:4:30:37 | new UncheckedSyncAction(...) [waitTime] : Number | ThreadResourceAbuse.java:72:15:72:17 | parameter this [waitTime] : Number |
8+
| ThreadResourceAbuse.java:30:28:30:36 | delayTime : Number | ThreadResourceAbuse.java:30:4:30:37 | new UncheckedSyncAction(...) [waitTime] : Number |
9+
| ThreadResourceAbuse.java:30:28:30:36 | delayTime : Number | ThreadResourceAbuse.java:66:30:66:41 | waitTime : Number |
10+
| ThreadResourceAbuse.java:66:30:66:41 | waitTime : Number | ThreadResourceAbuse.java:67:20:67:27 | waitTime : Number |
11+
| ThreadResourceAbuse.java:67:20:67:27 | waitTime : Number | ThreadResourceAbuse.java:67:4:67:7 | this [post update] [waitTime] : Number |
12+
| ThreadResourceAbuse.java:72:15:72:17 | parameter this [waitTime] : Number | ThreadResourceAbuse.java:74:18:74:25 | this <.field> [waitTime] : Number |
13+
| ThreadResourceAbuse.java:74:18:74:25 | this <.field> [waitTime] : Number | ThreadResourceAbuse.java:74:18:74:25 | waitTime |
614
| ThreadResourceAbuse.java:141:27:141:43 | getValue(...) : String | ThreadResourceAbuse.java:144:34:144:42 | delayTime |
715
| ThreadResourceAbuse.java:172:19:172:50 | getHeader(...) : String | ThreadResourceAbuse.java:177:17:177:26 | retryAfter |
16+
| ThreadResourceAbuse.java:208:28:208:56 | getParameter(...) : String | ThreadResourceAbuse.java:211:49:211:59 | uploadDelay : Number |
17+
| ThreadResourceAbuse.java:211:30:211:87 | new UploadListener(...) [slowUploads] : Number | UploadListener.java:28:14:28:19 | parameter this [slowUploads] : Number |
18+
| ThreadResourceAbuse.java:211:49:211:59 | uploadDelay : Number | ThreadResourceAbuse.java:211:30:211:87 | new UploadListener(...) [slowUploads] : Number |
19+
| ThreadResourceAbuse.java:211:49:211:59 | uploadDelay : Number | UploadListener.java:15:24:15:44 | sleepMilliseconds : Number |
20+
| UploadListener.java:15:24:15:44 | sleepMilliseconds : Number | UploadListener.java:16:17:16:33 | sleepMilliseconds : Number |
21+
| UploadListener.java:16:17:16:33 | sleepMilliseconds : Number | UploadListener.java:16:3:16:13 | this <.field> [post update] [slowUploads] : Number |
22+
| UploadListener.java:28:14:28:19 | parameter this [slowUploads] : Number | UploadListener.java:29:3:29:11 | this <.field> [slowUploads] : Number |
23+
| UploadListener.java:29:3:29:11 | this <.field> [slowUploads] : Number | UploadListener.java:30:3:30:15 | this <.field> [slowUploads] : Number |
24+
| UploadListener.java:30:3:30:15 | this <.field> [slowUploads] : Number | UploadListener.java:33:7:33:17 | this <.field> [slowUploads] : Number |
25+
| UploadListener.java:30:3:30:15 | this <.field> [slowUploads] : Number | UploadListener.java:35:18:35:28 | this <.field> [slowUploads] : Number |
26+
| UploadListener.java:33:7:33:17 | slowUploads : Number | UploadListener.java:35:18:35:28 | slowUploads |
27+
| UploadListener.java:33:7:33:17 | this <.field> [slowUploads] : Number | UploadListener.java:33:7:33:17 | slowUploads : Number |
28+
| UploadListener.java:35:18:35:28 | this <.field> [slowUploads] : Number | UploadListener.java:35:18:35:28 | slowUploads |
829
nodes
930
| ThreadResourceAbuse.java:18:25:18:57 | getParameter(...) : String | semmle.label | getParameter(...) : String |
31+
| ThreadResourceAbuse.java:21:4:21:37 | new UncheckedSyncAction(...) [waitTime] : Number | semmle.label | new UncheckedSyncAction(...) [waitTime] : Number |
1032
| ThreadResourceAbuse.java:21:28:21:36 | delayTime : Number | semmle.label | delayTime : Number |
1133
| ThreadResourceAbuse.java:29:82:29:114 | getParameter(...) : String | semmle.label | getParameter(...) : String |
34+
| ThreadResourceAbuse.java:30:4:30:37 | new UncheckedSyncAction(...) [waitTime] : Number | semmle.label | new UncheckedSyncAction(...) [waitTime] : Number |
1235
| ThreadResourceAbuse.java:30:28:30:36 | delayTime : Number | semmle.label | delayTime : Number |
36+
| ThreadResourceAbuse.java:66:30:66:41 | waitTime : Number | semmle.label | waitTime : Number |
37+
| ThreadResourceAbuse.java:67:4:67:7 | this [post update] [waitTime] : Number | semmle.label | this [post update] [waitTime] : Number |
38+
| ThreadResourceAbuse.java:67:20:67:27 | waitTime : Number | semmle.label | waitTime : Number |
39+
| ThreadResourceAbuse.java:72:15:72:17 | parameter this [waitTime] : Number | semmle.label | parameter this [waitTime] : Number |
40+
| ThreadResourceAbuse.java:74:18:74:25 | this <.field> [waitTime] : Number | semmle.label | this <.field> [waitTime] : Number |
1341
| ThreadResourceAbuse.java:74:18:74:25 | waitTime | semmle.label | waitTime |
1442
| ThreadResourceAbuse.java:141:27:141:43 | getValue(...) : String | semmle.label | getValue(...) : String |
1543
| ThreadResourceAbuse.java:144:34:144:42 | delayTime | semmle.label | delayTime |
1644
| ThreadResourceAbuse.java:172:19:172:50 | getHeader(...) : String | semmle.label | getHeader(...) : String |
1745
| ThreadResourceAbuse.java:177:17:177:26 | retryAfter | semmle.label | retryAfter |
46+
| ThreadResourceAbuse.java:208:28:208:56 | getParameter(...) : String | semmle.label | getParameter(...) : String |
47+
| ThreadResourceAbuse.java:211:30:211:87 | new UploadListener(...) [slowUploads] : Number | semmle.label | new UploadListener(...) [slowUploads] : Number |
48+
| ThreadResourceAbuse.java:211:49:211:59 | uploadDelay : Number | semmle.label | uploadDelay : Number |
49+
| UploadListener.java:15:24:15:44 | sleepMilliseconds : Number | semmle.label | sleepMilliseconds : Number |
50+
| UploadListener.java:16:3:16:13 | this <.field> [post update] [slowUploads] : Number | semmle.label | this <.field> [post update] [slowUploads] : Number |
51+
| UploadListener.java:16:17:16:33 | sleepMilliseconds : Number | semmle.label | sleepMilliseconds : Number |
52+
| UploadListener.java:28:14:28:19 | parameter this [slowUploads] : Number | semmle.label | parameter this [slowUploads] : Number |
53+
| UploadListener.java:29:3:29:11 | this <.field> [slowUploads] : Number | semmle.label | this <.field> [slowUploads] : Number |
54+
| UploadListener.java:30:3:30:15 | this <.field> [slowUploads] : Number | semmle.label | this <.field> [slowUploads] : Number |
55+
| UploadListener.java:33:7:33:17 | slowUploads : Number | semmle.label | slowUploads : Number |
56+
| UploadListener.java:33:7:33:17 | this <.field> [slowUploads] : Number | semmle.label | this <.field> [slowUploads] : Number |
57+
| UploadListener.java:35:18:35:28 | slowUploads | semmle.label | slowUploads |
58+
| UploadListener.java:35:18:35:28 | this <.field> [slowUploads] : Number | semmle.label | this <.field> [slowUploads] : Number |
59+
subpaths
60+
| ThreadResourceAbuse.java:21:28:21:36 | delayTime : Number | ThreadResourceAbuse.java:66:30:66:41 | waitTime : Number | ThreadResourceAbuse.java:67:4:67:7 | this [post update] [waitTime] : Number | ThreadResourceAbuse.java:21:4:21:37 | new UncheckedSyncAction(...) [waitTime] : Number |
61+
| ThreadResourceAbuse.java:30:28:30:36 | delayTime : Number | ThreadResourceAbuse.java:66:30:66:41 | waitTime : Number | ThreadResourceAbuse.java:67:4:67:7 | this [post update] [waitTime] : Number | ThreadResourceAbuse.java:30:4:30:37 | new UncheckedSyncAction(...) [waitTime] : Number |
62+
| ThreadResourceAbuse.java:211:49:211:59 | uploadDelay : Number | UploadListener.java:15:24:15:44 | sleepMilliseconds : Number | UploadListener.java:16:3:16:13 | this <.field> [post update] [slowUploads] : Number | ThreadResourceAbuse.java:211:30:211:87 | new UploadListener(...) [slowUploads] : Number |
1863
#select
1964
| ThreadResourceAbuse.java:74:18:74:25 | waitTime | ThreadResourceAbuse.java:18:25:18:57 | getParameter(...) : String | ThreadResourceAbuse.java:74:18:74:25 | waitTime | Vulnerability of uncontrolled resource consumption due to $@. | ThreadResourceAbuse.java:18:25:18:57 | getParameter(...) | user-provided value |
2065
| ThreadResourceAbuse.java:74:18:74:25 | waitTime | ThreadResourceAbuse.java:29:82:29:114 | getParameter(...) : String | ThreadResourceAbuse.java:74:18:74:25 | waitTime | Vulnerability of uncontrolled resource consumption due to $@. | ThreadResourceAbuse.java:29:82:29:114 | getParameter(...) | user-provided value |
2166
| ThreadResourceAbuse.java:144:34:144:42 | delayTime | ThreadResourceAbuse.java:141:27:141:43 | getValue(...) : String | ThreadResourceAbuse.java:144:34:144:42 | delayTime | Vulnerability of uncontrolled resource consumption due to $@. | ThreadResourceAbuse.java:141:27:141:43 | getValue(...) | user-provided value |
2267
| ThreadResourceAbuse.java:177:17:177:26 | retryAfter | ThreadResourceAbuse.java:172:19:172:50 | getHeader(...) : String | ThreadResourceAbuse.java:177:17:177:26 | retryAfter | Vulnerability of uncontrolled resource consumption due to $@. | ThreadResourceAbuse.java:172:19:172:50 | getHeader(...) | user-provided value |
68+
| UploadListener.java:35:18:35:28 | slowUploads | ThreadResourceAbuse.java:208:28:208:56 | getParameter(...) : String | UploadListener.java:35:18:35:28 | slowUploads | Vulnerability of uncontrolled resource consumption due to $@. | ThreadResourceAbuse.java:208:28:208:56 | getParameter(...) | user-provided value |

‎java/ql/test/experimental/query-tests/security/CWE-400/ThreadResourceAbuse.java‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,4 +192,23 @@ protected void doHead3(HttpServletRequest request, HttpServletResponse response)
192192
// ignore
193193
}
194194
}
195+
196+
private long getContentLength(HttpServletRequest request) {
197+
long size = -1;
198+
try {
199+
size = Long.parseLong(request.getHeader("Content-length"));
200+
} catch (NumberFormatException e) {
201+
}
202+
return size;
203+
}
204+
205+
protected void doHead4(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
206+
// Get thread pause time from request header
207+
try {
208+
String uploadDelayStr = request.getParameter("delay");
209+
int uploadDelay = Integer.parseInt(uploadDelayStr);
210+
211+
UploadListener listener = new UploadListener(uploadDelay, getContentLength(request));
212+
} catch (Exception e) { }
213+
}
195214
}

‎java/ql/test/experimental/query-tests/security/CWE-400/UploadListener.java‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
package test.cwe400.cwe.examples;
2+
3+
import java.io.Serializable;
4+
import java.util.Date;
5+
6+
import javax.servlet.http.HttpServletRequest;
7+
8+
import org.apache.commons.fileupload2.ProgressListener;
9+
10+
public class UploadListener implements ProgressListener, Serializable {
11+
protected int slowUploads = 0;
12+
private Long bytesRead = 0L;
13+
private long contentLength = 0L;
14+
15+
public UploadListener(int sleepMilliseconds, long requestSize) {
16+
slowUploads = sleepMilliseconds;
17+
contentLength = requestSize;
18+
}
19+
20+
public long getPercent() {
21+
return contentLength != 0 ? bytesRead * 100 / contentLength : 0;
22+
}
23+
24+
public long getBytesRead() {
25+
return bytesRead;
26+
}
27+
28+
public void update(long done, long total, int item) {
29+
bytesRead = done;
30+
contentLength = total;
31+
32+
// Just a way to slow down the upload process and see the progress bar in fast networks.
33+
if (slowUploads > 0 && done < total) {
34+
try {
35+
Thread.sleep(slowUploads);
36+
} catch (Exception e) {
37+
}
38+
}
39+
}
40+
}

‎java/ql/test/experimental/query-tests/security/CWE-400/options‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4
1+
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/apache-commons-fileupload-1.4

‎java/ql/test/stubs/apache-commons-fileupload-1.4/org/apache/commons/fileupload2/ProgressListener.java‎

Lines changed: 36 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)