Skip to content

Commit 5e92178

Browse files
atorralbaatorralba
authored
Merge pull request #14399 from ebickle/fix/thread-resource-arithmetic
Java: Flow taint through arithmetic expressions for java/thread-resource-abuse experimental query
2 parents b1ad61e + ee2d8f8 commit 5e92178

File tree

4 files changed

+73
-1
lines changed

4 files changed

+73
-1
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ 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(ThreadResourceAbuseAdditionalTaintStep c).step(pred, succ)
2626
}
2727

2828
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)