diff --git a/java/ql/src/experimental/Security/CWE/CWE-400/LocalThreadResourceAbuse.ql b/java/ql/src/experimental/Security/CWE/CWE-400/LocalThreadResourceAbuse.ql index 9d441dab1d0..1163c9fbff1 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-400/LocalThreadResourceAbuse.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-400/LocalThreadResourceAbuse.ql @@ -1,7 +1,7 @@ /** * @name Uncontrolled thread resource consumption from local input source - * @description Use user input directly to control thread sleep time could lead to performance problems - * or even resource exhaustion. + * @description Using user input directly to control a thread's sleep time could lead to + * performance problems or even resource exhaustion. * @kind path-problem * @id java/thread-resource-abuse * @problem.severity recommendation @@ -10,7 +10,7 @@ */ import java -import ThreadPauseSink +import ThreadResourceAbuse import semmle.code.java.dataflow.FlowSources import DataFlow::PathGraph @@ -40,18 +40,6 @@ class InitParameterInput extends LocalUserInput { InitParameterInput() { this.asExpr() instanceof GetInitParameterAccess } } -private class LessThanSanitizer extends DataFlow::BarrierGuard { - LessThanSanitizer() { this instanceof ComparisonExpr } - - override predicate checks(Expr e, boolean branch) { - e = this.(ComparisonExpr).getLesserOperand() and - branch = true - or - e = this.(ComparisonExpr).getGreaterOperand() and - branch = false - } -} - /** Taint configuration of uncontrolled thread resource consumption from local user input. */ class ThreadResourceAbuse extends TaintTracking::Configuration { ThreadResourceAbuse() { this = "ThreadResourceAbuse" } @@ -60,34 +48,8 @@ class ThreadResourceAbuse extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { sink instanceof PauseThreadSink } - override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { - exists( - Method rm, ClassInstanceExpr ce, Argument arg, Parameter p, FieldAccess fa, int i // thread.start() invokes the run() method of thread implementation - | - rm.hasName("run") and - ce.getConstructedType().getSourceDeclaration() = rm.getSourceDeclaration().getDeclaringType() and - ce.getConstructedType().getASupertype*().hasQualifiedName("java.lang", "Runnable") and - ce.getArgument(i) = arg and - ce.getConstructor().getParameter(i) = p and - fa.getEnclosingCallable() = rm and - DataFlow::localExprFlow(p.getAnAccess(), fa.getField().getAnAssignedValue()) and - node1.asExpr() = arg and - node2.asExpr() = fa - ) - or - exists(Method um, VarAccess va, FieldAccess fa, Constructor ce, AssignExpr ar | - um.getDeclaringType() - .getASupertype*() - .hasQualifiedName("org.apache.commons.fileupload", "ProgressListener") and - um.hasName("update") and - fa.getEnclosingCallable() = um and - ce.getDeclaringType() = um.getDeclaringType() and - va = ce.getAParameter().getAnAccess() and - node1.asExpr() = va and - node2.asExpr() = fa and - ar.getSource() = va and - ar.getDest() = fa.getField().getAnAccess() - ) + override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + any(ThreadResourceAbuseAdditionalTaintStep r).propagatesTaint(pred, succ) } override predicate isSanitizer(DataFlow::Node node) { @@ -106,6 +68,5 @@ class ThreadResourceAbuse extends TaintTracking::Configuration { from DataFlow::PathNode source, DataFlow::PathNode sink, ThreadResourceAbuse conf where conf.hasFlowPath(source, sink) -select sink.getNode(), source, sink, - "Vulnerability of uncontrolled resource consumption due to $@.", source.getNode(), - "local user-provided value" +select sink.getNode(), source, sink, "Possible uncontrolled resource consumption due to $@.", + source.getNode(), "local user-provided value" diff --git a/java/ql/src/experimental/Security/CWE/CWE-400/ThreadPauseSink.qll b/java/ql/src/experimental/Security/CWE/CWE-400/ThreadPauseSink.qll deleted file mode 100644 index df4903e7a9a..00000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-400/ThreadPauseSink.qll +++ /dev/null @@ -1,32 +0,0 @@ -/** Provides sink models related to pausing thread operations. */ - -import java -import semmle.code.java.dataflow.DataFlow -import semmle.code.java.dataflow.ExternalFlow - -/** `java.lang.Math` data model for value comparison in the new CSV format. */ -private class MathCompDataModel extends SummaryModelCsv { - override predicate row(string row) { - row = - [ - "java.lang;Math;false;min;;;Argument[0..1];ReturnValue;value", - "java.lang;Math;false;max;;;Argument[0..1];ReturnValue;value" - ] - } -} - -/** Thread pause data model in the new CSV format. */ -private class PauseThreadDataModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - "java.lang;Thread;true;sleep;;;Argument[0];thread-pause", - "java.util.concurrent;TimeUnit;true;sleep;;;Argument[0];thread-pause" - ] - } -} - -/** A sink representing methods pausing a thread. */ -class PauseThreadSink extends DataFlow::Node { - PauseThreadSink() { sinkNode(this, "thread-pause") } -} 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 8dcca66e344..4a80abd546b 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.qhelp @@ -5,22 +5,22 @@ -

Thread.sleep method is used to pause the execution of current thread for specified -time. When it is used to keep several relevant tasks in synchronization and the sleep time is -user-controlled data, especially in the web application context, it can be abused to cause all -of a server's threads to sleep, leading to denial of service.

+

The Thread.sleep method is used to pause the execution of current thread for +specified time. When the sleep time is user-controlled, especially in the web application context, +it can be abused to cause all of a server's threads to sleep, leading to denial of service.

To guard against this attack, consider specifying an upper range of allowed sleep time or adopting the producer/consumer design pattern with Object.wait method to avoid performance -problems or even resource exhaustion.

+problems or even resource exhaustion. For more information, refer to the concurrency tutorial of Oracle +listed below or java/ql/src/Likely Bugs/Concurrency queries of CodeQL.

 -

The following example shows the bad situation and the good situation respectively. In bad situation, -a thread is spawned with the sleep time directly from user input. In good situation, an upper range -check on maximum allowed sleep time is enforced.

+

The following example shows a bad situation and a good situation respectively. In the bad situation, +a thread is spawned with a sleep time coming directly from user input. In the good situation, an upper +range check on the maximum sleep time allowed is enforced.

 @@ -40,5 +40,9 @@ The blog of a gypsy engineer: CVE-2019-17555: DoS via Retry-After header in Apache Olingo. +
  • +Oracle: +The Java Concurrency Tutorials +
    • 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 967b7816584..9c85e3c0274 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.ql @@ -1,7 +1,7 @@ /** * @name Uncontrolled thread resource consumption - * @description Use user input directly to control thread sleep time could lead to performance problems - * or even resource exhaustion. + * @description Using user input directly to control a thread's sleep time could lead to + * performance problems or even resource exhaustion. * @kind path-problem * @id java/thread-resource-abuse * @problem.severity warning @@ -10,22 +10,10 @@ */ import java -import ThreadPauseSink +import ThreadResourceAbuse import semmle.code.java.dataflow.FlowSources import DataFlow::PathGraph -private class LessThanSanitizer extends DataFlow::BarrierGuard { - LessThanSanitizer() { this instanceof ComparisonExpr } - - override predicate checks(Expr e, boolean branch) { - e = this.(ComparisonExpr).getLesserOperand() and - branch = true - or - e = this.(ComparisonExpr).getGreaterOperand() and - branch = false - } -} - /** Taint configuration of uncontrolled thread resource consumption. */ class ThreadResourceAbuse extends TaintTracking::Configuration { ThreadResourceAbuse() { this = "ThreadResourceAbuse" } @@ -34,34 +22,8 @@ class ThreadResourceAbuse extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { sink instanceof PauseThreadSink } - override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { - exists( - Method rm, ClassInstanceExpr ce, Argument arg, Parameter p, FieldAccess fa, int i // thread.start() invokes the run() method of thread implementation - | - rm.hasName("run") and - ce.getConstructedType().getSourceDeclaration() = rm.getSourceDeclaration().getDeclaringType() and - ce.getConstructedType().getASupertype*().hasQualifiedName("java.lang", "Runnable") and - ce.getArgument(i) = arg and - ce.getConstructor().getParameter(i) = p and - fa.getEnclosingCallable() = rm and - DataFlow::localExprFlow(p.getAnAccess(), fa.getField().getAnAssignedValue()) and - node1.asExpr() = arg and - node2.asExpr() = fa - ) - or - exists(Method um, VarAccess va, FieldAccess fa, Constructor ce, AssignExpr ar | - um.getDeclaringType() - .getASupertype*() - .hasQualifiedName("org.apache.commons.fileupload", "ProgressListener") and - um.hasName("update") and - fa.getEnclosingCallable() = um and - ce.getDeclaringType() = um.getDeclaringType() and - va = ce.getAParameter().getAnAccess() and - node1.asExpr() = va and - node2.asExpr() = fa and - ar.getSource() = va and - ar.getDest() = fa.getField().getAnAccess() - ) + override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + any(ThreadResourceAbuseAdditionalTaintStep r).propagatesTaint(pred, succ) } override predicate isSanitizer(DataFlow::Node node) { diff --git a/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.qll b/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.qll new file mode 100644 index 00000000000..8f955a0dd50 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.qll @@ -0,0 +1,90 @@ +/** Provides sink models and classes related to pausing thread operations. */ + +import java +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.dataflow.ExternalFlow + +/** `java.lang.Math` data model for value comparison in the new CSV format. */ +private class MathCompDataModel extends SummaryModelCsv { + override predicate row(string row) { + row = + [ + "java.lang;Math;false;min;;;Argument[0..1];ReturnValue;value", + "java.lang;Math;false;max;;;Argument[0..1];ReturnValue;value" + ] + } +} + +/** Thread pause data model in the new CSV format. */ +private class PauseThreadDataModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "java.lang;Thread;true;sleep;;;Argument[0];thread-pause", + "java.util.concurrent;TimeUnit;true;sleep;;;Argument[0];thread-pause" + ] + } +} + +/** A sink representing methods pausing a thread. */ +class PauseThreadSink extends DataFlow::Node { + PauseThreadSink() { sinkNode(this, "thread-pause") } +} + +/** A sanitizer for lessThan check. */ +class LessThanSanitizer extends DataFlow::BarrierGuard instanceof ComparisonExpr { + override predicate checks(Expr e, boolean branch) { + e = this.(ComparisonExpr).getLesserOperand() and + branch = true + or + e = this.(ComparisonExpr).getGreaterOperand() and + branch = false + } +} + +/** + * A unit class for adding additional taint steps that are specific to thread resource abuse. + */ +class ThreadResourceAbuseAdditionalTaintStep extends Unit { + /** + * Holds if the step from `pred` to `succ` should be considered a taint + * step for thread resource abuse. + */ + abstract predicate propagatesTaint(DataFlow::Node pred, DataFlow::Node succ); +} + +private class RunnableAdditionalTaintStep extends ThreadResourceAbuseAdditionalTaintStep { + override predicate propagatesTaint(DataFlow::Node pred, DataFlow::Node succ) { + exists( + Method rm, ClassInstanceExpr ce, Argument arg, Parameter p, FieldAccess fa, int i // thread.start() invokes the run() method of thread implementation + | + rm.hasName("run") and + ce.getConstructedType().getSourceDeclaration() = rm.getSourceDeclaration().getDeclaringType() and + ce.getConstructedType().getASupertype*().hasQualifiedName("java.lang", "Runnable") and + ce.getArgument(i) = arg and + ce.getConstructor().getParameter(i) = p and + fa.getEnclosingCallable() = rm and + DataFlow::localExprFlow(p.getAnAccess(), fa.getField().getAnAssignedValue()) and + pred.asExpr() = arg and + succ.asExpr() = fa + ) + } +} + +private class ApacheFileUploadAdditionalTaintStep extends ThreadResourceAbuseAdditionalTaintStep { + override predicate propagatesTaint(DataFlow::Node pred, DataFlow::Node succ) { + exists(Method um, VarAccess va, FieldAccess fa, Constructor ce, AssignExpr ar | + um.getDeclaringType() + .getASupertype*() + .hasQualifiedName("org.apache.commons.fileupload", "ProgressListener") and + um.hasName("update") and + fa.getEnclosingCallable() = um and + ce.getDeclaringType() = um.getDeclaringType() and + va = ce.getAParameter().getAnAccess() and + pred.asExpr() = va and + succ.asExpr() = fa and + ar.getSource() = va and + ar.getDest() = fa.getField().getAnAccess() + ) + } +}