Skip to content

Commit 000c1f7

Browse files
ebickleebickle
committed
Java: Flow taint through ArithExpr for ThreadResourceAbuse
Ensure that tainted values flow through arithmetic operations when checking for ThreadResourceAbuse vulnerabilities. For example, multiplying 'number of seconds' by 1000 as an input to Thread.Sleep, which accepts milliseconds, is a common scenario.
1 parent eb3f196 commit 000c1f7

File tree

4 files changed

+74
-1
lines changed

4 files changed

+74
-1
lines changed

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@ module ThreadResourceAbuseConfig implements DataFlow::ConfigSig {
2222
predicate isSink(DataFlow::Node sink) { sink instanceof PauseThreadSink }
2323

2424
predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
25-
any(AdditionalValueStep r).step(pred, succ)
25+
any(AdditionalValueStep r).step(pred, succ) or
26+
any(ThreadResourceAbuseAdditionalTaintStep c).step(pred, succ)
2627
}
2728

2829
predicate isBarrier(DataFlow::Node node) {

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import java
44
import semmle.code.java.dataflow.DataFlow
55
private import semmle.code.java.dataflow.ExternalFlow
6+
import semmle.code.java.arithmetic.Overflow
67
import semmle.code.java.dataflow.FlowSteps
78
import semmle.code.java.controlflow.Guards
89

@@ -61,3 +62,34 @@ private class ApacheFileUploadProgressUpdateStep extends AdditionalValueStep {
6162
)
6263
}
6364
}
65+
66+
/**
67+
* A unit class for adding additional taint steps.
68+
*
69+
* Extend this class to add additional taint steps that should apply to the `ThreadResourceAbuseConfig`.
70+
*/
71+
class ThreadResourceAbuseAdditionalTaintStep extends Unit {
72+
/**
73+
* Holds if the step from `node1` to `node2` should be considered a taint
74+
* step for the `ThreadResourceAbuseConfig` configuration.
75+
*/
76+
abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
77+
}
78+
79+
/** A set of additional taint steps to consider when taint tracking thread resource abuse related data flows. */
80+
private class DefaultThreadResourceAbuseAdditionalTaintStep extends ThreadResourceAbuseAdditionalTaintStep
81+
{
82+
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
83+
threadResourceAbuseArithmeticTaintStep(node1, node2)
84+
}
85+
}
86+
87+
/**
88+
* Holds if the step `node1` -> `node2` is an additional taint-step that performs an addition, multiplication,
89+
* subtraction, or division.
90+
*/
91+
private predicate threadResourceAbuseArithmeticTaintStep(
92+
DataFlow::Node fromNode, DataFlow::Node toNode
93+
) {
94+
toNode.asExpr().(ArithExpr).getAnOperand() = fromNode.asExpr()
95+
}

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,11 @@ edges
1717
| ThreadResourceAbuse.java:209:30:209:87 | new UploadListener(...) : UploadListener [slowUploads] : Number | UploadListener.java:28:14:28:19 | parameter this : UploadListener [slowUploads] : Number |
1818
| ThreadResourceAbuse.java:209:49:209:59 | uploadDelay : Number | ThreadResourceAbuse.java:209:30:209:87 | new UploadListener(...) : UploadListener [slowUploads] : Number |
1919
| ThreadResourceAbuse.java:209:49:209:59 | uploadDelay : Number | UploadListener.java:15:24:15:44 | sleepMilliseconds : Number |
20+
| ThreadResourceAbuse.java:215:19:215:50 | getHeader(...) : String | ThreadResourceAbuse.java:219:17:219:26 | retryAfter : Number |
21+
| ThreadResourceAbuse.java:219:17:219:26 | retryAfter : Number | ThreadResourceAbuse.java:219:17:219:33 | ... * ... |
22+
| ThreadResourceAbuse.java:227:19:227:50 | getHeader(...) : String | ThreadResourceAbuse.java:230:3:230:12 | retryAfter : Number |
23+
| ThreadResourceAbuse.java:230:3:230:12 | retryAfter : Number | ThreadResourceAbuse.java:230:3:230:20 | ...*=... : Number |
24+
| ThreadResourceAbuse.java:230:3:230:20 | ...*=... : Number | ThreadResourceAbuse.java:233:17:233:26 | retryAfter |
2025
| UploadListener.java:15:24:15:44 | sleepMilliseconds : Number | UploadListener.java:16:17:16:33 | sleepMilliseconds : Number |
2126
| UploadListener.java:16:17:16:33 | sleepMilliseconds : Number | UploadListener.java:16:3:16:13 | this <.field> [post update] : UploadListener [slowUploads] : Number |
2227
| UploadListener.java:28:14:28:19 | parameter this : UploadListener [slowUploads] : Number | UploadListener.java:29:3:29:11 | this <.field> : UploadListener [slowUploads] : Number |
@@ -46,6 +51,13 @@ nodes
4651
| ThreadResourceAbuse.java:206:28:206:56 | getParameter(...) : String | semmle.label | getParameter(...) : String |
4752
| ThreadResourceAbuse.java:209:30:209:87 | new UploadListener(...) : UploadListener [slowUploads] : Number | semmle.label | new UploadListener(...) : UploadListener [slowUploads] : Number |
4853
| ThreadResourceAbuse.java:209:49:209:59 | uploadDelay : Number | semmle.label | uploadDelay : Number |
54+
| ThreadResourceAbuse.java:215:19:215:50 | getHeader(...) : String | semmle.label | getHeader(...) : String |
55+
| ThreadResourceAbuse.java:219:17:219:26 | retryAfter : Number | semmle.label | retryAfter : Number |
56+
| ThreadResourceAbuse.java:219:17:219:33 | ... * ... | semmle.label | ... * ... |
57+
| ThreadResourceAbuse.java:227:19:227:50 | getHeader(...) : String | semmle.label | getHeader(...) : String |
58+
| ThreadResourceAbuse.java:230:3:230:12 | retryAfter : Number | semmle.label | retryAfter : Number |
59+
| ThreadResourceAbuse.java:230:3:230:20 | ...*=... : Number | semmle.label | ...*=... : Number |
60+
| ThreadResourceAbuse.java:233:17:233:26 | retryAfter | semmle.label | retryAfter |
4961
| UploadListener.java:15:24:15:44 | sleepMilliseconds : Number | semmle.label | sleepMilliseconds : Number |
5062
| UploadListener.java:16:3:16:13 | this <.field> [post update] : UploadListener [slowUploads] : Number | semmle.label | this <.field> [post update] : UploadListener [slowUploads] : Number |
5163
| UploadListener.java:16:17:16:33 | sleepMilliseconds : Number | semmle.label | sleepMilliseconds : Number |
@@ -65,4 +77,6 @@ subpaths
6577
| 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 |
6678
| 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 |
6779
| ThreadResourceAbuse.java:176:17:176:26 | retryAfter | ThreadResourceAbuse.java:172:19:172:50 | getHeader(...) : String | ThreadResourceAbuse.java:176:17:176:26 | retryAfter | Vulnerability of uncontrolled resource consumption due to $@. | ThreadResourceAbuse.java:172:19:172:50 | getHeader(...) | user-provided value |
80+
| ThreadResourceAbuse.java:219:17:219:33 | ... * ... | ThreadResourceAbuse.java:215:19:215:50 | getHeader(...) : String | ThreadResourceAbuse.java:219:17:219:33 | ... * ... | Vulnerability of uncontrolled resource consumption due to $@. | ThreadResourceAbuse.java:215:19:215:50 | getHeader(...) | user-provided value |
81+
| ThreadResourceAbuse.java:233:17:233:26 | retryAfter | ThreadResourceAbuse.java:227:19:227:50 | getHeader(...) : String | ThreadResourceAbuse.java:233:17:233:26 | retryAfter | Vulnerability of uncontrolled resource consumption due to $@. | ThreadResourceAbuse.java:227:19:227:50 | getHeader(...) | user-provided value |
6882
| UploadListener.java:35:18:35:28 | slowUploads | ThreadResourceAbuse.java:206:28:206:56 | getParameter(...) : String | UploadListener.java:35:18:35:28 | slowUploads | Vulnerability of uncontrolled resource consumption due to $@. | ThreadResourceAbuse.java:206:28:206:56 | getParameter(...) | user-provided value |

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,4 +209,30 @@ protected void doHead4(HttpServletRequest request, HttpServletResponse response)
209209
UploadListener listener = new UploadListener(uploadDelay, getContentLength(request));
210210
} catch (Exception e) { }
211211
}
212+
213+
protected void doHead5(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
214+
// BAD: Get thread pause time from request header with binary multiplication expression and without validation
215+
String header = request.getHeader("Retry-After");
216+
int retryAfter = Integer.parseInt(header);
217+
218+
try {
219+
Thread.sleep(retryAfter * 1000);
220+
} catch (InterruptedException ignore) {
221+
// ignore
222+
}
223+
}
224+
225+
protected void doHead6(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
226+
// BAD: Get thread pause time from request header with multiplication assignment operator and without validation
227+
String header = request.getHeader("Retry-After");
228+
int retryAfter = Integer.parseInt(header);
229+
230+
retryAfter *= 1000;
231+
232+
try {
233+
Thread.sleep(retryAfter);
234+
} catch (InterruptedException ignore) {
235+
// ignore
236+
}
237+
}
212238
}

0 commit comments

Comments
 (0)