diff --git a/java/change-notes/2021-01-12-unsafe-hostname-verification.md b/java/change-notes/2021-01-12-unsafe-hostname-verification.md new file mode 100644 index 000000000000..6da83c2509e4 --- /dev/null +++ b/java/change-notes/2021-01-12-unsafe-hostname-verification.md @@ -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. 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 000000000000..f50424b96c4f --- /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 000000000000..520174bf4c52 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp @@ -0,0 +1,50 @@ + + + +

+If a HostnameVerifier always returns true 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. +

+ +

+An attack might look like this: +

+ +
    +
  1. The program connects to https://example.com.
    2. +
  2. The attacker intercepts this connection and presents an apparently-valid certificate of their choosing.
    3. +
  3. The TrustManager of the program verifies that the certificate has been issued by a trusted certificate authority.
    4. +
  4. The Java HTTPS library checks whether the certificate has been issued for the host example.com. This check fails because the certificate has been issued for a domain controlled by the attacker, for example: malicious.domain.
    5. +
  5. The HTTPS library wants to reject the certificate because the hostname does not match. Before doing this it checks whether a HostnameVerifier exists.
    6. +
  6. Your HostnameVerifier is called which returns true for any certificate so also for this one.
    7. +
  7. The program proceeds with the connection since your HostnameVerifier accepted it.
    8. +
  8. The attacker can now read the data your program sends to https://example.com +and/or alter its replies while the program thinks the connection is secure.
    9. +
+ + + + +

+Do not use an open HostnameVerifier. +If you have a configuration problem with TLS/HTTPS, you should always solve the configuration problem instead of using an open verifier. +

+ + + + +

+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 developers: Security with HTTPS and SSL.
    • +
  • Terse systems blog: Fixing Hostname Verification.
    • +     +     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 000000000000..9c060565f284 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -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" diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java index 65698ac33a30..b3536fa7d1de 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java +++ b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.java @@ -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() { diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp index 2e8d08fd68b9..9a6bdb82cbac 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.qhelp @@ -4,10 +4,10 @@ -

    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.

    +

    Java offers two mechanisms for SSL authentication - trust manager and hostname verifier (checked by the java/insecure-hostname-verifier 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.

    And when SSLSocket or SSLEngine is created without a valid parameter of setEndpointIdentificationAlgorithm, hostname verification is disabled by default.

    -

    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.

    -

    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.

    +

    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.

    +

    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.

     @@ -15,7 +15,7 @@ -

    The following two examples show two ways of configuring X509 trust cert manager and hostname verifier. In the 'BAD' case, +

    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.

     diff --git a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql index 497e87b37ac3..3dca54a8de64 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-273/UnsafeCertTrust.ql @@ -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 diff --git a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll index 2d568670b26f..4359fdbcbc20 100644 --- a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll +++ b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll @@ -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 diff --git a/java/ql/src/semmle/code/java/security/Encryption.qll b/java/ql/src/semmle/code/java/security/Encryption.qll index ea7a33151f8f..0fb491d7499c 100644 --- a/java/ql/src/semmle/code/java/security/Encryption.qll +++ b/java/ql/src/semmle/code/java/security/Encryption.qll @@ -29,13 +29,22 @@ class SSLContext extends RefType { SSLContext() { hasQualifiedName("javax.net.ssl", "SSLContext") } } +/** The `javax.net.ssl.SSLSession` class. */ +class SSLSession extends RefType { + SSLSession() { hasQualifiedName("javax.net.ssl", "SSLSession") } +} + class HostnameVerifier extends RefType { HostnameVerifier() { hasQualifiedName("javax.net.ssl", "HostnameVerifier") } } +/** The `verify` method of the class `javax.net.ssl.HostnameVerifier`. */ class HostnameVerifierVerify extends Method { HostnameVerifierVerify() { - hasName("verify") and getDeclaringType().getASupertype*() instanceof HostnameVerifier + hasName("verify") and + getDeclaringType().getASupertype*() instanceof HostnameVerifier and + getParameterType(0) instanceof TypeString and + getParameterType(1) instanceof SSLSession } } @@ -74,6 +83,14 @@ class SetHostnameVerifierMethod extends Method { } } +/** The `setDefaultHostnameVerifier` method of the class `javax.net.ssl.HttpsURLConnection`. */ +class SetDefaultHostnameVerifierMethod extends Method { + SetDefaultHostnameVerifierMethod() { + hasName("setDefaultHostnameVerifier") and + getDeclaringType().getASupertype*() instanceof HttpsURLConnection + } +} + bindingset[algorithmString] private string algorithmRegex(string algorithmString) { // Algorithms usually appear in names surrounded by characters that are not 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 index c0ea40f9bdb0..5d2da21289e5 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrust.expected @@ -1,7 +1,5 @@ | UnsafeCertTrustTest.java:27:4:27:74 | init(...) | Unsafe configuration of trusted certificates | | UnsafeCertTrustTest.java:42:4:42:38 | init(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:55:3:60:4 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:73:3:73:57 | setDefaultHostnameVerifier(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:124:25:124:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:135:25:135:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | -| UnsafeCertTrustTest.java:144:34:144:83 | createSocket(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:92:25:92:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:103:25:103:52 | createSSLEngine(...) | Unsafe configuration of trusted certificates | +| UnsafeCertTrustTest.java:112:34:112:83 | createSocket(...) | Unsafe configuration of trusted certificates | 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 index ff62035fd333..a45727a74064 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java +++ b/java/ql/test/experimental/query-tests/security/CWE-273/UnsafeCertTrustTest.java @@ -48,31 +48,6 @@ public SSLSocketFactory testTrustAllCertManagerOfVariable() { } } - /** - * 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) @@ -109,13 +84,6 @@ public X509Certificate[] getAcceptedIssuers() { } }; - public static final HostnameVerifier ALLOW_ALL_HOSTNAME_VERIFIER = new HostnameVerifier() { - @Override - public boolean verify(String hostname, SSLSession session) { - return true; // Noncompliant - } - }; - /** * Test the endpoint identification of SSL engine is set to null */ 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 000000000000..acb9f1eabe0b --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected @@ -0,0 +1,21 @@ +edges +| UnsafeHostnameVerification.java:66:37:80:9 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:81:55:81:62 | verifier | +| UnsafeHostnameVerification.java:88:37:93:9 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:94:55:94:62 | verifier | +| UnsafeHostnameVerification.java:97:72:102:5 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:34:59:34:85 | ALLOW_ALL_HOSTNAME_VERIFIER | +nodes +| UnsafeHostnameVerification.java:14:55:19:9 | new (...) | semmle.label | new (...) | +| UnsafeHostnameVerification.java:26:55:26:71 | ...->... | semmle.label | ...->... | +| UnsafeHostnameVerification.java:34:59:34:85 | ALLOW_ALL_HOSTNAME_VERIFIER | semmle.label | ALLOW_ALL_HOSTNAME_VERIFIER | +| UnsafeHostnameVerification.java:47:55:47:71 | ...->... | semmle.label | ...->... | +| UnsafeHostnameVerification.java:59:59:59:85 | ...->... | semmle.label | ...->... | +| UnsafeHostnameVerification.java:66:37:80:9 | new (...) : new HostnameVerifier(...) { ... } | semmle.label | new (...) : new HostnameVerifier(...) { ... } | +| UnsafeHostnameVerification.java:81:55:81:62 | verifier | semmle.label | verifier | +| UnsafeHostnameVerification.java:88:37:93:9 | new (...) : new HostnameVerifier(...) { ... } | semmle.label | new (...) : new HostnameVerifier(...) { ... } | +| UnsafeHostnameVerification.java:94:55:94:62 | verifier | semmle.label | verifier | +| UnsafeHostnameVerification.java:97:72:102:5 | new (...) : new HostnameVerifier(...) { ... } | semmle.label | new (...) : new HostnameVerifier(...) { ... } | +#select +| UnsafeHostnameVerification.java:14:55:19:9 | new (...) | UnsafeHostnameVerification.java:14:55:19:9 | new (...) | UnsafeHostnameVerification.java:14:55:19:9 | new (...) | $@ that is defined $@ and accepts any certificate as valid, is used here. | UnsafeHostnameVerification.java:14:55:19:9 | new (...) | This hostname verifier | UnsafeHostnameVerification.java:14:59:14:74 | new HostnameVerifier(...) { ... } | here | +| UnsafeHostnameVerification.java:26:55:26:71 | ...->... | UnsafeHostnameVerification.java:26:55:26:71 | ...->... | UnsafeHostnameVerification.java:26:55:26:71 | ...->... | $@ that is defined $@ and accepts any certificate as valid, is used here. | UnsafeHostnameVerification.java:26:55:26:71 | ...->... | This hostname verifier | UnsafeHostnameVerification.java:26:55:26:71 | new HostnameVerifier(...) { ... } | here | +| UnsafeHostnameVerification.java:47:55:47:71 | ...->... | UnsafeHostnameVerification.java:47:55:47:71 | ...->... | UnsafeHostnameVerification.java:47:55:47:71 | ...->... | $@ that is defined $@ and accepts any certificate as valid, is used here. | UnsafeHostnameVerification.java:47:55:47:71 | ...->... | This hostname verifier | UnsafeHostnameVerification.java:47:55:47:71 | new HostnameVerifier(...) { ... } | here | +| UnsafeHostnameVerification.java:81:55:81:62 | verifier | UnsafeHostnameVerification.java:66:37:80:9 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:81:55:81:62 | verifier | $@ that is defined $@ and accepts any certificate as valid, is used here. | UnsafeHostnameVerification.java:66:37:80:9 | new (...) : new HostnameVerifier(...) { ... } | This hostname verifier | UnsafeHostnameVerification.java:66:41:66:56 | new HostnameVerifier(...) { ... } | here | +| UnsafeHostnameVerification.java:94:55:94:62 | verifier | UnsafeHostnameVerification.java:88:37:93:9 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:94:55:94:62 | verifier | $@ that is defined $@ and accepts any certificate as valid, is used here. | UnsafeHostnameVerification.java:88:37:93:9 | new (...) : new HostnameVerifier(...) { ... } | This hostname verifier | UnsafeHostnameVerification.java:88:41:88:56 | new HostnameVerifier(...) { ... } | here | 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 000000000000..005271998ba6 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java @@ -0,0 +1,103 @@ +import javax.net.ssl.HostnameVerifier; +import javax.net.ssl.HttpsURLConnection; +import javax.net.ssl.SSLSession; +import java.security.cert.Certificate; + +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, because it depends on a + // feature + // flag. + } + } + + public void testTrustAllHostnameWithExceptions() { + HostnameVerifier verifier = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + verify(hostname, session.getPeerCertificates()); + return true; // GOOD [but detected as BAD]. The verification of the certificate is done in + // another method and + // in the case of a mismatch, an `Exception` is thrown so the `return true` + // statement never gets executed. + } + + // Black-box method that properly verifies the certificate but throws an + // `Exception` in the case of a mismatch. + private void verify(String hostname, Certificate[] certs) { + } + }; + HttpsURLConnection.setDefaultHostnameVerifier(verifier); + } + + /** + * 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 000000000000..2e449e119d18 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-297/UnsafeHostnameVerification.ql