[MDEV-15935] Client-side implementation of connection redirection mechanism #226
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… 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.
7 tasks
If the server responds to our initial connection with an error packet having error number 4196 (`ER_SERVER_REDIRECT`), and an error string formatted as `|Human-readable message|host[:port]`, then redirect accordingly. This redirection is performed in `mthd_my_real_connect`, which appears to be the most appropriate location given that it already contains code for retrying/repeating a new server connection. 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.
9EOR9
reviewed
Jul 6, 2023
include/mysql.h
Outdated
| @@ -227,6 +227,8 @@ extern const char *SQLSTATE_UNKNOWN; | |||
| /* MariaDB specific */ | |||
| MYSQL_PROGRESS_CALLBACK=5999, | |||
| MYSQL_OPT_NONBLOCK, | |||
| MYSQL_OPT_FOLLOW_SERVER_REDIRECTS, | |||
There was a problem hiding this comment.
Should be prefixed with MARIADB unless MySQL/Oracle has the same option.
There was a problem hiding this comment.
Okay, will do…
But why are the immediately preceding options both prefixed with MYSQL and marked as /* MariaDB specific */?
Those are contradictory, are they not?
9EOR9
reviewed
Jul 6, 2023
| @@ -337,12 +339,19 @@ struct st_mysql_options { | |||
| char *bind_address; | |||
| my_bool secure_auth; | |||
| my_bool report_data_truncation; | |||
| my_bool follow_server_redirects; | |||
There was a problem hiding this comment.
Please add new options in mysql->options.extension to keep MYSQL structure binary compatible.
There was a problem hiding this comment.
I can do that but, as I commented below, I don't understand this requirement:
… to keep MYSQL structure binary compatible.
The structure st_mysql cannot possibly be used as a serialization format for inter-process communication, let alone network-based communication, because it contains pointers. and nothing is done to ensure consistent endianness or alignment.
So why does it need to be kept binary-compatible? With what?
This adds a new boolean option, `follow-server-redirects` (ENABLED by default), to allow the client to disable server redirection and simply return an error message. 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.
Set the limit to 8 for now. This will prevent the client from getting stuck in redirect loops. 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.
|
In 54886ac?w=1 and MariaDB/server@4ffff66, I made As discussed with @atcurtis on the developers mailing list, I'll further…
|
|
@dlenski, I'm a bit confused. I thought it was a PoC patch to show how to do forced redirection with an error packet. And now you seems to work on making it the production quality. This is, of course, fine. But there's yet no decision, as far as I know, to use the error message approach. It'd be kind of a waste if a perfectly good patch would be rejected because it implements a wrong approach |
|
We went on with the sysvar tracker approach as more flexible for a DBA to use. It's unrelated to the forced/optional discussion, because a server can disconnect after sending the redirection info to the client either way, whether it sends an error message or a sysvar tracking packet. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the client counterpart of MariaDB/server#2681, which provides an initial server-side mechanism for redirecting new client connections.
It modifies the Connector/C library, so that if the server responds to our initial connection with an error packet having
error number 4196 (
ER_SERVER_REDIRECT), and an error string formatted as|Human-readable message|host[:port], the client will redirect its connection accordingly.This redirection is performed in
mthd_my_real_connect, which appears to be the most appropriate location given that it already contains code for retrying/repeating a new server connection.This PR also incorporates fixes for two client-security vulnerabilities:
Both of these vulnerabilities should be fixed regardless of whether a connection redirection mechanism is implemented, but their implications are very severe if a connection redirection mechanism exists (→ trivial for MITM attackers to redirect to attacker-controlled servers).