Java: CWE-273 Unsafe certificate trust

github · aschackmull · Jun 29, 2020 · May 24, 2020 · May 28, 2020 · Jun 23, 2020
commit 6d1ba3f8997a633af8fe93fe3c6e53e94966b5ef
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,68 @@
+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
+			}
+		};
+	}
+}
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,33 @@
+<!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>Unsafe implementation of the interface X509TrustManager and HostnameVerifier 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 the hostname verifier is turned off.</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>
+</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,95 @@
+/**
+ * @id java/unsafe-cert-trust
+ * @name Unsafe implementation of trusting any certificate in SSL configuration
+ * @description Unsafe implementation of the interface X509TrustManager and HostnameVerifier ignores all SSL certificate validation errors when establishing an HTTPS connection, thereby making the app vulnerable to man-in-the-middle attacks.
+ * @kind problem
+ * @tags security
+ *       external/cwe-273
+ */
+
+import java
+import semmle.code.java.security.Encryption
+import semmle.code.java.dataflow.DataFlow
+import DataFlow
+
+/**
+ * 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);
+      )
+    )
+  }
+}
+
+from MethodAccess aa
+where aa instanceof TrustAllHostnameVerify or aa instanceof X509TrustAllManagerInit
+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,4 @@
+| UnsafeCertTrustTest.java:19:4:19:74 | init(...) | Unsafe configuration of trusted certificates |
+| UnsafeCertTrustTest.java:34:4:34:38 | init(...) | Unsafe configuration of trusted certificates |
+| UnsafeCertTrustTest.java:47:3:52:4 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates |
+| UnsafeCertTrustTest.java:65:3:65:57 | setDefaultHostnameVerifier(...) | 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
diff --git a/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java
@@ -0,0 +1,110 @@
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.HttpsURLConnection;
+import javax.net.ssl.SSLSession;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLSocketFactory;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.X509TrustManager;
+import java.security.cert.CertificateException;
+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("SSL");
+			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("SSL");
+			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);
+		}
+	}
+
+	/**
+	 * Test the implementation of trusting all hostnames as an anonymous class
+	 */
+	public void testTrustAllHostnameOfAnonymousClass() {
+		HttpsURLConnection.setDefaultHostnameVerifier(new HostnameVerifier() {
+			@Override
+			public boolean verify(String hostname, SSLSession session) {
+				return true; // Noncompliant
+			}
+		});
+	}
+
+	/**
+	 * Test the implementation of trusting all hostnames as a variable
+	 */
+	public void testTrustAllHostnameOfVariable() {
+		HostnameVerifier verifier = new HostnameVerifier() {
+			@Override
+			public boolean verify(String hostname, SSLSession session) {
+				return true; // Noncompliant
+			}
+		};
+		HttpsURLConnection.setDefaultHostnameVerifier(verifier);
+	}
+
+	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
+		}
+	};
+
+	public static final HostnameVerifier ALLOW_ALL_HOSTNAME_VERIFIER = new HostnameVerifier() {
+		@Override
+		public boolean verify(String hostname, SSLSession session) {
+			return true; // Noncompliant
+		}
+	};
+}
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql