Add support for java.util.concurrent.ThreadLocalRandom

smowton · smowton · commit 4a4f4b01a1c2 · 2021-03-08T10:59:53.000Z
diff --git a/java/ql/src/semmle/code/java/security/Random.qll b/java/ql/src/semmle/code/java/security/Random.qll
@@ -88,20 +88,39 @@ class StdlibRandomSource extends RandomDataSource {
     m.getDeclaringType() instanceof StdlibRandom
   }
 
+  // Note for the following bounds functions: `java.util.Random` only defines no-arg versions
+  // of `nextInt` and `nextLong` plus `nextInt(int x)`, bounded to the range [0, x)
+  // However `ThreadLocalRandom` provides one- and two-arg versions of `nextInt` and `nextLong`
+  // which allow both lower and upper bounds for both types.
   override int getLowerBound() {
-    // If this call is to `nextInt(int)`, the lower bound is zero.
-    m.hasName("nextInt") and
+    // If this call is to `nextInt(int)` or `nextLong(long), the lower bound is zero.
+    m.hasName(["nextInt", "nextLong"]) and
     m.getNumberOfParameters() = 1 and
     result = 0
+    or
+    result = super.getLowerBound() // Include a lower bound provided via `getLowerBoundExpr`
   }
 
-  override Expr getUpperBoundExpr() {
-    // If this call is to `nextInt(int)`, the upper bound is the first argument.
-    m.hasName("nextInt") and
-    m.getNumberOfParameters() = 1 and
+  override Expr getLowerBoundExpr() {
+    // If this call is to `nextInt(int, int)` or `nextLong(long, long)`, the lower bound is the first argument.
+    m.hasName(["nextInt", "nextLong"]) and
+    m.getNumberOfParameters() = 2 and
     result = this.getArgument(0)
   }
 
+  override Expr getUpperBoundExpr() {
+    // If this call is to `nextInt(int)` or `nextLong(long)`, the upper bound is the first argument.
+    // If it calls `nextInt(int, int)` or `nextLong(long, long)`, the upper bound is the first argument.
+    m.hasName(["nextInt", "nextLong"]) and
+    (
+      m.getNumberOfParameters() = 1 and
+      result = this.getArgument(0)
+      or
+      m.getNumberOfParameters() = 2 and
+      result = this.getArgument(1)
+    )
+  }
+
   override predicate resultMayBeBounded() {
     // `next` may be restricted by its `bits` argument,
     // `nextBoolean` can't possibly be usefully bounded,
@@ -110,7 +129,7 @@ class StdlibRandomSource extends RandomDataSource {
     m.hasName(["next", "nextBoolean", "nextDouble", "nextFloat", "nextGaussian"])
     or
     m.hasName(["nextInt", "nextLong"]) and
-    m.getNumberOfParameters() = 1
+    m.getNumberOfParameters() = [1, 2]
   }
 
   override Expr getOutput() {
diff --git a/java/ql/test/query-tests/BadAbsOfRandom/BadAbsOfRandom.expected b/java/ql/test/query-tests/BadAbsOfRandom/BadAbsOfRandom.expected
@@ -1,4 +1,6 @@
-| Test.java:9:5:9:25 | abs(...) | Incorrect computation of abs of signed integral random value. |
-| Test.java:10:5:10:26 | abs(...) | Incorrect computation of abs of signed integral random value. |
-| Test.java:13:5:13:35 | abs(...) | Incorrect computation of abs of signed integral random value. |
-| Test.java:14:5:14:36 | abs(...) | Incorrect computation of abs of signed integral random value. |
+| Test.java:10:5:10:25 | abs(...) | Incorrect computation of abs of signed integral random value. |
+| Test.java:11:5:11:26 | abs(...) | Incorrect computation of abs of signed integral random value. |
+| Test.java:14:5:14:35 | abs(...) | Incorrect computation of abs of signed integral random value. |
+| Test.java:15:5:15:36 | abs(...) | Incorrect computation of abs of signed integral random value. |
+| Test.java:20:5:20:27 | abs(...) | Incorrect computation of abs of signed integral random value. |
+| Test.java:21:5:21:28 | abs(...) | Incorrect computation of abs of signed integral random value. |
diff --git a/java/ql/test/query-tests/BadAbsOfRandom/Test.java b/java/ql/test/query-tests/BadAbsOfRandom/Test.java
@@ -1,4 +1,5 @@
 import java.util.Random;
+import java.util.concurrent.ThreadLocalRandom;
 import org.apache.commons.lang3.RandomUtils;
 
 public class Test {
@@ -15,6 +16,14 @@ public static void test() {
     Math.abs(RandomUtils.nextInt(1, 10)); // GOOD: random value already has a restricted range
     Math.abs(RandomUtils.nextLong(1, 10)); // GOOD: random value already has a restricted range
 
+    ThreadLocalRandom tlr = ThreadLocalRandom.current();
+    Math.abs(tlr.nextInt());
+    Math.abs(tlr.nextLong());
+    Math.abs(tlr.nextInt(10)); // GOOD: random value already has a restricted range
+    Math.abs(tlr.nextLong(10)); // GOOD: random value already has a restricted range
+    Math.abs(tlr.nextInt(1, 10)); // GOOD: random value already has a restricted range
+    Math.abs(tlr.nextLong(1, 10)); // GOOD: random value already has a restricted range
+
   }
 
 }
