Skip to content

Commit 3c8f6e3

Browse files
smowtonsmowton
authored
Merge pull request #6717 from luchua-bc/java/thread-resource-abuse
Java: CWE-400 - Query to detect uncontrolled thread resource consumption
2 parents 4b3b1d2 + b0031a0 commit 3c8f6e3

13 files changed

Lines changed: 638 additions & 0 deletions

File tree

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

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
/**
2+
* @name Uncontrolled thread resource consumption from local input source
3+
* @description Using user input directly to control a thread's sleep time could lead to
4+
* performance problems or even resource exhaustion.
5+
* @kind path-problem
6+
* @id java/thread-resource-abuse
7+
* @problem.severity recommendation
8+
* @tags security
9+
* external/cwe/cwe-400
10+
*/
11+
12+
import java
13+
import ThreadResourceAbuse
14+
import semmle.code.java.dataflow.FlowSources
15+
import DataFlow::PathGraph
16+
17+
/** The `getInitParameter` method of servlet or JSF. */
18+
class GetInitParameter extends Method {
19+
GetInitParameter() {
20+
(
21+
this.getDeclaringType()
22+
.getASupertype*()
23+
.hasQualifiedName(["javax.servlet", "jakarta.servlet"],
24+
["FilterConfig", "Registration", "ServletConfig", "ServletContext"]) or
25+
this.getDeclaringType()
26+
.getASupertype*()
27+
.hasQualifiedName(["javax.faces.context", "jakarta.faces.context"], "ExternalContext")
28+
) and
29+
this.getName() = "getInitParameter"
30+
}
31+
}
32+
33+
/** An access to the `getInitParameter` method. */
34+
class GetInitParameterAccess extends MethodAccess {
35+
GetInitParameterAccess() { this.getMethod() instanceof GetInitParameter }
36+
}
37+
38+
/* Init parameter input of a Java EE web application. */
39+
class InitParameterInput extends LocalUserInput {
40+
InitParameterInput() { this.asExpr() instanceof GetInitParameterAccess }
41+
}
42+
43+
/** Taint configuration of uncontrolled thread resource consumption from local user input. */
44+
class ThreadResourceAbuse extends TaintTracking::Configuration {
45+
ThreadResourceAbuse() { this = "ThreadResourceAbuse" }
46+
47+
override predicate isSource(DataFlow::Node source) { source instanceof LocalUserInput }
48+
49+
override predicate isSink(DataFlow::Node sink) { sink instanceof PauseThreadSink }
50+
51+
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
52+
any(AdditionalValueStep r).step(pred, succ)
53+
}
54+
55+
override predicate isSanitizer(DataFlow::Node node) {
56+
exists(
57+
MethodAccess ma // Math.min(sleepTime, MAX_INTERVAL)
58+
|
59+
ma.getMethod().hasQualifiedName("java.lang", "Math", "min") and
60+
node.asExpr() = ma.getAnArgument()
61+
)
62+
}
63+
64+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
65+
guard instanceof LessThanSanitizer // if (sleepTime > 0 && sleepTime < 5000) { ... }
66+
}
67+
}
68+
69+
from DataFlow::PathNode source, DataFlow::PathNode sink, ThreadResourceAbuse conf
70+
where conf.hasFlowPath(source, sink)
71+
select sink.getNode(), source, sink, "Possible uncontrolled resource consumption due to $@.",
72+
source.getNode(), "local user-provided value"

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
class SleepTest {
2+
public void test(int userSuppliedWaitTime) throws Exception {
3+
// BAD: no boundary check on wait time
4+
Thread.sleep(userSuppliedWaitTime);
5+
6+
// GOOD: enforce an upper limit on wait time
7+
if (userSuppliedWaitTime > 0 && userSuppliedWaitTime < 5000) {
8+
Thread.sleep(userSuppliedWaitTime);
9+
}
10+
}
11+
}

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
7+
<overview>
8+
<p>The <code>Thread.sleep</code> method is used to pause the execution of current thread for
9+
specified time. When the sleep time is user-controlled, especially in the web application context,
10+
it can be abused to cause all of a server's threads to sleep, leading to denial of service.</p>
11+
</overview>
12+
13+
<recommendation>
14+
<p>To guard against this attack, consider specifying an upper range of allowed sleep time or adopting
15+
the producer/consumer design pattern with <code>Object.wait</code> method to avoid performance
16+
problems or even resource exhaustion. For more information, refer to the concurrency tutorial of Oracle
17+
listed below or <code>java/ql/src/Likely Bugs/Concurrency</code> queries of CodeQL.</p>
18+
</recommendation>
19+
20+
<example>
21+
<p>The following example shows a bad situation and a good situation respectively. In the bad situation,
22+
a thread sleep time comes directly from user input. In the good situation, an upper
23+
range check on the maximum sleep time allowed is enforced.</p>
24+
<sample src="ThreadResourceAbuse.java" />
25+
</example>
26+
27+
<references>
28+
<li>
29+
Snyk:
30+
<a href="https://snyk.io/vuln/SNYK-JAVA-COMGOOGLECODEGWTUPLOAD-569506">Denial of Service (DoS)
31+
in com.googlecode.gwtupload:gwtupload</a>.
32+
</li>
33+
<li>
34+
gwtupload:
35+
<a href="https://github.com/manolo/gwtupload/issues/33">[Fix DOS issue] Updating the
36+
AbstractUploadListener.java file</a>.
37+
</li>
38+
<li>
39+
The blog of a gypsy engineer:
40+
<a href="https://blog.gypsyengineer.com/en/security/cve-2019-17555-dos-via-retry-after-header-in-apache-olingo.html">
41+
CVE-2019-17555: DoS via Retry-After header in Apache Olingo</a>.
42+
</li>
43+
<li>
44+
Oracle:
45+
<a href="https://docs.oracle.com/javase/tutorial/essential/concurrency/guardmeth.html">The Java Concurrency Tutorials</a>
46+
</li>
47+
</references>
48+
</qhelp>

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
/**
2+
* @name Uncontrolled thread resource consumption
3+
* @description Using user input directly to control a thread's sleep time could lead to
4+
* performance problems or even resource exhaustion.
5+
* @kind path-problem
6+
* @id java/thread-resource-abuse
7+
* @problem.severity warning
8+
* @tags security
9+
* external/cwe/cwe-400
10+
*/
11+
12+
import java
13+
import ThreadResourceAbuse
14+
import semmle.code.java.dataflow.FlowSources
15+
import DataFlow::PathGraph
16+
17+
/** Taint configuration of uncontrolled thread resource consumption. */
18+
class ThreadResourceAbuse extends TaintTracking::Configuration {
19+
ThreadResourceAbuse() { this = "ThreadResourceAbuse" }
20+
21+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
22+
23+
override predicate isSink(DataFlow::Node sink) { sink instanceof PauseThreadSink }
24+
25+
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
26+
any(AdditionalValueStep r).step(pred, succ)
27+
}
28+
29+
override predicate isSanitizer(DataFlow::Node node) {
30+
exists(
31+
MethodAccess ma // Math.min(sleepTime, MAX_INTERVAL)
32+
|
33+
ma.getMethod().hasQualifiedName("java.lang", "Math", "min") and
34+
node.asExpr() = ma.getAnArgument()
35+
)
36+
}
37+
38+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
39+
guard instanceof LessThanSanitizer // if (sleepTime > 0 && sleepTime < 5000) { ... }
40+
}
41+
}
42+
43+
from DataFlow::PathNode source, DataFlow::PathNode sink, ThreadResourceAbuse conf
44+
where conf.hasFlowPath(source, sink)
45+
select sink.getNode(), source, sink,
46+
"Vulnerability of uncontrolled resource consumption due to $@.", source.getNode(),
47+
"user-provided value"

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

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
/** Provides sink models and classes related to pausing thread operations. */
2+
3+
import java
4+
import semmle.code.java.dataflow.DataFlow
5+
import semmle.code.java.dataflow.ExternalFlow
6+
import semmle.code.java.dataflow.FlowSteps
7+
8+
/** `java.lang.Math` data model for value comparison in the new CSV format. */
9+
private class MathCompDataModel extends SummaryModelCsv {
10+
override predicate row(string row) {
11+
row =
12+
[
13+
"java.lang;Math;false;min;;;Argument[0..1];ReturnValue;value",
14+
"java.lang;Math;false;max;;;Argument[0..1];ReturnValue;value"
15+
]
16+
}
17+
}
18+
19+
/** Thread pause data model in the new CSV format. */
20+
private class PauseThreadDataModel extends SinkModelCsv {
21+
override predicate row(string row) {
22+
row =
23+
[
24+
"java.lang;Thread;true;sleep;;;Argument[0];thread-pause",
25+
"java.util.concurrent;TimeUnit;true;sleep;;;Argument[0];thread-pause"
26+
]
27+
}
28+
}
29+
30+
/** A sink representing methods pausing a thread. */
31+
class PauseThreadSink extends DataFlow::Node {
32+
PauseThreadSink() { sinkNode(this, "thread-pause") }
33+
}
34+
35+
/** A sanitizer for lessThan check. */
36+
class LessThanSanitizer extends DataFlow::BarrierGuard instanceof ComparisonExpr {
37+
override predicate checks(Expr e, boolean branch) {
38+
e = super.getLesserOperand() and
39+
branch = true
40+
or
41+
e = super.getGreaterOperand() and
42+
branch = false
43+
}
44+
}
45+
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")
53+
|
54+
pred.asExpr() = cc and
55+
succ.(DataFlow::InstanceParameterNode).getEnclosingCallable() = m
56+
)
57+
}
58+
}
59+
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()
69+
.getASupertype*()
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
76+
)
77+
}
78+
}

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
edges
2+
| ThreadResourceAbuse.java:37:25:37:73 | getInitParameter(...) : String | ThreadResourceAbuse.java:40:28:40:36 | delayTime : Number |
3+
| ThreadResourceAbuse.java:40:4:40:37 | new UncheckedSyncAction(...) [waitTime] : Number | ThreadResourceAbuse.java:71:15:71:17 | parameter this [waitTime] : Number |
4+
| ThreadResourceAbuse.java:40:28:40:36 | delayTime : Number | ThreadResourceAbuse.java:40:4:40:37 | new UncheckedSyncAction(...) [waitTime] : Number |
5+
| ThreadResourceAbuse.java:40:28:40:36 | delayTime : Number | ThreadResourceAbuse.java:66:30:66:41 | waitTime : Number |
6+
| ThreadResourceAbuse.java:66:30:66:41 | waitTime : Number | ThreadResourceAbuse.java:67:20:67:27 | waitTime : Number |
7+
| ThreadResourceAbuse.java:67:20:67:27 | waitTime : Number | ThreadResourceAbuse.java:67:4:67:7 | this [post update] [waitTime] : Number |
8+
| ThreadResourceAbuse.java:71:15:71:17 | parameter this [waitTime] : Number | ThreadResourceAbuse.java:74:18:74:25 | this <.field> [waitTime] : Number |
9+
| ThreadResourceAbuse.java:74:18:74:25 | this <.field> [waitTime] : Number | ThreadResourceAbuse.java:74:18:74:25 | waitTime |
10+
nodes
11+
| ThreadResourceAbuse.java:37:25:37:73 | getInitParameter(...) : String | semmle.label | getInitParameter(...) : String |
12+
| ThreadResourceAbuse.java:40:4:40:37 | new UncheckedSyncAction(...) [waitTime] : Number | semmle.label | new UncheckedSyncAction(...) [waitTime] : Number |
13+
| ThreadResourceAbuse.java:40:28:40:36 | delayTime : Number | semmle.label | delayTime : Number |
14+
| ThreadResourceAbuse.java:66:30:66:41 | waitTime : Number | semmle.label | waitTime : Number |
15+
| ThreadResourceAbuse.java:67:4:67:7 | this [post update] [waitTime] : Number | semmle.label | this [post update] [waitTime] : Number |
16+
| ThreadResourceAbuse.java:67:20:67:27 | waitTime : Number | semmle.label | waitTime : Number |
17+
| ThreadResourceAbuse.java:71:15:71:17 | parameter this [waitTime] : Number | semmle.label | parameter this [waitTime] : Number |
18+
| ThreadResourceAbuse.java:74:18:74:25 | this <.field> [waitTime] : Number | semmle.label | this <.field> [waitTime] : Number |
19+
| ThreadResourceAbuse.java:74:18:74:25 | waitTime | semmle.label | waitTime |
20+
subpaths
21+
| ThreadResourceAbuse.java:40:28:40:36 | delayTime : Number | ThreadResourceAbuse.java:66:30:66:41 | waitTime : Number | ThreadResourceAbuse.java:67:4:67:7 | this [post update] [waitTime] : Number | ThreadResourceAbuse.java:40:4:40:37 | new UncheckedSyncAction(...) [waitTime] : Number |
22+
#select
23+
| ThreadResourceAbuse.java:74:18:74:25 | waitTime | ThreadResourceAbuse.java:37:25:37:73 | getInitParameter(...) : String | ThreadResourceAbuse.java:74:18:74:25 | waitTime | Possible uncontrolled resource consumption due to $@. | ThreadResourceAbuse.java:37:25:37:73 | getInitParameter(...) | local user-provided value |

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
experimental/Security/CWE/CWE-400/LocalThreadResourceAbuse.ql

0 commit comments

Comments
 (0)