[Java] CWE-295 - Incorrect Hostname Verification

java/ql/src/experimental/Security/CWE/CWE-295/AlwaysTrueHostnameVerifier.java

@@ -0,0 +1,6 @@
+HostnameVerifier hostnameVerifier = new HostnameVerifier() {
+    @Override
+    public boolean verify(String hostname, SSLSession session) {
+        return true; // BAD, verifies anything as correct
+    }
+};

java/ql/src/experimental/Security/CWE/CWE-295/AndroidHostnameVerifier.LICENSE

@@ -0,0 +1,38 @@
+// Licensed under Apache 2.0 per https://developer.android.com/license
+/*
+Copyright 2020 Android/Android
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+
+// Create an HostnameVerifier that hardwires the expected hostname.
+// Note that is different than the URL's hostname:
+// example.com versus example.org
+HostnameVerifier hostnameVerifier = new HostnameVerifier() {
+    @Override
+    public boolean verify(String hostname, SSLSession session) {
+        HostnameVerifier hv =
+            HttpsURLConnection.getDefaultHostnameVerifier();
+        return hv.verify("example.com", session); // OKAY-ISH: verify that the certificate matches
+        // example.com and only accept example.com as an alternative to example.org
+        // BETTER: fix the underlying configuration problem that causes example.org to present the certificate for the wrong domain.
+    }
+};
+
+// Tell the URLConnection to use our HostnameVerifier
+URL url = new URL("https://example.org/");
+HttpsURLConnection urlConnection =
+    (HttpsURLConnection)url.openConnection();
+urlConnection.setHostnameVerifier(hostnameVerifier);
+InputStream in = urlConnection.getInputStream();

java/ql/src/experimental/Security/CWE/CWE-295/AndroidHostnameVerifier.java

@@ -0,0 +1,20 @@
+// Create an HostnameVerifier that hardwires the expected hostname.
+// Note that is different than the URL's hostname:
+// example.com versus example.org
+HostnameVerifier hostnameVerifier = new HostnameVerifier() {
+    @Override
+    public boolean verify(String hostname, SSLSession session) {
+        HostnameVerifier hv =
+            HttpsURLConnection.getDefaultHostnameVerifier();
+        return hv.verify("example.com", session); // OKAY-ISH: verify that the certificate matches
+        // example.com and only accept example.com as an alternative to example.org
+        // BETTER: fix the underlying configuration problem that causes example.org to present the certificate for the wrong domain.
+    }
+};
+
+// Tell the URLConnection to use our HostnameVerifier
+URL url = new URL("https://example.org/");
+HttpsURLConnection urlConnection =
+    (HttpsURLConnection)url.openConnection();
+urlConnection.setHostnameVerifier(hostnameVerifier);
+InputStream in = urlConnection.getInputStream();

java/ql/src/experimental/Security/CWE/CWE-295/EverythingAcceptingHostnameVerifier.qhelp

@@ -0,0 +1,46 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+If a HostnameVerifier 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 on 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 HostnameVerifier!
+<li>If you use an unverifying verifier to solve a configuration problem with TLS/HTTPS you should solve the configuration problem instead.
+</li>
+<li>If you use an unverifying verifier to connect to a local host you should instead create a self-signed certificate for your local host and import that certificate to the java key store</li>
+If you do not verify the hostname, you allow any attacker to perform a man-in-the-middle attack.
+</p>
+
+</recommendation>
+
+<example>
+<p>
+In the first 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.
+</p>
+<sample src="AlwaysTrueHostnameVerifier.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/295.html">CWE-295</a>.</li>
+</references>
+</qhelp>

java/ql/src/experimental/Security/CWE/CWE-295/EverythingAcceptingHostnameVerifier.ql

@@ -0,0 +1,17 @@
+/**
+ * @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 alert
+ * @problem.severity error
+ * @precision high
+ * @id java/everything-accepting-hostname-verifier
+ * @tags security
+ *       external/cwe/cwe-295
+ */
+
+import java
+import HostnameValidation
+
+from AlwaysAcceptingVerifyMethod m
+where not m.getDeclaringType() instanceof TestClass
+select m, "$@ accepts any certificate as valid for a host.", m, "This method"

java/ql/src/experimental/Security/CWE/CWE-295/HostnameValidation.qll

@@ -0,0 +1,201 @@
+import java
+import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.security.Encryption
+
+/** A method that overrides the `javax.net.ssl.HostnameVerifier.verify` method. */
+class OverridenVerifyMethod extends HostnameVerifierVerify {
+  OverridenVerifyMethod() { this instanceof HostnameVerifierVerify }
+
+  /**
+   * The `hostname` parameter of this method.
+   */
+  Parameter getHostnameParameter() { result = getParameter(0) }
+
+  /**
+   * The `session` parameter of this method.
+   */
+  Parameter getSessionParameter() { result = getParameter(1) }
+}
+
+/** A return statement that returns `true`. */
+class TrueReturnStmt extends ReturnStmt {
+  TrueReturnStmt() { getResult().(CompileTimeConstantExpr).getBooleanValue() = true }
+}
+
+/**
+ * Holds if `m` always returns `true` ignoring any exceptional flow.
+ */
+private predicate alwaysReturnsTrue(OverridenVerifyMethod m) {
+  forex(ReturnStmt rs | rs.getEnclosingCallable() = m | rs instanceof TrueReturnStmt)
+}
+
+/** A method that overrides the `javax.net.ssl.HostnameVerifier.verify` method in a dangerous way. */
+abstract class DangerousVerifyMethod extends OverridenVerifyMethod {
+  /** A `Stmt` that makes this `HostnameVerifier` dangerous. */
+  abstract Stmt getADangerousStmt();
+}
+
+/**
+ * A method that overrides the `javax.net.ssl.HostnameVerifier.verify` method and **always** returns `true`, thus
+ * accepting any certificate despite a hostname mismatch.
+ */
+class AlwaysAcceptingVerifyMethod extends DangerousVerifyMethod {
+  AlwaysAcceptingVerifyMethod() { alwaysReturnsTrue(this) }
+
+  override Stmt getADangerousStmt() {
+    exists(TrueReturnStmt r | r.getEnclosingCallable() = this | result = r)
+  }
+}
+
+/** An access to any method whose name starts with `equals` and which is defined in the `String` class. */
+class StringEqualsMethodAccess extends MethodAccess {
+  StringEqualsMethodAccess() {
+    getMethod().getName().matches("equals%") and
+    getMethod().getDeclaringType() instanceof TypeString
+  }
+
+  // TODO there is probably a better name for "anEqualityPart" but I don't know any right now
+  /** Returns a part of this equality check, i.e. in the case of `foo.equals(bar)` this returns `foo` and `bar`. */
+  Expr getAnEqualityPart() { result = getQualifier() or result = getArgument(0) }
+}
+
+/**
+ * Get the nearest enclosing if statement, if there exists one.
+ * For example consider this case:
+ * ````
+ * if(foo) {
+ *   if(bar) {
+ *     stmt
+ *   }
+ * }
+ * ````
+ * This query would then return the `bar` if statement and **not** the `foo` if statement.
+ */
+private IfStmt getNearestEnclosingIfStmt(Stmt stmt) {
+  //result = getNerestEnclosingIfStmtRecursive(stmt) and result != stmt
+  exists(IfStmt ifStmt |
+    result = ifStmt and
+    (
+      ifStmt.getElse().getAChild*() = stmt and
+      /*
+       * Ensure that the `IfStmt` we found does not contain another `IfStmt`.
+       * If it contains one, the result can not be the nearest enclosing `IfStmt`.
+       */
+
+      not any(ifStmt.getElse().getAChild*()) instanceof IfStmt
+      or
+      ifStmt.getThen().getAChild*() = stmt and
+      /*
+       * Ensure that the `IfStmt` we found does not contain another `IfStmt`.
+       * If it contains one, the result can not be the nearest enclosing `IfStmt`.
+       */
+
+      not any(ifStmt.getThen().getAChild*()) instanceof IfStmt
+    )
+  )
+}
+
+/** The `javax.net.ssl.SSLSession.getPeerHost` method. */
+private class SSLSessionGetPeerHostMethodAccess extends MethodAccess {
+  SSLSessionGetPeerHostMethodAccess() {
+    getMethod().hasName("getPeerHost") and
+    getMethod().getDeclaringType() instanceof SSLSession
+  }
+}
+
+/**
+ * A `TaintTracking::Configuration` where the `source` is a parameter.
+ * This configuration considers any getter on `SSLSession` as being tainted, but ignores any calls to `SSLSession.getPeerHost`.
+ */
+/*
+ * private class ParameterTaintTracking extends TaintTracking::Configuration {
+ *  ParameterTaintTracking() { this = "ParameterTaintTracking" }
+ */
+
+/*override */ predicate isSource(DataFlow::Node source) {
+  source instanceof DataFlow::ParameterNode
+}
+
+/*override */ predicate isSink(DataFlow::Node sink) { sink instanceof DataFlow::ExprNode }
+
+/*override */ predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+  exists(MethodAccess ma |
+    ma.getMethod().getName().matches("get%") and
+    ma.getMethod().getDeclaringType() instanceof SSLSession and
+    not ma instanceof SSLSessionGetPeerHostMethodAccess
+  |
+    node2.asExpr() = ma and
+    ma.getQualifier() = node1.asExpr()
+  )
+}
+
+class TaintedQualifierTaintStep extends TaintTracking::AdditionalTaintStep {
+  override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
+    isAdditionalTaintStep(node1, node2)
+  }
+}
+
+/*
+ * override predicate isSanitizer(DataFlow::Node node) {
+ *    node.asExpr() instanceof SSLSessionGetPeerHostMethodAccess
+ *  }
+ * }
+ */
+
+/** Holds if `e` is derived from `parameter`. This is approximated by checking whether `e` gets tainted by `parameter`. */
+private predicate isDerivedFromParameter(Parameter parameter, Expr e) {
+  exists(DataFlow::ExprNode sink, DataFlow::ParameterNode source |
+    source = DataFlow::parameterNode(parameter) and sink = DataFlow::exprNode(e)
+  |
+    TaintTracking::localTaint(source, sink)
+  )
+}
+
+/**
+ * A (dangerous) `StringEqualsMethodAccess` where the equality does:
+ * 1. not depend on a expression that is derived from the `session` parameter of the `verify` method.
+ * 2. depend on a expression that is derived from the `hostname` parameter of the `verify` method.
+ */
+class SSLSessionParameterIgnoringStringEqualsMethodAccess extends StringEqualsMethodAccess {
+  SSLSessionParameterIgnoringStringEqualsMethodAccess() {
+    exists(Expr hostname, Expr other, Parameter hostnameParameter, Parameter sessionParameter |
+      hostnameParameter = getEnclosingCallable().(OverridenVerifyMethod).getHostnameParameter() and
+      sessionParameter = getEnclosingCallable().(OverridenVerifyMethod).getSessionParameter() and
+      getAnEqualityPart() = hostname and
+      getAnEqualityPart() = other and
+      hostname != other
+    |
+      not isDerivedFromParameter(sessionParameter, other) and
+      isDerivedFromParameter(hostnameParameter, hostname)
+    )
+  }
+}
+
+/**
+ * A `verify` method that does not verify the `hostname` correctly. It is incorrect
+ * because it does not validate the `hostname` against any expression that is derived from the `session` parameter of the `verify` method.
+ */
+class IncorrectHostnameVerifyMethod extends DangerousVerifyMethod {
+  Stmt dangerousStmt;
+
+  IncorrectHostnameVerifyMethod() {
+    /* We return `true` based on an if statement that verifies the `hostname` incorrectly.*/
+    exists(TrueReturnStmt r, IfStmt i |
+      getBody().getAChild*() = r and i = getNearestEnclosingIfStmt(r)
+    |
+      i.getCondition().getAChildExpr*() instanceof
+        SSLSessionParameterIgnoringStringEqualsMethodAccess and
+      dangerousStmt = i
+    )
+    or
+    /* We return the result of an `equals` call that verifies the `hostname` incorrectly.*/
+    exists(ReturnStmt r | getBody().getAChild*() = r |
+      not r.getResult() instanceof BooleanLiteral and
+      r.getResult().getAChildExpr*() instanceof SSLSessionParameterIgnoringStringEqualsMethodAccess and
+      dangerousStmt = r
+    )
+  }
+
+  override Stmt getADangerousStmt() { result = dangerousStmt }
+}