Move duplicate code to the shared library and update qldoc

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"

java/ql/src/experimental/Security/CWE/CWE-400/ThreadPauseSink.qll

@@ -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") }
-}

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

@@ -5,22 +5,22 @@
 
 
 <overview>
-<p><code>Thread.sleep</code> 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.</p>
+<p>The <code>Thread.sleep</code> 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.</p>
 </overview>
 
 <recommendation>
 <p>To guard against this attack, consider specifying an upper range of allowed sleep time or adopting
 the producer/consumer design pattern with <code>Object.wait</code> method to avoid performance 
-problems or even resource exhaustion.</p>
+problems or even resource exhaustion. For more information, refer to the concurrency tutorial of Oracle
+listed below or <code>java/ql/src/Likely Bugs/Concurrency</code> queries of CodeQL.</p>
 </recommendation>
 
 <example>
-<p>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.</p>
+<p>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.</p>
 <sample src="ThreadResourceAbuse.java" />
 </example>
 
@@ -40,5 +40,9 @@ The blog of a gypsy engineer:
 <a href="https://blog.gypsyengineer.com/en/security/cve-2019-17555-dos-via-retry-after-header-in-apache-olingo.html">
 CVE-2019-17555: DoS via Retry-After header in Apache Olingo</a>.
 </li>
+<li>
+Oracle:
+<a href="https://docs.oracle.com/javase/tutorial/essential/concurrency/guardmeth.html">The Java Concurrency Tutorials</a>
+</li>
 </references>
 </qhelp>

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

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()
+    )
+  }
+}