Merge pull request github#5337 from smowton/smowton/feature/commons-lang-random-sources

Java: Add support for Commons-Lang's RandomUtils

Author: aschackmull

java/change-notes/2021-03-05-commons-lang-randomutils.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Added models for the Apache Commons Lang `RandomUtils` class. This may lead to extra results from queries that check for proper use of random-number generators or those which check the range of possible random values that could be returned, including `java/improper-validation-of-array-index-code-specified` and `java/uncontrolled-arithmetic`.

java/ql/src/Likely Bugs/Arithmetic/BadAbsOfRandom.ql

@@ -11,15 +11,15 @@
  */
 
 import java
+import semmle.code.java.security.Random
 
-from MethodAccess ma, Method abs, Method nextIntOrLong, MethodAccess nma
+from MethodAccess ma, Method abs, Method nextIntOrLong, RandomDataSource nma
 where
   ma.getMethod() = abs and
   abs.hasName("abs") and
   abs.getDeclaringType().hasQualifiedName("java.lang", "Math") and
   ma.getAnArgument() = nma and
   nma.getMethod() = nextIntOrLong and
-  (nextIntOrLong.hasName("nextInt") or nextIntOrLong.hasName("nextLong")) and
-  nextIntOrLong.getDeclaringType().hasQualifiedName("java.util", "Random") and
-  nextIntOrLong.hasNoParameters()
+  nextIntOrLong.hasName(["nextInt", "nextLong"]) and
+  not nma.resultMayBeBounded()
 select ma, "Incorrect computation of abs of signed integral random value."

java/ql/src/Likely Bugs/Arithmetic/RandomUsedOnce.ql

@@ -12,10 +12,8 @@
  */
 
 import java
+import semmle.code.java.security.Random
 
-from MethodAccess ma, Method random
-where
-  random.getDeclaringType().hasQualifiedName("java.util", "Random") and
-  ma.getMethod() = random and
-  ma.getQualifier() instanceof ClassInstanceExpr
+from RandomDataSource ma
+where ma.getQualifier() instanceof ClassInstanceExpr
 select ma, "Random object created and used only once."

java/ql/src/Security/CWE/CWE-129/ArraySizing.qll

@@ -1,6 +1,7 @@
 import java
 import semmle.code.java.dataflow.DataFlow
 import semmle.code.java.dataflow.DefUse
+import semmle.code.java.security.Random
 private import BoundingChecks
 
 /**
@@ -124,33 +125,16 @@ abstract class BoundedFlowSource extends DataFlow::Node {
 }
 
 /**
- * Input that is constructed using a `Random` value.
+ * Input that is constructed using a random value.
  */
 class RandomValueFlowSource extends BoundedFlowSource {
-  RandomValueFlowSource() {
-    exists(RefType random, MethodAccess nextAccess |
-      random.hasQualifiedName("java.util", "Random")
-    |
-      nextAccess.getCallee().getDeclaringType().getAnAncestor() = random and
-      nextAccess.getCallee().getName().matches("next%") and
-      nextAccess = this.asExpr()
-    )
-  }
+  RandomDataSource nextAccess;
 
-  override int lowerBound() {
-    // If this call is to `nextInt()`, the lower bound is zero.
-    this.asExpr().(MethodAccess).getCallee().hasName("nextInt") and
-    this.asExpr().(MethodAccess).getNumArgument() = 1 and
-    result = 0
-  }
+  RandomValueFlowSource() { this.asExpr() = nextAccess }
 
-  override int upperBound() {
-    // If this call specified an argument to `nextInt()`, and that argument is a compile time constant,
-    // it forms the upper bound.
-    this.asExpr().(MethodAccess).getCallee().hasName("nextInt") and
-    this.asExpr().(MethodAccess).getNumArgument() = 1 and
-    result = this.asExpr().(MethodAccess).getArgument(0).(CompileTimeConstantExpr).getIntValue()
-  }
+  override int lowerBound() { result = nextAccess.getLowerBound() }
+
+  override int upperBound() { result = nextAccess.getUpperBound() }
 
   override string getDescription() { result = "Random value" }
 }

java/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql

@@ -13,34 +13,14 @@
 
 import java
 import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.security.Random
 import semmle.code.java.security.SecurityTests
 import ArithmeticCommon
 import DataFlow::PathGraph
 
 class TaintSource extends DataFlow::ExprNode {
   TaintSource() {
-    // Either this is an access to a random number generating method of the right kind, ...
-    exists(Method def |
-      def = this.getExpr().(MethodAccess).getMethod() and
-      (
-        // Some random-number methods are omitted:
-        // `nextDouble` and `nextFloat` are between 0 and 1,
-        // `nextGaussian` is extremely unlikely to hit max values.
-        def.getName() = "nextInt" or
-        def.getName() = "nextLong"
-      ) and
-      def.getNumberOfParameters() = 0 and
-      def.getDeclaringType().hasQualifiedName("java.util", "Random")
-    )
-    or
-    // ... or this is the array parameter of `nextBytes`, which is filled with random bytes.
-    exists(MethodAccess m, Method def |
-      m.getAnArgument() = this.getExpr() and
-      m.getMethod() = def and
-      def.getName() = "nextBytes" and
-      def.getNumberOfParameters() = 1 and
-      def.getDeclaringType().hasQualifiedName("java.util", "Random")
-    )
+    exists(RandomDataSource m | not m.resultMayBeBounded() | m.getOutput() = this.getExpr())
   }
 }
 

java/ql/src/semmle/code/java/dataflow/RangeAnalysis.qll

@@ -68,6 +68,7 @@ private import SSA
 private import RangeUtils
 private import semmle.code.java.dataflow.internal.rangeanalysis.SsaReadPositionCommon
 private import semmle.code.java.controlflow.internal.GuardsLogic
+private import semmle.code.java.security.Random
 private import SignAnalysis
 private import ModulusAnalysis
 private import semmle.code.java.Reflection
@@ -486,14 +487,17 @@ private predicate boundFlowStep(Expr e2, Expr e1, int delta, boolean upper) {
   or
   e2.(AssignOrExpr).getSource() = e1 and positive(e2) and delta = 0 and upper = false
   or
-  exists(MethodAccess ma, Method m |
-    e2 = ma and
-    ma.getMethod() = m and
-    m.hasName("nextInt") and
-    m.getDeclaringType().hasQualifiedName("java.util", "Random") and
-    e1 = ma.getAnArgument() and
-    delta = -1 and
-    upper = true
+  exists(RandomDataSource rds |
+    e2 = rds.getOutput() and
+    (
+      e1 = rds.getUpperBoundExpr() and
+      delta = -1 and
+      upper = true
+      or
+      e1 = rds.getLowerBoundExpr() and
+      delta = 0 and
+      upper = false
+    )
   )
   or
   exists(MethodAccess ma, Method m |

java/ql/src/semmle/code/java/security/Random.qll

@@ -2,15 +2,159 @@ import java
 import semmle.code.java.dataflow.DefUse
 import semmle.code.java.dataflow.DataFlow
 
+/**
+ * The `java.security.SecureRandom` class.
+ */
 class SecureRandomNumberGenerator extends RefType {
   SecureRandomNumberGenerator() { this.hasQualifiedName("java.security", "SecureRandom") }
 }
 
-class GetRandomData extends MethodAccess {
-  GetRandomData() {
-    this.getMethod().getName().matches("next%") and
-    this.getQualifier().getType() instanceof SecureRandomNumberGenerator
+/**
+ * A method access that returns random data or writes random data to an argument.
+ */
+abstract class RandomDataSource extends MethodAccess {
+  /**
+   * Gets the integer lower bound, inclusive, of the values returned by this call,
+   * if applicable to this method's type and a constant bound is known.
+   */
+  int getLowerBound() { result = this.getLowerBoundExpr().(CompileTimeConstantExpr).getIntValue() }
+
+  /**
+   * Gets the integer lower bound, inclusive, of the values returned by this call,
+   * if applicable to this method's type and a bound is known.
+   */
+  Expr getLowerBoundExpr() { none() }
+
+  /**
+   * Gets the integer upper bound, exclusive, of the values returned by this call,
+   * if applicable to this method's type and a constant bound is known.
+   */
+  int getUpperBound() { result = this.getUpperBoundExpr().(CompileTimeConstantExpr).getIntValue() }
+
+  /**
+   * Gets the integer upper bound, exclusive, of the values returned by this call,
+   * if applicable to this method's type and a bound is known.
+   */
+  Expr getUpperBoundExpr() { none() }
+
+  /**
+   * Holds if this source of random data may return bounded values (e.g. integers between 1 and 10).
+   * If it does not hold, it may return any value in the range of its result type (e.g., any possible integer).
+   */
+  predicate resultMayBeBounded() { none() }
+
+  /**
+   * Gets the result of this source of randomness: either the method access itself, or some argument
+   * in the case where it writes random data to that argument.
+   */
+  abstract Expr getOutput();
+}
+
+/**
+ * A method access calling a method declared on `java.util.Random`
+ * that returns random data or writes random data to an argument.
+ */
+class StdlibRandomSource extends RandomDataSource {
+  Method m;
+
+  StdlibRandomSource() {
+    m = this.getMethod() and
+    m.getName().matches("next%") and
+    m.getDeclaringType().getAnAncestor().hasQualifiedName("java.util", "Random")
+  }
+
+  // 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)` 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 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 second 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,
+    // `nextDouble` and `nextFloat` are between 0 and 1,
+    // `nextGaussian` is extremely unlikely to hit max values.
+    m.hasName(["next", "nextBoolean", "nextDouble", "nextFloat", "nextGaussian"])
+    or
+    m.hasName(["nextInt", "nextLong"]) and
+    m.getNumberOfParameters() = [1, 2]
+  }
+
+  override Expr getOutput() {
+    if m.hasName("getBytes") then result = this.getArgument(0) else result = this
+  }
+}
+
+/**
+ * A method access calling a method declared on `org.apache.commons.lang3.RandomUtils`
+ * that returns random data or writes random data to an argument.
+ */
+class ApacheCommonsRandomSource extends RandomDataSource {
+  Method m;
+
+  ApacheCommonsRandomSource() {
+    m = this.getMethod() and
+    m.getName().matches("next%") and
+    m.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "RandomUtils")
+  }
+
+  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, int)` or `nextLong(long, long)`, the upper bound is the second argument.
+    m.hasName(["nextInt", "nextLong"]) and
+    m.getNumberOfParameters() = 2 and
+    result = this.getArgument(1)
   }
+
+  override predicate resultMayBeBounded() {
+    m.hasName(["nextDouble", "nextFloat"])
+    or
+    m.hasName(["nextInt", "nextLong"]) and
+    m.getNumberOfParameters() = 2
+  }
+
+  override Expr getOutput() { result = this }
+}
+
+/**
+ * A method access calling a method declared on `java.security.SecureRandom`
+ * that returns random data or writes random data to an argument.
+ */
+class GetRandomData extends StdlibRandomSource {
+  GetRandomData() { this.getQualifier().getType() instanceof SecureRandomNumberGenerator }
 }
 
 private predicate isSeeded(RValue use) {

java/ql/test/query-tests/BadAbsOfRandom/BadAbsOfRandom.expected

@@ -0,0 +1,6 @@
+| 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. |

java/ql/test/query-tests/BadAbsOfRandom/BadAbsOfRandom.qlref

@@ -0,0 +1 @@
+Likely Bugs/Arithmetic/BadAbsOfRandom.ql

java/ql/test/query-tests/BadAbsOfRandom/Test.java

@@ -0,0 +1,29 @@
+import java.util.Random;
+import java.util.concurrent.ThreadLocalRandom;
+import org.apache.commons.lang3.RandomUtils;
+
+public class Test {
+
+  public static void test() {
+
+    Random r = new Random();
+    Math.abs(r.nextInt());
+    Math.abs(r.nextLong());
+    Math.abs(r.nextInt(100)); // GOOD: random value already has a restricted range
+
+    Math.abs(RandomUtils.nextInt());
+    Math.abs(RandomUtils.nextLong());
+    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
+
+  }
+
+}