From 3f2e5749cd325050df0e5f923bb24b6d67b3c2a4 Mon Sep 17 00:00:00 2001 From: intrigus Date: Mon, 1 Jun 2020 23:43:12 +0200 Subject: [PATCH 1/3] [Java] CWE-295 - Incorrect Hostname Verification --- .../CWE-295/AlwaysTrueHostnameVerifier.java | 6 + .../CWE-295/AndroidHostnameVerifier.LICENSE | 38 ++++ .../CWE/CWE-295/AndroidHostnameVerifier.java | 20 ++ .../EverythingAcceptingHostnameVerifier.qhelp | 46 +++++ .../EverythingAcceptingHostnameVerifier.ql | 17 ++ .../CWE/CWE-295/HostnameValidation.qll | 183 ++++++++++++++++++ .../CWE-295/IncorrectHostnameVerifier.qhelp | 80 ++++++++ .../CWE/CWE-295/IncorrectHostnameVerifier.ql | 17 ++ .../CWE-295/LocalhostHostnameVerifier.java | 23 +++ .../CWE/CWE-295/PeerHostHostnameVerifier.java | 7 + .../RawSSLSocketEndpointIdentification.java | 9 + .../RawSSLSocketHostnameVerifierAndroid.java | 14 ++ ...erythingAcceptingHostnameVerifier.expected | 1 + .../EverythingAcceptingHostnameVerifier.java | 21 ++ .../EverythingAcceptingHostnameVerifier.qlref | 1 + .../IncorrectHostnameVerifier.expected | 7 + .../CWE-295/IncorrectHostnameVerifier.java | 100 ++++++++++ .../CWE-295/IncorrectHostnameVerifier.qlref | 1 + 18 files changed, 591 insertions(+) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-295/AlwaysTrueHostnameVerifier.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-295/AndroidHostnameVerifier.LICENSE create mode 100644 java/ql/src/experimental/Security/CWE/CWE-295/AndroidHostnameVerifier.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-295/EverythingAcceptingHostnameVerifier.qhelp create mode 100644 java/ql/src/experimental/Security/CWE/CWE-295/EverythingAcceptingHostnameVerifier.ql create mode 100644 java/ql/src/experimental/Security/CWE/CWE-295/HostnameValidation.qll create mode 100644 java/ql/src/experimental/Security/CWE/CWE-295/IncorrectHostnameVerifier.qhelp create mode 100644 java/ql/src/experimental/Security/CWE/CWE-295/IncorrectHostnameVerifier.ql create mode 100644 java/ql/src/experimental/Security/CWE/CWE-295/LocalhostHostnameVerifier.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-295/PeerHostHostnameVerifier.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-295/RawSSLSocketEndpointIdentification.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-295/RawSSLSocketHostnameVerifierAndroid.java create mode 100644 java/ql/test/experimental/query-tests/security/CWE-295/EverythingAcceptingHostnameVerifier.expected create mode 100644 java/ql/test/experimental/query-tests/security/CWE-295/EverythingAcceptingHostnameVerifier.java create mode 100644 java/ql/test/experimental/query-tests/security/CWE-295/EverythingAcceptingHostnameVerifier.qlref create mode 100644 java/ql/test/experimental/query-tests/security/CWE-295/IncorrectHostnameVerifier.expected create mode 100644 java/ql/test/experimental/query-tests/security/CWE-295/IncorrectHostnameVerifier.java create mode 100644 java/ql/test/experimental/query-tests/security/CWE-295/IncorrectHostnameVerifier.qlref diff --git a/java/ql/src/experimental/Security/CWE/CWE-295/AlwaysTrueHostnameVerifier.java b/java/ql/src/experimental/Security/CWE/CWE-295/AlwaysTrueHostnameVerifier.java new file mode 100644 index 000000000000..ccc6f07caaf8 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-295/AlwaysTrueHostnameVerifier.java @@ -0,0 +1,6 @@ +HostnameVerifier hostnameVerifier = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + return true; // BAD, verifies anything as correct + } +}; \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-295/AndroidHostnameVerifier.LICENSE b/java/ql/src/experimental/Security/CWE/CWE-295/AndroidHostnameVerifier.LICENSE new file mode 100644 index 000000000000..0a9014d93e9b --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-295/AndroidHostnameVerifier.LICENSE @@ -0,0 +1,38 @@ +// Licensed under Apache 2.0 per https://developer.android.com/license +/* +Copyright 2020 Android/Android + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + + +// Create an HostnameVerifier that hardwires the expected hostname. +// Note that is different than the URL's hostname: +// example.com versus example.org +HostnameVerifier hostnameVerifier = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + HostnameVerifier hv = + HttpsURLConnection.getDefaultHostnameVerifier(); + return hv.verify("example.com", session); // OKAY-ISH: verify that the certificate matches + // example.com and only accept example.com as an alternative to example.org + // BETTER: fix the underlying configuration problem that causes example.org to present the certificate for the wrong domain. + } +}; + +// Tell the URLConnection to use our HostnameVerifier +URL url = new URL("https://example.org/"); +HttpsURLConnection urlConnection = + (HttpsURLConnection)url.openConnection(); +urlConnection.setHostnameVerifier(hostnameVerifier); +InputStream in = urlConnection.getInputStream(); \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-295/AndroidHostnameVerifier.java b/java/ql/src/experimental/Security/CWE/CWE-295/AndroidHostnameVerifier.java new file mode 100644 index 000000000000..9a3e7fe7a303 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-295/AndroidHostnameVerifier.java @@ -0,0 +1,20 @@ +// Create an HostnameVerifier that hardwires the expected hostname. +// Note that is different than the URL's hostname: +// example.com versus example.org +HostnameVerifier hostnameVerifier = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + HostnameVerifier hv = + HttpsURLConnection.getDefaultHostnameVerifier(); + return hv.verify("example.com", session); // OKAY-ISH: verify that the certificate matches + // example.com and only accept example.com as an alternative to example.org + // BETTER: fix the underlying configuration problem that causes example.org to present the certificate for the wrong domain. + } +}; + +// Tell the URLConnection to use our HostnameVerifier +URL url = new URL("https://example.org/"); +HttpsURLConnection urlConnection = + (HttpsURLConnection)url.openConnection(); +urlConnection.setHostnameVerifier(hostnameVerifier); +InputStream in = urlConnection.getInputStream(); \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-295/EverythingAcceptingHostnameVerifier.qhelp b/java/ql/src/experimental/Security/CWE/CWE-295/EverythingAcceptingHostnameVerifier.qhelp new file mode 100644 index 000000000000..899e9335c8a4 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-295/EverythingAcceptingHostnameVerifier.qhelp @@ -0,0 +1,46 @@ + + + +

+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 on 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. +
    • +
  • If you use an unverifying verifier to connect to a local host you should instead create a self-signed certificate for your local host and import that certificate to the java key store
    • +If you do not verify the hostname, you allow any attacker to perform a man-in-the-middle attack. +

    + +     + + +

    +In the first 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. +

    + +     + + +
  • Android Security Guide for TLS/HTTPS.
    • +
  • Further Information on Hostname Verification.
    • +
  • OWASP: CWE-295.
    • +     +     diff --git a/java/ql/src/experimental/Security/CWE/CWE-295/EverythingAcceptingHostnameVerifier.ql b/java/ql/src/experimental/Security/CWE/CWE-295/EverythingAcceptingHostnameVerifier.ql new file mode 100644 index 000000000000..e850d85c3e99 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-295/EverythingAcceptingHostnameVerifier.ql @@ -0,0 +1,17 @@ +/** + * @name Disabled hostname verification + * @description Accepting any certificate as valid for a host allows an attacker to perform a man-in-the-middle attack. + * @kind alert + * @problem.severity error + * @precision high + * @id java/everything-accepting-hostname-verifier + * @tags security + * external/cwe/cwe-295 + */ + +import java +import HostnameValidation + +from AlwaysAcceptingVerifyMethod m +where not m.getDeclaringType() instanceof TestClass +select m, "$@ accepts any certificate as valid for a host.", m, "This method" diff --git a/java/ql/src/experimental/Security/CWE/CWE-295/HostnameValidation.qll b/java/ql/src/experimental/Security/CWE/CWE-295/HostnameValidation.qll new file mode 100644 index 000000000000..2add152c21db --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-295/HostnameValidation.qll @@ -0,0 +1,183 @@ +import java +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.security.Encryption + +/** A method that overrides the `javax.net.ssl.HostnameVerifier.verify` method. */ +class OverridenVerifyMethod extends HostnameVerifierVerify { + OverridenVerifyMethod() { this instanceof HostnameVerifierVerify } + + /** + * The `hostname` parameter of this method. + */ + Parameter getHostnameParameter() { result = getParameter(0) } + + /** + * The `session` parameter of this method. + */ + Parameter getSessionParameter() { result = getParameter(1) } +} + +/** A return statement that returns `true`. */ +class TrueReturnStmt extends ReturnStmt { + TrueReturnStmt() { getResult().(CompileTimeConstantExpr).getBooleanValue() = true } +} + +/** + * Holds if `m` always returns `true` ignoring any exceptional flow. + */ +private predicate alwaysReturnsTrue(OverridenVerifyMethod m) { + forex(ReturnStmt rs | rs.getEnclosingCallable() = m | rs instanceof TrueReturnStmt) +} + +/** A method that overrides the `javax.net.ssl.HostnameVerifier.verify` method in a dangerous way. */ +abstract class DangerousVerifyMethod extends OverridenVerifyMethod { + /** A `Stmt` that makes this `HostnameVerifier` dangerous. */ + abstract Stmt getADangerousStmt(); +} + +/** + * A method that overrides the `javax.net.ssl.HostnameVerifier.verify` method and **always** returns `true`, thus + * accepting any certificate despite a hostname mismatch. + */ +class AlwaysAcceptingVerifyMethod extends DangerousVerifyMethod { + AlwaysAcceptingVerifyMethod() { alwaysReturnsTrue(this) } + + override Stmt getADangerousStmt() { + exists(TrueReturnStmt r | r.getEnclosingCallable() = this | result = r) + } +} + +/** An access to any method whose name starts with `equals` and which is defined in the `String` class. */ +class StringEqualsMethodAccess extends MethodAccess { + StringEqualsMethodAccess() { + getMethod().getName().matches("equals%") and + getMethod().getDeclaringType() instanceof TypeString + } + + // TODO there is probably a better name for "anEqualityPart" but I don't know any right now + /** Returns a part of this equality check, i.e. in the case of `foo.equals(bar)` this returns `foo` and `bar`. */ + Expr getAnEqualityPart() { result = getQualifier() or result = getArgument(0) } +} + +/** + * Get the nearest enclosing if statement, if there exists one. + * For example consider this case: + * ```` + * if(foo) { + * if(bar) { + * stmt + * } + * } + * ```` + * This query would then return the `bar` if statement and **not** the `foo` if statement. + */ +private IfStmt getNearestEnclosingIfStmt(Stmt stmt) { + //result = getNerestEnclosingIfStmtRecursive(stmt) and result != stmt + exists(IfStmt ifStmt | + result = ifStmt and + ( + ifStmt.getElse().getAChild*() = stmt and + /* + * Ensure that the `IfStmt` we found does not contain another `IfStmt`. + * If it contains one, the result can not be the nearest enclosing `IfStmt`. + */ + + not any(ifStmt.getElse().getAChild*()) instanceof IfStmt + or + ifStmt.getThen().getAChild*() = stmt and + /* + * Ensure that the `IfStmt` we found does not contain another `IfStmt`. + * If it contains one, the result can not be the nearest enclosing `IfStmt`. + */ + + not any(ifStmt.getThen().getAChild*()) instanceof IfStmt + ) + ) +} + +/** The `javax.net.ssl.SSLSession.getPeerHost` method. */ +private class SSLSessionGetPeerHostMethodAccess extends MethodAccess { + SSLSessionGetPeerHostMethodAccess() { + getMethod().hasName("getPeerHost") and + getMethod().getDeclaringType() instanceof SSLSession + } +} + +/** + * A `TaintTracking::Configuration` where the `source` is a parameter. + * This configuration considers any getter on `SSLSession` as being tainted, but ignores any calls to `SSLSession.getPeerHost`. + */ +private class ParameterTaintTracking extends TaintTracking::Configuration { + ParameterTaintTracking() { this = "ParameterTaintTracking" } + + override predicate isSource(DataFlow::Node source) { source instanceof DataFlow::ParameterNode } + + override predicate isSink(DataFlow::Node sink) { sink instanceof DataFlow::ExprNode } + + override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { + node1.asExpr().getType() instanceof SSLSession and + exists(MethodAccess ma | ma.getMethod().getName().matches("get%") | node2.asExpr() = ma) + } + + override predicate isSanitizer(DataFlow::Node node) { + node.asExpr() instanceof SSLSessionGetPeerHostMethodAccess + } +} + +/** Holds if `e` is derived from `parameter`. This is approximated by checking whether `e` gets tainted by `parameter`. */ +private predicate isDerivedFromParameter(Parameter parameter, Expr e) { + exists(ParameterTaintTracking t, DataFlow::ExprNode sink, DataFlow::ParameterNode source | + source = DataFlow::parameterNode(parameter) and sink = DataFlow::exprNode(e) + | + t.hasFlow(source, sink) + ) +} + +/** + * A (dangerous) `StringEqualsMethodAccess` where the equality does: + * 1. not depend on a expression that is derived from the `session` parameter of the `verify` method. + * 2. depend on a expression that is derived from the `hostname` parameter of the `verify` method. + */ +class SSLSessionParameterIgnoringStringEqualsMethodAccess extends StringEqualsMethodAccess { + SSLSessionParameterIgnoringStringEqualsMethodAccess() { + exists(Expr hostname, Expr other, Parameter hostnameParameter, Parameter sessionParameter | + hostnameParameter = getEnclosingCallable().(OverridenVerifyMethod).getHostnameParameter() and + sessionParameter = getEnclosingCallable().(OverridenVerifyMethod).getSessionParameter() and + getAnEqualityPart() = hostname and + getAnEqualityPart() = other and + hostname != other + | + not isDerivedFromParameter(sessionParameter, other) and + isDerivedFromParameter(hostnameParameter, hostname) + ) + } +} + +/** + * A `verify` method that does not verify the `hostname` correctly. It is incorrect + * because it does not validate the `hostname` against any expression that is derived from the `session` parameter of the `verify` method. + */ +class IncorrectHostnameVerifyMethod extends DangerousVerifyMethod { + Stmt dangerousStmt; + + IncorrectHostnameVerifyMethod() { + /* We return `true` based on an if statement that verifies the `hostname` incorrectly.*/ + exists(TrueReturnStmt r, IfStmt i | + getBody().getAChild*() = r and i = getNearestEnclosingIfStmt(r) + | + i.getCondition().getAChildExpr*() instanceof + SSLSessionParameterIgnoringStringEqualsMethodAccess and + dangerousStmt = i + ) + or + /* We return the result of an `equals` call that verifies the `hostname` incorrectly.*/ + exists(ReturnStmt r | getBody().getAChild*() = r | + not r.getResult() instanceof BooleanLiteral and + r.getResult().getAChildExpr*() instanceof SSLSessionParameterIgnoringStringEqualsMethodAccess and + dangerousStmt = r + ) + } + + override Stmt getADangerousStmt() { result = dangerousStmt } +} diff --git a/java/ql/src/experimental/Security/CWE/CWE-295/IncorrectHostnameVerifier.qhelp b/java/ql/src/experimental/Security/CWE/CWE-295/IncorrectHostnameVerifier.qhelp new file mode 100644 index 000000000000..b4da1b8feaf6 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-295/IncorrectHostnameVerifier.qhelp @@ -0,0 +1,80 @@ + + + +

    +If a HostnameVerifier incorrectly verifies the hostname it might not verify the hostname at all. +This allows an attacker to perform a man-in-the-middle attack on the application therefore breaking any security Transport Layer Security (TLS) gives. +

    +     + + +

    +Do NOT use an incorrect HostnameVerifier! +A HostnameVerifier is typically used for either an URL/HttpsURLConnection or a raw SSLSocket. +A raw SSLSocket does not do any hostname verification so a working hostname verification is important. +When you use an URL/HttpsURLConnection: +

    +

  • If you use a verifier to solve a configuration problem with TLS/HTTPS you should solve the configuration problem instead. +
    • +
  • If you use a verifier to connect to a local host you should instead create a self-signed certificate for your local host and import that certificate to the java key store
    • +

    + +When you use a raw SSLSocket: +

    +

  • Use SSLParameters.setEndpointIdentificationAlgorithm("HTTPS") to enable hostname verification. This is available starting with Java 7 and Android Api Level 24.
    • +
  • Before Android Api Level 24 you should use the HostnameVerifier from HttpsURLConnection.getDefaultHostnameVerifier() to verify the hostname.
    • +If you do not verify the hostname, you allow any attacker to perform a man-in-the-middle attack. +

    + +     + + +

    +In the first example, the HostnameVerifier verifies the hostname against session.getPeerHost(). +This allows an attacker to perform a man-in-the-middle attack, because getPeerHost() is not authenticated and will always be equal to the hostname. +

    + + +

    +In the second example, the HostnameVerifier verifies the hostname against different local hosts. +Comparing hostname against localhost does not verify the certificate. In theory traffic to localhost +should never leave the host. In practice this may not always be the case due to misconfigurations. +Comparing hostname against 127.0.0.1 or ::1 does not verify the certificate. These adresses are loopback adresses and traffic +MUST never leave the host. So this SHOULD be safe, but nevertheless it would be better to use a proper self-signed certificate. +

    + + +

    +In the third example, we want to connect to the example.org host, but the host presents a certificate for example.com. +Assuming that example.org and example.com are under our control (and only then this is ok) we can use the default HostnameVerifier +to verify that the certificate belongs to example.com. +Note that you should really fix the configuration problem that causes example.org to present the certificate for example.com instead of using this method. +This method **only** works on Android, on (OpenJDK) Java the default HostnameVerifier will always return false. +On (OpenJDK) Java you can use this implementation of a working +hostname verifier instead of the default HostnameVerifier. +

    + + +

    +In the fourth example, we properly use a raw SSLSocket with enabled SSLParameters.setEndpointIdentificationAlgorithm("HTTPS"), thus verifying hostnames correctly. +Note that this method is only available with Java 7 and Android Api Level 24. +The next example shows an alternative for Android Api Levels before 24. +

    + + +

    +In the fifth example, we properly use a raw SSLSocket and verify the hostname using the HostnameVerifier from HttpsURLConnection.getDefaultHostnameVerifier(). +Note that this is also works on Android Api Levels before 24. +But also note that this will **not** work on (OpenJDK) Java, because the default HostnameVerifier will always return false. +

    + +     + + +
  • Android Security Guide for TLS/HTTPS.
    • +
  • Further Information on Hostname Verification.
    • +
  • OWASP: CWE-295.
    • +     +     diff --git a/java/ql/src/experimental/Security/CWE/CWE-295/IncorrectHostnameVerifier.ql b/java/ql/src/experimental/Security/CWE/CWE-295/IncorrectHostnameVerifier.ql new file mode 100644 index 000000000000..3d2ea747defa --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-295/IncorrectHostnameVerifier.ql @@ -0,0 +1,17 @@ +/** + * @name Incorrect hostname verification + * @description Incorrectly verifying a hostname allows an attacker to perform a man-in-the-middle attack. + * @kind alert + * @problem.severity error + * @precision high + * @id java/incorrect-hostname-verifier + * @tags security + * external/cwe/cwe-295 + */ + +import java +import HostnameValidation + +from IncorrectHostnameVerifyMethod m, Stmt dangerousStmt +where dangerousStmt = m.getADangerousStmt() and not m.getDeclaringType() instanceof TestClass +select dangerousStmt, "$@ incorrectly verifies the hostname.", dangerousStmt, "This statement" diff --git a/java/ql/src/experimental/Security/CWE/CWE-295/LocalhostHostnameVerifier.java b/java/ql/src/experimental/Security/CWE/CWE-295/LocalhostHostnameVerifier.java new file mode 100644 index 000000000000..11bbcab00970 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-295/LocalhostHostnameVerifier.java @@ -0,0 +1,23 @@ +HostnameVerifier localHostHostnameVerifier = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + if (hostname.equals("localhost")) { // BAD, does not verify the certificate. In theory traffic to + // "localhost" should never leave the host. In practice this may not + // always be the case due to misconfigurations. + return true; + } else if (hostname.equals("127.0.0.1")) { // BAD, does not verify the certificate [Debatable Security + // Impact]. "127.0.0.1" is the IPv4 loopback adress and traffic + // MUST never leave the host. So this SHOULD be safe, but + // nevertheless it would be better to use a proper self-signed + // certificate. + return true; + } else if (hostname.equals("::1")) { // BAD, does not verify the certificate [Debatable Security Impact]. + // "::1" is the IPv6 loopback adress and traffic MUST never leave + // the host. So this SHOULD be safe, but nevertheless it would be + // better to use a proper self-signed certificate. + return true; + } + return false; + + } +}; \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-295/PeerHostHostnameVerifier.java b/java/ql/src/experimental/Security/CWE/CWE-295/PeerHostHostnameVerifier.java new file mode 100644 index 000000000000..2c1312a1e05c --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-295/PeerHostHostnameVerifier.java @@ -0,0 +1,7 @@ +HostnameVerifier hostnameVerifier = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + return hostname.equals(session.getPeerHost()); // BAD, getPeerHost() is not authenticated + // and will always be equal to the hostname, therefore trusting ALL hostnames + } +}; \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-295/RawSSLSocketEndpointIdentification.java b/java/ql/src/experimental/Security/CWE/CWE-295/RawSSLSocketEndpointIdentification.java new file mode 100644 index 000000000000..66b5dc1680a1 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-295/RawSSLSocketEndpointIdentification.java @@ -0,0 +1,9 @@ +// Open SSLSocket directly to example.org. +SocketFactory sf = SSLSocketFactory.getDefault(); +SSLSocket socket = (SSLSocket) sf.createSocket("example.org", 443); +SSLParameters sslParams = new SSLParameters(); +sslParams.setEndpointIdentificationAlgorithm("HTTPS"); // GOOD, enable hostname verification using the `HTTPS` + // verification algorithm. +socket.setSSLParameters(sslParams); + +// Hostname verification has been performed and we can proceed. \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-295/RawSSLSocketHostnameVerifierAndroid.java b/java/ql/src/experimental/Security/CWE/CWE-295/RawSSLSocketHostnameVerifierAndroid.java new file mode 100644 index 000000000000..a430087141da --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-295/RawSSLSocketHostnameVerifierAndroid.java @@ -0,0 +1,14 @@ +// Open SSLSocket directly to example.org. +SocketFactory sf = SSLSocketFactory.getDefault(); +SSLSocket socket = (SSLSocket) sf.createSocket("example.org", 443); +HostnameVerifier hv = HttpsURLConnection.getDefaultHostnameVerifier(); +SSLSession s = socket.getSession(); + +// Verify that the certificate matches the hostname. +if (!hv.verify(s.getPeerHost(), s)) { // GOOD + // We use the default hostname verifier from Android to verify that the + // certificate matches. + throw new SSLHandshakeException("Expected <" + s.getPeerHost() + ">, but " + "found " + s.getPeerPrincipal()); +} + +// Hostname verification has been performed and we can proceed. \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-295/EverythingAcceptingHostnameVerifier.expected b/java/ql/test/experimental/query-tests/security/CWE-295/EverythingAcceptingHostnameVerifier.expected new file mode 100644 index 000000000000..f2f92bd65bef --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-295/EverythingAcceptingHostnameVerifier.expected @@ -0,0 +1 @@ +| EverythingAcceptingHostnameVerifier.java:7:18:7:23 | verify | $@ accepts any certificate as valid for a host. | EverythingAcceptingHostnameVerifier.java:7:18:7:23 | verify | This method | diff --git a/java/ql/test/experimental/query-tests/security/CWE-295/EverythingAcceptingHostnameVerifier.java b/java/ql/test/experimental/query-tests/security/CWE-295/EverythingAcceptingHostnameVerifier.java new file mode 100644 index 000000000000..fc491b81fd0f --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-295/EverythingAcceptingHostnameVerifier.java @@ -0,0 +1,21 @@ +import javax.net.ssl.HostnameVerifier; +import javax.net.ssl.SSLSession; + +public class EverythingAcceptingHostnameVerifier { + HostnameVerifier hostnameVerifier = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + return true; // BAD, always returns true, accepts everything + } + }; + + HostnameVerifier hostnameVerifier2 = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + if (hostname.equals("localhost")) { + return true; // BAD [Should not be detected by `EverythingAcceptingHostnameVerifier.ql`] + } + return false; + } + }; +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-295/EverythingAcceptingHostnameVerifier.qlref b/java/ql/test/experimental/query-tests/security/CWE-295/EverythingAcceptingHostnameVerifier.qlref new file mode 100644 index 000000000000..ea321c0ba9fa --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-295/EverythingAcceptingHostnameVerifier.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-295/EverythingAcceptingHostnameVerifier.ql diff --git a/java/ql/test/experimental/query-tests/security/CWE-295/IncorrectHostnameVerifier.expected b/java/ql/test/experimental/query-tests/security/CWE-295/IncorrectHostnameVerifier.expected new file mode 100644 index 000000000000..1e784ba0c564 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-295/IncorrectHostnameVerifier.expected @@ -0,0 +1,7 @@ +| EverythingAcceptingHostnameVerifier.java:15:4:15:36 | stmt | $@ incorrectly verifies the hostname. | EverythingAcceptingHostnameVerifier.java:15:4:15:36 | stmt | This statement | +| IncorrectHostnameVerifier.java:9:4:9:49 | stmt | $@ incorrectly verifies the hostname. | IncorrectHostnameVerifier.java:9:4:9:49 | stmt | This statement | +| IncorrectHostnameVerifier.java:18:4:18:40 | stmt | $@ incorrectly verifies the hostname. | IncorrectHostnameVerifier.java:18:4:18:40 | stmt | This statement | +| IncorrectHostnameVerifier.java:26:4:26:36 | stmt | $@ incorrectly verifies the hostname. | IncorrectHostnameVerifier.java:26:4:26:36 | stmt | This statement | +| IncorrectHostnameVerifier.java:30:11:30:43 | stmt | $@ incorrectly verifies the hostname. | IncorrectHostnameVerifier.java:30:11:30:43 | stmt | This statement | +| IncorrectHostnameVerifier.java:36:11:36:37 | stmt | $@ incorrectly verifies the hostname. | IncorrectHostnameVerifier.java:36:11:36:37 | stmt | This statement | +| IncorrectHostnameVerifier.java:97:4:97:46 | stmt | $@ incorrectly verifies the hostname. | IncorrectHostnameVerifier.java:97:4:97:46 | stmt | This statement | diff --git a/java/ql/test/experimental/query-tests/security/CWE-295/IncorrectHostnameVerifier.java b/java/ql/test/experimental/query-tests/security/CWE-295/IncorrectHostnameVerifier.java new file mode 100644 index 000000000000..995cc56ca929 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-295/IncorrectHostnameVerifier.java @@ -0,0 +1,100 @@ +import javax.net.ssl.HostnameVerifier; +import javax.net.ssl.SSLSession; +import javax.net.ssl.HttpsURLConnection; + +public class IncorrectHostnameVerifier { + HostnameVerifier peerHostHostnameVerifier = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + return hostname.equals(session.getPeerHost()); // BAD, getPeerHost() is not authenticated and will always be + // equal to the hostname, therefore trusting ALL hostnames. + } + }; + + HostnameVerifier peerHostHostnameVerifier2 = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + String peerHostname = session.getPeerHost(); + return hostname.equals(peerHostname); // BAD, getPeerHost() is not authenticated and will always be equal to + // the hostname, therefore trusting ALL hostnames. + } + }; + + HostnameVerifier localHostHostnameVerifier = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + if (hostname.equals("localhost")) { // BAD, does not verify the certificate. In theory traffic to + // "localhost" should never leave the host. In practice this may not + // always be the case due to misconfigurations. + return true; + } else if (hostname.equals("127.0.0.1")) { // BAD, does not verify the certificate [Debatable Security + // Impact]. "127.0.0.1" is the IPv4 loopback adress and traffic + // MUST never leave the host. So this SHOULD be safe, but + // nevertheless it would be better to use a proper self-signed + // certificate. + return true; + } else if (hostname.equals("::1")) { // BAD, does not verify the certificate [Debatable Security Impact]. + // "::1" is the IPv6 loopback adress and traffic MUST never leave + // the host. So this SHOULD be safe, but nevertheless it would be + // better to use a proper self-signed certificate. + return true; + } + return false; + + } + }; + + HostnameVerifier delegatingHostnameVerifier = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + HostnameVerifier verifier = HttpsURLConnection.getDefaultHostnameVerifier(); + if (hostname.equals("example.com")) { + return verifier.verify("example.org", session); // "GOOD-ISH", if and only if this holds: + // We want to make a connection to `https://example.com`, but the webserver is + // misconfigured and wrongly returns the certificate for `https://example.org`. + // IF we control BOTH example.com and example.org we accept an certificate for + // `example.org` as valid for `example.com`. + // NOTE: It's STRONGLY suggested to fix the certificate problem instead. + // NOTE: This will only work on Android, on (OpenJDK) Java the default + // `HostnameVerifier` always returns false! + } + return false; + } + }; + + HostnameVerifier externalMethodHostnameVerifier = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + return checkHostname(hostname); // BAD [NOT DETECTED], only the implementation of `verify` method is + // checked. If the `verify` method calls other methods we can not reason + // about them. A simple implementation would be to detect that the external + // method only checks `hostname` and not a value derived from `session`. + } + + private boolean checkHostname(String hostname) { + return hostname.equals("example.com"); // BAD [NOT DETECTED], does not verify the certificate. + } + }; + + HostnameVerifier externalMethodHostnameVerifier2 = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + return check(hostname, session); // BAD [NOT DETECTED], only the implementation of the `verify` method is + // checked. If the `verify` method calls other methods we can not reason + // about them. The simple implementation from above + // (`externalMethodHostnameVerifier`) (if implemented) should not detect + // this. + } + + private boolean check(String hostname, SSLSession session) { + return hostname.equals("example.com"); // BAD [NOT DETECTED], does not verify the certificate. + } + }; + + HostnameVerifier hostSpecificHostnameVerifier = new HostnameVerifier() { + @Override + public boolean verify(String hostname, SSLSession session) { + return hostname.equals("host.example.org"); // BAD, does not verify the certificate for `host.example.org`. + } + }; +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-295/IncorrectHostnameVerifier.qlref b/java/ql/test/experimental/query-tests/security/CWE-295/IncorrectHostnameVerifier.qlref new file mode 100644 index 000000000000..aa730dfa296c --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-295/IncorrectHostnameVerifier.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-295/IncorrectHostnameVerifier.ql From 3f1d01df0d7afa50aaa4727b292cc3ff2ad781fd Mon Sep 17 00:00:00 2001 From: intrigus Date: Mon, 1 Jun 2020 23:43:48 +0200 Subject: [PATCH 2/3] [Java] Add SSLSession Type --- java/ql/src/semmle/code/java/security/Encryption.qll | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/java/ql/src/semmle/code/java/security/Encryption.qll b/java/ql/src/semmle/code/java/security/Encryption.qll index c9d2b6e4ddb7..760c2c0fb6a5 100644 --- a/java/ql/src/semmle/code/java/security/Encryption.qll +++ b/java/ql/src/semmle/code/java/security/Encryption.qll @@ -21,6 +21,10 @@ class SSLSocketFactory extends RefType { SSLSocketFactory() { this.hasQualifiedName("javax.net.ssl", "SSLSocketFactory") } } +class SSLSession extends RefType { + SSLSession() { hasQualifiedName("javax.net.ssl", "SSLSession") } +} + class SSLContext extends RefType { SSLContext() { hasQualifiedName("javax.net.ssl", "SSLContext") } } From f5d49b20b4dbd4ecdb2b6cfbbac84828bcdbd946 Mon Sep 17 00:00:00 2001 From: intrigus Date: Wed, 24 Jun 2020 23:49:07 +0200 Subject: [PATCH 3/3] Temporary fix --- .../CWE/CWE-295/HostnameValidation.qll | 42 +++++++++++++------ 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-295/HostnameValidation.qll b/java/ql/src/experimental/Security/CWE/CWE-295/HostnameValidation.qll index 2add152c21db..96e44f885407 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-295/HostnameValidation.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-295/HostnameValidation.qll @@ -108,29 +108,47 @@ private class SSLSessionGetPeerHostMethodAccess extends MethodAccess { * A `TaintTracking::Configuration` where the `source` is a parameter. * This configuration considers any getter on `SSLSession` as being tainted, but ignores any calls to `SSLSession.getPeerHost`. */ -private class ParameterTaintTracking extends TaintTracking::Configuration { - ParameterTaintTracking() { this = "ParameterTaintTracking" } +/* + * private class ParameterTaintTracking extends TaintTracking::Configuration { + * ParameterTaintTracking() { this = "ParameterTaintTracking" } + */ - override predicate isSource(DataFlow::Node source) { source instanceof DataFlow::ParameterNode } +/*override */ predicate isSource(DataFlow::Node source) { + source instanceof DataFlow::ParameterNode +} - override predicate isSink(DataFlow::Node sink) { sink instanceof DataFlow::ExprNode } +/*override */ predicate isSink(DataFlow::Node sink) { sink instanceof DataFlow::ExprNode } - override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { - node1.asExpr().getType() instanceof SSLSession and - exists(MethodAccess ma | ma.getMethod().getName().matches("get%") | node2.asExpr() = ma) - } +/*override */ predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(MethodAccess ma | + ma.getMethod().getName().matches("get%") and + ma.getMethod().getDeclaringType() instanceof SSLSession and + not ma instanceof SSLSessionGetPeerHostMethodAccess + | + node2.asExpr() = ma and + ma.getQualifier() = node1.asExpr() + ) +} - override predicate isSanitizer(DataFlow::Node node) { - node.asExpr() instanceof SSLSessionGetPeerHostMethodAccess +class TaintedQualifierTaintStep extends TaintTracking::AdditionalTaintStep { + override predicate step(DataFlow::Node node1, DataFlow::Node node2) { + isAdditionalTaintStep(node1, node2) } } +/* + * override predicate isSanitizer(DataFlow::Node node) { + * node.asExpr() instanceof SSLSessionGetPeerHostMethodAccess + * } + * } + */ + /** Holds if `e` is derived from `parameter`. This is approximated by checking whether `e` gets tainted by `parameter`. */ private predicate isDerivedFromParameter(Parameter parameter, Expr e) { - exists(ParameterTaintTracking t, DataFlow::ExprNode sink, DataFlow::ParameterNode source | + exists(DataFlow::ExprNode sink, DataFlow::ParameterNode source | source = DataFlow::parameterNode(parameter) and sink = DataFlow::exprNode(e) | - t.hasFlow(source, sink) + TaintTracking::localTaint(source, sink) ) }