github · aschackmull · Jan 13, 2021 · Dec 4, 2020 · Dec 4, 2020 · Dec 4, 2020
@@ -0,0 +1,4 @@
+lgtm,codescanning
+* A new query "Unsafe hostname verification"
+  (`java/unsafe-hostname-verification`) has been added. This query finds unsafe
+  `HostnameVerifier`s that allow man-in-the-middle attacks.
@@ -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);
+	}
+
+}
@@ -0,0 +1,50 @@
+<!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 stops Transport Layer Security (TLS) providing any security and allows an attacker to perform a man-in-the-middle attack against the application.
+</p>
+
+<p>
+An attack might look like this:
+</p>
+
+<ol>
+  <li>The program connects to <code>https://example.com</code>.</li>
+  <li>The attacker intercepts this connection and presents an apparently-valid certificate of their choosing.</li>
+  <li>The <code>TrustManager</code> of the program verifies that the certificate has been issued by a trusted certificate authority.</li>
+  <li>The Java HTTPS library checks whether the certificate has been issued for the host <code>example.com</code>. This check fails because the certificate has been issued for a domain controlled by the attacker, for example: <code>malicious.domain</code>.</li>
+  <li>The HTTPS library wants to reject the certificate because the hostname does not match. Before doing this it checks whether a <code>HostnameVerifier</code> exists.</li>
+  <li>Your <code>HostnameVerifier</code> is called which returns <code>true</code> for any certificate so also for this one.</li>
+  <li>The program proceeds with the connection since your <code>HostnameVerifier</code> accepted it.</li>
+  <li>The attacker can now read the data your program sends to <code>https://example.com</code> 
+and/or alter its replies while the program thinks the connection is secure.</li>
+</ol>
+
+</overview>
+
+<recommendation>
+<p>
+Do not use an open <code>HostnameVerifier</code>.
+If you have a configuration problem with TLS/HTTPS, you should always solve the configuration problem instead of using an open verifier.
+</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>Android developers: <a href="https://developer.android.com/training/articles/security-ssl">Security with HTTPS and SSL</a>.</li>
+<li>Terse systems blog: <a href="https://tersesystems.com/blog/2014/03/23/fixing-hostname-verification/">Fixing Hostname Verification</a>.</li>
+</references>
+</qhelp>
@@ -0,0 +1,163 @@
+/**
+ * @name Unsafe hostname verification
+ * @description Marking a certificate as valid for a host without checking the certificate hostname allows an attacker to perform a machine-in-the-middle attack.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/unsafe-hostname-verification
+ * @tags security
+ *       external/cwe/cwe-297
+ */
+
+import java
+import semmle.code.java.controlflow.Guards
+import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.security.Encryption
+import DataFlow::PathGraph
+
+/**
+ * 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` (though it could also exit due to an uncaught exception), 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()
+    )
+  }
+
+  override predicate isBarrier(DataFlow::Node barrier) {
+    // ignore nodes that are in functions that intentionally disable hostname verification
+    barrier
+        .getEnclosingCallable()
+        .getName()
+        /*
+         * Regex: (_)* :
+         * some methods have underscores.
+         * Regex: (no|ignore|disable)(strictssl|ssl|verify|verification|hostname)
+         * noStrictSSL ignoreSsl
+         * Regex: (set)?(accept|trust|ignore|allow)(all|every|any)
+         * acceptAll trustAll ignoreAll setTrustAnyHttps
+         * Regex: (use|do|enable)insecure
+         * useInsecureSSL
+         * Regex: (set|do|use)?no.*(check|validation|verify|verification)
+         * setNoCertificateCheck
+         * Regex: disable
+         * disableChecks
+         */
+
+        .regexpMatch("^(?i)(_)*((no|ignore|disable)(strictssl|ssl|verify|verification|hostname)" +
+            "|(set)?(accept|trust|ignore|allow)(all|every|any)" +
+            "|(use|do|enable)insecure|(set|do|use)?no.*(check|validation|verify|verification)|disable).*$")
+  }
+}
+
+bindingset[result]
+private string getAFlagName() {
+  result
+      .regexpMatch("(?i).*(secure|disable|selfCert|selfSign|validat|verif|trust|ignore|nocertificatecheck).*")
+}
+
+/**
+ * A flag has to either be of type `String`, `boolean` or `Boolean`.
+ */
+private class FlagType extends Type {
+  FlagType() {
+    this instanceof TypeString
+    or
+    this instanceof BooleanType
+  }
+}
+
+private predicate isEqualsIgnoreCaseMethodAccess(MethodAccess ma) {
+  ma.getMethod().hasName("equalsIgnoreCase") and
+  ma.getMethod().getDeclaringType() instanceof TypeString
+}
+
+/** Holds if `source` should is considered a flag. */
+private predicate isFlag(DataFlow::Node source) {
+  exists(VarAccess v | v.getVariable().getName() = getAFlagName() |
+    source.asExpr() = v and v.getType() instanceof FlagType
+  )
+  or
+  exists(StringLiteral s | s.getRepresentedString() = getAFlagName() | source.asExpr() = s)
+  or
+  exists(MethodAccess ma | ma.getMethod().getName() = getAFlagName() |
+    source.asExpr() = ma and
+    ma.getType() instanceof FlagType and
+    not isEqualsIgnoreCaseMethodAccess(ma)
+  )
+}
+
+/** Holds if there is flow from `node1` to `node2` either due to local flow or due to custom flow steps. */
+private predicate flagFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+  DataFlow::localFlowStep(node1, node2)
+  or
+  exists(MethodAccess ma | ma.getMethod() = any(EnvReadMethod m) |
+    ma = node2.asExpr() and ma.getAnArgument() = node1.asExpr()
+  )
+  or
+  exists(MethodAccess ma |
+    ma.getMethod().hasName("parseBoolean") and
+    ma.getMethod().getDeclaringType().hasQualifiedName("java.lang", "Boolean")
+  |
+    ma = node2.asExpr() and ma.getAnArgument() = node1.asExpr()
+  )
+}
+
+/** Gets a guard that depends on a flag. */
+private Guard getAGuard() {
+  exists(DataFlow::Node source, DataFlow::Node sink |
+    isFlag(source) and
+    flagFlowStep*(source, sink) and
+    sink.asExpr() = result
+  )
+}
+
+/** 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 = getAGuard())
+}
+
+from
+  DataFlow::PathNode source, DataFlow::PathNode sink, TrustAllHostnameVerifierConfiguration cfg,
+  RefType verifier
+where
+  cfg.hasFlowPath(source, sink) and
+  not isNodeGuardedByFlag(sink.getNode()) and
+  verifier = source.getNode().asExpr().(ClassInstanceExpr).getConstructedType()
+select sink, source, sink,
+  "$@ that is defined $@ and accepts any certificate as valid, is used here.", source,
+  "This hostname verifier", verifier, "here"
@@ -1,30 +1,4 @@
 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() {

@@ -4,18 +4,18 @@
 <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>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>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>
+<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>
 </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,
+<p>The following two examples show two ways of configuring X509 trust cert manager. 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>

@@ -1,6 +1,6 @@
 /**
- * @name Unsafe certificate trust and improper hostname verification
- * @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.
+ * @name Unsafe certificate trust
+ * @description 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.
  * @kind problem
  * @id java/unsafe-cert-trust
  * @tags security
@@ -53,42 +53,6 @@ class X509TrustAllManagerInit extends MethodAccess {
   }
 }
 
-/**
- * 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 and
-        v.getInitializer().getType() instanceof TrustAllHostnameVerifier //Scenario of HttpsURLConnection.setDefaultHostnameVerifier(verifier);
-      )
-    )
-  }
-}
-
 class SSLEngine extends RefType {
   SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") }
 }
@@ -239,7 +203,6 @@ class RabbitMQEnableHostnameVerificationNotSet extends MethodAccess {
 
 from MethodAccess aa
 where
-  aa instanceof TrustAllHostnameVerify or
   aa instanceof X509TrustAllManagerInit or
   aa instanceof SSLEndpointIdentificationNotSet or
   aa instanceof RabbitMQEnableHostnameVerificationNotSet

@@ -226,7 +226,7 @@ class EnvInput extends LocalUserInput {
     )
     or
     // Results from various specific methods.
-    this.asExpr().(MethodAccess).getMethod() instanceof EnvTaintedMethod
+    this.asExpr().(MethodAccess).getMethod() instanceof EnvReadMethod
     or
     // Access to `System.in`.
     exists(Field f | this.asExpr() = f.getAnAccess() | f instanceof SystemIn)
@@ -292,8 +292,9 @@ private class SpringWebRequestGetMethod extends Method {
   }
 }
 
-private class EnvTaintedMethod extends Method {
-  EnvTaintedMethod() {
+/** A method that reads from the environment, such as `System.getProperty` or `System.getenv`. */
+class EnvReadMethod extends Method {
+  EnvReadMethod() {
     this instanceof MethodSystemGetenv or
     this instanceof PropertiesGetPropertyMethod or
     this instanceof MethodSystemGetProperty