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

Conversation

dlenski
Copy link
Contributor

@dlenski dlenski commented Jul 3, 2023

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:

/* Server does not mishandle information sent in the plaintext
 * login request packet sent prior to the TLS handshake. As a result, the
 * client can safely send an empty/dummy packet contianing no
 * identifying information. Indicates that MDEV-31585 has been fixed.
 * Since ??.?.
 */
#define MARIADB_CLIENT_CAN_SEND_DUMMY_HANDSHAKE_PACKET    (1ULL << 37)

This PR also incorporates fixes for two previously-identified client-security vulnerabilities:

dlenski added 4 commits June 12, 2023 16:45
… 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.
…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.
…_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.
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.
{
my_set_error(mysql, CR_SERVER_LOST, SQLSTATE_UNKNOWN,
ER(CR_SERVER_LOST_EXTENDED),
"sending connection information to server",
"sending dummy client reply packet to server",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not a dummy packet unless server_allows_dummy_packet is set, better write something like

    server_allows_dummy_packet ? "sending dummy client reply packet to server"
                               : "sending connection information to server"

Copy link
Contributor Author

@dlenski dlenski Jul 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even in the status quo situation where this packet contains "too much information", it still doesn't contain complete "connection information."

Part of the problem here is that the naming of these exchanges is inconsistent both internally to the code and with the documentation, so it's hard to know what the "best" name to call any of it is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷‍♂️ okay

@vuvova
Copy link
Member

vuvova commented Jul 4, 2023

First, I only looked at the CONC-654 commit, not at other commits that this PR incorporates.

I think this looks pretty good. One conceptual question, though. Should it be a very specific flag "client can send dummy packet" or should it generally mean, like, "secure TLS mode"? Specifically if the server sets SECURE_SSL_MODE capability and the client replies with a 2-byte packet, equal to CLIENT_SSL, then the client will ignore all unencrypted packets from the server (or replace with a generic error, whatever). Also if there're more problems in SSL, they can all be fixed under the same flag

@vuvova vuvova self-assigned this Jul 4, 2023
@dlenski
Copy link
Contributor Author

dlenski commented Jul 4, 2023

@vuvova wrote:

First, I only looked at the CONC-654 commit, not at other commits that this PR incorporates.

Right, that makes sense. Those also exist in separate PRs, fine to consider them separately.

One conceptual question, though. Should it be a very specific flag "client can send dummy packet" or should it generally mean, like, "secure TLS mode"?

Yeah, I thought about this as well.

The problem is that we have no idea what other TLS-related bugs are still remaining; unfortunately, I won't be surprised to find more of them, given the very ad-hoc nature of the TLS setup code.

If we call this flag SERVER_SUPPORTS_ACTUALLY_SECURE_TLS_MODE, then it's going to be misleading and out of date as soon as we identify one more TLS-related bug.

All that we can really guarantee at the time of introducing this flag is that the server has fixed this one, specific TLS bug MDEV-31585 "Server improperly requires client capability bits to be sent IN PLAINTEXT prior to TLS handshake".

Perhaps the name could be improved on though. Maybe something like CLIENT_CAN_SEND_ONLY_2BYTE_SSLREQUEST_PACKET? Any thoughts on an optimal name?

@vuvova
Copy link
Member

vuvova commented Jul 4, 2023

Another thought. In all the discussions about protocol weaknesses you were considering a model with the active MitM attacker.

But if there's an active MitM attacker, he can turn this flag off, cannot he? And the client will leak again. Should it be a concern?

@dlenski
Copy link
Contributor Author

dlenski commented Jul 5, 2023

@vuvova wrote:

But if there's an active MitM attacker, he can turn this flag off, cannot he? And the client will leak again. Should it be a concern?

Yes, absolutely! That's a straightforward downgrade attack, akin to POODLE wherein MITM attackers downgrade clients who want to use TLSv1.x to SSLv3 by modifying the handshake packets to tell the client that the server only supports SSLv3.

That's why we need to make a client-side change which is backwards-incompatible by default in order to fix this.

In order not to be susceptible to this downgrade attack, the client would need to send the send the stripped-down "dummy" pre-(TLS handshake) unconditionally, even if the server doesn't actually advertise support for it (as I wrote in MariaDB/server#2684 (comment)).

However, but we can provide a --backwards-compatible-insecure-ssl client option/flag for clients that need to keep supporting unpatched/insecure servers for some time. As I wrote in MariaDB/server#2684 (comment), I have already implemented this further change in dlenski@force_client_to_opt_in_to_old_insecure_TLS?w=1.

If you agree with this, I'll add those further changes to this PR; I haven't done it so far, since I'm trying not to combine too many changes all at once in here.

@bgrainger
Copy link

That's why we need to make a client-side change which is backwards-incompatible by default in order to fix this.

In order not to be susceptible to this downgrade attack, the client would need to send the send the stripped-down "dummy" pre-(TLS handshake) unconditionally, even if the server doesn't actually advertise support for it (as I wrote in MariaDB/server#2684 (comment)).

This would bifurcate the client ecosystem into MySQL clients and MariaDB clients.

I'm the author of MySqlConnector, a .NET client library that supports MySQL, MariaDB, Percona Server, Amazon Aurora, Google Cloud SQL, Azure Database for MySQL, and anything else that can speak the MySQL protocol. There are probably dozens of people like me who maintain the OSS Python, Rust, Go, JavaScript, Perl, Haskell, etc. etc. drivers. (Some of these are FFI wrappers around the mysql C library, but many are full reimplementations in that language/runtime that avoid a native code dependency.) These OSS authors would have the following choices:

  1. Do nothing. (Maybe we haven't become aware of this issue, or maybe we don't think it's a big deal.)
  2. Add an opt-in connection-time flag for MariaDB users to avoid downgrade attacks.
  3. Break the ability to connect to any server except the very latest version of MariaDB and add an opt-out flag to send the old Handshake Response packet.

I don't collect analytics on server type, but I would guess it's (at least) 90% MySQL, 10% MariaDB. Unless the server-side change first gets accepted into MySQL Server Community Edition, and then also into the majority of cloud-hosted MySQL servers (some of which are proprietary forks of MySQL Server or reimplementations of the protocol), I don't think any client developer will choose option (3) and break their library for the vast majority of their users. It will probably be option (1) and then maybe option (2) if Oracle picks this up for MySQL Server or enough MariaDB users (of each library) specifically ask for it (or if MariaDB Server makes a breaking change that stops all those clients connecting).

Maybe MariaDB will solve this in their server and client implementation, but for now I don't see this achieving widespread adoption across the entire MySQL ecosystem.

@vuvova
Copy link
Member

vuvova commented Jul 6, 2023

So, let's consider three cases:

  1. new Connector/C connecting to the old MariaDB or or a third-party server
  2. new Connector/C connecting to the new MariaDB server
  3. old Connector/C or any third-party client connecting to the new MariaDB server

In the first case what a client can do to reduce the exposure?

  • No MariaDB or MySQL server ever (since when SSL support was added to MySQL in 2000) used max_allowed_packet or a charset from the client's SSL Request Packet. Meaning C/C can safely send hard-coded values there before the TLS handshare and send real values after. Or, may be, not send those bytes at all, but then it'll become an identifiable property on itself
  • a client can and should enable SSL by default and validate by default whaveter is possible (as described in MDEV-28634).

all this is safe and will work with any server implementation.

In the second case:

  • the client can detect the server (using the capability bit or a version string, see below) and send a 2-byte dummy packet as in this your PR. Passive MitM will only know it's a new client, but that's it.
  • Additionally, if you'll send not a 2-byte dummy but a proper SSL Request Packet with all bits hard-coded, passive MitM won't even know it's a new client. A new server will use capability bits from after the TLS handshake.
  • active MitM can downgrade this case to the first one and learn true client capabilities, but nothing beyond that.
  • with MDEV-31609 the server can send its version in the first OK packet after authentication. If the client will make its decisions based on the server version (not based on the capability bit), it can compare the version from the welcome packet with the version from the OK packet and detect active MitM (that's why using the version might be preferable)

In the third case

  • this is client side protection, the server cannot do anything, we just need to take care not to break other clients
  • old clients won't set a special combination of capability bits in the post-TLS-handshake packet, so the server will know it's an old client and won't re-read its capabilities.

This approach doesn't seem to break anything and only leaks client capabilities in case of the old server or active MitM.

@dlenski
Copy link
Contributor Author

dlenski commented Jul 6, 2023

@bgrainger wrote:

This would bifurcate the client ecosystem into MySQL clients and MariaDB clients.

I would prefer to describe this as "bifurcate the client ecosystem into clients that leak too much information before the TLS handshake, and clients that don't".

There's pretty clear evidence that MySQL/Oracle have long been aware of the fact that the clients were sending "too much information" before the TLS handshake…

https://github.com/mysql/mysql-server/blob/ea7087d885006918ad54458e7aad215b1650312c/sql/auth/sql_authentication.cc#L2830-L2840

… and yet the MySQL server implementation still requires the client to send a full 4 bytes of capabilities in the pre-TLS client intro packet. Just like MariaDB, it will not read the capability fields from the post-TLS client intro packet.

So MySQL is equally vulnerable to fingerprinting clients based on the capability bits sent in the pre-TLS client intro packet, and equally incapable of fixing this on the client side in a way that's backwards-compatible with existing servers.

@dlenski
Copy link
Contributor Author

dlenski commented Jul 6, 2023

@vuvova wrote:

No MariaDB or MySQL server ever (since when SSL support was added to MySQL in 2000) used max_allowed_packet or a charset from the client's SSL Request Packet. Meaning C/C can safely send hard-coded values there before the TLS handshare and send real values after. Or, may be, not send those bytes at all, but then it'll become an identifiable property on itself

Unfortunately, this is not quite true; the MySQL server does require the charset in the pre-TLS client packet to precisely match* the charset value in the post-TLS client packet, otherwise the connection aborts:

https://github.com/mysql/mysql-server/blob/ea7087d885006918ad54458e7aad215b1650312c/sql/auth/sql_authentication.cc#L2922-L2926

The comment a few lines above claims that they "don't parse" the fields in the post-TLS client packet, but this is not quite correct:

https://github.com/mysql/mysql-server/blob/ea7087d885006918ad54458e7aad215b1650312c/sql/auth/sql_authentication.cc#L2922-L2926

There appears to be an escape hatch in the MySQL code, which is that if the pre-TLS client packet is only 4 bytes long (= capability fields only) and has CLIENT_SSL bit set, then they skip straight to the TLS handshake.

… but if you follow the following lines carefully, you'll see that they've completely botched it:

tl;dr:

  1. MySQL server does require that the charset in the pre-TLS client packet matches the charset in the post-TLS client packet.
  2. The only exception is in the case where the client attempt's to send MySQL's version of a truncated dummy packet (4 bytes), which has a bug that makes it basically unusable.

@dlenski
Copy link
Contributor Author

dlenski commented Jul 7, 2023

@vuvova wrote:

This approach doesn't seem to break anything and only leaks client capabilities in case of the old server or active MitM.

If we take an approach that doesn't defend against an active MiTM downgrading the connection, we aren't appreciably improving the security of M/M client-server connections on the Internet.

Active MiTM is the threat model that everyone has worried about, because we've known that it is actively being employed by multiple governments for at least a decade:

  1. $ADVERSARY sees every packet on the Internet,
  2. inspects them,
  3. notices the interesting ones (e.g. M/M server greeting packets that indicate that the server supports SSL)
  4. … and then statefully tracks the connection and interferes with it in order to extract information, downgrade it to plaintext, etc.

Active MiTM is the reason that the only remedy for POODLE was to disable SSLv2/3 entirely; for the same reason, the only real fix for this issue is to make the client refuse to downgrade, even if the server says it has to downgrade.

@vuvova
Copy link
Member

vuvova commented Jul 7, 2023

@dlenski wrote:

@vuvova wrote:

This approach doesn't seem to break anything and only leaks client capabilities in case of the old server or active MitM.

If we take an approach that doesn't defend against an active MiTM downgrading the connection, we aren't appreciably improving the security of M/M client-server connections on the Internet.

See above, it still detects active MitM post-factum, after the TLS was established, and…

Active MiTM is the threat model that everyone has worried about, because we've known that it is actively being employed by multiple governments for at least a decade:

  1. $ADVERSARY sees every packet on the Internet,
  2. inspects them,
  3. notices the interesting ones (e.g. M/M server greeting packets that indicate that the server supports SSL)
  4. … and then statefully tracks the connection and interferes with it in order to extract information, downgrade it to plaintext, etc.

… the client can abort the connection at once, so that it couldn't be statefully tracked.

Downgrade won't be allowed after MDEV-28634, MitM or not.

@vuvova
Copy link
Member

vuvova commented Jul 7, 2023

@dlenski wrote:

@vuvova wrote:

No MariaDB or MySQL server ever (since when SSL support was added to MySQL in 2000) used max_allowed_packet or a charset from the client's SSL Request Packet. Meaning C/C can safely send hard-coded values there before the TLS handshare and send real values after. Or, may be, not send those bytes at all, but then it'll become an identifiable property on itself

Unfortunately, this is not quite true; the MySQL server does require the charset in the pre-TLS client packet to precisely match* the charset value in the post-TLS client packet, otherwise the connection aborts:

Ouch. Indeed, sorry. I only checked the oldest MySQL from 2000 (when SSL support was added) and the recent MariaDB. But apparently at some point max_allowed_packet and charset were read before SSL, we've changed it in
MariaDB/server@b3e15f8 back in 2012.

Okay, then.

MariaDB server

  • if the client has "new client" capability bit, reload capabilities after TLS handshake

MariaDB connectors

  • client verifies server's version
  • if it's new mariadb, it sends
    • a hard-coded SSL Request packet, always the same
    • after TLS handshake it sends real values with a "new client" capability bit
  • if it's not a new mariadb, send a packet with
    • real capabilities
    • max possible max_allowed_packet
    • hard-coded charset
  • after TLS handshake
    • verify that it's not a new mariadb (MDEV-31609)
      • otherwise abort
    • issue SET NAMES to set a correct charset
    • control max_allowed_packet from the client side

This way,

  • "old or 3rd party client → new server" will work as before
  • "new client → old or 3rd part server" will only expose capabilities
  • "new client → new server" will only expose capabilities if active MitM, but it'll be detected at once

There are trade-offs, particularly in the "new client old server" configuration, but everything will still work.

@bgrainger
Copy link

"new client → new server" will only expose capabilities if active MitM, but it'll be detected at once

Do new clients also need to eliminate the "Preferred" and "Required" SSL Modes (to use MySQL terminology) and only support "VerifyCA"? Otherwise, a malicious proxy/MitM could establish a TLS connection (using a self-signed cert) with the new client and downgrade the MariaDB version that it sends, tricking the client into thinking it's talking to an old MariaDB server and activating the "old server" code path that doesn't terminate the connection. This could allow all data to be logged as it's proxied, even though it's sent over a "secure" connection.

@vuvova
Copy link
Member

vuvova commented Jul 7, 2023

@bgrainger,

first, by "new client" I meant "new releases of MariaDB Connector/C, Connector/J, etc", because I only described what we in MariaDB can do to secure the connection and without breaking other clients and servers. Of course, I cannot speak for what MySqlConnector will do. Although we might create a pull request for you at some point.

Now, about "modes", I suggested an approach in MDEV-28634, which is,

  • if no ssl command-line (or config) options are specified, it's like MySQL "preferred" mode — this is already the case since 10.10
  • if the user specifies --disable-ssl — ssl will not be used (not surprisingly)
  • if the user will specify --ssl, it'll still be "preferred", and a warning "--ssl is deprecated, insecure, should be avoided, etc"
  • if the user will specify anything else, it'll be used to verify the server. if ssl-ca is specified, it'll be VerifyCA. if --tls-fingerprint is specified, the fingerprint is checked. And --ssl-verify-server-cert will work as before, as VerifyCA.

This looks mostly backward compatible. But doesn't have a "required" mode (which is "I require ssl but I don't care who will decrypt the data, server or someone in the middle")

@dlenski
Copy link
Contributor Author

dlenski commented Jul 7, 2023

@vuvova wrote:

But if there's an active MitM attacker, he can turn this flag off, cannot he? And the client will leak again. Should it be a concern?

To which I replied…

That's why we need to make a client-side change which is backwards-incompatible by default in order to fix this.

To which @bgrainger replied…

This would bifurcate the client ecosystem into MySQL clients and MariaDB clients.

———

It turns out there is a good way around this (MariaDB/server#2684 (comment)).

Summary:

  1. Unless we change the client in a backwards-incompatible way, an active MITM can downgrade the client to keep leaking the information that we want it to stop leaking.
  2. But the downgrade attack can be detected by the server.
  3. In MariaDB/server@056d083, I make the server detect this attack, log it, and abort the connection.

This will largely blunk the MITM attack, while preserving backwards-compatibility-by-default with servers that require the client to keep leaking. What do you think?

@bgrainger
Copy link

This sounds good to me.

It occurs to me that because this will trigger a new code path both in both client and server (client: if "send dummy" bit is set, send the dummy handshake; server: if client sends the dummy handshake, load everything from the post-TLS handshake), that we could take the opportunity to fix anything else that's broken in the handshake. (Obviously there needs to be some overlap in the location of the capabilities bits flags, to handle the case where MitM is changing the packets, and the server doesn't know which handshake to expect, but there's a lot of leeway beyond that.) Nothing's jumping out at me immediately, but it seemed worth bringing up in case anyone else had ideas.

Perhaps the name could be improved on though. Maybe something like CLIENT_CAN_SEND_ONLY_2BYTE_SSLREQUEST_PACKET? Any thoughts on an optimal name?

I'd suggest that this whole path be named "SSL Request V2", not "dummy handshake". (This is not going as far as calling it "SECURE_SSL_MODE" as suggested above, but simply denoting that this is indeed a new version of the SSL Handshake packet.)

  • MARIADB_CLIENT_CAN_SEND_DUMMY_HANDSHAKE_PACKETMARIADB_CLIENT_CAN_SEND_SSL_REQUEST_V2 (or just MARIADB_CLIENT_SSL_V2 if having a short name is important)
  • https://mariadb.com/kb/en/connection/#sslrequest-packet → add description of SSLRequestV2 packet
  • pre_tls_client_packet_is_dummypre_tls_client_packet_is_ssl_request_v2
  • etc

CLIENT_SSL uses the current 32-byte SSL Request packet; CLIENT_SSL with MARIADB_CLIENT_CAN_SEND_SSL_REQUEST_V2 sends the new 2-byte SSL Request V2 packet.

@dlenski
Copy link
Contributor Author

dlenski commented Jul 8, 2023

@bgrainger wrote:

It occurs to me that because this will trigger a new code path both in both client and server (client: if "send dummy" bit is set, send the dummy handshake; server: if client sends the dummy handshake, load everything from the post-TLS handshake), that we could take the opportunity to fix anything else that's broken in the handshake.

I'm not aware of any other desired changes that could fit neatly into the scope of this change, but perhaps @vuvova and other core devs are.

I'd suggest that this whole path be named "SSL Request V2", not "dummy handshake". (This is not going as far as calling it "SECURE_SSL_MODE" as suggested above, but simply denoting that this is indeed a new version of the SSL Handshake packet.)

👍, good idea.

My naming was focusing too much on the technical details, rather than the overall purpose. I'm on board.

@dlenski
Copy link
Contributor Author

dlenski commented Jul 11, 2023

In the latest updates, I've changed the naming to SSL_V2, per @bgrainger's very good suggestion.

In MariaDB/server@eb5c26a and cf8638c, I also moved the CLIENT_CAN_SSL_V2 capability bit from the MariaDB extensions (1ULL << 37) to the shared MySQL/MariaDB capability bits (1ULL << 28).

MySQL is affected by these vulnerabilities as well, and if we want to enable a better TLS handshake for all client/server combinations, this capability bit needs to be sent between all client/server combinations.

@bgrainger
Copy link

the shared MySQL/MariaDB capability bits (1ULL << 28).

Oracle has already defined a meaning for that bit: https://github.com/mysql/mysql-server/blob/ea7087d885006918ad54458e7aad215b1650312c/include/mysql_com.h#L734-L747

@dlenski
Copy link
Contributor Author

dlenski commented Jul 11, 2023

the shared MySQL/MariaDB capability bits (1ULL << 28).

Oracle has already defined a meaning for that bit: https://github.com/mysql/mysql-server/blob/ea7087d885006918ad54458e7aad215b1650312c/include/mysql_com.h#L734-L747

Thanks @bgrainger.

😭 … looks like we're all out of the low 32 capability bits which could plausibly be shared between Oracle/MySQL and MariaDB.
https://github.com/mysql/mysql-server/blob/ea7087d885006918ad54458e7aad215b1650312c/include/mysql_com.h#L734-L774

Well, we can just pop the top commit off here and keep this a MariaDB-only fix, huh? 😬

@bgrainger
Copy link

MySQL is affected by these vulnerabilities as well, and if we want to enable a better TLS handshake for all client/server combinations, this capability bit needs to be sent between all client/server combinations.

Strictly speaking, there's no reason MySQL couldn't use 1ULL << 37 (if Oracle wanted to adopt this change). That byte sits in the middle of string[10] reserved in the MySQL HandshakeV10.

That could theoretically be changed to:

string[6] - reserved; all 0s
int<1> - bit 0x20 is set for SSL v2
string[3] - reserved; all 0s

(Or you could change the MariaDB flag to be (1ULL << 56) so the result from MySQL's perspective looks like string[9] followed by int<1> set to 0x01 for SSL v2; i.e., it looks more like a Boolean flag. But probably best to get a discussion started at bugs.mysql.com first before getting too detailed about the on-the-wire format.)

@vuvova
Copy link
Member

vuvova commented Jul 12, 2023

I'd still suggest a simpler approach. Something like:

  • the server sends the welcome packet as before
  • the client sends back capabilities, 0xFFFFFFFF for max_allowed_packet and some common charset, say, utf8mb3_general_ci (= 33) or, better, a compile-time default (now utf8mb4_general_ci (=45))
  • TLS handshake
  • if CLIENT_MYSQL bit in the server handshake was not set — send real max_allowed_packet and charset here.
  • authentication
  • if CLIENT_MYSQL bit in the server handshake was set, send mysql_real_query("SET NAMES <real charset>")

This works for all clients and all servers. Doesn't show max_allowed_packet and charset for all servers (MySQL and forks included) at the cost of one extra roundtrip (which can be easily avoided). Totally safe against active MitM because the first client packet doesn't depend on any new capability bits, so MitM cannot change any bits to get more information.

Additionally we can make the server to prefer capability bits from after the TLS. But the client still has to send them before TLS too, see above, for non-MariaDB servers and to have the same behavior for different servers.

@dlenski
Copy link
Contributor Author

dlenski commented Jul 12, 2023

@vuvova wrote:

I'd still suggest a simpler approach. Something like:

Hmmm. This seems like a more complicated approach to me. In particular:

Additionally we can make the server to prefer capability bits from after the TLS. But the client still has to send them before TLS too, …

What does "prefer" mean, exactly? What happens if the client sends a 0 in a pre-TLS cap bit, and a 1 in a post-TLS cap bit? What if it's the other way around? 🤷‍♂️

Requiring a client to send these twice means allowing them to send them inconsistently. Inevitably, some clients will send them inconsistently, and it will be very difficult to discover the inconsistencies and decide what to do with them.


What's currently implemented here and in MariaDB/server#2684 seems more futureproof and less error-prone to me:

  • If the server indicates CAN_SSL_V2 in its greeting…
    1. The client sends the 2-byte packet with only the CLIENT_SSL bit in response.
    2. The server recognizes this as the "V2" SSLRequest packet, and ignores all the other bits.
    3. TLS handshake
    4. The client sends the complete Login Request packet over TLS, and the server reads all the relevant information from this packet..
  • If the server does not indicate CAN_SSL_V2
    1. Everything happens as before (with the old leaky SSLRequest packet)
    2. 🛡️ When the client re-sends its capabilities in the post-TLS Login Request packet, the server checks for the CAN_SSL_V2 bit in the client's capabilities: if the client supports "V2", but didn't do V2, it must have been downgraded by an active MITM → Abort the connection. (MariaDB/server@056d083)

@dlenski
Copy link
Contributor Author

dlenski commented Jul 12, 2023

@bgrainger wrote:

MySQL is affected by these vulnerabilities as well, and if we want to enable a better TLS handshake for all client/server combinations, this capability bit needs to be sent between all client/server combinations.

Strictly speaking, there's no reason MySQL couldn't use 1ULL << 37 (if Oracle wanted to adopt this change). That byte sits in the middle of string[10] reserved in the MySQL HandshakeV10.

Yep, agreed.

Okay, I've popped the top commit from here and MariaDB/server#2684. If/when Oracle/MySQL want to start doing the "v2" SSL handshake, they can start looking at bit 37 🥳

@vuvova
Copy link
Member

vuvova commented Jul 13, 2023

@dlenski wrote:

@vuvova wrote:

I'd still suggest a simpler approach. Something like:

Hmmm. This seems like a more complicated approach to me. In particular:

Additionally we can make the server to prefer capability bits from after the TLS. But the client still has to send them before TLS too, …

What does "prefer" mean, exactly? What happens if the client sends a 0 in a pre-TLS cap bit, and a 1 in a post-TLS cap bit? What if it's the other way around? man_shrugging

First, it was "additionally", not part of the proposal, more like an idea that needs to be discussed.
There're different options here:

  • do nothing (like now), use capabilities from the first packet
  • verify they're the same in both packets (meaning, supposedly, active MitM didn't change the first packet)
  • take them from the second packet, only use CLIENT_SSL from the first (this is rather useless without a new server capability bit)

What's currently implemented here and in MariaDB/server#2684 seems more futureproof and less error-prone to me:

  • it requires new capability bit (a limited resource, you said yourself that MySQL already run out of them)
  • it opens the door for active MitM, it can gather information by removing CAN_SSL_V2 bit, because
  • without CAN_SSL_V2 the client leaks the charset and max_allowed_packet just as before
  • and with CAN_SSL_V2 the client signals that it understands it by sending 2-byte response, which MitM will see
  • works only for new MariaDB servers

In the "simpler approach" none of this happens, because the client replies with the packet exactly the same as before. Like any other old or a third-party client does. To all servers with any capabilities.

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants