diff --git a/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.qhelp b/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.qhelp index 432e29f99d0..8dcca66e344 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.qhelp @@ -26,7 +26,7 @@ check on maximum allowed sleep time is enforced.

  • -snyk: +Snyk: Denial of Service (DoS) in com.googlecode.gwtupload:gwtupload.
    • diff --git a/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.ql b/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.ql index 7dfc6394363..b1ba2d67248 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.ql @@ -40,11 +40,14 @@ class InitParameterInput extends LocalUserInput { } private class LessThanSanitizer extends DataFlow::BarrierGuard { - LessThanSanitizer() { this instanceof LTExpr } + LessThanSanitizer() { this instanceof ComparisonExpr } override predicate checks(Expr e, boolean branch) { - e = this.(LTExpr).getLeftOperand() and + e = this.(ComparisonExpr).getLesserOperand() and branch = true + or + e = this.(ComparisonExpr).getGreaterOperand() and + branch = false } } @@ -59,13 +62,11 @@ class ThreadResourceAbuse extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { sink instanceof PauseThreadSink } override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(ConditionalExpr ce | ce.getAChildExpr() = node1.asExpr() and ce = node2.asExpr()) // request.getParameter("nodelay") != null ? 0 : sleepTime - or exists( Method rm, ClassInstanceExpr ce, Argument arg, FieldAccess fa // thread.start() invokes the run() method of thread implementation | rm.hasName("run") and - ce.getConstructedType() = rm.getSourceDeclaration().getDeclaringType() and + ce.getConstructedType().getSourceDeclaration() = rm.getSourceDeclaration().getDeclaringType() and ce.getConstructedType().getASupertype*().hasQualifiedName("java.lang", "Runnable") and ce.getAnArgument() = arg and fa = rm.getAnAccessedField().getAnAccess() and diff --git a/java/ql/test/experimental/query-tests/security/CWE-400/ThreadResourceAbuse.expected b/java/ql/test/experimental/query-tests/security/CWE-400/ThreadResourceAbuse.expected index ea57e7950ca..c416af6cf70 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-400/ThreadResourceAbuse.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-400/ThreadResourceAbuse.expected @@ -1,18 +1,27 @@ edges -| ThreadResourceAbuse.java:15:25:15:57 | getParameter(...) : String | ThreadResourceAbuse.java:18:28:18:36 | delayTime : Number | -| ThreadResourceAbuse.java:18:28:18:36 | delayTime : Number | ThreadResourceAbuse.java:62:18:62:25 | waitTime | -| ThreadResourceAbuse.java:25:25:25:73 | getInitParameter(...) : String | ThreadResourceAbuse.java:28:28:28:36 | delayTime : Number | -| ThreadResourceAbuse.java:28:28:28:36 | delayTime : Number | ThreadResourceAbuse.java:62:18:62:25 | waitTime | -| ThreadResourceAbuse.java:97:27:97:43 | getValue(...) : String | ThreadResourceAbuse.java:100:34:100:42 | delayTime | +| ThreadResourceAbuse.java:18:25:18:57 | getParameter(...) : String | ThreadResourceAbuse.java:21:28:21:36 | delayTime : Number | +| ThreadResourceAbuse.java:21:28:21:36 | delayTime : Number | ThreadResourceAbuse.java:74:18:74:25 | waitTime | +| ThreadResourceAbuse.java:29:82:29:114 | getParameter(...) : String | ThreadResourceAbuse.java:30:28:30:36 | delayTime : Number | +| ThreadResourceAbuse.java:30:28:30:36 | delayTime : Number | ThreadResourceAbuse.java:74:18:74:25 | waitTime | +| ThreadResourceAbuse.java:37:25:37:73 | getInitParameter(...) : String | ThreadResourceAbuse.java:40:28:40:36 | delayTime : Number | +| ThreadResourceAbuse.java:40:28:40:36 | delayTime : Number | ThreadResourceAbuse.java:74:18:74:25 | waitTime | +| ThreadResourceAbuse.java:141:27:141:43 | getValue(...) : String | ThreadResourceAbuse.java:144:34:144:42 | delayTime | +| ThreadResourceAbuse.java:172:19:172:50 | getHeader(...) : String | ThreadResourceAbuse.java:177:17:177:26 | retryAfter | nodes -| ThreadResourceAbuse.java:15:25:15:57 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| ThreadResourceAbuse.java:18:28:18:36 | delayTime : Number | semmle.label | delayTime : Number | -| ThreadResourceAbuse.java:25:25:25:73 | getInitParameter(...) : String | semmle.label | getInitParameter(...) : String | -| ThreadResourceAbuse.java:28:28:28:36 | delayTime : Number | semmle.label | delayTime : Number | -| ThreadResourceAbuse.java:62:18:62:25 | waitTime | semmle.label | waitTime | -| ThreadResourceAbuse.java:97:27:97:43 | getValue(...) : String | semmle.label | getValue(...) : String | -| ThreadResourceAbuse.java:100:34:100:42 | delayTime | semmle.label | delayTime | +| ThreadResourceAbuse.java:18:25:18:57 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| ThreadResourceAbuse.java:21:28:21:36 | delayTime : Number | semmle.label | delayTime : Number | +| ThreadResourceAbuse.java:29:82:29:114 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| ThreadResourceAbuse.java:30:28:30:36 | delayTime : Number | semmle.label | delayTime : Number | +| ThreadResourceAbuse.java:37:25:37:73 | getInitParameter(...) : String | semmle.label | getInitParameter(...) : String | +| ThreadResourceAbuse.java:40:28:40:36 | delayTime : Number | semmle.label | delayTime : Number | +| ThreadResourceAbuse.java:74:18:74:25 | waitTime | semmle.label | waitTime | +| ThreadResourceAbuse.java:141:27:141:43 | getValue(...) : String | semmle.label | getValue(...) : String | +| ThreadResourceAbuse.java:144:34:144:42 | delayTime | semmle.label | delayTime | +| ThreadResourceAbuse.java:172:19:172:50 | getHeader(...) : String | semmle.label | getHeader(...) : String | +| ThreadResourceAbuse.java:177:17:177:26 | retryAfter | semmle.label | retryAfter | #select -| ThreadResourceAbuse.java:62:18:62:25 | waitTime | ThreadResourceAbuse.java:15:25:15:57 | getParameter(...) : String | ThreadResourceAbuse.java:62:18:62:25 | waitTime | Vulnerability of uncontrolled resource consumption due to $@. | ThreadResourceAbuse.java:15:25:15:57 | getParameter(...) | user-provided value | -| ThreadResourceAbuse.java:62:18:62:25 | waitTime | ThreadResourceAbuse.java:25:25:25:73 | getInitParameter(...) : String | ThreadResourceAbuse.java:62:18:62:25 | waitTime | Vulnerability of uncontrolled resource consumption due to $@. | ThreadResourceAbuse.java:25:25:25:73 | getInitParameter(...) | user-provided value | -| ThreadResourceAbuse.java:100:34:100:42 | delayTime | ThreadResourceAbuse.java:97:27:97:43 | getValue(...) : String | ThreadResourceAbuse.java:100:34:100:42 | delayTime | Vulnerability of uncontrolled resource consumption due to $@. | ThreadResourceAbuse.java:97:27:97:43 | getValue(...) | user-provided value | +| 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 | +| 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:74:18:74:25 | waitTime | ThreadResourceAbuse.java:37:25:37:73 | getInitParameter(...) : String | ThreadResourceAbuse.java:74:18:74:25 | waitTime | Vulnerability of uncontrolled resource consumption due to $@. | ThreadResourceAbuse.java:37:25:37:73 | getInitParameter(...) | 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: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 | diff --git a/java/ql/test/experimental/query-tests/security/CWE-400/ThreadResourceAbuse.java b/java/ql/test/experimental/query-tests/security/CWE-400/ThreadResourceAbuse.java index e0c59cffe6d..4ac4f9dce94 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-400/ThreadResourceAbuse.java +++ b/java/ql/test/experimental/query-tests/security/CWE-400/ThreadResourceAbuse.java @@ -10,6 +10,9 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; public class ThreadResourceAbuse extends HttpServlet { + static final int DEFAULT_RETRY_AFTER = 5*1000; + static final int MAX_RETRY_AFTER = 10*1000; + protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { // Get thread pause time from request parameter String delayTimeStr = request.getParameter("DelayTime"); @@ -20,6 +23,15 @@ public class ThreadResourceAbuse extends HttpServlet { } } + protected void doGet2(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { + // Get thread pause time from request parameter + try { + int delayTime = request.getParameter("nodelay") != null ? 0 : Integer.valueOf(request.getParameter("DelayTime")); + new UncheckedSyncAction(delayTime).start(); + } catch (NumberFormatException e) { + } + } + protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { // Get thread pause time from init container parameter String delayTimeStr = getServletContext().getInitParameter("DelayTime"); @@ -78,11 +90,43 @@ public class ThreadResourceAbuse extends HttpServlet { public void run() { try { if (waitTime > 0 && waitTime < 5000) { - Thread.sleep(waitTime); - // Do other updates + Thread.sleep(waitTime); + // Do other updates + } + } catch (InterruptedException e) { } - } catch (InterruptedException e) { - } + } + } + + class CheckedSyncAction2 extends Thread { + int waitTime; + + public CheckedSyncAction2(int waitTime) { + this.waitTime = waitTime; + } + + // GOOD: enforce an upper limit on wait time + @Override + public void run() { + try { + if (waitTime >= 5000) { + // No action + } else { + Thread.sleep(waitTime); + } + // Do other updates + } catch (InterruptedException e) { + } + } + } + + protected void doPost2(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { + // Get thread pause time from init container parameter + String delayTimeStr = getServletContext().getInitParameter("DelayTime"); + try { + int delayTime = Integer.valueOf(delayTimeStr); + new CheckedSyncAction2(delayTime).start(); + } catch (NumberFormatException e) { } } @@ -105,4 +149,47 @@ public class ThreadResourceAbuse extends HttpServlet { } } } + + int parseReplyAfter(String value) { + if (value == null || value.isEmpty()) { + return DEFAULT_RETRY_AFTER; + } + + try { + int n = Integer.parseInt(value); + if (n < 0) { + return DEFAULT_RETRY_AFTER; + } + + return Math.min(n, MAX_RETRY_AFTER); + } catch (NumberFormatException e) { + return DEFAULT_RETRY_AFTER; + } + } + + protected void doHead2(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { + // Get thread pause time from request header + String header = request.getHeader("Retry-After"); + int retryAfter = Integer.parseInt(header); + + try { + // BAD: wait for retry-after without input validation + Thread.sleep(retryAfter); + } catch (InterruptedException ignore) { + // ignore + } + } + + protected void doHead3(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { + // Get thread pause time from request header + String header = request.getHeader("Retry-After"); + int retryAfter = parseReplyAfter(header); + + try { + // GOOD: wait for retry-after with input validation + Thread.sleep(retryAfter); + } catch (InterruptedException ignore) { + // ignore + } + } }