Merge pull request #6171 from atorralba/atorralba/promote-unsafe-cert…

…ificate-trust

Java: Promote Unsafe certificate trust query from experimental

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

@@ -14,6 +14,11 @@ class TypeSocket extends RefType {
   TypeSocket() { this.hasQualifiedName("java.net", "Socket") }
 }
 
+/** The type `javax.net.SocketFactory` */
+class TypeSocketFactory extends RefType {
+  TypeSocketFactory() { this.hasQualifiedName("javax.net", "SocketFactory") }
+}
+
 /** The type `java.net.URL`. */
 class TypeUrl extends RefType {
   TypeUrl() { this.hasQualifiedName("java.net", "URL") }
@@ -42,6 +47,15 @@ class SocketGetInputStreamMethod extends Method {
   }
 }
 
+/** The method `java.net.Socket::getOutputStream`. */
+class SocketGetOutputStreamMethod extends Method {
+  SocketGetOutputStreamMethod() {
+    this.getDeclaringType() instanceof TypeSocket and
+    this.hasName("getOutputStream") and
+    this.hasNoParameters()
+  }
+}
+
 /** A method or constructor call that returns a new `URI`. */
 class UriCreation extends Call {
   UriCreation() {
@@ -143,6 +157,22 @@ class UrlOpenConnectionMethod extends Method {
   }
 }
 
+/** The method `javax.net.SocketFactory::createSocket`. */
+class CreateSocketMethod extends Method {
+  CreateSocketMethod() {
+    this.hasName("createSocket") and
+    this.getDeclaringType().getASupertype*() instanceof TypeSocketFactory
+  }
+}
+
+/** The method `javax.net.Socket::connect`. */
+class SocketConnectMethod extends Method {
+  SocketConnectMethod() {
+    this.hasName("connect") and
+    this.getDeclaringType() instanceof TypeSocket
+  }
+}
+
 /**
  * A string matching private host names of IPv4 and IPv6, which only matches the host portion therefore checking for port is not necessary.
  * Several examples are localhost, reserved IPv4 IP addresses including 127.0.0.1, 10.x.x.x, 172.16.x,x, 192.168.x,x, and reserved IPv6 addresses including [0:0:0:0:0:0:0:1] and [::1]

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

@@ -34,6 +34,21 @@ class SSLSession extends RefType {
   SSLSession() { this.hasQualifiedName("javax.net.ssl", "SSLSession") }
 }
 
+/** The `javax.net.ssl.SSLEngine` class. */
+class SSLEngine extends RefType {
+  SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") }
+}
+
+/** The `javax.net.ssl.SSLSocket` class. */
+class SSLSocket extends RefType {
+  SSLSocket() { this.hasQualifiedName("javax.net.ssl", "SSLSocket") }
+}
+
+/** The `javax.net.ssl.SSLParameters` class. */
+class SSLParameters extends RefType {
+  SSLParameters() { this.hasQualifiedName("javax.net.ssl", "SSLParameters") }
+}
+
 class HostnameVerifier extends RefType {
   HostnameVerifier() { this.hasQualifiedName("javax.net.ssl", "HostnameVerifier") }
 }
@@ -79,6 +94,14 @@ class GetSocketFactory extends Method {
   }
 }
 
+/** The `createSSLEngine` method of the class `javax.net.ssl.SSLContext` */
+class CreateSslEngineMethod extends Method {
+  CreateSslEngineMethod() {
+    this.hasName("createSSLEngine") and
+    this.getDeclaringType() instanceof SSLContext
+  }
+}
+
 class SetConnectionFactoryMethod extends Method {
   SetConnectionFactoryMethod() {
     this.hasName("setSSLSocketFactory") and
@@ -101,6 +124,38 @@ class SetDefaultHostnameVerifierMethod extends Method {
   }
 }
 
+/** The `beginHandshake` method of the class `javax.net.ssl.SSLEngine`. */
+class BeginHandshakeMethod extends Method {
+  BeginHandshakeMethod() {
+    this.hasName("beginHandshake") and
+    this.getDeclaringType().getASupertype*() instanceof SSLEngine
+  }
+}
+
+/** The `wrap` method of the class `javax.net.ssl.SSLEngine`. */
+class SslWrapMethod extends Method {
+  SslWrapMethod() {
+    this.hasName("wrap") and
+    this.getDeclaringType().getASupertype*() instanceof SSLEngine
+  }
+}
+
+/** The `unwrap` method of the class `javax.net.ssl.SSLEngine`. */
+class SslUnwrapMethod extends Method {
+  SslUnwrapMethod() {
+    this.hasName("unwrap") and
+    this.getDeclaringType().getASupertype*() instanceof SSLEngine
+  }
+}
+
+/** The `getSession` method of the class `javax.net.ssl.SSLSession`.select */
+class GetSslSessionMethod extends Method {
+  GetSslSessionMethod() {
+    hasName("getSession") and
+    getDeclaringType().getASupertype*() instanceof SSLSession
+  }
+}
+
 bindingset[algorithmString]
 private string algorithmRegex(string algorithmString) {
   // Algorithms usually appear in names surrounded by characters that are not

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

@@ -0,0 +1,98 @@
+/** Provides classes and predicates to reason about unsafe certificate trust vulnerablities. */
+
+import java
+private import semmle.code.java.frameworks.Networking
+private import semmle.code.java.security.Encryption
+private import semmle.code.java.dataflow.DataFlow
+
+/**
+ * The creation of an object that prepares an SSL connection.
+ *
+ * This is a source for `SslEndpointIdentificationFlowConfig`.
+ */
+class SslConnectionInit extends DataFlow::Node {
+  SslConnectionInit() {
+    exists(MethodAccess ma, Method m |
+      this.asExpr() = ma and
+      ma.getMethod() = m
+    |
+      m instanceof CreateSslEngineMethod
+      or
+      m instanceof CreateSocketMethod and isSslSocket(ma)
+    )
+  }
+}
+
+/**
+ * A call to a method that establishes an SSL connection.
+ *
+ * This is a sink for `SslEndpointIdentificationFlowConfig`.
+ */
+class SslConnectionCreation extends DataFlow::Node {
+  SslConnectionCreation() {
+    exists(MethodAccess ma, Method m |
+      m instanceof BeginHandshakeMethod or
+      m instanceof SslWrapMethod or
+      m instanceof SslUnwrapMethod or
+      m instanceof SocketGetOutputStreamMethod
+    |
+      ma.getMethod() = m and
+      this.asExpr() = ma.getQualifier()
+    )
+  }
+}
+
+/**
+ * An SSL object that correctly verifies hostnames, or doesn't need to (for instance, because it's a server).
+ *
+ * This is a sanitizer for `SslEndpointIdentificationFlowConfig`.
+ */
+abstract class SslUnsafeCertTrustSanitizer extends DataFlow::Node { }
+
+/**
+ * An `SSLEngine` set in server mode.
+ */
+private class SslEngineServerMode extends SslUnsafeCertTrustSanitizer {
+  SslEngineServerMode() {
+    exists(MethodAccess ma, Method m |
+      m.hasName("setUseClientMode") and
+      m.getDeclaringType().getASupertype*() instanceof SSLEngine and
+      ma.getMethod() = m and
+      ma.getArgument(0).(CompileTimeConstantExpr).getBooleanValue() = false and
+      this.asExpr() = ma.getQualifier()
+    )
+  }
+}
+
+/**
+ * Holds if the return value of `createSocket` is cast to `SSLSocket`
+ * or the qualifier of `createSocket` is an instance of `SSLSocketFactory`.
+ */
+private predicate isSslSocket(MethodAccess createSocket) {
+  createSocket = any(CastExpr ce | ce.getType() instanceof SSLSocket).getExpr()
+  or
+  createSocket.getQualifier().getType().(RefType).getASupertype*() instanceof SSLSocketFactory
+}
+
+/**
+ * A call to a method that enables SSL (`useSslProtocol` or `setSslContextFactory`)
+ * on an instance of `com.rabbitmq.client.ConnectionFactory` that doesn't set `enableHostnameVerification`.
+ */
+class RabbitMQEnableHostnameVerificationNotSet extends MethodAccess {
+  RabbitMQEnableHostnameVerificationNotSet() {
+    this.getMethod().hasName(["useSslProtocol", "setSslContextFactory"]) and
+    this.getMethod().getDeclaringType() instanceof RabbitMQConnectionFactory and
+    exists(Variable v |
+      v.getType() instanceof RabbitMQConnectionFactory and
+      this.getQualifier() = v.getAnAccess() and
+      not exists(MethodAccess ma |
+        ma.getMethod().hasName("enableHostnameVerification") and
+        ma.getQualifier() = v.getAnAccess()
+      )
+    )
+  }
+}
+
+private class RabbitMQConnectionFactory extends RefType {
+  RabbitMQConnectionFactory() { this.hasQualifiedName("com.rabbitmq.client", "ConnectionFactory") }
+}

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

@@ -0,0 +1,65 @@
+/** Provides taint tracking configurations to be used by unsafe certificate trust queries. */
+
+import java
+import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.security.UnsafeCertTrust
+import semmle.code.java.security.Encryption
+
+/**
+ * A taint flow configuration for SSL connections created without a proper certificate trust configuration.
+ */
+class SslEndpointIdentificationFlowConfig extends TaintTracking::Configuration {
+  SslEndpointIdentificationFlowConfig() { this = "SslEndpointIdentificationFlowConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof SslConnectionInit }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof SslConnectionCreation }
+
+  override predicate isSanitizer(DataFlow::Node sanitizer) {
+    sanitizer instanceof SslUnsafeCertTrustSanitizer
+  }
+}
+
+/**
+ * An SSL object that was assigned a safe `SSLParameters` object and can be considered safe.
+ */
+private class SslConnectionWithSafeSslParameters extends SslUnsafeCertTrustSanitizer {
+  SslConnectionWithSafeSslParameters() {
+    exists(SafeSslParametersFlowConfig config, DataFlow::Node safe, DataFlow::Node sanitizer |
+      config.hasFlowTo(safe) and
+      sanitizer = DataFlow::exprNode(safe.asExpr().(Argument).getCall().getQualifier()) and
+      DataFlow::localFlow(sanitizer, this)
+    )
+  }
+}
+
+private class SafeSslParametersFlowConfig extends DataFlow2::Configuration {
+  SafeSslParametersFlowConfig() { this = "SafeSslParametersFlowConfig" }
+
+  override predicate isSource(DataFlow::Node source) {
+    exists(MethodAccess ma |
+      ma instanceof SafeSetEndpointIdentificationAlgorithm and
+      DataFlow::getInstanceArgument(ma) = source.(DataFlow::PostUpdateNode).getPreUpdateNode()
+    )
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(MethodAccess ma, RefType t | t instanceof SSLSocket or t instanceof SSLEngine |
+      ma.getMethod().hasName("setSSLParameters") and
+      ma.getMethod().getDeclaringType().getASupertype*() = t and
+      ma.getArgument(0) = sink.asExpr()
+    )
+  }
+}
+
+/**
+ * A call to `SSLParameters.setEndpointIdentificationAlgorithm` with a non-null and non-empty parameter.
+ */
+private class SafeSetEndpointIdentificationAlgorithm extends MethodAccess {
+  SafeSetEndpointIdentificationAlgorithm() {
+    this.getMethod().hasName("setEndpointIdentificationAlgorithm") and
+    this.getMethod().getDeclaringType() instanceof SSLParameters and
+    not this.getArgument(0) instanceof NullLiteral and
+    not this.getArgument(0).(CompileTimeConstantExpr).getStringValue() = ""
+  }
+}

java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.qhelp

@@ -0,0 +1,50 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>Java offers two mechanisms for SSL authentication - trust manager and hostname verifier (the later is checked by the <code>java/insecure-hostname-verifier</code> query). The trust manager validates the peer's certificate chain while hostname verification establishes that the hostname in the URL matches the hostname in the server's identification.</p>
+<p>When <code>SSLSocket</code> or <code>SSLEngine</code> are created without a secure <code>setEndpointIdentificationAlgorithm</code>, hostname verification is disabled by default.</p>
+<p>This query checks whether <code>setEndpointIdentificationAlgorithm</code> is missing, thereby making the application vulnerable to man-in-the-middle attacks. The query also covers insecure configurations of <code>com.rabbitmq.client.ConnectionFactory</code>.</p>
+</overview>
+
+<recommendation>
+<p>Validate SSL certificates in SSL authentication.</p>
+</recommendation>
+
+<example>
+<p>The following two examples show two ways of configuring SSLSocket/SSLEngine. In the 'BAD' case,
+<code>setEndpointIdentificationAlgorithm</code> is not called, thus no hostname verification takes place. In the 'GOOD' case, <code>setEndpointIdentificationAlgorithm</code> is called.</p>
+<sample src="UnsafeCertTrust.java" />
+</example>
+
+<references>
+<li>
+<a href="https://github.com/OWASP/owasp-mstg/blob/master/Document/0x05g-Testing-Network-Communication.md">Testing Endpoint Identify Verification (MSTG-NETWORK-3)</a>.
+</li>
+<li>
+  <a href="https://docs.oracle.com/en/java/javase/11/docs/api/java.base/javax/net/ssl/SSLParameters.html#setEndpointIdentificationAlgorithm(java.lang.String)">SSLParameters.setEndpointIdentificationAlgorithm documentation</a>.
+</li>
+<li>
+  RabbitMQ:
+  <a href="https://rabbitmq.github.io/rabbitmq-java-client/api/current/com/rabbitmq/client/ConnectionFactory.html#enableHostnameVerification()">ConnectionFactory.enableHostnameVerification documentation</a>.
+</li>
+<li>
+  RabbitMQ:
+  <a href="https://www.rabbitmq.com/ssl.html#java-client">Using TLS in the Java Client</a>.
+</li>
+<li>
+<a href="https://github.com/advisories/GHSA-xvch-r4wf-h8w9">CVE-2018-17187: Apache Qpid Proton-J transport issue with hostname verification</a>.
+</li>
+<li>
+<a href="https://github.com/advisories/GHSA-46j3-r4pj-4835">CVE-2018-8034: Apache Tomcat - host name verification when using TLS with the WebSocket client</a>.
+</li>
+<li>
+<a href="https://github.com/advisories/GHSA-w4g2-9hj6-5472">CVE-2018-11087: Pivotal Spring AMQP vulnerability due to lack of hostname validation</a>.
+</li>
+<li>
+<a href="https://github.com/advisories/GHSA-m9w8-v359-9ffr">CVE-2018-11775: TLS hostname verification issue when using the Apache ActiveMQ Client</a>.
+</li>
+</references>
+</qhelp>

java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql

@@ -0,0 +1,23 @@
+/**
+ * @name Unsafe certificate trust
+ * @description SSLSocket/SSLEngine ignores all SSL certificate validation
+ *              errors when establishing an HTTPS connection, thereby making
+ *              the app vulnerable to man-in-the-middle attacks.
+ * @kind problem
+ * @problem.severity warning
+ * @precision medium
+ * @id java/unsafe-cert-trust
+ * @tags security
+ *       external/cwe/cwe-273
+ */
+
+import java
+import semmle.code.java.security.UnsafeCertTrustQuery
+
+from Expr unsafeTrust
+where
+  unsafeTrust instanceof RabbitMQEnableHostnameVerificationNotSet or
+  exists(SslEndpointIdentificationFlowConfig config |
+    config.hasFlowTo(DataFlow::exprNode(unsafeTrust))
+  )
+select unsafeTrust, "Unsafe configuration of trusted certificates."

java/ql/src/change-notes/2021-06-28-unsafe-cert-trust-query.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* The query "Unsafe certificate trust" (`java/unsafe-cert-trust`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/3550).