github · aschackmull · Jun 29, 2020 · May 24, 2020 · May 28, 2020 · Jun 23, 2020
diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java
@@ -0,0 +1,101 @@
+public static void main(String[] args) {
+	{
+		HostnameVerifier verifier = new HostnameVerifier() {
+			@Override
+			public boolean verify(String hostname, SSLSession session) {
+				try { //GOOD: verify the certificate
+					Certificate[] certs = session.getPeerCertificates();
+					X509Certificate x509 = (X509Certificate) certs[0];
+					check(new String[]{host}, x509);
+					return true;
+				} catch (SSLException e) {
+					return false;
+				}
+			}
+		};
+		HttpsURLConnection.setDefaultHostnameVerifier(verifier);
+	}
+
+	{
+		HostnameVerifier verifier = new HostnameVerifier() {
+			@Override
+			public boolean verify(String hostname, SSLSession session) {
+				return true; // BAD: accept even if the hostname doesn't match
+			}
+		};
+		HttpsURLConnection.setDefaultHostnameVerifier(verifier);
+	}
+
+	{
+		X509TrustManager trustAllCertManager = new X509TrustManager() {
+			@Override
+			public void checkClientTrusted(final X509Certificate[] chain, final String authType)
+				throws CertificateException {
+			}
+
+			@Override
+			public void checkServerTrusted(final X509Certificate[] chain, final String authType)
+				throws CertificateException {
+				// BAD: trust any server cert
+			}
+
+			@Override
+			public X509Certificate[] getAcceptedIssuers() {
+				return null; //BAD: doesn't check cert issuer
+			}
+		};
+	}
+
+	{
+		X509TrustManager trustCertManager = new X509TrustManager() {
+			@Override
+			public void checkClientTrusted(final X509Certificate[] chain, final String authType)
+				throws CertificateException {
+			}
+
+			@Override
+			public void checkServerTrusted(final X509Certificate[] chain, final String authType)
+				throws CertificateException {
+				pkixTrustManager.checkServerTrusted(chain, authType); //GOOD: validate the server cert
+			}
+
+			@Override
+			public X509Certificate[] getAcceptedIssuers() {
+				return new X509Certificate[0]; //GOOD: Validate the cert issuer
+			}
+		};
+	}
+
+	{
+		SSLContext sslContext = SSLContext.getInstance("TLS");
+		SSLEngine sslEngine = sslContext.createSSLEngine();
+		SSLParameters sslParameters = sslEngine.getSSLParameters();
+		sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); //GOOD: Set a valid endpointIdentificationAlgorithm for SSL engine to trigger hostname verification
+		sslEngine.setSSLParameters(sslParameters);
+	}
+
+	{
+		SSLContext sslContext = SSLContext.getInstance("TLS");
+		SSLEngine sslEngine = sslContext.createSSLEngine();  //BAD: No endpointIdentificationAlgorithm set
+	}
+
+	{
+		SSLContext sslContext = SSLContext.getInstance("TLS");
+		final SSLSocketFactory socketFactory = sslContext.getSocketFactory();
+		SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443); 
+		SSLParameters sslParameters = sslEngine.getSSLParameters();
+		sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); //GOOD: Set a valid endpointIdentificationAlgorithm for SSL socket to trigger hostname verification
+		socket.setSSLParameters(sslParameters);
+	}
+
+	{
+		com.rabbitmq.client.ConnectionFactory connectionFactory = new com.rabbitmq.client.ConnectionFactory();
+		connectionFactory.useSslProtocol();
+		connectionFactory.enableHostnameVerification();  //GOOD: Enable hostname verification for rabbitmq ConnectionFactory
+	}
+
+	{
+		com.rabbitmq.client.ConnectionFactory connectionFactory = new com.rabbitmq.client.ConnectionFactory();
+		connectionFactory.useSslProtocol(); //BAD: Hostname verification for rabbitmq ConnectionFactory is not enabled
+	}
+}
diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp
@@ -0,0 +1,46 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>Java offers two mechanisms for SSL authentication - trust manager and hostname verifier. 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>And when SSLSocket or SSLEngine is created without a valid parameter of setEndpointIdentificationAlgorithm, hostname verification is disabled by default.</p>
+<p>Unsafe implementation of the interface X509TrustManager, HostnameVerifier, and SSLSocket/SSLEngine ignores all SSL certificate validation errors when establishing an HTTPS connection, thereby making the app vulnerable to man-in-the-middle attacks.</p>
+<p>This query checks whether trust manager is set to trust all certificates, the hostname verifier is turned off, or setEndpointIdentificationAlgorithm is missing. The query also covers a special implementation com.rabbitmq.client.ConnectionFactory.</p>
+</overview>
+
+<recommendation>
+<p>Validate SSL certificate in SSL authentication.</p>
+</recommendation>
+
+<example>
+<p>The following two examples show two ways of configuring X509 trust cert manager and hostname verifier. In the 'BAD' case,
+no validation is performed thus any certificate is trusted. In the 'GOOD' case, the proper validation is performed.</p>
+<sample src="UnsafeCertTrust.java" />
+</example>
+
+<references>
+<li>
+<a href="https://cwe.mitre.org/data/definitions/273.html">CWE-273</a>
+</li>
+<li>
+<a href="https://support.google.com/faqs/answer/6346016?hl=en">How to fix apps containing an unsafe implementation of TrustManager</a>
+</li>
+<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://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>
diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql
@@ -0,0 +1,228 @@
+/**
+ * @name Unsafe implementation of trusting any certificate or missing hostname verification in SSL configuration
+ * @description Unsafe implementation of the interface X509TrustManager, HostnameVerifier, and 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
+ * @id java/unsafe-cert-trust
+ * @tags security
+ *       external/cwe-273
+ */
+
+import java
+import semmle.code.java.security.Encryption
+
+/**
+ * X509TrustManager class that blindly trusts all certificates in server SSL authentication
+ */
+class X509TrustAllManager extends RefType {
+  X509TrustAllManager() {
+    this.getASupertype*() instanceof X509TrustManager and
+    exists(Method m1 |
+      m1.getDeclaringType() = this and
+      m1.hasName("checkServerTrusted") and
+      m1.getBody().getNumStmt() = 0
+    ) and
+    exists(Method m2, ReturnStmt rt2 |
+      m2.getDeclaringType() = this and
+      m2.hasName("getAcceptedIssuers") and
+      rt2.getEnclosingCallable() = m2 and
+      rt2.getResult() instanceof NullLiteral
+    )
+  }
+}
+
+/**
+ * The init method of SSLContext with the trust all manager, which is sslContext.init(..., serverTMs, ...)
+ */
+class X509TrustAllManagerInit extends MethodAccess {
+  X509TrustAllManagerInit() {
+    this.getMethod().hasName("init") and
+    this.getMethod().getDeclaringType() instanceof SSLContext and //init method of SSLContext
+    (
+      exists(ArrayInit ai |
+        ai.getInit(0).(VarAccess).getVariable().getInitializer().getType().(Class).getASupertype*()
+          instanceof X509TrustAllManager //Scenario of context.init(null, new TrustManager[] { TRUST_ALL_CERTIFICATES }, null);
+      )
+      or
+      exists(Variable v, ArrayInit ai |
+        this.getArgument(1).(VarAccess).getVariable() = v and
+        ai.getParent() = v.getAnAccess().getVariable().getAnAssignedValue() and
+        ai.getInit(0).getType().(Class).getASupertype*() instanceof X509TrustAllManager //Scenario of context.init(null, serverTMs, null);
+      )
+    )
+  }
+}
+
+/**
+ * HostnameVerifier class that allows a certificate whose CN (Common Name) does not match the host name in the URL
+ */
+class TrustAllHostnameVerifier extends RefType {
+  TrustAllHostnameVerifier() {
+    this.getASupertype*() instanceof HostnameVerifier and
+    exists(Method m, ReturnStmt rt |
+      m.getDeclaringType() = this and
+      m.hasName("verify") and
+      rt.getEnclosingCallable() = m and
+      rt.getResult().(BooleanLiteral).getBooleanValue() = true
+    )
+  }
+}
+
+/**
+ * The setDefaultHostnameVerifier method of HttpsURLConnection with the trust all configuration
+ */
+class TrustAllHostnameVerify extends MethodAccess {
+  TrustAllHostnameVerify() {
+    this.getMethod().hasName("setDefaultHostnameVerifier") and
+    this.getMethod().getDeclaringType() instanceof HttpsURLConnection and //httpsURLConnection.setDefaultHostnameVerifier method
+    (
+      exists(NestedClass nc |
+        nc.getASupertype*() instanceof TrustAllHostnameVerifier and
+        this.getArgument(0).getType() = nc //Scenario of HttpsURLConnection.setDefaultHostnameVerifier(new HostnameVerifier() {...});
+      )
+      or
+      exists(Variable v |
+        this.getArgument(0).(VarAccess).getVariable() = v.getAnAccess().getVariable() and
+        v.getInitializer().getType() instanceof TrustAllHostnameVerifier //Scenario of HttpsURLConnection.setDefaultHostnameVerifier(verifier);
+      )
+    )
+  }
+}
+
+class SSLEngine extends RefType {
+  SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") }
+}
+
+class Socket extends RefType {
+  Socket() { this.hasQualifiedName("java.net", "Socket") }
+}
+
+class SSLSocket extends RefType {
+  SSLSocket() { this.hasQualifiedName("javax.net.ssl", "SSLSocket") }
+}
+
+/**
+ * has setEndpointIdentificationAlgorithm set correctly
+ */
+predicate setEndpointIdentificationAlgorithm(MethodAccess createSSL) {
+  exists(
+    Variable sslo, MethodAccess ma, Variable sslparams //setSSLParameters with valid setEndpointIdentificationAlgorithm set
+  |
+    createSSL = sslo.getAnAssignedValue() and
+    ma.getQualifier() = sslo.getAnAccess() and
+    ma.getMethod().hasName("setSSLParameters") and
+    ma.getArgument(0).(VarAccess) = sslparams.getAnAccess() and
+    exists(MethodAccess setepa |
+      setepa.getQualifier().(VarAccess) = sslparams.getAnAccess() and
+      setepa.getMethod().hasName("setEndpointIdentificationAlgorithm") and
+      not setepa.getArgument(0) instanceof NullLiteral
+    )
+  )
+}
+
+/**
+ * has setEndpointIdentificationAlgorithm set correctly
+ */
+predicate hasEndpointIdentificationAlgorithm(Variable ssl) {
+  exists(
+    MethodAccess ma, Variable sslparams //setSSLParameters with valid setEndpointIdentificationAlgorithm set
+  |
+    ma.getQualifier() = ssl.getAnAccess() and
+    ma.getMethod().hasName("setSSLParameters") and
+    ma.getArgument(0).(VarAccess) = sslparams.getAnAccess() and
+    exists(MethodAccess setepa |
+      setepa.getQualifier().(VarAccess) = sslparams.getAnAccess() and
+      setepa.getMethod().hasName("setEndpointIdentificationAlgorithm") and
+      not setepa.getArgument(0) instanceof NullLiteral
+    )
+  )
+}
+
+/**
+ * SSL object is created in a separate method call or in the same method
+ */
+predicate hasFlowPath(MethodAccess createSSL, Variable ssl) {
+  (
+    createSSL = ssl.getAnAssignedValue()
+    or
+    exists(CastExpr ce |
+      ce.getExpr().(MethodAccess) = createSSL and
+      ce.getControlFlowNode().getASuccessor().(VariableAssign).getDestVar() = ssl //With a type cast like SSLSocket socket = (SSLSocket) socketFactory.createSocket("www.example.com", 443);
+    )
+  )
+  or
+  exists(MethodAccess tranm |
+    createSSL.getEnclosingCallable().(Method) = tranm.getMethod() and
+    tranm.getControlFlowNode().getASuccessor().(VariableAssign).getDestVar() = ssl and
+    not setEndpointIdentificationAlgorithm(createSSL) //Check the scenario of invocation before used in the current method
+  )
+}
+
+/**
+ * Not have the SSLParameter set
+ */
+predicate hasNoEndpointIdentificationSet(MethodAccess createSSL, Variable ssl) {
+  //No setSSLParameters set
+  hasFlowPath(createSSL, ssl) and
+  not exists(MethodAccess ma |
+    ma.getQualifier() = ssl.getAnAccess() and
+    ma.getMethod().hasName("setSSLParameters")
+  )
+  or
+  //No endpointIdentificationAlgorithm set with setSSLParameters
+  hasFlowPath(createSSL, ssl) and
+  not setEndpointIdentificationAlgorithm(createSSL)
+}
+
+/**
+ * The setEndpointIdentificationAlgorithm method of SSLParameters with the ssl engine or socket
+ */
+class SSLEndpointIdentificationNotSet extends MethodAccess {
+  SSLEndpointIdentificationNotSet() {
+    (
+      this.getMethod().hasName("createSSLEngine") and
+      this.getMethod().getDeclaringType() instanceof SSLContext //createEngine method of SSLContext
+      or
+      this.getMethod().hasName("createSocket") and
+      this.getMethod().getReturnType() instanceof Socket //createSocket method of SSLSocketFactory
+    ) and
+    exists(Variable ssl |
+      hasNoEndpointIdentificationSet(this, ssl) and //Not set in itself
+      not exists(VariableAssign ar, Variable newSsl |
+        ar.getSource() = this.getCaller().getAReference() and
+        ar.getDestVar() = newSsl and
+        hasEndpointIdentificationAlgorithm(newSsl) //Not set in its caller either
+      )
+    ) and
+    not exists(MethodAccess ma | ma.getMethod() instanceof HostnameVerifierVerify) //Reduce false positives since this method access set default hostname verifier
+  }
+}
+
+class RabbitMQConnectionFactory extends RefType {
+  RabbitMQConnectionFactory() { this.hasQualifiedName("com.rabbitmq.client", "ConnectionFactory") }
+}
+
+/**
+ * The com.rabbitmq.client.ConnectionFactory useSslProtocol method access without enableHostnameVerification
+ */
+class RabbitMQEnableHostnameVerificationNotSet extends MethodAccess {
+  RabbitMQEnableHostnameVerificationNotSet() {
+    this.getMethod().hasName("useSslProtocol") and
+    this.getMethod().getDeclaringType() instanceof RabbitMQConnectionFactory and
+    exists(VarAccess va |
+      va.getVariable().getType() instanceof RabbitMQConnectionFactory and
+      this.getQualifier() = va.getVariable().getAnAccess() and
+      not exists(MethodAccess ma |
+        ma.getMethod().hasName("enableHostnameVerification") and
+        ma.getQualifier() = va.getVariable().getAnAccess()
+      )
+    )
+  }
+}
+
+from MethodAccess aa
+where
+  aa instanceof TrustAllHostnameVerify or
+  aa instanceof X509TrustAllManagerInit or
+  aa instanceof SSLEndpointIdentificationNotSet or
+  aa instanceof RabbitMQEnableHostnameVerificationNotSet
+select aa, "Unsafe configuration of trusted certificates"
diff --git a/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.expected b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.expected
@@ -0,0 +1,7 @@
+| UnsafeCertTrustTest.java:26:4:26:74 | init(...) | Unsafe configuration of trusted certificates |
+| UnsafeCertTrustTest.java:41:4:41:38 | init(...) | Unsafe configuration of trusted certificates |
+| UnsafeCertTrustTest.java:54:3:59:4 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates |
+| UnsafeCertTrustTest.java:72:3:72:57 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates |
+| UnsafeCertTrustTest.java:123:25:123:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates |
+| UnsafeCertTrustTest.java:134:25:134:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates |
+| UnsafeCertTrustTest.java:143:34:143:83 | createSocket(...) | Unsafe configuration of trusted certificates |
diff --git a/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.qlref b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.qlref
@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql