Merge pull request #13608 from egregius313/egregius313/weak-randomness

Java: Add Weak Randomness Query (CWE-330/338)

java/ql/lib/ext/java.security.spec.model.yml

@@ -3,6 +3,11 @@ extensions:
       pack: codeql/java-all
       extensible: sinkModel
     data:
+      - ["java.security.spec", "DSAPrivateKeySpec", False, "DSAPrivateKeySpec", "", "", "Argument[0..3]", "credentials-key", "manual"]
+      - ["java.security.spec", "ECPrivateKeySpec", False, "ECPrivateKeySpec", "", "", "Argument[0]", "credentials-key", "manual"]
       - ["java.security.spec", "EncodedKeySpec", False, "EncodedKeySpec", "(byte[])", "", "Argument[0]", "credentials-key", "hq-generated"]
       - ["java.security.spec", "PKCS8EncodedKeySpec", False, "PKCS8EncodedKeySpec", "(byte[])", "", "Argument[0]", "credentials-key", "hq-generated"]
+      - ["java.security.spec", "RSAMultiPrimePrivateCrtKeySpec", False, "RSAMultiPrimePrivateCrtKeySpec", "", "", "Argument[2]", "credentials-key", "manual"]
+      - ["java.security.spec", "RSAPrivateCrtKeySpec", False, "RSAPrivateCrtKeySpec", "", "", "Argument[2]", "credentials-key", "manual"]
+      - ["java.security.spec", "RSAPrivateKeySpec", False, "RSAPrivateKeySpec", "", "", "Argument[1]", "credentials-key", "manual"]
       - ["java.security.spec", "X509EncodedKeySpec", False, "X509EncodedKeySpec", "(byte[])", "", "Argument[0]", "credentials-key", "hq-generated"]

java/ql/lib/ext/javax.crypto.spec.model.yml

@@ -11,9 +11,6 @@ extensions:
       pack: codeql/java-all
       extensible: sinkModel
     data:
-      - ["javax.crypto.spec", "PBEKeySpec", False, "PBEKeySpec", "(char[])", "", "Argument[0]", "credentials-password", "hq-generated"]
-      - ["javax.crypto.spec", "PBEKeySpec", False, "PBEKeySpec", "(char[],byte[],int)", "", "Argument[0]", "credentials-password", "hq-generated"]
-      - ["javax.crypto.spec", "PBEKeySpec", False, "PBEKeySpec", "(char[],byte[],int,int)", "", "Argument[0]", "credentials-password", "hq-generated"]
       - ["javax.crypto.spec", "DESKeySpec", False, "DESKeySpec", "(byte[])", "", "Argument[0]", "credentials-key", "hq-generated"]
       - ["javax.crypto.spec", "DESKeySpec", False, "DESKeySpec", "(byte[],int)", "", "Argument[0]", "credentials-key", "hq-generated"]
       - ["javax.crypto.spec", "DESKeySpec", False, "isParityAdjusted", "(byte[],int)", "", "Argument[0]", "credentials-key", "hq-generated"]
@@ -21,5 +18,11 @@ extensions:
       - ["javax.crypto.spec", "DESedeKeySpec", False, "DESedeKeySpec", "(byte[])", "", "Argument[0]", "credentials-key", "hq-generated"]
       - ["javax.crypto.spec", "DESedeKeySpec", False, "DESedeKeySpec", "(byte[],int)", "", "Argument[0]", "credentials-key", "hq-generated"]
       - ["javax.crypto.spec", "DESedeKeySpec", False, "isParityAdjusted", "(byte[],int)", "", "Argument[0]", "credentials-key", "hq-generated"]
+      - ["javax.crypto.spec", "DHPrivateKeySpec", False, "DHPrivateKeySpec", "", "", "Argument[0]", "credentials-key", "manual"]
+      - ["javax.crypto.spec", "PBEKeySpec", False, "PBEKeySpec", "", "", "Argument[1]", "encryption-salt", "manual"]
+      - ["javax.crypto.spec", "PBEKeySpec", False, "PBEKeySpec", "(char[])", "", "Argument[0]", "credentials-password", "hq-generated"]
+      - ["javax.crypto.spec", "PBEKeySpec", False, "PBEKeySpec", "(char[],byte[],int)", "", "Argument[0]", "credentials-password", "hq-generated"]
+      - ["javax.crypto.spec", "PBEKeySpec", False, "PBEKeySpec", "(char[],byte[],int,int)", "", "Argument[0]", "credentials-password", "hq-generated"]
+      - ["javax.crypto.spec", "PBEParameterSpec", False, "PBEParameterSpec", "", "", "Argument[0]", "encryption-salt", "manual"]
       - ["javax.crypto.spec", "SecretKeySpec", False, "SecretKeySpec", "(byte[],String)", "", "Argument[0]", "credentials-key", "hq-generated"]
       - ["javax.crypto.spec", "SecretKeySpec", False, "SecretKeySpec", "(byte[],int,int,String)", "", "Argument[0]", "credentials-key", "hq-generated"]

java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll

@@ -0,0 +1,97 @@
+/** Provides classes and predicates for reasoning about insecure randomness. */
+
+import java
+private import semmle.code.java.frameworks.Servlets
+private import semmle.code.java.security.SensitiveActions
+private import semmle.code.java.security.SensitiveApi
+private import semmle.code.java.dataflow.TaintTracking
+private import semmle.code.java.dataflow.ExternalFlow
+private import semmle.code.java.security.RandomQuery
+
+/**
+ * A node representing a source of insecure randomness.
+ *
+ * For example, use of `java.util.Random` or `java.lang.Math.random`.
+ */
+abstract class InsecureRandomnessSource extends DataFlow::Node { }
+
+private class RandomMethodSource extends InsecureRandomnessSource {
+  RandomMethodSource() {
+    exists(RandomDataSource s | this.asExpr() = s.getOutput() |
+      not s.getQualifier().getType() instanceof SafeRandomImplementation
+    )
+  }
+}
+
+/**
+ * A type which is an implementation of `java.util.Random` but considered to be safe.
+ *
+ * For example, `java.security.SecureRandom`.
+ */
+abstract private class SafeRandomImplementation extends RefType { }
+
+private class TypeSecureRandom extends SafeRandomImplementation instanceof SecureRandomNumberGenerator
+{ }
+
+private class TypeHadoopOsSecureRandom extends SafeRandomImplementation {
+  TypeHadoopOsSecureRandom() {
+    this.hasQualifiedName("org.apache.hadoop.crypto.random", "OsSecureRandom")
+  }
+}
+
+/**
+ * A node representing an operation which should not use a Insecurely random value.
+ */
+abstract class InsecureRandomnessSink extends DataFlow::Node { }
+
+/**
+ * A node which sets the value of a cookie.
+ */
+private class CookieSink extends InsecureRandomnessSink {
+  CookieSink() {
+    exists(Call c |
+      c.(ClassInstanceExpr).getConstructedType() instanceof TypeCookie and
+      this.asExpr() = c.getArgument(1)
+      or
+      c.(MethodCall).getMethod().getDeclaringType() instanceof TypeCookie and
+      c.(MethodCall).getMethod().hasName("setValue") and
+      this.asExpr() = c.getArgument(0)
+    )
+  }
+}
+
+private class SensitiveActionSink extends InsecureRandomnessSink {
+  SensitiveActionSink() { this.asExpr() instanceof SensitiveExpr }
+}
+
+private class CredentialsSink extends InsecureRandomnessSink instanceof CredentialsSinkNode { }
+
+/**
+ * A taint-tracking configuration for Insecure randomness.
+ */
+module InsecureRandomnessConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node src) { src instanceof InsecureRandomnessSource }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof InsecureRandomnessSink }
+
+  predicate isBarrierIn(DataFlow::Node n) { isSource(n) }
+
+  predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
+    n1.asExpr() = n2.asExpr().(BinaryExpr).getAnOperand()
+    or
+    n1.asExpr() = n2.asExpr().(UnaryExpr).getExpr()
+    or
+    exists(MethodCall mc, string methodName |
+      mc.getMethod().hasQualifiedName("org.owasp.esapi", "Encoder", methodName) and
+      methodName.matches("encode%")
+    |
+      n1.asExpr() = mc.getArgument(0) and
+      n2.asExpr() = mc
+    )
+  }
+}
+
+/**
+ * Taint-tracking flow of a Insecurely random value into a sensitive sink.
+ */
+module InsecureRandomnessFlow = TaintTracking::Global<InsecureRandomnessConfig>;

java/ql/lib/semmle/code/java/security/RandomDataSource.qll

@@ -107,6 +107,15 @@ class StdlibRandomSource extends RandomDataSource {
   }
 }
 
+/**
+ * A method access calling the `random` of `java.lang.Math`.
+ */
+class MathRandomSource extends RandomDataSource {
+  MathRandomSource() { this.getMethod().hasQualifiedName("java.lang", "Math", "random") }
+
+  override Expr getOutput() { 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.
@@ -143,3 +152,19 @@ class ApacheCommonsRandomSource extends RandomDataSource {
 
   override Expr getOutput() { result = this }
 }
+
+/**
+ * A method access calling a method declared on `org.apache.commons.lang3.RandomStringUtils`
+ */
+class ApacheCommonsRandomStringSource extends RandomDataSource {
+  ApacheCommonsRandomStringSource() {
+    exists(Method m | m = this.getMethod() |
+      m.getName().matches("random%") and
+      m.getDeclaringType()
+          .hasQualifiedName(["org.apache.commons.lang3", "org.apache.commons.lang"],
+            "RandomStringUtils")
+    )
+  }
+
+  override Expr getOutput() { result = this }
+}

java/ql/src/Security/CWE/CWE-330/InsecureRandomness.qhelp

@@ -0,0 +1,64 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+	<overview>
+		<p>
+			If you use a cryptographically weak pseudo-random number generator to generate security-sensitive values,
+			such as passwords, attackers can more easily predict those values.
+		</p>
+		<p>
+			Pseudo-random number generators generate a sequence of numbers that only approximates the properties
+			of random numbers. The sequence is not truly random because it is completely determined by a
+			relatively small set of initial values (the seed). If the random number generator is
+			cryptographically weak, then this sequence may be easily predictable through outside observations.
+		</p>
+
+	</overview>
+	<recommendation>
+        <p>
+           The <code>java.util.Random</code> random number generator is not cryptographically secure. Use a secure random number generator such as <code>java.security.SecureRandom</code> instead.
+        </p>
+        <p>
+		Use a cryptographically secure pseudo-random number generator if the output is to be used in a
+		security-sensitive context. As a general rule, a value should be considered "security-sensitive"
+		if predicting it would allow the attacker to perform an action that they would otherwise be unable
+		to perform. For example, if an attacker could predict the random password generated for a new user,
+		they would be able to log in as that new user.
+	</p>
+	</recommendation>
+
+	<example>
+
+		<p>
+			The following examples show different ways of generating a cookie with a random value.
+		</p>
+
+        <p>
+            In the first (BAD) case, we generate a fresh cookie by appending a random integer to the end of a static
+            string. The random number generator used (<code>Random</code>) is not cryptographically secure,
+            so it may be possible for an attacker to predict the generated cookie.
+        </p>
+
+        <sample src="examples/InsecureRandomnessCookie.java" />
+
+        <p>
+            In the second (GOOD) case, we generate a fresh cookie by appending a random integer to the end of a static
+            string. The random number generator used (<code>SecureRandom</code>) is cryptographically secure,
+            so it is not possible for an attacker to predict the generated cookie.
+        </p>
+
+        <sample src="examples/SecureRandomnessCookie.java" />
+
+	</example>
+
+	<references>
+		<li>Wikipedia: <a href="http://en.wikipedia.org/wiki/Pseudorandom_number_generator">Pseudo-random number generator</a>.</li>
+        <li>
+            Java Docs: <a href="http://docs.oracle.com/javase/8/docs/api/java/util/Random.html">Random</a>.
+        </li>
+        <li>
+            Java Docs: <a href="http://docs.oracle.com/javase/8/docs/api/java/security/SecureRandom.html">SecureRandom</a>.
+        </li>
+	</references>
+</qhelp>

java/ql/src/Security/CWE/CWE-330/InsecureRandomness.ql

@@ -0,0 +1,23 @@
+/**
+ * @name Insecure randomness
+ * @description Using a cryptographically Insecure pseudo-random number generator to generate a
+ *              security-sensitive value may allow an attacker to predict what value will
+ *              be generated.
+ * @kind path-problem
+ * @problem.severity warning
+ * @security-severity 7.8
+ * @precision high
+ * @id java/insecure-randomness
+ * @tags security
+ *      external/cwe/cwe-330
+ *      external/cwe/cwe-338
+ */
+
+import java
+import semmle.code.java.security.InsecureRandomnessQuery
+import InsecureRandomnessFlow::PathGraph
+
+from InsecureRandomnessFlow::PathNode source, InsecureRandomnessFlow::PathNode sink
+where InsecureRandomnessFlow::flowPath(source, sink)
+select sink.getNode(), source, sink, "Potential Insecure randomness due to a $@.", source.getNode(),
+  "Insecure randomness source."

java/ql/src/Security/CWE/CWE-330/examples/InsecureRandomnessCookie.java

@@ -0,0 +1,9 @@
+Random r = new Random();
+
+byte[] bytes = new byte[16];
+r.nextBytes(bytes);
+
+String cookieValue = encode(bytes);
+
+Cookie cookie = new Cookie("name", cookieValue);
+response.addCookie(cookie);

java/ql/src/Security/CWE/CWE-330/examples/SecureRandomnessCookie.java

@@ -0,0 +1,9 @@
+SecureRandom r = new SecureRandom();
+
+byte[] bytes = new byte[16];
+r.nextBytes(bytes);
+
+String cookieValue = encode(bytes);
+
+Cookie cookie = new Cookie("name", cookieValue);
+response.addCookie(cookie);

java/ql/src/change-notes/2023-11-08-weak-randomness-query.md

@@ -0,0 +1,5 @@
+---
+category: newQuery
+---
+* Added the `java/insecure-randomness` query to detect uses of weakly random values which an attacker may be able to predict. Also added the `crypto-parameter` sink kind for sinks which represent the parameters and keys of cryptographic operations. 
+

java/ql/test/query-tests/security/CWE-330/InsecureRandomnessTest.expected

@@ -0,0 +1,2 @@
+failures
+testFailures

java/ql/test/query-tests/security/CWE-330/InsecureRandomnessTest.ql

@@ -0,0 +1,19 @@
+import java
+import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.security.InsecureRandomnessQuery
+import TestUtilities.InlineExpectationsTest
+
+module WeakRandomTest implements TestSig {
+  string getARelevantTag() { result = "hasWeakRandomFlow" }
+
+  predicate hasActualResult(Location location, string element, string tag, string value) {
+    tag = "hasWeakRandomFlow" and
+    exists(DataFlow::Node sink | InsecureRandomnessFlow::flowTo(sink) |
+      sink.getLocation() = location and
+      element = sink.toString() and
+      value = ""
+    )
+  }
+}
+
+import MakeTest<WeakRandomTest>

java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java

@@ -0,0 +1,61 @@
+import java.io.IOException;
+import java.util.Random;
+import java.util.concurrent.ThreadLocalRandom;
+import java.security.SecureRandom;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServlet;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import javax.servlet.http.Cookie;
+import org.apache.commons.lang3.RandomStringUtils;
+import org.owasp.esapi.Encoder;
+
+public class WeakRandomCookies extends HttpServlet {
+    HttpServletResponse response;
+
+    public void doGet() {
+        Random r = new Random();
+
+        int c = r.nextInt();
+        // BAD: The cookie value may be predictable.
+        Cookie cookie = new Cookie("name", Integer.toString(c)); // $hasWeakRandomFlow
+
+        Encoder enc = null;
+        int c2 = r.nextInt();
+        String value = enc.encodeForHTML(Integer.toString(c2));
+        // BAD: The cookie value may be predictable.
+        Cookie cookie2 = new Cookie("name", value); // $hasWeakRandomFlow
+
+        byte[] bytes = new byte[16];
+        r.nextBytes(bytes);
+        // BAD: The cookie value may be predictable.
+        Cookie cookie3 = new Cookie("name", new String(bytes)); // $hasWeakRandomFlow
+
+        SecureRandom sr = new SecureRandom();
+
+        byte[] bytes2 = new byte[16];
+        sr.nextBytes(bytes2);
+        // GOOD: The cookie value is unpredictable.
+        Cookie cookie4 = new Cookie("name", new String(bytes2)); 
+        
+        ThreadLocalRandom tlr = ThreadLocalRandom.current();
+
+        Cookie cookie5 = new Cookie("name", Integer.toString(tlr.nextInt())); // $hasWeakRandomFlow
+
+        Cookie cookie6 = new Cookie("name", RandomStringUtils.random(10)); // $hasWeakRandomFlow
+
+        Cookie cookie7 = new Cookie("name", RandomStringUtils.randomAscii(10)); // $hasWeakRandomFlow
+
+        long c3 = r.nextLong();
+        // BAD: The cookie value may be predictable.
+        Cookie cookie8 = new Cookie("name", Long.toString(c3 * 5)); // $hasWeakRandomFlow
+
+        double c4 = Math.random();
+        // BAD: The cookie value may be predictable.
+        Cookie cookie9 = new Cookie("name", Double.toString(c4)); // $hasWeakRandomFlow
+
+        double c5 = Math.random();
+        // BAD: The cookie value may be predictable.
+        Cookie cookie10 = new Cookie("name", Double.toString(++c5)); // $hasWeakRandomFlow
+    }
+}

java/ql/test/query-tests/security/CWE-330/options

@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/servlet-api-2.4:${testdir}/../../../stubs/apache-commons-lang3-3.7:${testdir}/../../../stubs/esapi-2.0.1

java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/Encoder.java

@@ -2,4 +2,6 @@ package org.owasp.esapi;
 
 public interface Encoder {
   String encodeForLDAP(String input);
+
+  String encodeForHTML(String untrustedData);
 }

java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/reference/DefaultEncoder.java

@@ -5,4 +5,5 @@ import org.owasp.esapi.Encoder;
 public class DefaultEncoder implements Encoder {
   public static Encoder getInstance() { return null; }
   public String encodeForLDAP(String input) { return null; }
+  public String encodeForHTML(String untrustedData) { return null; }
 }