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.

Sign up for GitHub

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

Jump to bottom

[MDEV-15935] Client-side implementation of connection redirection mechanism #226

Closed
wants to merge 7 commits into from
Closed

[MDEV-15935] Client-side implementation of connection redirection mechanism #226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
abad64b
[CONC-648] Do not trust error packets received prior to TLS handshake…
dlenski Jun 12, 2023
165110d
[MDEV-28634] Do not allow silent downgrade of connections where SSL i…
dlenski Jun 14, 2023
2ebbeb9
[MDEV-28634] Move check for server TLS/SSL capability to mthd_my_real…
dlenski Jun 14, 2023
b090994
Add comments about usage of TLS/SSL in connection establishment
dlenski Jun 14, 2023
c73f9bd
[MDEV-15935] Follow server redirection error packets
dlenski Jun 20, 2023
54886ac
Allow clients to disable following server redirects
dlenski Jul 5, 2023
9768c6e
Limit number of server redirects followed by client
dlenski Jul 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions include/mysql.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,8 @@ extern const char *SQLSTATE_UNKNOWN;
/* MariaDB specific */
MYSQL_PROGRESS_CALLBACK=5999,
MYSQL_OPT_NONBLOCK,
MARIADB_OPT_FOLLOW_SERVER_REDIRECTS,

/* MariaDB Connector/C specific */
MYSQL_DATABASE_DRIVER=7000,
MARIADB_OPT_SSL_FP, /* deprecated, use MARIADB_OPT_TLS_PEER_FP instead */
Expand Down Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add new options in mysql->options.extension to keep MYSQL structure binary compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?


/* function pointers for local infile support */
int (*local_infile_init)(void **, const char *, void *);
int (*local_infile_read)(void *, char *, unsigned int);
void (*local_infile_end)(void *);
int (*local_infile_error)(void *, char *, unsigned int);
void *local_infile_userdata;

/* WHY are some options relegated to this "extension" structure?
* What is the logic for distinguishing the "main" options from
* the extension options?
*/
struct st_mysql_options_extension *extension;
};

Expand Down
3 changes: 2 additions & 1 deletion include/mysqld_error.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1236,5 +1236,6 @@
#define ER_JSON_HISTOGRAM_PARSE_FAILED 4186
#define ER_SF_OUT_INOUT_ARG_NOT_ALLOWED 4187
#define ER_INCONSISTENT_SLAVE_TEMP_TABLE 4188
#define ER_ERROR_LAST 4188
#define ER_SERVER_REDIRECT 4196
#define ER_ERROR_LAST 4196
#endif /* ER_ERROR_FIRST */
76 changes: 74 additions & 2 deletions libmariadb/mariadb_lib.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
#define MA_RPL_VERSION_HACK "5.5.5-"

#define CHARSET_NAME_LEN 64
#define SERVER_REDIRECT_LIMIT 8

#undef max_allowed_packet
#undef net_buffer_length
Expand Down Expand Up @@ -657,6 +658,7 @@ struct st_default_options mariadb_defaults[] =
{{MYSQL_SET_CHARSET_NAME}, MARIADB_OPTION_STR, "default-character-set"},
{{MARIADB_OPT_INTERACTIVE}, MARIADB_OPTION_NONE, "interactive-timeout"},
{{MYSQL_OPT_CONNECT_TIMEOUT}, MARIADB_OPTION_INT, "connect-timeout"},
{{MARIADB_OPT_FOLLOW_SERVER_REDIRECTS}, MARIADB_OPTION_BOOL, "follow-server-redirects"},
{{MYSQL_OPT_LOCAL_INFILE}, MARIADB_OPTION_BOOL, "local-infile"},
{{0}, 0 ,"disable-local-infile",},
{{MYSQL_OPT_SSL_CIPHER}, MARIADB_OPTION_STR, "ssl-cipher"},
Expand Down Expand Up @@ -1283,6 +1285,7 @@ mysql_init(MYSQL *mysql)
goto error;
mysql->options.report_data_truncation= 1;
mysql->options.connect_timeout=CONNECT_TIMEOUT;
mysql->options.follow_server_redirects= TRUE;
mysql->charset= mysql_find_charset_name(MARIADB_DEFAULT_CHARSET);
mysql->methods= &MARIADB_DEFAULT_METHODS;
strcpy(mysql->net.sqlstate, "00000");
Expand Down Expand Up @@ -1568,7 +1571,7 @@ MYSQL *mthd_my_real_connect(MYSQL *mysql, const char *host, const char *user,
my_bool is_multi= 0;
char *host_copy= NULL;
struct st_host *host_list= NULL;
int connect_attempts= 0;
int connect_attempts= 0, server_redirects=0;

if (!mysql->methods)
mysql->methods= &MARIADB_DEFAULT_METHODS;
Expand Down Expand Up @@ -1708,6 +1711,7 @@ MYSQL *mthd_my_real_connect(MYSQL *mysql, const char *host, const char *user,
else
#endif
{
tcp_redirect:
cinfo.unix_socket=0; /* This is not used */
if (!port)
port=mysql_port;
Expand Down Expand Up @@ -1787,7 +1791,12 @@ MYSQL *mthd_my_real_connect(MYSQL *mysql, const char *host, const char *user,
*/
if ((pkt_length=ma_net_safe_read(mysql)) == packet_error)
{
if (mysql->net.last_errno == CR_SERVER_LOST)
if (mysql->options.use_ssl)
my_set_error(mysql, CR_CONNECTION_ERROR, SQLSTATE_UNKNOWN,
"Received error packet before completion of TLS handshake. "
"Suppressing its details so that the client cannot vary its behavior "
"based on this UNTRUSTED input.");
else if (mysql->net.last_errno == CR_SERVER_LOST)
my_set_error(mysql, CR_SERVER_LOST, SQLSTATE_UNKNOWN,
ER(CR_SERVER_LOST_EXTENDED),
"handshake: reading initial communication packet",
Expand Down Expand Up @@ -1918,6 +1927,16 @@ MYSQL *mthd_my_real_connect(MYSQL *mysql, const char *host, const char *user,
}
}

/* We now know the server's capabilities. If the client wants TLS/SSL,
* but the server doesn't support it, we should immediately abort.
*/
if (mysql->options.use_ssl && !(mysql->server_capabilities & CLIENT_SSL))
{
SET_CLIENT_ERROR(mysql, CR_SSL_CONNECTION_ERROR, SQLSTATE_UNKNOWN,
"Client requires TLS/SSL, but the server does not support it");
goto error;
}

/* Set character set */
if (mysql->options.charset_name)
mysql->charset= mysql_find_charset_name(mysql->options.charset_name);
Expand All @@ -1936,9 +1955,59 @@ MYSQL *mthd_my_real_connect(MYSQL *mysql, const char *host, const char *user,

mysql->client_flag= client_flag;

/* Until run_plugin_auth has completed, the connection
* cannot have been secured with TLS/SSL.
*
* This means that any client which expects to use a
* TLS/SSL-secured connection SHOULD NOT trust any
* communication received from the server prior to this
* point as being genuine; nor should either the client
* or the server send any confidential information up
* to this point.
*/

if (run_plugin_auth(mysql, scramble_data, scramble_len,
scramble_plugin, db))
{
if (mysql->net.last_errno == ER_SERVER_REDIRECT)
{
if (!mysql->options.follow_server_redirects)
{
/* Client has disabled server redirection. Fall through and treat this
* as a "normal" error. */
}
else if (server_redirects >= SERVER_REDIRECT_LIMIT)
{
/* Too many server redirects */
my_set_error(mysql, ER_SERVER_REDIRECT, SQLSTATE_UNKNOWN,
"Too many server redirects (>= %d)",
SERVER_REDIRECT_LIMIT);
}
else
{
char *p= mysql->net.last_error; /* Should look like '|message|host[:port]' */
if (p && p[0] == '|')
p= strchr(p + 1, '|') ? : NULL;
if (p && *++p) {
host= p;
p= strchr(p, ':') ? : NULL;
if (p) {
*p++ = '\0';
port= atoi(p);
}
else
{
/* Restore to the default port, rather than reusing our current one */
port= 0;
}
fprintf(stderr, "Got server redirect to '%s' (port %d)\n", host, port);
++server_redirects;
goto tcp_redirect;
}
}
}
goto error;
}

if (mysql->client_flag & CLIENT_COMPRESS ||
mysql->client_flag & CLIENT_ZSTD_COMPRESSION)
Expand Down Expand Up @@ -3445,6 +3514,9 @@ mysql_optionsv(MYSQL *mysql,enum mysql_option option, ...)
case MYSQL_OPT_RECONNECT:
mysql->options.reconnect= *(my_bool *)arg1;
break;
case MARIADB_OPT_FOLLOW_SERVER_REDIRECTS:
mysql->options.follow_server_redirects= *(my_bool *)arg1;
break;
case MYSQL_OPT_PROTOCOL:
mysql->options.protocol= *((uint *)arg1);
break;
Expand Down
34 changes: 16 additions & 18 deletions plugins/auth/my_auth.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,22 +245,6 @@ static int send_client_reply_packet(MCPVIO_EXT *mpvio,
mysql->server_capabilities &= ~(CLIENT_SSL);
}

/* if server doesn't support SSL and verification of server certificate
was set to mandatory, we need to return an error */
if (mysql->options.use_ssl && !(mysql->server_capabilities & CLIENT_SSL))
{
if ((mysql->client_flag & CLIENT_SSL_VERIFY_SERVER_CERT) ||
(mysql->options.extension && (mysql->options.extension->tls_fp ||
mysql->options.extension->tls_fp_list)))
{
my_set_error(mysql, CR_SSL_CONNECTION_ERROR, SQLSTATE_UNKNOWN,
ER(CR_SSL_CONNECTION_ERROR),
"SSL is required, but the server does not support it");
goto error;
}
}


/* Remove options that server doesn't support */
mysql->client_flag= mysql->client_flag &
(~(CLIENT_COMPRESS | CLIENT_ZSTD_COMPRESSION | CLIENT_SSL | CLIENT_PROTOCOL_41)
Expand Down Expand Up @@ -337,8 +321,19 @@ static int send_client_reply_packet(MCPVIO_EXT *mpvio,
(mysql->client_flag & CLIENT_SSL))
{
/*
Send mysql->client_flag, max_packet_size - unencrypted otherwise
the server does not know we want to do SSL
Send UNENCRYPTED "Login Request" packet with mysql->client_flag
and max_packet_size, but no username; without this, the server
does not know we want to switch to SSL/TLS

FIXME: Sending this packet is a very very VERY bad idea. It
contains the client's preferred charset and flags in plaintext;
this can be used for fingerprinting the client software version,
and probable geographic location.

This offers a glaring opportunity for pervasive attackers to
easily target, intercept, and exploit the client-server
connection (e.g. "MITM all connections from known-vulnerable
client versions originating from countries X, Y, and Z").
*/
if (ma_net_write(net, (unsigned char *)buff, (size_t) (end-buff)) || ma_net_flush(net))
{
Expand All @@ -348,6 +343,9 @@ static int send_client_reply_packet(MCPVIO_EXT *mpvio,
errno);
goto error;
}
/* This is where the socket is actually converted from a plain
* TCP/IP socket to a TLS/SSL-wrapped socket.
*/
if (ma_pvio_start_ssl(mysql->net.pvio))
goto error;
}
Expand Down