diff --git a/Documentation/Filters/Masking.md b/Documentation/Filters/Masking.md index 90df5f52b..e3d99b599 100644 --- a/Documentation/Filters/Masking.md +++ b/Documentation/Filters/Masking.md @@ -231,8 +231,8 @@ The `match` value must be a valid pcre2 regular expression. ``` "replace": { - "column": "ssn" - "match": "(123)", + "column": "ssn", + "match": "(123)" }, "with": { "fill": "X#" @@ -319,7 +319,7 @@ of the mask value to get the lengths to match. "column": "creditcard" }, "with": { - "value": "1234123412341234" + "value": "1234123412341234", "fill": "0" }, "applies_to": [ ... ], diff --git a/include/maxscale/authenticator.h b/include/maxscale/authenticator.h index 21afedefc..4c9653376 100644 --- a/include/maxscale/authenticator.h +++ b/include/maxscale/authenticator.h @@ -130,6 +130,7 @@ typedef struct mxs_authenticator #define MXS_AUTH_SSL_INCOMPLETE 5 /**< SSL connection is not yet complete */ #define MXS_AUTH_SSL_COMPLETE 6 /**< SSL connection complete or not required */ #define MXS_AUTH_NO_SESSION 7 +#define MXS_AUTH_BAD_HANDSHAKE 8 /**< Malformed client packet */ /** Return values for the loadusers entry point */ #define MXS_AUTH_LOADUSERS_OK 0 /**< Users loaded successfully */ diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index c141c9b1d..a3da2010a 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -50,7 +50,7 @@ add_library(testcore SHARED testconnections.cpp nodes.cpp mariadb_nodes.cpp maxs mariadb_func.cpp get_com_select_insert.cpp maxadmin_operations.cpp big_transaction.cpp sql_t1.cpp test_binlog_fnc.cpp get_my_ip.cpp big_load.cpp get_com_select_insert.cpp different_size.cpp fw_copy_rules maxinfo_func.cpp config_operations.cpp rds_vpc.cpp execute_cmd.cpp - blob_test.cpp keepalived_func.cpp + blob_test.cpp keepalived_func.cpp tcp_connection.cpp # Include the CDC connector in the core library ${CMAKE_SOURCE_DIR}/../connectors/cdc-connector/cdc_connector.cpp) target_link_libraries(testcore ${MYSQL_CLIENT} ${JANSSON_LIBRARIES} z m pthread ssl dl rt crypto crypt) @@ -970,6 +970,10 @@ add_test_executable_notest(long_sysbench.cpp long_sysbench replication LABELS re # test effect of local_address in configuration file add_test_executable(local_address.cpp local_address local_address LABELS REPL_BACKEND) +# MXS-1628: Security scanner says MaxScale is vulnerable to ancient MySQL vulnerability +# https://jira.mariadb.org/browse/MXS-1628 +add_test_executable(mxs1628_bad_handshake.cpp mxs1628_bad_handshake replication LABELS REPL_BACKEND) + configure_file(templates.h.in templates.h @ONLY) include(CTest) diff --git a/maxscale-system-test/mxs1628_bad_handshake.cpp b/maxscale-system-test/mxs1628_bad_handshake.cpp new file mode 100644 index 000000000..72cf8a5f7 --- /dev/null +++ b/maxscale-system-test/mxs1628_bad_handshake.cpp @@ -0,0 +1,71 @@ +#include "testconnections.h" +#include "tcp_connection.h" + +#include + +int main(int argc, char *argv[]) +{ + TestConnections test(argc, argv); + test.set_timeout(30); + + uint32_t caps = 1 | 8 | 512; + uint32_t max_packet = 65535; + uint8_t charset = 8; + std::string username = "username"; + uint8_t token_len = 20; // SHA1 hash size + std::string database = "database"; + + // Capabilities, max packet size and client charset + std::vector wbuf; + auto it = std::back_inserter(wbuf); + + for (auto a: {(uint8_t)(caps), (uint8_t)(caps >> 8), (uint8_t)(caps >> 16), (uint8_t)(caps >> 24), + (uint8_t)(max_packet), (uint8_t)(max_packet >> 8), (uint8_t)(max_packet >> 16), (uint8_t)(max_packet >> 24), + charset}) + { + *it++ = a; + } + + // Reserved filler space + std::fill_n(it, 23, 0); + + // Username without terminating null character + for (auto a: username) + { + *it++ = (uint8_t)a; + } + + // Auth token length and the token itself + *it++ = token_len; + std::fill_n(it, token_len, 123); + + // Database without terminating null character + for (auto a: database) + { + *it++ = (uint8_t)a; + } + + // Payload length and sequence number + uint8_t bufsize = wbuf.size(); + wbuf.insert(wbuf.begin(), {(uint8_t)(bufsize), (uint8_t)(bufsize >> 8), (uint8_t)(bufsize >> 16), 2}); + + + tcp::Connection conn; + conn.connect(test.maxscales->ip(), test.maxscales->rwsplit_port[0]); + + // Read the handshake + uint8_t buf[512] = {}; + conn.read(buf, sizeof(buf)); + + // Send the handshake response + conn.write(&wbuf[0], wbuf.size()); + + // Read MaxScale's response + conn.read(buf, sizeof(buf)); + + const char response[] = "Bad handshake"; + test.add_result(memmem(buf, sizeof(buf), response, sizeof(response) - 1) == NULL, + "MaxScale should respond with 'Bad handshake'"); + + return test.global_result; +} diff --git a/maxscale-system-test/mxs1719.cpp b/maxscale-system-test/mxs1719.cpp index b206a0cab..110a94bc0 100644 --- a/maxscale-system-test/mxs1719.cpp +++ b/maxscale-system-test/mxs1719.cpp @@ -44,6 +44,13 @@ void run(TestConnections& test) { // One multi-statement with two UPDATEs. test.try_query(pMysql, "UPDATE MXS_1719 SET a=1; UPDATE MXS_1719 SET a=1;"); + + // Sleep a while, so that the log is flushed. + sleep(5); + // This is actually related to MXS-1861 "masking filter logs warnings with + // multistatements" but it seems excessive to create a specific test for that. + test.log_excludes(0, "Received data, although expected nothing"); + // This will hang immediately, so we can shorten the timeout. test.set_timeout(5); test.try_query(pMysql, "SELECT * FROM MXS_1719"); diff --git a/maxscale-system-test/tcp_connection.cpp b/maxscale-system-test/tcp_connection.cpp new file mode 100644 index 000000000..e33ef8f35 --- /dev/null +++ b/maxscale-system-test/tcp_connection.cpp @@ -0,0 +1,89 @@ +#include "tcp_connection.h" + +#include +#include +#include +#include +#include +#include + + +namespace +{ + +static void set_port(struct sockaddr_storage *addr, uint16_t port) +{ + if (addr->ss_family == AF_INET) + { + struct sockaddr_in *ip = (struct sockaddr_in*)addr; + ip->sin_port = htons(port); + } + else if (addr->ss_family == AF_INET6) + { + struct sockaddr_in6 *ip = (struct sockaddr_in6*)addr; + ip->sin6_port = htons(port); + } +} + +int open_network_socket(struct sockaddr_storage *addr, const char *host, uint16_t port) +{ + struct addrinfo *ai = NULL, hint = {}; + int so = -1; + hint.ai_socktype = SOCK_STREAM; + hint.ai_family = AF_UNSPEC; + hint.ai_flags = AI_ALL; + + /* Take the first one */ + if (getaddrinfo(host, NULL, &hint, &ai) == 0 && ai) + { + if ((so = socket(ai->ai_family, SOCK_STREAM, 0)) != -1) + { + memcpy(addr, ai->ai_addr, ai->ai_addrlen); + set_port(addr, port); + freeaddrinfo(ai); + } + } + + return so; +} + +} + +namespace tcp +{ + +Connection::~Connection() +{ + if (m_so != -1) + { + close(m_so); + } +} + +bool Connection::connect(const char* host, uint16_t port) +{ + struct sockaddr_storage addr; + + if ((m_so = open_network_socket(&addr, host, port)) != -1) + { + if (::connect(m_so, (struct sockaddr*)&addr, sizeof(addr)) != 0) + { + close(m_so); + m_so = -1; + } + } + + return m_so != -1; +} + +int Connection::write(void* buf, size_t size) +{ + return ::write(m_so, buf, size); +} + +int Connection::read(void* buf, size_t size) +{ + return ::read(m_so, buf, size); +} + +} diff --git a/maxscale-system-test/tcp_connection.h b/maxscale-system-test/tcp_connection.h new file mode 100644 index 000000000..928b0c0cb --- /dev/null +++ b/maxscale-system-test/tcp_connection.h @@ -0,0 +1,49 @@ +#pragma once + +#include +#include + +namespace tcp +{ + +// A raw TCP connection +class Connection +{ +public: + ~Connection(); + + /** + * Connect to the target server + * + * @param host Server hostname + * @param port Server port + * + * @return True if connection was successfully created + */ + bool connect(const char* host, uint16_t port); + + /** + * Write to socket + * + * @param buf Buffer to read from + * @param size Number of bytes to read from @c buf + * + * @return Number of written bytes or -1 on error + */ + int write(void* buf, size_t size); + + /** + * Read from socket + * + * @param buf Buffer to write to + * @param size Size of @c buf + * + * @return Number of read bytes or -1 on error + */ + int read(void* buf, size_t size); + +private: + int m_so = -1; +}; + +} diff --git a/server/modules/authenticator/MySQLAuth/mysql_auth.cc b/server/modules/authenticator/MySQLAuth/mysql_auth.cc index 3c3d8f456..474e7a839 100644 --- a/server/modules/authenticator/MySQLAuth/mysql_auth.cc +++ b/server/modules/authenticator/MySQLAuth/mysql_auth.cc @@ -423,11 +423,21 @@ mysql_auth_set_client_data( if (client_auth_packet_size > MYSQL_AUTH_PACKET_BASE_SIZE) { /* Should have a username */ - char *first_letter_of_username = (char *)(client_auth_packet + MYSQL_AUTH_PACKET_BASE_SIZE); - int user_length = strlen(first_letter_of_username); + uint8_t* name = client_auth_packet + MYSQL_AUTH_PACKET_BASE_SIZE; + uint8_t* end = client_auth_packet + sizeof(client_auth_packet); + int user_length = 0; - ss_dassert(client_auth_packet_size > (MYSQL_AUTH_PACKET_BASE_SIZE + user_length) - && user_length <= MYSQL_USER_MAXLEN); + while (name < end && *name) + { + name++; + user_length++; + } + + if (name == end) + { + // The name is not null terminated + return false; + } if (client_auth_packet_size > (MYSQL_AUTH_PACKET_BASE_SIZE + user_length + 1)) { @@ -435,14 +445,14 @@ mysql_auth_set_client_data( packet_length_used = MYSQL_AUTH_PACKET_BASE_SIZE + user_length + 1; /* We should find an authentication token next */ /* One byte of packet is the length of authentication token */ - memcpy(&client_data->auth_token_len, - client_auth_packet + packet_length_used, 1); + client_data->auth_token_len = client_auth_packet[packet_length_used]; if (client_auth_packet_size > (packet_length_used + client_data->auth_token_len)) { - /* Packet is large enough for authentication token */ - if (NULL != (client_data->auth_token = (uint8_t *)MXS_MALLOC(client_data->auth_token_len))) + client_data->auth_token = (uint8_t*)MXS_MALLOC(client_data->auth_token_len); + + if (client_data->auth_token) { /* The extra 1 is for the token length byte, just extracted*/ memcpy(client_data->auth_token, @@ -461,7 +471,12 @@ mysql_auth_set_client_data( return false; } } + else + { + return false; + } } + return true; } diff --git a/server/modules/filter/masking/maskingfiltersession.cc b/server/modules/filter/masking/maskingfiltersession.cc index 2dd3503e3..49ab3f652 100644 --- a/server/modules/filter/masking/maskingfiltersession.cc +++ b/server/modules/filter/masking/maskingfiltersession.cc @@ -131,7 +131,21 @@ void MaskingFilterSession::handle_response(GWBUF* pPacket) switch (response.type()) { case ComResponse::OK_PACKET: - // We'll end up here also in the case of a multi-result. + { + ComOK ok(response); + + if (ok.status() & SERVER_MORE_RESULTS_EXIST) + { + m_res.reset_multi(); + m_state = EXPECTING_RESPONSE; + } + else + { + m_state = EXPECTING_NOTHING; + } + } + break; + case ComResponse::LOCAL_INFILE_PACKET: // GET_MORE_CLIENT_DATA/SEND_MORE_CLIENT_DATA m_state = EXPECTING_NOTHING; break; diff --git a/server/modules/filter/masking/mysql.hh b/server/modules/filter/masking/mysql.hh index 47654e458..6d5ba0d92 100644 --- a/server/modules/filter/masking/mysql.hh +++ b/server/modules/filter/masking/mysql.hh @@ -658,6 +658,65 @@ private: uint16_t m_status; }; +class ComOK : public ComResponse +{ +public: + ComOK(GWBUF* pPacket) + : ComResponse(pPacket) + { + ss_dassert(m_type == OK_PACKET); + + extract_payload(); + } + + ComOK(const ComResponse& response) + : ComResponse(response) + { + ss_dassert(m_type == OK_PACKET); + + extract_payload(); + } + + uint64_t affected_rows() const + { + return m_affected_rows; + } + + uint64_t last_insert_id() const + { + return m_last_insert_id; + } + + uint16_t warnings() const + { + return m_warnings; + } + + uint16_t status() const + { + return m_status; + } + +private: + void extract_payload() + { + m_affected_rows = LEncInt(&m_pData).value(); + m_last_insert_id = LEncInt(&m_pData).value(); + + m_status = *m_pData++; + m_status += (*m_pData++ << 8); + + m_warnings = *m_pData++; + m_warnings += (*m_pData++ << 8); + } + +private: + uint64_t m_affected_rows; + uint64_t m_last_insert_id; + uint16_t m_status; + uint16_t m_warnings; +}; + /** * @class ComRequest * diff --git a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc index 245608a16..119a0813f 100644 --- a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc +++ b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc @@ -535,6 +535,34 @@ int gw_read_client_event(DCB* dcb) return return_code; } +/** + * Get length of a null-terminated string + * + * @param str String to measure + * @param len Maximum length to read + * + * @return Length of @c str or -1 if the string is not null-terminated + */ +static int get_zstr_len(const char* str, int len) +{ + const char* end = str + len; + int slen = 0; + + while (str < end && *str) + { + str++; + slen++; + } + + if (str == end) + { + // The string is not null terminated + slen = -1; + } + + return slen; +} + /** * @brief Store client connection information into the DCB * @param dcb Client DCB @@ -563,22 +591,28 @@ static void store_client_information(DCB *dcb, GWBUF *buffer) if (len > MYSQL_AUTH_PACKET_BASE_SIZE) { - strcpy(ses->user, (char*)data + MYSQL_AUTH_PACKET_BASE_SIZE); + const char* username = (const char*)data + MYSQL_AUTH_PACKET_BASE_SIZE; + int userlen = get_zstr_len(username, len - MYSQL_AUTH_PACKET_BASE_SIZE); + + if (userlen != -1 && (int)sizeof(ses->user) > userlen) + { + strcpy(ses->user, username); + } if (proto->client_capabilities & GW_MYSQL_CAPABILITIES_CONNECT_WITH_DB) { - /** Client supports default database on connect */ - size_t userlen = strlen(ses->user) + 1; - - /** Skip the authentication token, it is handled by the authenticators */ + /** Client is connecting with a default database */ uint8_t authlen = data[MYSQL_AUTH_PACKET_BASE_SIZE + userlen]; - size_t dboffset = MYSQL_AUTH_PACKET_BASE_SIZE + userlen + authlen + 1; - if (data[dboffset]) + if (dboffset < len) { - /** Client is connecting with a default database */ - strcpy(ses->db, (char*)data + dboffset); + int dblen = get_zstr_len((const char*)data + dboffset, len - dboffset); + + if (dblen != -1 && (int)sizeof(ses->db) < dblen) + { + strcpy(ses->db, (const char*)data + dboffset); + } } } } @@ -667,6 +701,10 @@ gw_read_do_authentication(DCB *dcb, GWBUF *read_buffer, int nbytes_read) { auth_val = dcb->authfunc.authenticate(dcb); } + else + { + auth_val = MXS_AUTH_BAD_HANDSHAKE; + } MySQLProtocol *protocol = (MySQLProtocol *)dcb->protocol; @@ -1223,6 +1261,10 @@ mysql_client_auth_error_handling(DCB *dcb, int auth_val, int packet_number) modutil_send_mysql_err_packet(dcb, packet_number, 0, 1045, "28000", fail_str); break; + case MXS_AUTH_BAD_HANDSHAKE: + modutil_send_mysql_err_packet(dcb, packet_number, 0, 1045, "08S01", "Bad handshake"); + break; + default: MXS_DEBUG("authentication failed. fd %d, state unrecognized.", dcb->fd); /** Send error 1045 to client */ diff --git a/server/modules/protocol/MySQL/mysql_common.cc b/server/modules/protocol/MySQL/mysql_common.cc index b9166f6db..550fdabb8 100644 --- a/server/modules/protocol/MySQL/mysql_common.cc +++ b/server/modules/protocol/MySQL/mysql_common.cc @@ -29,6 +29,7 @@ #include #include #include +#include uint8_t null_client_sha1[MYSQL_SCRAMBLE_LEN] = ""; @@ -1706,7 +1707,17 @@ static bool kill_func(DCB *dcb, void *data) dcb->session->ses_id == info->target_id) { MySQLProtocol* proto = (MySQLProtocol*)dcb->protocol; - info->targets.push_back(std::make_pair(dcb->server, proto->thread_id)); + + if (proto->thread_id) + { + // DCB is connected and we know the thread ID so we can kill it + info->targets.push_back(std::make_pair(dcb->server, proto->thread_id)); + } + else + { + // DCB is not yet connected, send a hangup to forcibly close it + poll_fake_hangup_event(dcb); + } } return true;