Java: Remove overlapping code

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

@@ -1,45 +1,5 @@
 public static void main(String[] args) {
 
-	{
-		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();

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

@@ -4,10 +4,9 @@
 <qhelp>
 
 <overview>
-<p>Java offers two mechanisms for SSL authentication - trust manager and hostname verifier (checked by the <code>java/insecure-hostname-verifier</code> query). 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 SSLSocket or SSLEngine is created without a valid parameter of setEndpointIdentificationAlgorithm, hostname verification is disabled by default.</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 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 or setEndpointIdentificationAlgorithm is missing. The query also covers a special implementation com.rabbitmq.client.ConnectionFactory.</p>
+<p>This query checks whether setEndpointIdentificationAlgorithm is missing. The query also covers a special implementation com.rabbitmq.client.ConnectionFactory.</p>
 </overview>
 
 <recommendation>
@@ -15,8 +14,8 @@
 </recommendation>
 
 <example>
-<p>The following two examples show two ways of configuring X509 trust cert manager. In the 'BAD' case,
+<p>The following two examples show two ways of configuring SSLSocket/SSLEngine. In the 'BAD' case,
-no validation is performed thus any certificate is trusted. In the 'GOOD' case, the proper validation is performed.</p>
+setEndpointIdentificationAlgorithm is not called, thus no hostname verification takes place. In the 'GOOD' case, setEndpointIdentificationAlgorithm is called.</p>
 <sample src="UnsafeCertTrust.java" />
 </example>
 
@@ -25,9 +24,6 @@ no validation is performed thus any certificate is trusted. In the 'GOOD' case,
 <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>

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

@@ -1,7 +1,6 @@
 /**
  * @name Unsafe certificate trust
- * @description Unsafe implementation of the interface X509TrustManager and
+ * @description SSLSocket/SSLEngine ignores all SSL certificate validation
- *              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
@@ -15,49 +14,6 @@
 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 |
-        this.getArgument(1).(ArrayCreationExpr).getInit() = ai and
-        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.getAnAssignedValue() and
-        ai.getInit(0).getType().(Class).getASupertype*() instanceof X509TrustAllManager //Scenario of context.init(null, serverTMs, null);
-      )
-    )
-  }
-}
-
 class SSLEngine extends RefType {
   SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") }
 }
@@ -208,7 +164,6 @@ class RabbitMQEnableHostnameVerificationNotSet extends MethodAccess {
 
 from MethodAccess aa
 where
-  aa instanceof X509TrustAllManagerInit or
   aa instanceof SSLEndpointIdentificationNotSet or
   aa instanceof RabbitMQEnableHostnameVerificationNotSet
 select aa, "Unsafe configuration of trusted certificates"

java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java

@@ -18,72 +18,6 @@ import java.security.cert.X509Certificate;
 
 public class UnsafeCertTrustTest {
 
-	/**
-	 * Test the implementation of trusting all server certs as a variable
-	 */
-	public SSLSocketFactory testTrustAllCertManager() {
-		try {
-			final SSLContext context = SSLContext.getInstance("TLS");
-			context.init(null, new TrustManager[] { TRUST_ALL_CERTIFICATES }, null);
-			final SSLSocketFactory socketFactory = context.getSocketFactory();
-			return socketFactory;
-		} catch (final Exception x) {
-			throw new RuntimeException(x);
-		}
-	}
-
-	/**
-	 * Test the implementation of trusting all server certs as an anonymous class
-	 */
-	public SSLSocketFactory testTrustAllCertManagerOfVariable() {
-		try {
-			SSLContext context = SSLContext.getInstance("TLS");
-			TrustManager[] serverTMs = new TrustManager[] { new X509TrustAllManager() };
-			context.init(null, serverTMs, null);
-
-			final SSLSocketFactory socketFactory = context.getSocketFactory();
-			return socketFactory;
-		} catch (final Exception x) {
-			throw new RuntimeException(x);
-		}
-	}
-
-	private static final X509TrustManager TRUST_ALL_CERTIFICATES = 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 {
-			// Noncompliant
-		}
-
-		@Override
-		public X509Certificate[] getAcceptedIssuers() {
-			return null; // Noncompliant
-		}
-	};
-
-	private class X509TrustAllManager implements 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 {
-			// Noncompliant
-		}
-
-		@Override
-		public X509Certificate[] getAcceptedIssuers() {
-			return null; // Noncompliant
-		}
-	};
-
 	/**
 	 * Test the endpoint identification of SSL engine is set to null
 	 */