From 8df5d77398b847616441ed20c3cb1dd4b07138d5 Mon Sep 17 00:00:00 2001 From: intrigus Date: Sat, 5 Dec 2020 00:30:56 +0100 Subject: [PATCH 01/25] Java: Model `HostnameVerifier` method Model `HostnameVerifier#setDefaultHostnameVerifier` --- java/ql/src/semmle/code/java/security/Encryption.qll | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/java/ql/src/semmle/code/java/security/Encryption.qll b/java/ql/src/semmle/code/java/security/Encryption.qll index ea7a33151f8f..35b9c367dd3f 100644 --- a/java/ql/src/semmle/code/java/security/Encryption.qll +++ b/java/ql/src/semmle/code/java/security/Encryption.qll @@ -74,6 +74,13 @@ class SetHostnameVerifierMethod extends Method { } } +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 From 3da1cb08798386516a32add43ad6a3d991cb1ffe Mon Sep 17 00:00:00 2001 From: intrigus Date: Sat, 5 Dec 2020 00:32:11 +0100 Subject: [PATCH 02/25] Java: Add unsafe hostname verification query --- .../CWE-297/UnsafeHostnameVerification.java | 30 +++++++ .../CWE-297/UnsafeHostnameVerification.qhelp | 45 ++++++++++ .../CWE/CWE-297/UnsafeHostnameVerification.ql | 75 +++++++++++++++++ .../UnsafeHostnameVerification.expected | 18 ++++ .../CWE-297/UnsafeHostnameVerification.java | 84 +++++++++++++++++++ .../CWE-297/UnsafeHostnameVerification.qlref | 1 + 6 files changed, 253 insertions(+) create mode 100644 java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.java create mode 100644 java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp create mode 100644 java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql create mode 100644 java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected create mode 100644 java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java create mode 100644 java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.qlref 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..71c28a93159a --- /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 000000000000..f661f4acde91 --- /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 000000000000..2c15e7024a88 --- /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 000000000000..d23d83613e05 --- /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 000000000000..2e449e119d18 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-297/UnsafeHostnameVerification.ql From 70b0703952ef4c364990a4a183348ab44e901944 Mon Sep 17 00:00:00 2001 From: intrigus Date: Sat, 5 Dec 2020 00:32:44 +0100 Subject: [PATCH 03/25] Java: Remove overlapping code --- .../Security/CWE/CWE-273/UnsafeCertTrust.java | 26 ------------ .../CWE/CWE-273/UnsafeCertTrust.qhelp | 8 ++-- .../Security/CWE/CWE-273/UnsafeCertTrust.ql | 41 +------------------ .../security/CWE-273/UnsafeCertTrustTest.java | 32 --------------- 4 files changed, 6 insertions(+), 101 deletions(-) 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..223b2c3772ad 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 (not checked by this 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/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 */ From 0a9df07df74fbbbd2aaa87ddbb273742450d067b Mon Sep 17 00:00:00 2001 From: intrigus Date: Wed, 9 Dec 2020 23:29:59 +0100 Subject: [PATCH 04/25] Apply suggestions from review. --- .../Security/CWE/CWE-297/UnsafeHostnameVerification.ql | 10 +++++++--- .../Security/CWE/CWE-273/UnsafeCertTrust.qhelp | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql index f661f4acde91..2b5ae17448bf 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -4,16 +4,20 @@ * @kind path-problem * @problem.severity error * @precision high - * @id java/everything-accepting-hostname-verifier + * @id java/insecure-hostname-verifier * @tags security * external/cwe/cwe-297 */ import java -import semmle.code.java.security.Encryption + +import semmle.code.java.controlflow.Guards import semmle.code.java.dataflow.DataFlow +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.dataflow.TaintTracking2 +import semmle.code.java.security.Encryption + import DataFlow::PathGraph -import semmle.code.java.controlflow.Guards /** * Holds if `m` always returns `true` ignoring any exceptional flow. 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 223b2c3772ad..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,7 +4,7 @@ -

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

    +

    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 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.

    From e021158b5fa034d3aecb4267b55fefb2cc8cf84c Mon Sep 17 00:00:00 2001 From: intrigus Date: Wed, 9 Dec 2020 23:37:29 +0100 Subject: [PATCH 05/25] Java: Tighter model of `HostnameVerifier#verify` This more tightly models `HostnameVerifier#verify` previously it was possible to accidentally match other methods called `verify`. --- java/ql/src/semmle/code/java/security/Encryption.qll | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/java/ql/src/semmle/code/java/security/Encryption.qll b/java/ql/src/semmle/code/java/security/Encryption.qll index 35b9c367dd3f..4ab5a0f5be27 100644 --- a/java/ql/src/semmle/code/java/security/Encryption.qll +++ b/java/ql/src/semmle/code/java/security/Encryption.qll @@ -29,13 +29,20 @@ class SSLContext extends RefType { SSLContext() { hasQualifiedName("javax.net.ssl", "SSLContext") } } +class SSLSession extends RefType { + SSLSession() { hasQualifiedName("javax.net.ssl", "SSLSession") } +} + class HostnameVerifier extends RefType { HostnameVerifier() { hasQualifiedName("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 } } From d98b17199804a70cba87d20878616e84caee45a9 Mon Sep 17 00:00:00 2001 From: intrigus Date: Wed, 9 Dec 2020 23:40:27 +0100 Subject: [PATCH 06/25] Java: Make `EnvTaintedMethod` public + QL-Doc --- java/ql/src/semmle/code/java/dataflow/FlowSources.qll | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll index 2d568670b26f..42aaf1372205 100644 --- a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll +++ b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll @@ -292,7 +292,8 @@ private class SpringWebRequestGetMethod extends Method { } } -private class EnvTaintedMethod extends Method { +/** Models methods that are tainted by the environment of the user, such as `System.getProperty` or `System.getenv()`. */ +class EnvTaintedMethod extends Method { EnvTaintedMethod() { this instanceof MethodSystemGetenv or this instanceof PropertiesGetPropertyMethod or From a62a2e58dd35599b1c9d21a285f14bab7edd2e81 Mon Sep 17 00:00:00 2001 From: intrigus Date: Wed, 9 Dec 2020 23:42:00 +0100 Subject: [PATCH 07/25] Java: Improve QL-Doc --- java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql index 2b5ae17448bf..b62707d3ef22 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -29,7 +29,7 @@ private predicate alwaysReturnsTrue(HostnameVerifierVerify m) { } /** - * A class that overrides the `javax.net.ssl.HostnameVerifier.verify` method and **always** returns `true`, thus + * A class that overrides the `javax.net.ssl.HostnameVerifier.verify` method and **always** returns `true` (ignoring exceptional flow), thus * accepting any certificate despite a hostname mismatch. */ class TrustAllHostnameVerifier extends RefType { From 9e2ef9bd74d6d53441d5d68ea780822867ccf2e5 Mon Sep 17 00:00:00 2001 From: intrigus Date: Wed, 9 Dec 2020 23:42:32 +0100 Subject: [PATCH 08/25] Java: Filter results by feature flags. This ignores results that are guarded by a feature flag that suggests an intentionally insecure feature. Inspired by Go's `InsecureFeatureFlag.qll` and `DisabledCertificateCheck.ql`. --- .../CWE/CWE-297/UnsafeHostnameVerification.ql | 64 +++++++++++++++---- 1 file changed, 53 insertions(+), 11 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql index b62707d3ef22..2e47f501f7b9 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -62,18 +62,60 @@ class TrustAllHostnameVerifierConfiguration extends DataFlow::Configuration { } } +bindingset[result] +private string getAFlagName() { + result.regexpMatch("(?i).*(secure|(en|dis)able|selfCert|selfSign|validat|verif|trust|ignore).*") +} + +/** A configuration to model the flow of feature flags into `Guard`s. This is used to determine whether something is guarded by such a flag. */ +private class FlagTaintTracking extends TaintTracking2::Configuration { + FlagTaintTracking() { this = "FlagTaintTracking" } + + override predicate isSource(DataFlow::Node source) { + exists(VarAccess v | v.getVariable().getName() = getAFlagName() | source.asExpr() = v) + or + exists(StringLiteral s | s.getRepresentedString() = getAFlagName() | source.asExpr() = s) + or + exists(MethodAccess ma | ma.getMethod().getName() = getAFlagName() | source.asExpr() = ma) + } + + override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof Guard } + + override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(MethodAccess ma | ma.getMethod() = any(EnvTaintedMethod 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(FlagTaintTracking cfg, DataFlow::Node source, DataFlow::Node sink | + cfg.hasFlow(source, sink) + | + 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 - .(VarAccess) - .getVariable() - .getName() - .regexpMatch("(?i).*(secure|(en|dis)able|selfCert|selfSign|validat|verif|trust|ignore).*") - ) + exists(Guard g | g.controls(node.asExpr().getBasicBlock(), _) | g = getAGuard()) } -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" +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 $@.", source, + "This hostname verifier", verifier, "here", sink, "here" From 33b0ff28d83ac84685d42717b8fec9ed4810e729 Mon Sep 17 00:00:00 2001 From: intrigus Date: Wed, 9 Dec 2020 23:50:28 +0100 Subject: [PATCH 09/25] Java: Update test --- .../CWE-297/UnsafeHostnameVerification.java | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java index d23d83613e05..11f3cf65a2b6 100644 --- a/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java +++ b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java @@ -1,6 +1,7 @@ import javax.net.ssl.HostnameVerifier; import javax.net.ssl.HttpsURLConnection; import javax.net.ssl.SSLSession; +import java.security.cert.Certificate; public class UnsafeHostnameVerification { @@ -55,12 +56,26 @@ private void functionThatActuallyDisablesVerification() { 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. + 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 */ From c88f07dde4884ae8bf337dec075065ada01e07ab Mon Sep 17 00:00:00 2001 From: intrigus Date: Wed, 9 Dec 2020 23:50:58 +0100 Subject: [PATCH 10/25] Java: Accept test output --- .../UnsafeHostnameVerification.expected | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected index 2c15e7024a88..18628e4f7ccc 100644 --- a/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected +++ b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected @@ -1,18 +1,21 @@ 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 | +| UnsafeHostnameVerification.java:65:31:75:3 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:76:49:76:56 | verifier | +| UnsafeHostnameVerification.java:83:31:88:3 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:89:49:89:56 | verifier | +| UnsafeHostnameVerification.java:92:69:97:2 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:34:50:34: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(...) { ... } | +| UnsafeHostnameVerification.java:14:49:19:3 | new (...) | semmle.label | new (...) | +| UnsafeHostnameVerification.java:26:49:26:65 | ...->... | semmle.label | ...->... | +| UnsafeHostnameVerification.java:34:50:34:76 | ALLOW_ALL_HOSTNAME_VERIFIER | semmle.label | ALLOW_ALL_HOSTNAME_VERIFIER | +| UnsafeHostnameVerification.java:47:49:47:65 | ...->... | semmle.label | ...->... | +| UnsafeHostnameVerification.java:59:50:59:76 | ...->... | semmle.label | ...->... | +| UnsafeHostnameVerification.java:65:31:75:3 | new (...) : new HostnameVerifier(...) { ... } | semmle.label | new (...) : new HostnameVerifier(...) { ... } | +| UnsafeHostnameVerification.java:76:49:76:56 | verifier | semmle.label | verifier | +| UnsafeHostnameVerification.java:83:31:88:3 | new (...) : new HostnameVerifier(...) { ... } | semmle.label | new (...) : new HostnameVerifier(...) { ... } | +| UnsafeHostnameVerification.java:89:49:89:56 | verifier | semmle.label | verifier | +| UnsafeHostnameVerification.java:92:69:97: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 | +| UnsafeHostnameVerification.java:14:49:19:3 | new (...) | UnsafeHostnameVerification.java:14:49:19:3 | new (...) | UnsafeHostnameVerification.java:14:49:19:3 | new (...) | $@ that is defined $@ and accepts any certificate as valid, is used $@. | UnsafeHostnameVerification.java:14:49:19:3 | new (...) | This hostname verifier | UnsafeHostnameVerification.java:14:53:14:68 | new HostnameVerifier(...) { ... } | here | UnsafeHostnameVerification.java:14:49:19:3 | new (...) | here | +| UnsafeHostnameVerification.java:26:49:26:65 | ...->... | UnsafeHostnameVerification.java:26:49:26:65 | ...->... | UnsafeHostnameVerification.java:26:49:26:65 | ...->... | $@ that is defined $@ and accepts any certificate as valid, is used $@. | UnsafeHostnameVerification.java:26:49:26:65 | ...->... | This hostname verifier | UnsafeHostnameVerification.java:26:49:26:65 | new HostnameVerifier(...) { ... } | here | UnsafeHostnameVerification.java:26:49:26:65 | ...->... | here | +| UnsafeHostnameVerification.java:47:49:47:65 | ...->... | UnsafeHostnameVerification.java:47:49:47:65 | ...->... | UnsafeHostnameVerification.java:47:49:47:65 | ...->... | $@ that is defined $@ and accepts any certificate as valid, is used $@. | UnsafeHostnameVerification.java:47:49:47:65 | ...->... | This hostname verifier | UnsafeHostnameVerification.java:47:49:47:65 | new HostnameVerifier(...) { ... } | here | UnsafeHostnameVerification.java:47:49:47:65 | ...->... | here | +| UnsafeHostnameVerification.java:76:49:76:56 | verifier | UnsafeHostnameVerification.java:65:31:75:3 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:76:49:76:56 | verifier | $@ that is defined $@ and accepts any certificate as valid, is used $@. | UnsafeHostnameVerification.java:65:31:75:3 | new (...) : new HostnameVerifier(...) { ... } | This hostname verifier | UnsafeHostnameVerification.java:65:35:65:50 | new HostnameVerifier(...) { ... } | here | UnsafeHostnameVerification.java:76:49:76:56 | verifier | here | +| UnsafeHostnameVerification.java:89:49:89:56 | verifier | UnsafeHostnameVerification.java:83:31:88:3 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:89:49:89:56 | verifier | $@ that is defined $@ and accepts any certificate as valid, is used $@. | UnsafeHostnameVerification.java:83:31:88:3 | new (...) : new HostnameVerifier(...) { ... } | This hostname verifier | UnsafeHostnameVerification.java:83:35:83:50 | new HostnameVerifier(...) { ... } | here | UnsafeHostnameVerification.java:89:49:89:56 | verifier | here | From 10fc2cf9f803acf2c4d5eb34071ad2b6a1e16e87 Mon Sep 17 00:00:00 2001 From: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> Date: Mon, 14 Dec 2020 15:51:53 +0100 Subject: [PATCH 11/25] Apply suggestions from code review Co-authored-by: Chris Smowton --- .../Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp | 8 ++++---- .../Security/CWE/CWE-297/UnsafeHostnameVerification.ql | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp index 71c28a93159a..f8bfc87a9c35 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp @@ -15,14 +15,14 @@ An attack would look like this: 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. +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.

    -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. +Do not use an open HostnameVerifier. +
  • If you use an open verifier to solve a configuration problem with TLS/HTTPS you should solve the configuration problem instead.

    • @@ -42,4 +42,4 @@ In the second (good) example, the HostnameVerifier only returns 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 index 2e47f501f7b9..d89605588948 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -1,6 +1,6 @@ /** * @name Disabled hostname verification - * @description Accepting any certificate as valid for a host allows an attacker to perform a man-in-the-middle attack. + * @description Accepting any certificate as valid for a host allows an attacker to perform a machine-in-the-middle attack. * @kind path-problem * @problem.severity error * @precision high @@ -29,7 +29,7 @@ private predicate alwaysReturnsTrue(HostnameVerifierVerify m) { } /** - * A class that overrides the `javax.net.ssl.HostnameVerifier.verify` method and **always** returns `true` (ignoring exceptional flow), thus + * 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 { From 355cb6eeecb08916936b9f32f61734ca8f3f9eea Mon Sep 17 00:00:00 2001 From: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> Date: Wed, 16 Dec 2020 15:24:30 +0100 Subject: [PATCH 12/25] Fix Qhelp format Co-authored-by: Felicity Chapman --- .../src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp index f8bfc87a9c35..cca6b6e4a5bb 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp @@ -22,9 +22,9 @@ An attack would look like this:

    Do not use an open HostnameVerifier. +

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

     From 502e4c39f5ce6b894e2c5c532d30052b575affb4 Mon Sep 17 00:00:00 2001 From: intrigus Date: Fri, 18 Dec 2020 00:22:34 +0100 Subject: [PATCH 13/25] Java: Fix Qhelp --- .../src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp index cca6b6e4a5bb..7bf87b34ad2a 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp @@ -22,9 +22,8 @@ An attack would look like this:

    Do not use an open HostnameVerifier. +If you use an open verifier to solve a configuration problem with TLS/HTTPS you should solve the configuration problem instead.

    -
  • If you use an open verifier to solve a configuration problem with TLS/HTTPS you should solve the configuration problem instead. -
    •  From b8f3e64a0fe6d82103fd50d64d73d432f2209df8 Mon Sep 17 00:00:00 2001 From: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> Date: Mon, 4 Jan 2021 20:06:12 +0100 Subject: [PATCH 14/25] Apply suggestions from code review Co-authored-by: Felicity Chapman --- .../CWE-297/UnsafeHostnameVerification.qhelp | 34 +++++++++++-------- .../CWE/CWE-297/UnsafeHostnameVerification.ql | 2 +- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp index 7bf87b34ad2a..66b4153256be 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp @@ -5,24 +5,31 @@

    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. +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 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 your program sends to https://example.com and/or alter its replies while the program thinks the connection is secure. +

    +An attack might look like this:

    + +
      +
    1. The program connects to https://example.com.
      2. +
    2. The attacker intercepts this connection and presents one of their valid certificates they control, for example one from Let's Encrypt.
      3. +
    3. Java verifies that the certificate has been issued by a trusted certificate authority.
      4. +
    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. +
    5. Java wants to reject the certificate because the hostname does not match. Before doing this it checks whether there exists a HostnameVerifier.
      6. +
    6. Your HostnameVerifier is called which returns true for any certificate so also for this one.
      7. +
    7. Java 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 use an open verifier to solve a configuration problem with TLS/HTTPS you should solve the configuration problem instead. +If you have a configuration problem with TLS/HTTPS, you should always solve the configuration problem instead of using an open verifier.

    @@ -37,8 +44,7 @@ In the second (good) example, the HostnameVerifier only returns -
  • Android Security Guide for TLS/HTTPS.
    • -
  • Further Information on Hostname Verification.
    • -
  • OWASP: CWE-297.
    • +
  • 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 index d89605588948..f07d9da71d47 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -1,6 +1,6 @@ /** * @name Disabled hostname verification - * @description Accepting any certificate as valid for a host allows an attacker to perform a machine-in-the-middle attack. + * @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 From e11304a1cad701c0df14e342322c9bc02ed16c85 Mon Sep 17 00:00:00 2001 From: intrigus Date: Tue, 5 Jan 2021 11:49:37 +0100 Subject: [PATCH 15/25] Java: Autoformat --- .../CWE/CWE-297/UnsafeHostnameVerification.ql | 2 - .../CWE-297/UnsafeHostnameVerification.java | 166 +++++++++--------- 2 files changed, 85 insertions(+), 83 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql index f07d9da71d47..f911d4b4b265 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -10,13 +10,11 @@ */ import java - import semmle.code.java.controlflow.Guards import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.TaintTracking2 import semmle.code.java.security.Encryption - import DataFlow::PathGraph /** diff --git a/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java index 11f3cf65a2b6..005271998ba6 100644 --- a/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java +++ b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java @@ -5,95 +5,99 @@ public class UnsafeHostnameVerification { - private static final boolean DISABLE_VERIFICATION = true; + 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 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 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 - } - } + /** + * 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(); - } - } + 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. - } + 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 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. - } + 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); - } + // 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); - } + /** + * 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 - } - }; + public static final HostnameVerifier ALLOW_ALL_HOSTNAME_VERIFIER = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + return true; // BAD, always returns true + } + }; } - From f4b912cd8a949a3a5e1c1428603293f2a9a152e0 Mon Sep 17 00:00:00 2001 From: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> Date: Sat, 9 Jan 2021 23:15:19 +0100 Subject: [PATCH 16/25] Apply suggestions from doc review Co-authored-by: Felicity Chapman --- .../CWE/CWE-297/UnsafeHostnameVerification.qhelp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp index 66b4153256be..84d6e505075f 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp @@ -5,7 +5,7 @@

    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. +This stops Transport Layer Security (TLS) providing any security and allows an attacker to perform a man-in-the-middle attack against the application.

    @@ -14,10 +14,10 @@ An attack might look like this:

    1. The program connects to https://example.com.
      2. -
    2. The attacker intercepts this connection and presents one of their valid certificates they control, for example one from Let's Encrypt.
      3. -
    3. Java verifies that the certificate has been issued by a trusted certificate authority.
      4. -
    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. -
    5. Java wants to reject the certificate because the hostname does not match. Before doing this it checks whether there exists a HostnameVerifier.
      6. +
    6. The attacker intercepts this connection and presents an apparently-valid certificate of their choosing.
      7. +
    7. The `TrustManager` of the program verifies that the certificate has been issued by a trusted certificate authority.
      8. +
    8. Java 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.
      9. +
    9. Java wants to reject the certificate because the hostname does not match. Before doing this it checks whether a HostnameVerifier exists.
    10. Your HostnameVerifier is called which returns true for any certificate so also for this one.
    11. Java proceeds with the connection since your HostnameVerifier accepted it.
    12. The attacker can now read the data your program sends to https://example.com From b4692734b28edb09d6b72d8a947233ec1b433467 Mon Sep 17 00:00:00 2001 From: intrigus Date: Thu, 7 Jan 2021 20:30:13 +0100 Subject: [PATCH 17/25] Java: Add QLDoc improve query message --- .../ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql | 4 ++-- java/ql/src/semmle/code/java/security/Encryption.qll | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql index f911d4b4b265..b4776cfc1275 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -115,5 +115,5 @@ where 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 $@.", source, - "This hostname verifier", verifier, "here", sink, "here" + "$@ that is defined $@ and accepts any certificate as valid, is used here.", source, + "This hostname verifier", verifier, "here" diff --git a/java/ql/src/semmle/code/java/security/Encryption.qll b/java/ql/src/semmle/code/java/security/Encryption.qll index 4ab5a0f5be27..084254b5a410 100644 --- a/java/ql/src/semmle/code/java/security/Encryption.qll +++ b/java/ql/src/semmle/code/java/security/Encryption.qll @@ -29,6 +29,7 @@ class SSLContext extends RefType { SSLContext() { hasQualifiedName("javax.net.ssl", "SSLContext") } } +/** Models the `javax.net.ssl.SSLSession` class. */ class SSLSession extends RefType { SSLSession() { hasQualifiedName("javax.net.ssl", "SSLSession") } } @@ -37,6 +38,7 @@ class HostnameVerifier extends RefType { HostnameVerifier() { hasQualifiedName("javax.net.ssl", "HostnameVerifier") } } +/** Models the `verify` method of the class `javax.net.ssl.HostnameVerifier`. */ class HostnameVerifierVerify extends Method { HostnameVerifierVerify() { hasName("verify") and @@ -81,6 +83,7 @@ class SetHostnameVerifierMethod extends Method { } } +/** Models the `setDefaultHostnameVerifier` method of the class `javax.net.ssl.HttpsURLConnection`. */ class SetDefaultHostnameVerifierMethod extends Method { SetDefaultHostnameVerifierMethod() { hasName("setDefaultHostnameVerifier") and From 1eb2b7538941f9b6122bd65d3576ca048d535f2b Mon Sep 17 00:00:00 2001 From: intrigus Date: Sun, 10 Jan 2021 11:50:31 +0100 Subject: [PATCH 18/25] Java: Further reduce FPs, simply Flag2Guard flow --- .../CWE/CWE-297/UnsafeHostnameVerification.ql | 65 ++++++++++++++++--- 1 file changed, 57 insertions(+), 8 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql index b4776cfc1275..d475194a60da 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -13,7 +13,6 @@ import java import semmle.code.java.controlflow.Guards import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.FlowSources -import semmle.code.java.dataflow.TaintTracking2 import semmle.code.java.security.Encryption import DataFlow::PathGraph @@ -58,28 +57,78 @@ class TrustAllHostnameVerifierConfiguration extends DataFlow::Configuration { 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|(en|dis)able|selfCert|selfSign|validat|verif|trust|ignore).*") + 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 + exists(BoxedType boxedBoolean | boxedBoolean.getPrimitiveType().hasName("boolean") | + this = boxedBoolean or this = boxedBoolean.getPrimitiveType() + ) + } +} + +private predicate isEqualsIgnoreCaseMethodAccess(MethodAccess ma) { + ma.getMethod().hasName("equalsIgnoreCase") and + ma.getMethod().getDeclaringType() instanceof TypeString } /** A configuration to model the flow of feature flags into `Guard`s. This is used to determine whether something is guarded by such a flag. */ -private class FlagTaintTracking extends TaintTracking2::Configuration { - FlagTaintTracking() { this = "FlagTaintTracking" } +private class FlagToGuardFlow extends DataFlow::Configuration { + FlagToGuardFlow() { this = "FlagToGuardFlow" } override predicate isSource(DataFlow::Node source) { - exists(VarAccess v | v.getVariable().getName() = getAFlagName() | source.asExpr() = v) + 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) + exists(MethodAccess ma | ma.getMethod().getName() = getAFlagName() | + source.asExpr() = ma and + ma.getType() instanceof FlagType and + not isEqualsIgnoreCaseMethodAccess(ma) + ) } override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof Guard } - override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { + override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { exists(MethodAccess ma | ma.getMethod() = any(EnvTaintedMethod m) | ma = node2.asExpr() and ma.getAnArgument() = node1.asExpr() ) @@ -95,7 +144,7 @@ private class FlagTaintTracking extends TaintTracking2::Configuration { /** Gets a guard that depends on a flag. */ private Guard getAGuard() { - exists(FlagTaintTracking cfg, DataFlow::Node source, DataFlow::Node sink | + exists(FlagToGuardFlow cfg, DataFlow::Node source, DataFlow::Node sink | cfg.hasFlow(source, sink) | sink.asExpr() = result From 5c1e746c96be1a5b3154e5a63c12deeda50adf44 Mon Sep 17 00:00:00 2001 From: intrigus Date: Mon, 11 Jan 2021 13:41:13 +0100 Subject: [PATCH 19/25] Java: Rename to `EnvReadMethod` --- .../Security/CWE/CWE-297/UnsafeHostnameVerification.ql | 2 +- java/ql/src/semmle/code/java/dataflow/FlowSources.qll | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql index d475194a60da..f17808864af8 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -129,7 +129,7 @@ private class FlagToGuardFlow extends DataFlow::Configuration { override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof Guard } override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(MethodAccess ma | ma.getMethod() = any(EnvTaintedMethod m) | + exists(MethodAccess ma | ma.getMethod() = any(EnvReadMethod m) | ma = node2.asExpr() and ma.getAnArgument() = node1.asExpr() ) or diff --git a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll index 42aaf1372205..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,9 +292,9 @@ private class SpringWebRequestGetMethod extends Method { } } -/** Models methods that are tainted by the environment of the user, such as `System.getProperty` or `System.getenv()`. */ -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 From 4cfdb10ddc0f75ed2189bb299c16d08a05ac0041 Mon Sep 17 00:00:00 2001 From: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> Date: Mon, 11 Jan 2021 18:50:43 +0100 Subject: [PATCH 20/25] Java: Improve QLDoc & simplify code Co-authored-by: Anders Schack-Mulligen --- .../src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql | 4 +--- java/ql/src/semmle/code/java/security/Encryption.qll | 6 +++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql index f17808864af8..75ecd1112d02 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -97,9 +97,7 @@ private class FlagType extends Type { FlagType() { this instanceof TypeString or - exists(BoxedType boxedBoolean | boxedBoolean.getPrimitiveType().hasName("boolean") | - this = boxedBoolean or this = boxedBoolean.getPrimitiveType() - ) + this instanceof BooleanType } } diff --git a/java/ql/src/semmle/code/java/security/Encryption.qll b/java/ql/src/semmle/code/java/security/Encryption.qll index 084254b5a410..0fb491d7499c 100644 --- a/java/ql/src/semmle/code/java/security/Encryption.qll +++ b/java/ql/src/semmle/code/java/security/Encryption.qll @@ -29,7 +29,7 @@ class SSLContext extends RefType { SSLContext() { hasQualifiedName("javax.net.ssl", "SSLContext") } } -/** Models the `javax.net.ssl.SSLSession` class. */ +/** The `javax.net.ssl.SSLSession` class. */ class SSLSession extends RefType { SSLSession() { hasQualifiedName("javax.net.ssl", "SSLSession") } } @@ -38,7 +38,7 @@ class HostnameVerifier extends RefType { HostnameVerifier() { hasQualifiedName("javax.net.ssl", "HostnameVerifier") } } -/** Models the `verify` method of the class `javax.net.ssl.HostnameVerifier`. */ +/** The `verify` method of the class `javax.net.ssl.HostnameVerifier`. */ class HostnameVerifierVerify extends Method { HostnameVerifierVerify() { hasName("verify") and @@ -83,7 +83,7 @@ class SetHostnameVerifierMethod extends Method { } } -/** Models the `setDefaultHostnameVerifier` method of the class `javax.net.ssl.HttpsURLConnection`. */ +/** The `setDefaultHostnameVerifier` method of the class `javax.net.ssl.HttpsURLConnection`. */ class SetDefaultHostnameVerifierMethod extends Method { SetDefaultHostnameVerifierMethod() { hasName("setDefaultHostnameVerifier") and From 722bd4dafa8f54aa5a584fea4732971aa7c17048 Mon Sep 17 00:00:00 2001 From: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> Date: Mon, 11 Jan 2021 18:57:24 +0100 Subject: [PATCH 21/25] Java: Revise qhelp --- .../Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp index 84d6e505075f..520174bf4c52 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp @@ -15,11 +15,11 @@ An attack might look like this:
      1. The program connects to https://example.com.
      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. Java 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. Java wants to reject the certificate because the hostname does not match. Before doing this it checks whether a HostnameVerifier exists.
        6. +
      6. The TrustManager of the program verifies that the certificate has been issued by a trusted certificate authority.
        7. +
      7. 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.
        8. +
      8. The HTTPS library wants to reject the certificate because the hostname does not match. Before doing this it checks whether a HostnameVerifier exists.
      9. Your HostnameVerifier is called which returns true for any certificate so also for this one.
        10. -
      10. Java proceeds with the connection since your HostnameVerifier accepted it.
        11. +
      11. The program proceeds with the connection since your HostnameVerifier accepted it.
      12. 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.
      From 85286f362c89c14a5c5442b58b07f94dc526a6e7 Mon Sep 17 00:00:00 2001 From: intrigus Date: Mon, 11 Jan 2021 19:02:07 +0100 Subject: [PATCH 22/25] Java: Replace global flow by local flow --- .../CWE/CWE-297/UnsafeHostnameVerification.ql | 65 +++++++++---------- 1 file changed, 31 insertions(+), 34 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql index 75ecd1112d02..3ab49a866bb8 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -106,45 +106,42 @@ private predicate isEqualsIgnoreCaseMethodAccess(MethodAccess ma) { ma.getMethod().getDeclaringType() instanceof TypeString } -/** A configuration to model the flow of feature flags into `Guard`s. This is used to determine whether something is guarded by such a flag. */ -private class FlagToGuardFlow extends DataFlow::Configuration { - FlagToGuardFlow() { this = "FlagToGuardFlow" } - - override predicate isSource(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) - ) - } - - override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof Guard } +/** 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) + ) +} - override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { - 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() - ) - } +/** 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(FlagToGuardFlow cfg, DataFlow::Node source, DataFlow::Node sink | - cfg.hasFlow(source, sink) - | + exists(DataFlow::Node source, DataFlow::Node sink | + isFlag(source) and + flagFlowStep*(source, sink) and sink.asExpr() = result ) } From 4fa8f5eab2f0e63693b5630aa189b7bbb8be79cc Mon Sep 17 00:00:00 2001 From: intrigus Date: Tue, 12 Jan 2021 15:07:48 +0100 Subject: [PATCH 23/25] Java: Accept test changes --- .../security/CWE-273/UnsafeCertTrust.expected | 8 ++--- .../UnsafeHostnameVerification.expected | 36 +++++++++---------- 2 files changed, 21 insertions(+), 23 deletions(-) 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/query-tests/security/CWE-297/UnsafeHostnameVerification.expected b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected index 18628e4f7ccc..acb9f1eabe0b 100644 --- a/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected +++ b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected @@ -1,21 +1,21 @@ edges -| UnsafeHostnameVerification.java:65:31:75:3 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:76:49:76:56 | verifier | -| UnsafeHostnameVerification.java:83:31:88:3 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:89:49:89:56 | verifier | -| UnsafeHostnameVerification.java:92:69:97:2 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:34:50:34:76 | ALLOW_ALL_HOSTNAME_VERIFIER | +| 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:49:19:3 | new (...) | semmle.label | new (...) | -| UnsafeHostnameVerification.java:26:49:26:65 | ...->... | semmle.label | ...->... | -| UnsafeHostnameVerification.java:34:50:34:76 | ALLOW_ALL_HOSTNAME_VERIFIER | semmle.label | ALLOW_ALL_HOSTNAME_VERIFIER | -| UnsafeHostnameVerification.java:47:49:47:65 | ...->... | semmle.label | ...->... | -| UnsafeHostnameVerification.java:59:50:59:76 | ...->... | semmle.label | ...->... | -| UnsafeHostnameVerification.java:65:31:75:3 | new (...) : new HostnameVerifier(...) { ... } | semmle.label | new (...) : new HostnameVerifier(...) { ... } | -| UnsafeHostnameVerification.java:76:49:76:56 | verifier | semmle.label | verifier | -| UnsafeHostnameVerification.java:83:31:88:3 | new (...) : new HostnameVerifier(...) { ... } | semmle.label | new (...) : new HostnameVerifier(...) { ... } | -| UnsafeHostnameVerification.java:89:49:89:56 | verifier | semmle.label | verifier | -| UnsafeHostnameVerification.java:92:69:97:2 | new (...) : new HostnameVerifier(...) { ... } | semmle.label | new (...) : new HostnameVerifier(...) { ... } | +| 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:49:19:3 | new (...) | UnsafeHostnameVerification.java:14:49:19:3 | new (...) | UnsafeHostnameVerification.java:14:49:19:3 | new (...) | $@ that is defined $@ and accepts any certificate as valid, is used $@. | UnsafeHostnameVerification.java:14:49:19:3 | new (...) | This hostname verifier | UnsafeHostnameVerification.java:14:53:14:68 | new HostnameVerifier(...) { ... } | here | UnsafeHostnameVerification.java:14:49:19:3 | new (...) | here | -| UnsafeHostnameVerification.java:26:49:26:65 | ...->... | UnsafeHostnameVerification.java:26:49:26:65 | ...->... | UnsafeHostnameVerification.java:26:49:26:65 | ...->... | $@ that is defined $@ and accepts any certificate as valid, is used $@. | UnsafeHostnameVerification.java:26:49:26:65 | ...->... | This hostname verifier | UnsafeHostnameVerification.java:26:49:26:65 | new HostnameVerifier(...) { ... } | here | UnsafeHostnameVerification.java:26:49:26:65 | ...->... | here | -| UnsafeHostnameVerification.java:47:49:47:65 | ...->... | UnsafeHostnameVerification.java:47:49:47:65 | ...->... | UnsafeHostnameVerification.java:47:49:47:65 | ...->... | $@ that is defined $@ and accepts any certificate as valid, is used $@. | UnsafeHostnameVerification.java:47:49:47:65 | ...->... | This hostname verifier | UnsafeHostnameVerification.java:47:49:47:65 | new HostnameVerifier(...) { ... } | here | UnsafeHostnameVerification.java:47:49:47:65 | ...->... | here | -| UnsafeHostnameVerification.java:76:49:76:56 | verifier | UnsafeHostnameVerification.java:65:31:75:3 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:76:49:76:56 | verifier | $@ that is defined $@ and accepts any certificate as valid, is used $@. | UnsafeHostnameVerification.java:65:31:75:3 | new (...) : new HostnameVerifier(...) { ... } | This hostname verifier | UnsafeHostnameVerification.java:65:35:65:50 | new HostnameVerifier(...) { ... } | here | UnsafeHostnameVerification.java:76:49:76:56 | verifier | here | -| UnsafeHostnameVerification.java:89:49:89:56 | verifier | UnsafeHostnameVerification.java:83:31:88:3 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:89:49:89:56 | verifier | $@ that is defined $@ and accepts any certificate as valid, is used $@. | UnsafeHostnameVerification.java:83:31:88:3 | new (...) : new HostnameVerifier(...) { ... } | This hostname verifier | UnsafeHostnameVerification.java:83:35:83:50 | new HostnameVerifier(...) { ... } | here | UnsafeHostnameVerification.java:89:49:89:56 | verifier | here | +| 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 | From 1901f6bf55fdee3de7e22a2bab7935b878f7c03e Mon Sep 17 00:00:00 2001 From: intrigus Date: Tue, 12 Jan 2021 15:36:55 +0100 Subject: [PATCH 24/25] Java: Make @id @name of query more similar. --- .../ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql index 3ab49a866bb8..9c060565f284 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -1,10 +1,10 @@ /** - * @name Disabled hostname verification + * @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/insecure-hostname-verifier + * @id java/unsafe-hostname-verification * @tags security * external/cwe/cwe-297 */ From 2931e1f3fb4f68205c20db825d85d81f6782631b Mon Sep 17 00:00:00 2001 From: intrigus Date: Tue, 12 Jan 2021 15:37:45 +0100 Subject: [PATCH 25/25] Java: Add change note for #4771 --- java/change-notes/2021-01-12-unsafe-hostname-verification.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 java/change-notes/2021-01-12-unsafe-hostname-verification.md 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.