Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CONC-654] Stop leaking client identifying information to the server before the TLS handshake #227

Open
wants to merge 5 commits into
base: 3.3
Choose a base branch
from

Commits on Jun 12, 2023

  1. [CONC-648] Do not trust error packets received prior to TLS handshake…

    … completion
    
    MariaDB Connector/C does not distinguish [application-layer error
    packets](https://mariadb.com/kb/en/err_packet) that it receives prior to TLS
    handshake completion from those that it receives immediately after.
    
    (A trivially modified server built from
    dlenski/mariadb-server@demonstration_of_CONC-648_vulnerability
    can easily be used to demonstrate this.)
    
    Pre-TLS error packet received from this trivially modified server. This packet
    should NOT be trusted to actually originate from the server:
    
        $ mariadb --ssl --ssl-verify-server-cert -uUsername -pVerySecretPassword -h CONC-648.vuln.demo.server.com
        ERROR 1815 (HY000): Internal error: Client will accept this error as genuine even if running with --ssl --ssl-verify-server-cert, and even though this error is sent in plaintext PRIOR TO TLS HANDSHAKE.
    
    Post-(TLS handshake) error packet received from a normal MariaDB server upon
    an attempt to connect with incorrect credentials.  This error packet CAN be
    trusted to actually originate from the server, assuming transitive trust in
    the TLS protocol implementation and PKI-based certificate validation:
    
        $ mariadb --ssl --ssl-verify-server-cert -uUsername -pWrongPassword -h $NORMAL_MARIADB10.6.14_SERVER
        ERROR 1045 (28000): Access denied for user 'Username'@'A.B.C.D' (using password: YES)
    
    This client behavior opens up MariaDB Connector/C clients to an extremely
    straightforward [downgrade attack](https://en.wikipedia.org/wiki/Downgrade_attack).
    
    An on-path or pervasive attacker can inject errors into MariaDB
    client→server connections that are intended to be protected by TLS, and the
    client has no clear mechanism to distinguish such errors from errors that
    actually come from the server.
    
    An attacker could easily use this to DOS a client, or even influence its
    behavior.  For example, consider a client application which is configured…
    
    1. To use TLS with server certificate validation
       (`--ssl --ssl-verify-server-cert`), and
    2. To wait for a back-off period and then *retry* connection attempts if the server
       responds with `ER_CON_COUNT_ERROR` ("Too many connections") from the
       server, and
    3. To give up and shut down if its connection attempts fail with
       `ER_ACCESS_DENIED_ERROR` ("Access denied for user"), on the assumption
       that this is due to an incorrect or expired password, and cannot be
       resolved without human intervention.
    
    An attacker could completely disable the retry mechanism of this application
    by intercepting connection attempts and replying with
    `ER_ACCESS_DENIED_ERROR` packets.
    
    This patch modifies MariaDB Connector/C so that if the client is configured
    to use TLS, error packets received prior to the completion of the TLS
    handshake are untrusted, and are changed to a generic `CR_CONNECTION_ERROR`.
    
        $ mariadb --ssl --ssl-verify-server-cert -uUsername -pVerySecretPassword -h CONC-648.vuln.demo.server.com
        ERROR 2002 (HY000): Received error packet before completion of TLS handshake. Suppressing its details so that the client cannot vary its behavior based on this UNTRUSTED input.
    
    All new code of the whole pull request, including one or several files
    that are either new files or modified ones, are contributed under the
    BSD-new license. I am contributing on behalf of my employer Amazon Web
    Services, Inc.
    dlenski committed Jun 12, 2023
    Configuration menu
    Copy the full SHA
    abad64b View commit details
    Browse the repository at this point in the history

Commits on Jun 28, 2023

  1. [MDEV-28634] Do not allow silent downgrade of connections where SSL i…

    …s requested
    
    The 2015 commit
    mariadb-corporation@23895fbd4#diff-4339ae6506ef1fb201f6f836085257e72c191d2b4498df507d499fc30d891005
    was described as "Fixed gnutls support", which is an extremely incomplete
    description of what it actually does.
    
    In that commit, the Connector/C client authentication plugin was modified to
    abort following the initial server greeting packet if the client has
    requested a secure transport (`mariadb --ssl`), but the server does not
    advertise support for SSL/TLS.
    
    However, there's a crucial caveat here:
    
    If the client has requested secure transport (`mariadb --ssl`) but has not
    requested verification of the server's TLS certificate
    (`mariadb --ssl --ssl-verify-server-cert`), then the client will silently
    accept an unencrypted connection, without even printing a diagnostic message.
    
    Thus, any such client is susceptible to a trivial downgrade to an
    unencrypted connection by an on-path attacker who simply flips the TLS/SSL
    capability bit in the advertised server capabilities in the greeting packet.
    
    The entire design of MariaDB's TLS support is severely flawed in terms of
    user expectations surrounding secure-by-default connections: the `--ssl`
    option SHOULD imply `--ssl-verify-server-cert` BY DEFAULT; if a client
    actually wants a TLS connection that's susceptible to a trivial MITM'ed
    by any pervasive or on-path attacker, that should be an exceptional case
    (e.g. `--insecure-ssl-without-server-cert-verification`) rather than the
    default.
    
    Even without resolving the issue of no default verification of server
    certificates, the issue of silent downgrade to unencrypted connections can
    and should be resolved.
    
    Backwards compatibility: This is an INTENTIONAL backwards-incompatible
    change to prevent clients from being silently downgraded to unencrypted
    connections, when they've specified `--ssl` and thus clearly indicated that
    they do not want unencrypted connections.
    
    All new code of the whole pull request, including one or several files
    that are either new files or modified ones, are contributed under the
    BSD-new license. I am contributing on behalf of my employer Amazon Web
    Services, Inc.
    dlenski committed Jun 28, 2023
    Configuration menu
    Copy the full SHA
    165110d View commit details
    Browse the repository at this point in the history
  2. [MDEV-28634] Move check for server TLS/SSL capability to mthd_my_real…

    …_connect
    
    Two reasons:
    
    1. Reduction of attack surface
    
       As soon as the client receives the server's capability flags, it knows
       whether the server supports TLS/SSL.
    
       If the server does not support TLS/SSL, but the client expects and
       requires it, the client should immediately abort at this point in order
       to truncate any code paths by which it could inadvertently continue to
       communicate without TLS/SSL.
    
    2. Separation of concerns
    
       Whether or not the server supports TLS/SSL encryption at the transport
       layer (TLS stands for TRANSPORT-layer security) is a logically separate
       issue from what APPLICATION-layer authentication modes the client and
       server support or should use.
    
    All new code of the whole pull request, including one or several files
    that are either new files or modified ones, are contributed under the
    BSD-new license. I am contributing on behalf of my employer Amazon Web
    Services, Inc.
    dlenski committed Jun 28, 2023
    Configuration menu
    Copy the full SHA
    2ebbeb9 View commit details
    Browse the repository at this point in the history
  3. Add comments about usage of TLS/SSL in connection establishment

    All new code of the whole pull request, including one or several files
    that are either new files or modified ones, are contributed under the
    BSD-new license. I am contributing on behalf of my employer Amazon Web
    Services, Inc.
    dlenski committed Jun 28, 2023
    Configuration menu
    Copy the full SHA
    b090994 View commit details
    Browse the repository at this point in the history

Commits on Sep 15, 2023

  1. [CONC-654] Stop leaking client identifying information to the server …

    …before the TLS handshake
    
    The server implementation here was incorrect as well, unnecessarily
    reading—and TRUSTING—client identifying information sent before the TLS
    handshake.  That's in MDEV-31585.
    
    As a result of the server's mishandling of this information, it's not
    possible for the client to fix this in a way that's backwards-compatible
    with old servers.
    
    We rely on the server sending a capability bit to indicate that the
    server-side bug has been fixed:
    
        /* This capability is set if:
         *
         * - The CLIENT knows how to send a truncated 2-byte SSLRequest
         *   packet, containing no information other than the CLIENT_SSL flag
         *   which is necessary to trigger the TLS handshake, and to send its
         *   complete capability flags and other identifying information after
         *   the TLS handshake.
         * - The SERVER knows how to receive this truncated 2-byte SSLRequest
         *   packet, and to receive the client's complete capability bits
         *   after the TLS handshake.
         *
         */
        #define CLIENT_CAN_SSL_V2    (1ULL << 37)
    
    All new code of the whole pull request, including one or several files
    that are either new files or modified ones, are contributed under the
    BSD-new license. I am contributing on behalf of my employer Amazon Web
    Services, Inc.
    dlenski committed Sep 15, 2023
    Configuration menu
    Copy the full SHA
    5eec3aa View commit details
    Browse the repository at this point in the history