hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-25 08:45:14 +02:00
Code Issues Packages Projects Releases Wiki Activity

Java: Flow taint through ArithExpr for ThreadResourceAbuse

Browse Source 
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.
This commit is contained in:
Eric Bickle
2023-10-06 14:24:37 -07:00
parent eb3f1967a5
commit 000c1f7ec8
4 changed files with 74 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.ql
View File

@@ -22,7 +22,8 @@ module ThreadResourceAbuseConfig implements DataFlow::ConfigSig {
predicate isSink(DataFlow::Node sink) { sink instanceof PauseThreadSink }
predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
any(AdditionalValueStep r).step(pred, succ)
any(AdditionalValueStep r).step(pred, succ) or
any(ThreadResourceAbuseAdditionalTaintStep c).step(pred, succ)
}
predicate isBarrier(DataFlow::Node node) {

32
java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.qll
View File

@@ -3,6 +3,7 @@
import java
import semmle.code.java.dataflow.DataFlow
private import semmle.code.java.dataflow.ExternalFlow
import semmle.code.java.arithmetic.Overflow
import semmle.code.java.dataflow.FlowSteps
import semmle.code.java.controlflow.Guards
@@ -61,3 +62,34 @@ private class ApacheFileUploadProgressUpdateStep extends AdditionalValueStep {
)
}
}
/**
* A unit class for adding additional taint steps.
*
* Extend this class to add additional taint steps that should apply to the `ThreadResourceAbuseConfig`.
*/
class ThreadResourceAbuseAdditionalTaintStep extends Unit {
/**
* Holds if the step from `node1` to `node2` should be considered a taint
* step for the `ThreadResourceAbuseConfig` configuration.
*/
abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
}
/** A set of additional taint steps to consider when taint tracking thread resource abuse related data flows. */
private class DefaultThreadResourceAbuseAdditionalTaintStep extends ThreadResourceAbuseAdditionalTaintStep
{
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
threadResourceAbuseArithmeticTaintStep(node1, node2)
}
}
/**
* Holds if the step `node1` -> `node2` is an additional taint-step that performs an addition, multiplication,
* subtraction, or division.
*/
private predicate threadResourceAbuseArithmeticTaintStep(
DataFlow::Node fromNode, DataFlow::Node toNode
) {
toNode.asExpr().(ArithExpr).getAnOperand() = fromNode.asExpr()
}

14
java/ql/test/experimental/query-tests/security/CWE-400/ThreadResourceAbuse.expected
View File

@@ -17,6 +17,11 @@ edges
| ThreadResourceAbuse.java:209:30:209:87 | new UploadListener(...) : UploadListener [slowUploads] : Number | UploadListener.java:28:14:28:19 | parameter this : UploadListener [slowUploads] : Number |
| ThreadResourceAbuse.java:209:49:209:59 | uploadDelay : Number | ThreadResourceAbuse.java:209:30:209:87 | new UploadListener(...) : UploadListener [slowUploads] : Number |
| ThreadResourceAbuse.java:209:49:209:59 | uploadDelay : Number | UploadListener.java:15:24:15:44 | sleepMilliseconds : Number |
| ThreadResourceAbuse.java:215:19:215:50 | getHeader(...) : String | ThreadResourceAbuse.java:219:17:219:26 | retryAfter : Number |
| ThreadResourceAbuse.java:219:17:219:26 | retryAfter : Number | ThreadResourceAbuse.java:219:17:219:33 | ... * ... |
| ThreadResourceAbuse.java:227:19:227:50 | getHeader(...) : String | ThreadResourceAbuse.java:230:3:230:12 | retryAfter : Number |
| ThreadResourceAbuse.java:230:3:230:12 | retryAfter : Number | ThreadResourceAbuse.java:230:3:230:20 | ...*=... : Number |
| ThreadResourceAbuse.java:230:3:230:20 | ...*=... : Number | ThreadResourceAbuse.java:233:17:233:26 | retryAfter |
| UploadListener.java:15:24:15:44 | sleepMilliseconds : Number | UploadListener.java:16:17:16:33 | sleepMilliseconds : Number |
| UploadListener.java:16:17:16:33 | sleepMilliseconds : Number | UploadListener.java:16:3:16:13 | this <.field> [post update] : UploadListener [slowUploads] : Number |
| 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
| ThreadResourceAbuse.java:206:28:206:56 | getParameter(...) : String | semmle.label | getParameter(...) : String |
| ThreadResourceAbuse.java:209:30:209:87 | new UploadListener(...) : UploadListener [slowUploads] : Number | semmle.label | new UploadListener(...) : UploadListener [slowUploads] : Number |
| ThreadResourceAbuse.java:209:49:209:59 | uploadDelay : Number | semmle.label | uploadDelay : Number |
| ThreadResourceAbuse.java:215:19:215:50 | getHeader(...) : String | semmle.label | getHeader(...) : String |
| ThreadResourceAbuse.java:219:17:219:26 | retryAfter : Number | semmle.label | retryAfter : Number |
| ThreadResourceAbuse.java:219:17:219:33 | ... * ... | semmle.label | ... * ... |
| ThreadResourceAbuse.java:227:19:227:50 | getHeader(...) : String | semmle.label | getHeader(...) : String |
| ThreadResourceAbuse.java:230:3:230:12 | retryAfter : Number | semmle.label | retryAfter : Number |
| ThreadResourceAbuse.java:230:3:230:20 | ...*=... : Number | semmle.label | ...*=... : Number |
| ThreadResourceAbuse.java:233:17:233:26 | retryAfter | semmle.label | retryAfter |
| UploadListener.java:15:24:15:44 | sleepMilliseconds : Number | semmle.label | sleepMilliseconds : Number |
| UploadListener.java:16:3:16:13 | this <.field> [post update] : UploadListener [slowUploads] : Number | semmle.label | this <.field> [post update] : UploadListener [slowUploads] : Number |
| UploadListener.java:16:17:16:33 | sleepMilliseconds : Number | semmle.label | sleepMilliseconds : Number |
@@ -65,4 +77,6 @@ subpaths
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |

26
java/ql/test/experimental/query-tests/security/CWE-400/ThreadResourceAbuse.java
View File

@@ -209,4 +209,30 @@ public class ThreadResourceAbuse extends HttpServlet {
UploadListener listener = new UploadListener(uploadDelay, getContentLength(request));
} catch (Exception e) { }
}
protected void doHead5(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
// BAD: Get thread pause time from request header with binary multiplication expression and without validation
String header = request.getHeader("Retry-After");
int retryAfter = Integer.parseInt(header);
try {
Thread.sleep(retryAfter * 1000);
} catch (InterruptedException ignore) {
// ignore
}
}
protected void doHead6(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
// BAD: Get thread pause time from request header with multiplication assignment operator and without validation
String header = request.getHeader("Retry-After");
int retryAfter = Integer.parseInt(header);
retryAfter *= 1000;
try {
Thread.sleep(retryAfter);
} catch (InterruptedException ignore) {
// ignore
}
}
}