Java: Add unsafe hostname verification query

java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.java

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

java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp

@@ -0,0 +1,45 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+If a <code>HostnameVerifier</code> always returns <code>true</code> it will not verify the hostname at all.
+This allows an attacker to perform a Man-in-the-middle attack against the application therefore breaking any security Transport Layer Security (TLS) gives.
+
+An attack would look like this:
+1. The program connects to <code>https://example.com</code>.
+2. The attacker intercepts this connection and presents one of their valid certificates they control, for example one from Let's Encrypt.
+3. Java verifies that the certificate has been issued by a trusted certificate authority.
+4. Java verifies that the certificate has been issued for the host <code>example.com</code>, which will fail because the certificate has been issued for <code>malicious.domain</code>.
+5. Java wants to reject the certificate because the hostname does not match. Before doing this it checks whether there exists a <code>HostnameVerifier</code>.
+6. Your <code>HostnameVerifier</code> is called which returns <code>true</code> for any certificate so also for this one.
+7. Java proceeds with the connection since your <code>HostnameVerifier</code> accepted it.
+8. The attacker can now read the data (Man-in-the-middle) your program sends to <code>https://example.com</code> while the program thinks the connection is secure.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Do NOT use an unverifying <code>HostnameVerifier</code>!
+<li>If you use an unverifying verifier to solve a configuration problem with TLS/HTTPS you should solve the configuration problem instead.
+</li>
+</p>
+
+</recommendation>
+
+<example>
+<p>
+In the first (bad) example, the <code>HostnameVerifier</code> always returns <code>true</code>.
+This allows an attacker to perform a man-in-the-middle attack, because any certificate is accepted despite an incorrect hostname.
+In the second (good) example, the <code>HostnameVerifier</code> only returns <code>true</code> when the certificate has been correctly checked.
+</p>
+<sample src="UnsafeHostnameVerification.java" />
+</example>
+
+<references>
+<li><a href="https://developer.android.com/training/articles/security-ssl">Android Security Guide for TLS/HTTPS</a>.</li>
+<li><a href="https://tersesystems.com/blog/2014/03/23/fixing-hostname-verification/">Further Information on Hostname Verification</a>.</li>
+<li>OWASP: <a href="https://cwe.mitre.org/data/definitions/297.html">CWE-297</a>.</li>
+</references>
+</qhelp>

java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql

@@ -0,0 +1,75 @@
+/**
+ * @name Disabled hostname verification
+ * @description Accepting any certificate as valid for a host allows an attacker to perform a man-in-the-middle attack.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/everything-accepting-hostname-verifier
+ * @tags security
+ *       external/cwe/cwe-297
+ */
+
+import java
+import semmle.code.java.security.Encryption
+import semmle.code.java.dataflow.DataFlow
+import DataFlow::PathGraph
+import semmle.code.java.controlflow.Guards
+
+/**
+ * Holds if `m` always returns `true` ignoring any exceptional flow.
+ */
+private predicate alwaysReturnsTrue(HostnameVerifierVerify m) {
+  forex(ReturnStmt rs | rs.getEnclosingCallable() = m |
+    rs.getResult().(CompileTimeConstantExpr).getBooleanValue() = true
+  )
+}
+
+/**
+ * A class that overrides the `javax.net.ssl.HostnameVerifier.verify` method and **always** returns `true`, thus
+ * accepting any certificate despite a hostname mismatch.
+ */
+class TrustAllHostnameVerifier extends RefType {
+  TrustAllHostnameVerifier() {
+    this.getASupertype*() instanceof HostnameVerifier and
+    exists(HostnameVerifierVerify m |
+      m.getDeclaringType() = this and
+      alwaysReturnsTrue(m)
+    )
+  }
+}
+
+/**
+ * A configuration to model the flow of a `TrustAllHostnameVerifier` to a `set(Default)HostnameVerifier` call.
+ */
+class TrustAllHostnameVerifierConfiguration extends DataFlow::Configuration {
+  TrustAllHostnameVerifierConfiguration() { this = "TrustAllHostnameVerifierConfiguration" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source.asExpr().(ClassInstanceExpr).getConstructedType() instanceof TrustAllHostnameVerifier
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(MethodAccess ma, Method m |
+      (m instanceof SetDefaultHostnameVerifierMethod or m instanceof SetHostnameVerifierMethod) and
+      ma.getMethod() = m
+    |
+      ma.getArgument(0) = sink.asExpr()
+    )
+  }
+}
+
+/** Holds if `node` is guarded by a flag that suggests an intentionally insecure feature. */
+private predicate isNodeGuardedByFlag(DataFlow::Node node) {
+  exists(Guard g | g.controls(node.asExpr().getBasicBlock(), _) |
+    g
+        .(VarAccess)
+        .getVariable()
+        .getName()
+        .regexpMatch("(?i).*(secure|(en|dis)able|selfCert|selfSign|validat|verif|trust|ignore).*")
+  )
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, TrustAllHostnameVerifierConfiguration cfg
+where cfg.hasFlowPath(source, sink) and not isNodeGuardedByFlag(sink.getNode())
+select sink, source, sink, "$@ that accepts any certificate as valid, is used here.", source,
+  "This hostname verifier"

java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected

@@ -0,0 +1,18 @@
+edges
+| UnsafeHostnameVerification.java:68:31:73:3 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:74:49:74:56 | verifier |
+| UnsafeHostnameVerification.java:77:69:82:2 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:33:50:33:76 | ALLOW_ALL_HOSTNAME_VERIFIER |
+nodes
+| UnsafeHostnameVerification.java:13:49:18:3 | new (...) | semmle.label | new (...) |
+| UnsafeHostnameVerification.java:25:49:25:65 | ...->... | semmle.label | ...->... |
+| UnsafeHostnameVerification.java:33:50:33:76 | ALLOW_ALL_HOSTNAME_VERIFIER | semmle.label | ALLOW_ALL_HOSTNAME_VERIFIER |
+| UnsafeHostnameVerification.java:46:49:46:65 | ...->... | semmle.label | ...->... |
+| UnsafeHostnameVerification.java:58:50:58:76 | ...->... | semmle.label | ...->... |
+| UnsafeHostnameVerification.java:68:31:73:3 | new (...) : new HostnameVerifier(...) { ... } | semmle.label | new (...) : new HostnameVerifier(...) { ... } |
+| UnsafeHostnameVerification.java:74:49:74:56 | verifier | semmle.label | verifier |
+| UnsafeHostnameVerification.java:77:69:82:2 | new (...) : new HostnameVerifier(...) { ... } | semmle.label | new (...) : new HostnameVerifier(...) { ... } |
+#select
+| UnsafeHostnameVerification.java:13:49:18:3 | new (...) | UnsafeHostnameVerification.java:13:49:18:3 | new (...) | UnsafeHostnameVerification.java:13:49:18:3 | new (...) | $@ that accepts any certificate as valid, is used here. | UnsafeHostnameVerification.java:13:49:18:3 | new (...) | This hostname verifier |
+| UnsafeHostnameVerification.java:25:49:25:65 | ...->... | UnsafeHostnameVerification.java:25:49:25:65 | ...->... | UnsafeHostnameVerification.java:25:49:25:65 | ...->... | $@ that accepts any certificate as valid, is used here. | UnsafeHostnameVerification.java:25:49:25:65 | ...->... | This hostname verifier |
+| UnsafeHostnameVerification.java:46:49:46:65 | ...->... | UnsafeHostnameVerification.java:46:49:46:65 | ...->... | UnsafeHostnameVerification.java:46:49:46:65 | ...->... | $@ that accepts any certificate as valid, is used here. | UnsafeHostnameVerification.java:46:49:46:65 | ...->... | This hostname verifier |
+| UnsafeHostnameVerification.java:58:50:58:76 | ...->... | UnsafeHostnameVerification.java:58:50:58:76 | ...->... | UnsafeHostnameVerification.java:58:50:58:76 | ...->... | $@ that accepts any certificate as valid, is used here. | UnsafeHostnameVerification.java:58:50:58:76 | ...->... | This hostname verifier |
+| UnsafeHostnameVerification.java:74:49:74:56 | verifier | UnsafeHostnameVerification.java:68:31:73:3 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:74:49:74:56 | verifier | $@ that accepts any certificate as valid, is used here. | UnsafeHostnameVerification.java:68:31:73:3 | new (...) : new HostnameVerifier(...) { ... } | This hostname verifier |

java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java

@@ -0,0 +1,84 @@
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.HttpsURLConnection;
+import javax.net.ssl.SSLSession;
+
+public class UnsafeHostnameVerification {
+
+	private static final boolean DISABLE_VERIFICATION = true;
+
+	/**
+	 * 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; // BAD, always returns true
+			}
+		});
+	}
+
+	/**
+	 * Test the implementation of trusting all hostnames as a lambda.
+	 */
+	public void testTrustAllHostnameLambda() {
+		HttpsURLConnection.setDefaultHostnameVerifier((name, s) -> true); // BAD, always returns true
+	}
+
+	/**
+	 * Test an all-trusting hostname verifier that is guarded by a flag
+	 */
+	public void testGuardedByFlagTrustAllHostname() {
+		if (DISABLE_VERIFICATION) {
+			HttpsURLConnection.setDefaultHostnameVerifier(ALLOW_ALL_HOSTNAME_VERIFIER); // GOOD: The all-trusting
+																						// hostname verifier is guarded
+																						// by a feature flag
+		}
+	}
+
+	public void testGuardedByFlagAccrossCalls() {
+		if (DISABLE_VERIFICATION) {
+			functionThatActuallyDisablesVerification();
+		}
+	}
+
+	private void functionThatActuallyDisablesVerification() {
+		HttpsURLConnection.setDefaultHostnameVerifier((name, s) -> true); // GOOD [but detected as BAD], because we only
+																			// check guards inside a function
+		// and not accross function calls. This is considerer GOOD because the call to
+		// `functionThatActuallyDisablesVerification` is guarded by a feature flag in
+		// `testGuardedByFlagAccrossCalls`.
+		// Although this is not ideal as another function could directly call
+		// `functionThatActuallyDisablesVerification` WITHOUT checking the feature flag.
+	}
+
+	public void testTrustAllHostnameDependingOnDerivedValue() {
+		String enabled = System.getProperty("disableHostnameVerification");
+		if (Boolean.parseBoolean(enabled)) {
+			HttpsURLConnection.setDefaultHostnameVerifier((hostname, session) -> true);  // GOOD [but detected as BAD].
+			                                                                   // This is GOOD, because it depends on a feature
+			                                                                   // flag, but this is not detected by the query.
+		}
+	}
+
+	/**
+	 * 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; // BAD, always returns true
+			}
+		};
+		HttpsURLConnection.setDefaultHostnameVerifier(verifier);
+	}
+
+	public static final HostnameVerifier ALLOW_ALL_HOSTNAME_VERIFIER = new HostnameVerifier() {
+		@Override
+		public boolean verify(String hostname, SSLSession session) {
+			return true; // BAD, always returns true
+		}
+	};
+}
+

java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.qlref

@@ -0,0 +1 @@
+Security/CWE/CWE-297/UnsafeHostnameVerification.ql