Merge branch '2.2' into develop

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": [ ... ],

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 */

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)

maxscale-system-test/mxs1628_bad_handshake.cpp

@@ -0,0 +1,71 @@
+#include "testconnections.h"
+#include "tcp_connection.h"
+
+#include <vector>
+
+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<uint8_t> 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;
+}

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");

maxscale-system-test/tcp_connection.cpp

@@ -0,0 +1,89 @@
+#include "tcp_connection.h"
+
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <netinet/in.h>
+#include <unistd.h>
+#include <string.h>
+#include <netdb.h>
+
+
+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);
+}
+
+}

maxscale-system-test/tcp_connection.h

@@ -0,0 +1,49 @@
+#pragma once
+
+#include <cstdint>
+#include <cstddef>
+
+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;
+};
+
+}

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;
 }
 

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;

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
  *

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 */

server/modules/protocol/MySQL/mysql_common.cc

@@ -29,6 +29,7 @@
 #include <maxscale/protocol/mysql.h>
 #include <maxscale/utils.h>
 #include <maxscale/protocol/mariadb_client.hh>
+#include <maxscale/poll.h>
 
 
 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;