Merge pull request #14681 from atorralba/atorralba/java/weak-randomness-cve-coverage

Java: Add more sinks to the Insecure Randomness query

Author: atorralba

java/ql/lib/ext/org.pac4j.jwt.config.encryption.model.yml

@@ -0,0 +1,9 @@
+extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: sinkModel
+    data:
+      - ["org.pac4j.jwt.config.encryption", "SecretEncryptionConfiguration", True, "SecretEncryptionConfiguration", "", "", "Argument[0]", "credentials-key", "manual"]
+      - ["org.pac4j.jwt.config.encryption", "SecretEncryptionConfiguration", True, "setSecret", "", "", "Argument[0]", "credentials-key", "manual"]
+      - ["org.pac4j.jwt.config.encryption", "SecretEncryptionConfiguration", True, "setSecretBase64", "", "", "Argument[0]", "credentials-key", "manual"]
+      - ["org.pac4j.jwt.config.encryption", "SecretEncryptionConfiguration", True, "setSecretBytes", "", "", "Argument[0]", "credentials-key", "manual"]

java/ql/lib/ext/org.pac4j.jwt.config.signature.model.yml

@@ -0,0 +1,9 @@
+extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: sinkModel
+    data:
+      - ["org.pac4j.jwt.config.signature", "SecretSignatureConfiguration", True, "SecretEncryptionConfiguration", "", "", "Argument[0]", "credentials-key", "manual"]
+      - ["org.pac4j.jwt.config.signature", "SecretSignatureConfiguration", True, "setSecret", "", "", "Argument[0]", "credentials-key", "manual"]
+      - ["org.pac4j.jwt.config.signature", "SecretSignatureConfiguration", True, "setSecretBase64", "", "", "Argument[0]", "credentials-key", "manual"]
+      - ["org.pac4j.jwt.config.signature", "SecretSignatureConfiguration", True, "setSecretBytes", "", "", "Argument[0]", "credentials-key", "manual"]

java/ql/lib/semmle/code/java/frameworks/Netty.qll

@@ -0,0 +1,23 @@
+/** Provides definitions related to the Netty framework. */
+
+import java
+
+/** The interface `Cookie` in the packages `io.netty.handler.codec.http` and `io.netty.handler.codec.http.cookie`. */
+class NettyCookie extends Interface {
+  NettyCookie() { this.hasQualifiedName("io.netty.handler.codec.http" + [".cookie", ""], "Cookie") }
+}
+
+/** The class `DefaultCookie` in the packages `io.netty.handler.codec.http` and `io.netty.handler.codec.http.cookie`. */
+class NettyDefaultCookie extends Class {
+  NettyDefaultCookie() {
+    this.hasQualifiedName("io.netty.handler.codec.http" + [".cookie", ""], "DefaultCookie")
+  }
+}
+
+/** The method `setValue` of the interface `Cookie` or a class implementing it. */
+class NettySetCookieValueMethod extends Method {
+  NettySetCookieValueMethod() {
+    this.getDeclaringType*() instanceof NettyCookie and
+    this.hasName("setValue")
+  }
+}

java/ql/lib/semmle/code/java/frameworks/OpenSaml.qll

@@ -0,0 +1,29 @@
+/**
+ * Provides classes and predicates for working with the OpenSAML libraries.
+ */
+
+import java
+private import semmle.code.java.security.InsecureRandomnessQuery
+
+/** The interface  `org.opensaml.saml.saml2.core.RequestAbstractType`. */
+class SamlRequestAbstractType extends Interface {
+  SamlRequestAbstractType() {
+    this.hasQualifiedName("org.opensaml.saml.saml2.core", "RequestAbstractType")
+  }
+}
+
+/** The method `setID` of  the interface `RequestAbstractType`. */
+class SamlRequestSetIdMethod extends Method {
+  SamlRequestSetIdMethod() {
+    this.getDeclaringType() instanceof SamlRequestAbstractType and
+    this.hasName("setID")
+  }
+}
+
+private class SamlRequestSetIdSink extends InsecureRandomnessSink {
+  SamlRequestSetIdSink() {
+    exists(MethodCall c | c.getMethod() instanceof SamlRequestSetIdMethod |
+      c.getArgument(0) = this.asExpr()
+    )
+  }
+}

java/ql/lib/semmle/code/java/frameworks/Servlets.qll

@@ -244,7 +244,7 @@ class TypeCookie extends Class {
 }
 
 /**
- * The method `getValue(String)` declared in `javax.servlet.http.Cookie`.
+ * The method `getValue()` declared in `javax.servlet.http.Cookie`.
  */
 class CookieGetValueMethod extends Method {
   CookieGetValueMethod() {
@@ -254,6 +254,16 @@ class CookieGetValueMethod extends Method {
   }
 }
 
+/**
+ * The method `setValue(String)` declared in `javax.servlet.http.Cookie`.
+ */
+class CookieSetValueMethod extends Method {
+  CookieSetValueMethod() {
+    this.getDeclaringType() instanceof TypeCookie and
+    this.hasName("setValue")
+  }
+}
+
 /**
  * The method `getName()` declared in `javax.servlet.http.Cookie`.
  */

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

@@ -0,0 +1,32 @@
+/** Provides definitions to reason about HTTP cookies. */
+
+import java
+private import semmle.code.java.frameworks.Netty
+private import semmle.code.java.frameworks.Servlets
+
+/** An expression setting the value of a cookie. */
+abstract class SetCookieValue extends Expr { }
+
+private class ServletSetCookieValue extends SetCookieValue {
+  ServletSetCookieValue() {
+    exists(Call c |
+      c.(ClassInstanceExpr).getConstructedType() instanceof TypeCookie and
+      this = c.getArgument(1)
+      or
+      c.(MethodCall).getMethod() instanceof CookieSetValueMethod and
+      this = c.getArgument(0)
+    )
+  }
+}
+
+private class NettySetCookieValue extends SetCookieValue {
+  NettySetCookieValue() {
+    exists(Call c |
+      c.(ClassInstanceExpr).getConstructedType() instanceof NettyDefaultCookie and
+      this = c.getArgument(1)
+      or
+      c.(MethodCall).getMethod() instanceof NettySetCookieValueMethod and
+      this = c.getArgument(0)
+    )
+  }
+}

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

@@ -1,12 +1,14 @@
 /** Provides classes and predicates for reasoning about insecure randomness. */
 
 import java
+private import semmle.code.java.frameworks.OpenSaml
 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.dataflow.TaintTracking
+private import semmle.code.java.security.Cookies
 private import semmle.code.java.security.RandomQuery
+private import semmle.code.java.security.SensitiveActions
+private import semmle.code.java.security.SensitiveApi
 
 /**
  * A node representing a source of insecure randomness.
@@ -18,7 +20,7 @@ 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
+      not s.getSourceOfRandomness() instanceof SafeRandomImplementation
     )
   }
 }
@@ -40,24 +42,15 @@ private class TypeHadoopOsSecureRandom extends SafeRandomImplementation {
 }
 
 /**
- * A node representing an operation which should not use a Insecurely random value.
+ * A node representing an operation which should not use an 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)
-    )
-  }
+  CookieSink() { this.asExpr() instanceof SetCookieValue }
 }
 
 private class SensitiveActionSink extends InsecureRandomnessSink {
@@ -76,6 +69,8 @@ module InsecureRandomnessConfig implements DataFlow::ConfigSig {
 
   predicate isBarrierIn(DataFlow::Node n) { isSource(n) }
 
+  predicate isBarrierOut(DataFlow::Node n) { isSink(n) }
+
   predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
     n1.asExpr() = n2.asExpr().(BinaryExpr).getAnOperand()
     or
@@ -88,6 +83,17 @@ module InsecureRandomnessConfig implements DataFlow::ConfigSig {
       n1.asExpr() = mc.getArgument(0) and
       n2.asExpr() = mc
     )
+    or
+    // TODO: Once we have a default sanitizer for UUIDs, we can convert these to global summaries.
+    exists(Call c |
+      c.(ClassInstanceExpr).getConstructedType().hasQualifiedName("java.util", "UUID") and
+      n1.asExpr() = c.getAnArgument() and
+      n2.asExpr() = c
+      or
+      c.(MethodCall).getMethod().hasQualifiedName("java.util", "UUID", "toString") and
+      n1.asExpr() = c.getQualifier() and
+      n2.asExpr() = c
+    )
   }
 }
 

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

@@ -3,6 +3,7 @@
  */
 
 import java
+private import semmle.code.java.dataflow.TypeFlow
 
 /**
  * A method access that returns random data or writes random data to an argument.
@@ -43,6 +44,9 @@ abstract class RandomDataSource extends MethodCall {
    * in the case where it writes random data to that argument.
    */
   abstract Expr getOutput();
+
+  /** Gets the type of the source of randomness used by this call. */
+  RefType getSourceOfRandomness() { boundOrStaticType(this.getQualifier(), result) }
 }
 
 /**
@@ -167,4 +171,18 @@ class ApacheCommonsRandomStringSource extends RandomDataSource {
   }
 
   override Expr getOutput() { result = this }
+
+  override RefType getSourceOfRandomness() {
+    if
+      this.getMethod().hasStringSignature("random(int, int, int, boolean, boolean, char[], Random)")
+    then boundOrStaticType(this.getArgument(6), result)
+    else result.hasQualifiedName("java.util", "Random")
+  }
+}
+
+/** Holds if `t` is the static type of `e`, or an upper bound of the runtime type of `e`. */
+private predicate boundOrStaticType(Expr e, RefType t) {
+  exprTypeFlow(e, t, false)
+  or
+  t = e.getType()
 }

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

@@ -10,7 +10,7 @@
 import org.apache.commons.lang3.RandomStringUtils;
 import org.owasp.esapi.Encoder;
 
-public class WeakRandomCookies extends HttpServlet {
+public class InsecureRandomCookies extends HttpServlet {
     HttpServletResponse response;
 
     public void doGet() {
@@ -19,6 +19,14 @@ public void doGet() {
         int c = r.nextInt();
         // BAD: The cookie value may be predictable.
         Cookie cookie = new Cookie("name", Integer.toString(c)); // $hasWeakRandomFlow
+        cookie.setValue(Integer.toString(c)); // $hasWeakRandomFlow
+
+        io.netty.handler.codec.http.Cookie nettyCookie =
+                new io.netty.handler.codec.http.DefaultCookie("name", Integer.toString(c)); // $hasWeakRandomFlow
+        nettyCookie.setValue(Integer.toString(c)); // $hasWeakRandomFlow
+        io.netty.handler.codec.http.cookie.Cookie nettyCookie2 =
+                new io.netty.handler.codec.http.cookie.DefaultCookie("name", Integer.toString(c)); // $hasWeakRandomFlow
+        nettyCookie2.setValue(Integer.toString(c)); // $hasWeakRandomFlow
 
         Encoder enc = null;
         int c2 = r.nextInt();
@@ -36,8 +44,8 @@ public void doGet() {
         byte[] bytes2 = new byte[16];
         sr.nextBytes(bytes2);
         // GOOD: The cookie value is unpredictable.
-        Cookie cookie4 = new Cookie("name", new String(bytes2)); 
-        
+        Cookie cookie4 = new Cookie("name", new String(bytes2));
+
         ThreadLocalRandom tlr = ThreadLocalRandom.current();
 
         Cookie cookie5 = new Cookie("name", Integer.toString(tlr.nextInt())); // $hasWeakRandomFlow

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

@@ -1 +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
+//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:${testdir}/../../../stubs/netty-4.1.x