Correct the run method and add Math.min check

luchua-bc · luchua-bc · commit 5264936fc3e2 · 2021-09-24T21:00:53.000Z
diff --git a/java/ql/src/experimental/Security/CWE/CWE-400/ThreadPauseSink.qll b/java/ql/src/experimental/Security/CWE/CWE-400/ThreadPauseSink.qll
@@ -4,6 +4,17 @@ 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;taint",
+        "java.lang;Math;false;max;;;Argument[0..1];ReturnValue;taint"
+      ]
+  }
+}
+
 /** Thread pause data model in the new CSV format. */
 private class PauseThreadDataModel extends SinkModelCsv {
   override predicate row(string row) {
diff --git a/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.ql b/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.ql
@@ -63,19 +63,29 @@ class ThreadResourceAbuse extends TaintTracking::Configuration {
 
   override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
     exists(
-      Method rm, ClassInstanceExpr ce, Argument arg, FieldAccess fa // thread.start() invokes the run() method of thread implementation
+      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.getAnArgument() = arg and
-      fa = rm.getAnAccessedField().getAnAccess() and
-      arg.getType() = fa.getField().getType() 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
     )
   }
 
+  override predicate isSanitizer(DataFlow::Node node) {
+    exists(
+      MethodAccess ma // Math.min(sleepTime, MAX_INTERVAL)
+    |
+      ma.getMethod().hasQualifiedName("java.lang", "Math", "min") and
+      node.asExpr() = ma.getAnArgument()
+    )
+  }
+
   override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
     guard instanceof LessThanSanitizer // if (sleepTime > 0 && sleepTime < 5000) { ... }
   }
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
@@ -150,7 +150,7 @@ protected void doHead(HttpServletRequest request, HttpServletResponse response)
 		}
 	}
 
-	int parseReplyAfter(String value) {
+	int parseRetryAfter(String value) {
 		if (value == null || value.isEmpty()) {
 			return DEFAULT_RETRY_AFTER;
 		}
@@ -183,7 +183,7 @@ protected void doHead2(HttpServletRequest request, HttpServletResponse response)
 	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);
+		int retryAfter = parseRetryAfter(header);
 
 		try {
 			// GOOD: wait for retry-after with input validation

Original file line number	Diff line number	Diff line change
`@@ -150,7 +150,7 @@ protected void doHead(HttpServletRequest request, HttpServletResponse response)`
`150`	`150`	`}`
`151`	`151`	`}`
`152`	`152`
`153`		`- int parseReplyAfter(String value) {`
	`153`	`+ int parseRetryAfter(String value) {`
`154`	`154`	`if (value == null \|\| value.isEmpty()) {`
`155`	`155`	`return DEFAULT_RETRY_AFTER;`
`156`	`156`	`}`
`@@ -183,7 +183,7 @@ protected void doHead2(HttpServletRequest request, HttpServletResponse response)`
`183`	`183`	`protected void doHead3(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {`
`184`	`184`	`// Get thread pause time from request header`
`185`	`185`	`String header = request.getHeader("Retry-After");`
`186`		`- int retryAfter = parseReplyAfter(header);`
	`186`	`+ int retryAfter = parseRetryAfter(header);`
`187`	`187`
`188`	`188`	`try {`
`189`	`189`	`// GOOD: wait for retry-after with input validation`