Java: QL Query to Detect Security Sensitive non-CSPRNG usage

java/ql/src/Security/CWE/CWE-338/PredictableRandomNumberGenerator.java

@@ -0,0 +1,126 @@
+package org.jlleitschuh.bad.random;
+
+import org.apache.commons.lang.RandomStringUtils; // or org.apache.commons.lang3.RandomStringUtils
+import org.apache.commons.text.CharacterPredicates;
+import org.apache.commons.text.RandomStringGenerator;
+
+import java.nio.charset.Charset;
+import java.security.SecureRandom;
+import java.util.Random;
+import java.util.UUID;
+import java.util.concurrent.ThreadLocalRandom;
+
+public class PredictableRandomNumberGenerator {
+    private static final Random RANDOM = new Random();
+    private static final SecureRandom SECURE_RANDOM = new SecureRandom();
+
+    public static String generatePasswordResetTokenJavaBad() {
+        byte[] array = new byte[30];
+        // BAD!
+        RANDOM.nextBytes(array);
+        return new String(array, Charset.defaultCharset());
+    }
+
+    public static String generatePasswordResetTokenJavaGood() {
+        byte[] array = new byte[30];
+        // GOOD!
+        SECURE_RANDOM.nextBytes(array);
+        return new String(array, Charset.defaultCharset());
+    }
+
+    public static String generateSecretBad() {
+        final char[] CHARS = "abcdefghijklmnopqrstuvwxyz".toCharArray();
+        StringBuilder builder = new StringBuilder(30);
+        for (int i = 0; i < 30; i++) {
+            // BAD!
+            char c = CHARS[RANDOM.nextInt(CHARS.length)];
+            builder.append(c);
+        }
+        return builder.toString();
+    }
+
+    public static String generateSecretGood() {
+        final char[] CHARS = "abcdefghijklmnopqrstuvwxyz".toCharArray();
+        StringBuilder builder = new StringBuilder(30);
+        for (int i = 0; i < 30; i++) {
+            // GOOD!
+            char c = CHARS[SECURE_RANDOM.nextInt(CHARS.length)];
+            builder.append(c);
+        }
+        return builder.toString();
+    }
+
+
+    public static String generateSessionTokenJavaBad() {
+        // BAD!
+        return Long.toHexString(RANDOM.nextLong());
+    }
+
+    public static String generateSessionTokenJavaGood() {
+        // GOOD!
+        return Long.toHexString(SECURE_RANDOM.nextLong());
+    }
+
+    public static String generatePasswordResetTokenApacheCommonsBad() {
+        // BAD!
+        return RandomStringUtils.randomAscii(30);
+    }
+
+    public static String generatePasswordResetTokenApacheCommonsGood() {
+        // GOOD!
+        return RandomStringUtils.random(
+            30,
+            32,
+            127,
+            false,
+            false,
+            null,
+            SECURE_RANDOM
+        );
+    }
+
+    // BAD!
+    private static RandomStringGenerator STRING_GENERATOR =
+        new RandomStringGenerator.Builder()
+            .withinRange('0', 'z')
+            .filteredBy(CharacterPredicates.LETTERS, CharacterPredicates.DIGITS)
+            .build();
+
+    public static String generatePasswordResetTokenApacheTextBad() {
+        // GOOD!
+        return STRING_GENERATOR.generate(30);
+    }
+
+    // GOOD!
+    private static RandomStringGenerator SECURE_STRING_GENERATOR =
+        new RandomStringGenerator.Builder()
+            .usingRandom(SECURE_RANDOM::nextInt) // Notice! Using Secure Random Number Generator
+            .withinRange('0', 'z')
+            .filteredBy(CharacterPredicates.LETTERS, CharacterPredicates.DIGITS)
+            .build();
+
+    public static String generatePasswordResetTokenApacheTextGood() {
+        // GOOD!
+        return SECURE_STRING_GENERATOR.generate(30);
+    }
+
+    /**
+     * This is a real example from a vulnerability in the web server library Ratpack.
+     * See: CVE-2019-11808
+     */
+    public static UUID generateSessionTokenBad() {
+        // Thread local values uses a seed of a bitwise OR of System.currentTimeMillis() and System.nanoTime()
+        // at ThreadLocalRandom classload time.
+        // IE. If you know the launch time of the application, you can predict the seed.
+        ThreadLocalRandom random = ThreadLocalRandom.current();
+        // Bad!
+        return new UUID(random.nextLong(), random.nextLong());
+    }
+
+    public static UUID generateSessionTokenGood() {
+        // GOOD!
+        // Uses SecureRandom internally.
+        return UUID.randomUUID();
+    }
+
+}

java/ql/src/Security/CWE/CWE-338/PredictableRandomNumberGenerator.qhelp

@@ -0,0 +1,88 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Using a Psudeorandom Number Generator (PRNG)
+instead of a Cryptographically Secure Psudeorandom Number Generator (CSPRNG)
+in a context requiring unpredictability, it may be possible for an attacker to
+guess the next value that will be generated, and use this guess to impersonate
+another user or access sensitive information.
+</p>
+
+<h3>Example 1:<h3>
+
+<p>Assuming that the code uses an insecure source of randomess to generate a password reset URL.</p>
+
+<p>An attacker attacker (if able to obtain their own password reset URL) can compute the value for all other password resets for other accounts,
+thus allowing privilege escalation or account takeover.</p>
+
+<h3>Example 2:</h3>
+
+<p>Assuming that the code uses a <code>java.util.concurrent.ThreadLocalRandom</code>
+as the source of randomess to generate a session token.</p>
+
+<p>An attacker (if able to determine the classload time of <code>ThreadLocalRandom</code> (most likely at JVM launch))
+can determine the seed of the <code>ThreadLocalRandom</code> used by the JVM.
+This would allow them to compute any secrets generated by <code>ThreadLocalRandom</code>.</p>
+
+<h3>Criticality</h3>
+
+<p>Depending upon the criticality of random value being used, this vulnerability can have up to a
+<a href="https://nvd.nist.gov/vuln-metrics/cvss/v3-calculator?vector=AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H&version=3.1&amp;source=NIST">
+    CVSS v3.1 base score of 9.8/10
+</a>.</p>
+
+</overview>
+<recommendation>
+
+<p>When generating random secret data, always use <code>java.security.SecureRandom</code> to supply
+your source of randomness.</p>
+
+</recommendation>
+<example>
+
+<p>In the examples below, various ways of generating a random secure and insecure string of characters are demonstrated
+including common uses of Apache Commons.</p>
+
+<sample src="PredictableRandomNumberGenerator.java" />
+
+</example>
+<references>
+
+<li>
+  <a href="https://blog.cloudflare.com/why-randomness-matters/">
+    Cloudflare Blog: Why secure systems require random numbers
+  </a>
+</li>
+<li>
+  <a href="https://news.ycombinator.com/item?id=639976">
+    How I Hacked Hacker News (with arc security advisory)
+  </a>
+<li>
+<li>
+  Research (Hacking Apache Commons RandomStringUtils):
+  <a href="https://medium.com/@alex91ar/the-java-soothsayer-a-practical-application-for-insecure-randomness-c67b0cd148cd">
+    The Java Soothsayer: A practical application for insecure randomness. (Includes free 0day)
+  </a>
+</li>
+
+At the time of writing almost
+<a href="https://github.com/search?l=Java&amp;q=return+RandomStringUtils.randomAlphanumeric%28DEF_COUNT%29%3B&amp;type=Code">
+ 15k repositories
+</a>
+are vulnerable to this due to a vulnerability discovered in the project-bootstraping code generator
+<a href="https://github.com/jhipster/generator-jhipster">
+  JHipster Generator
+</a>
+(
+  <a href="https://nvd.nist.gov/vuln/detail/CVE-2019-16303">
+    CVE-2019-16303
+  </a>
+).
+
+<!--  LocalWords:  CWE random RNG PRNG CSPRNG SecureRandom
+ -->
+
+</references>
+</qhelp>

java/ql/src/Security/CWE/CWE-338/PredictableRandomNumberGenerator.ql

@@ -0,0 +1,125 @@
+/**
+ * @name Predictable Random Number Generation in Sensitive Location
+ * @description The use of predictable random number generation in a security sensitive location can
+ *              can leave an application vulnerable to account takeover as future random values can
+ *              be preditcted or computed by knowing any previous value.
+ * @kind path-problem
+ * @precision medium
+ * @problem.severity error
+ * @id java/predictable-random-number-generation
+ * @tags security
+ *       external/cwe/cwe-338
+ *       external/cwe/cwe-330
+ */
+
+import java
+import types
+import PredictableRandomNumberGeneratorSource
+import semmle.code.java.dataflow.FlowSources
+import DataFlow::PathGraph
+
+/**
+ * A 'Sensitive Name' is something that indicates that the name has some sort of security
+ * implication associated with it's usage.
+ */
+bindingset[name]
+private predicate isSensitiveName(string name) {
+  name.matches("%pass%") // also matches password
+  or
+  name.matches("%tok%") // also matches token
+  or
+  name.matches("%secret%")
+  or
+  name.matches("%reset%key%") and not name.matches("%value%") // Reduce false positive from 'keyValue' type of methods from maps
+  or
+  name.matches("%cred%") // also matches credentials
+  or
+  name.matches("%auth%") // also matches authentication
+  or
+  name.matches("%sess%id%") // also matches sessionid
+}
+
+abstract class SecureContextSink extends DataFlow::Node { }
+
+private class SensitiveVariable extends Variable {
+  SensitiveVariable() { isSensitiveName(this.getName().toLowerCase()) }
+}
+
+private class SensitiveMethod extends Method {
+  SensitiveMethod() { isSensitiveName(this.getName().toLowerCase()) }
+}
+
+/**
+ * Assignment to a variable with a name that indicates the data is potentially sensitive.
+ */
+private class SensitiveVariableSink extends SecureContextSink {
+  SensitiveVariableSink() {
+    exists(VariableAssign va |
+      va.getDestVar() instanceof SensitiveVariable and va.getSource() = this.asExpr()
+    )
+  }
+}
+
+/**
+ * Insecure RNG leaks into the context of a method with potentially senisitve behaviour.
+ */
+private class SensitiveMethodSink extends SecureContextSink {
+  SensitiveMethodSink() { this.getEnclosingCallable() instanceof SensitiveMethod }
+}
+
+class PredictableRandomConfig extends TaintTracking::Configuration {
+  PredictableRandomConfig() { this = "PredictableRandomConfig" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof PredictableRandomFlowSource
+  }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof SecureContextSink }
+
+  private predicate testSanitizer(DataFlow::Node node) {
+    // TODO: Don't merge with this
+    // Helps get test logic out of the results
+    node.asExpr().getFile().getAbsolutePath().toLowerCase().matches("%test%")
+  }
+
+  override predicate isSanitizerOut(DataFlow::Node node) { testSanitizer(node) }
+
+  override predicate isSanitizerIn(DataFlow::Node node) { testSanitizer(node) }
+
+  /**
+   * A UUID created with insecure RNG will itself be tainted.
+   */
+  private predicate isTaintedUuidCreation(Expr expSource, Expr expDest) {
+    exists(UUIDCreationExp c | c.getAnArgument() = expSource and c = expDest)
+  }
+
+  /**
+   * A char array that holds pre-aproved characters but the elements from that
+   * char array are selected with a random number generator that is insecure.
+   *
+   * Example:
+   * ```
+   * Random RANDOM = new Random()
+   * char[] CHARS = "abcdefghijklmnopqrstuvwxyz".toCharArray();
+   * // Insecurely chosen
+   * char c = CHARS[RANDOM.nextInt(CHARS.length)];
+   * ```
+   */
+  private predicate isTaintedCharArrayAccess(Expr expSource, Expr expDest) {
+    exists(ArrayAccess aa, Array a, CharacterType charType |
+      a.getComponentType() = charType and
+      aa.getArray().(VarAccess).getVariable().getType() = a and
+      aa.getIndexExpr() = expSource and
+      aa = expDest
+    )
+  }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+    isTaintedUuidCreation(node1.asExpr(), node2.asExpr()) or
+    isTaintedCharArrayAccess(node1.asExpr(), node2.asExpr())
+  }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, PredictableRandomConfig conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Potentially predictably generated random value"