From 8f8823dc416d988119112d27bc2ecfa4ed34bbb2 Mon Sep 17 00:00:00 2001
From: Johan Wikman <johan.wikman@mariadb.com>
Date: Tue, 25 Oct 2016 17:59:46 +0300
Subject: [PATCH] qc_sqlite: Protect against misuse

- Ensure contiguous buffer of expected size.
- Ensure COM_QUERY content.
---
 query_classifier/qc_sqlite/qc_sqlite.c | 66 ++++++++++++++++++--------
 query_classifier/test/compare.cc       | 13 ++---
 2 files changed, 52 insertions(+), 27 deletions(-)

diff --git a/query_classifier/qc_sqlite/qc_sqlite.c b/query_classifier/qc_sqlite/qc_sqlite.c
index 942e01a6f..a78238111 100644
--- a/query_classifier/qc_sqlite/qc_sqlite.c
+++ b/query_classifier/qc_sqlite/qc_sqlite.c
@@ -433,36 +433,60 @@ static bool parse_query(GWBUF* query)
     bool parsed = false;
     ss_dassert(!query_is_parsed(query));
 
-    QC_SQLITE_INFO* info = info_alloc();
-
-    if (info)
+    if (GWBUF_IS_CONTIGUOUS(query))
     {
-        this_thread.info = info;
-
-        // TODO: Somewhere it needs to be ensured that this buffer is contiguous.
-        // TODO: Where is it checked that the GWBUF really contains a query?
         uint8_t* data = (uint8_t*) GWBUF_DATA(query);
-        size_t len = MYSQL_GET_PACKET_LEN(data) - 1; // Subtract 1 for packet type byte.
 
-        const char* s = (const char*) &data[5]; // TODO: Are there symbolic constants somewhere?
+        if ((GWBUF_LENGTH(query) >= MYSQL_HEADER_LEN + 1) &&
+            (GWBUF_LENGTH(query) == MYSQL_HEADER_LEN + MYSQL_GET_PACKET_LEN(data)))
+        {
+            if (MYSQL_GET_COMMAND(data) == MYSQL_COM_QUERY)
+            {
+                QC_SQLITE_INFO* info = info_alloc();
 
-        this_thread.info->query = s;
-        this_thread.info->query_len = len;
-        parse_query_string(s, len);
-        this_thread.info->query = NULL;
-        this_thread.info->query_len = 0;
+                if (info)
+                {
+                    this_thread.info = info;
 
-        // TODO: Add return value to gwbuf_add_buffer_object.
-        // Always added; also when it was not recognized. If it was not recognized now,
-        // it won't be if we try a second time.
-        gwbuf_add_buffer_object(query, GWBUF_PARSING_INFO, info, buffer_object_free);
-        parsed = true;
+                    size_t len = MYSQL_GET_PACKET_LEN(data) - 1; // Subtract 1 for packet type byte.
 
-        this_thread.info = NULL;
+                    const char* s = (const char*) &data[MYSQL_HEADER_LEN + 1];
+
+                    this_thread.info->query = s;
+                    this_thread.info->query_len = len;
+                    parse_query_string(s, len);
+                    this_thread.info->query = NULL;
+                    this_thread.info->query_len = 0;
+
+                    // TODO: Add return value to gwbuf_add_buffer_object.
+                    // Always added; also when it was not recognized. If it was not recognized now,
+                    // it won't be if we try a second time.
+                    gwbuf_add_buffer_object(query, GWBUF_PARSING_INFO, info, buffer_object_free);
+                    parsed = true;
+
+                    this_thread.info = NULL;
+                }
+                else
+                {
+                    MXS_ERROR("qc_sqlite: Could not allocate structure for containing parse data.");
+                }
+            }
+            else
+            {
+                MXS_ERROR("qc_sqlite: The provided buffer does not contain a COM_QUERY, but a %s.",
+                          STRPACKETTYPE(MYSQL_GET_COMMAND(data)));
+            }
+        }
+        else
+        {
+            MXS_ERROR("qc_sqlite: Packet size %ld, provided buffer is %ld.",
+                      MYSQL_HEADER_LEN + MYSQL_GET_PACKET_LEN(data),
+                      GWBUF_LENGTH(query));
+        }
     }
     else
     {
-        MXS_ERROR("qc_sqlite: Could not allocate structure for containing parse data.");
+        MXS_ERROR("qc_sqlite: Provided buffer is not contiguous.");
     }
 
     return parsed;
diff --git a/query_classifier/test/compare.cc b/query_classifier/test/compare.cc
index 964a41dd0..5b9d2cac7 100644
--- a/query_classifier/test/compare.cc
+++ b/query_classifier/test/compare.cc
@@ -117,17 +117,18 @@ ostream& operator << (ostream& out, qc_parse_result_t x)
 
 GWBUF* create_gwbuf(const string& s)
 {
-    size_t len = s.length() + 1;
-    size_t gwbuf_len = len + MYSQL_HEADER_LEN + 1;
+    size_t len = s.length();
+    size_t payload_len = len + 1;
+    size_t gwbuf_len = MYSQL_HEADER_LEN + payload_len;
 
     GWBUF* gwbuf = gwbuf_alloc(gwbuf_len);
 
-    *((unsigned char*)((char*)GWBUF_DATA(gwbuf))) = len;
-    *((unsigned char*)((char*)GWBUF_DATA(gwbuf) + 1)) = (len >> 8);
-    *((unsigned char*)((char*)GWBUF_DATA(gwbuf) + 2)) = (len >> 16);
+    *((unsigned char*)((char*)GWBUF_DATA(gwbuf))) = payload_len;
+    *((unsigned char*)((char*)GWBUF_DATA(gwbuf) + 1)) = (payload_len >> 8);
+    *((unsigned char*)((char*)GWBUF_DATA(gwbuf) + 2)) = (payload_len >> 16);
     *((unsigned char*)((char*)GWBUF_DATA(gwbuf) + 3)) = 0x00;
     *((unsigned char*)((char*)GWBUF_DATA(gwbuf) + 4)) = 0x03;
-    memcpy((char*)GWBUF_DATA(gwbuf) + 5, s.c_str(), s.length() + 1);
+    memcpy((char*)GWBUF_DATA(gwbuf) + 5, s.c_str(), len);
 
     return gwbuf;
 }