diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.java b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.java new file mode 100644 index 00000000000..f50424b96c4 --- /dev/null +++ b/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); + } + +} \ No newline at end of file diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp new file mode 100644 index 00000000000..71c28a93159 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp @@ -0,0 +1,45 @@ + + + +

+If a HostnameVerifier always returns true 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 https://example.com. +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 example.com, which will fail because the certificate has been issued for malicious.domain. +5. Java wants to reject the certificate because the hostname does not match. Before doing this it checks whether there exists a HostnameVerifier. +6. Your HostnameVerifier is called which returns true for any certificate so also for this one. +7. Java proceeds with the connection since your HostnameVerifier accepted it. +8. The attacker can now read the data (Man-in-the-middle) your program sends to https://example.com while the program thinks the connection is secure. +

+
+ + +

+Do NOT use an unverifying HostnameVerifier! +

  • If you use an unverifying verifier to solve a configuration problem with TLS/HTTPS you should solve the configuration problem instead. +
  • +

    + +
    + + +

    +In the first (bad) example, the HostnameVerifier always returns true. +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 HostnameVerifier only returns true when the certificate has been correctly checked. +

    + +
    + + +
  • Android Security Guide for TLS/HTTPS.
  • +
  • Further Information on Hostname Verification.
  • +
  • OWASP: CWE-297.
  • +
    +
    \ No newline at end of file diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql new file mode 100644 index 00000000000..f661f4acde9 --- /dev/null +++ b/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" diff --git a/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected new file mode 100644 index 00000000000..2c15e7024a8 --- /dev/null +++ b/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 | diff --git a/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java new file mode 100644 index 00000000000..d23d83613e0 --- /dev/null +++ b/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 + } + }; +} + diff --git a/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.qlref b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.qlref new file mode 100644 index 00000000000..2e449e119d1 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-297/UnsafeHostnameVerification.ql