[Spice-commits] Branch '0.12' - 3 commits - server/reds.c

[Christophe Fergau]

 server/reds.c |   35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

New commits:
commit a957a90baf2c62d31f3547e56bba7d0e812d2331
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Mon May 15 15:57:28 2017 +0100

    reds: Avoid buffer overflows handling monitor configuration
    
    It was also possible for a malicious client to set
    VDAgentMonitorsConfig::num_of_monitors to a number larger
    than the actual size of VDAgentMOnitorsConfig::monitors.
    This would lead to buffer overflows, which could allow the guest to
    read part of the host memory. This might cause write overflows in the
    host as well, but controlling the content of such buffers seems
    complicated.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

diff --git a/server/reds.c b/server/reds.c
index e1c8c108..3a42c375 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -1000,6 +1000,7 @@ static void reds_on_main_agent_monitors_config(
     VDAgentMessage *msg_header;
     VDAgentMonitorsConfig *monitors_config;
     RedsClientMonitorsConfig *cmc = &reds->client_monitors_config;
+    uint32_t max_monitors;
 
     // limit size of message sent by the client as this can cause a DoS through
     // memory exhaustion, or potentially some integer overflows
@@ -1028,6 +1029,12 @@ static void reds_on_main_agent_monitors_config(
         goto overflow;
     }
     monitors_config = (VDAgentMonitorsConfig *)(cmc->buffer + sizeof(*msg_header));
+    // limit the monitor number to avoid buffer overflows
+    max_monitors = (msg_header->size - sizeof(VDAgentMonitorsConfig)) /
+                   sizeof(VDAgentMonConfig);
+    if (monitors_config->num_of_monitors > max_monitors) {
+        goto overflow;
+    }
     spice_debug("%s: %d", __func__, monitors_config->num_of_monitors);
     red_dispatcher_client_monitors_config(monitors_config);
     reds_client_monitors_config_cleanup();
commit ec6229c79abe05d731953df5f7e9a05ec9f6df79
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Mon May 15 15:57:28 2017 +0100

    reds: Avoid integer overflows handling monitor configuration
    
    Avoid VDAgentMessage::size integer overflows.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

diff --git a/server/reds.c b/server/reds.c
index 7be85fdf..e1c8c108 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -1024,6 +1024,9 @@ static void reds_on_main_agent_monitors_config(
         spice_debug("not enough data yet. %d", cmc->buffer_size);
         return;
     }
+    if (msg_header->size < sizeof(VDAgentMonitorsConfig)) {
+        goto overflow;
+    }
     monitors_config = (VDAgentMonitorsConfig *)(cmc->buffer + sizeof(*msg_header));
     spice_debug("%s: %d", __func__, monitors_config->num_of_monitors);
     red_dispatcher_client_monitors_config(monitors_config);
commit f1e7ec03e26ab6b8ca9b7ec060846a5b706a963d
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Mon May 15 15:57:28 2017 +0100

    reds: Disconnect when receiving overly big ClientMonitorsConfig
    
    Total message size received from the client was unlimited. There is
    a 2kiB size check on individual agent messages, but the MonitorsConfig
    message can be split in multiple chunks, and the size of the
    non-chunked MonitorsConfig message was never checked. This could easily
    lead to memory exhaustion on the host.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

diff --git a/server/reds.c b/server/reds.c
index f439a366..7be85fdf 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -993,19 +993,34 @@ static void reds_client_monitors_config_cleanup(void)
 static void reds_on_main_agent_monitors_config(
         MainChannelClient *mcc, void *message, size_t size)
 {
+    const unsigned int MAX_MONITORS = 256;
+    const unsigned int MAX_MONITOR_CONFIG_SIZE =
+       sizeof(VDAgentMonitorsConfig) + MAX_MONITORS * sizeof(VDAgentMonConfig);
+
     VDAgentMessage *msg_header;
     VDAgentMonitorsConfig *monitors_config;
     RedsClientMonitorsConfig *cmc = &reds->client_monitors_config;
 
+    // limit size of message sent by the client as this can cause a DoS through
+    // memory exhaustion, or potentially some integer overflows
+    if (sizeof(VDAgentMessage) + MAX_MONITOR_CONFIG_SIZE - cmc->buffer_size < size) {
+        goto overflow;
+    }
     cmc->buffer_size += size;
     cmc->buffer = realloc(cmc->buffer, cmc->buffer_size);
     spice_assert(cmc->buffer);
     cmc->mcc = mcc;
     memcpy(cmc->buffer + cmc->buffer_pos, message, size);
     cmc->buffer_pos += size;
+    if (sizeof(VDAgentMessage) > cmc->buffer_size) {
+        spice_debug("not enough data yet. %d", cmc->buffer_size);
+        return;
+    }
     msg_header = (VDAgentMessage *)cmc->buffer;
-    if (sizeof(VDAgentMessage) > cmc->buffer_size ||
-            msg_header->size > cmc->buffer_size - sizeof(VDAgentMessage)) {
+    if (msg_header->size > MAX_MONITOR_CONFIG_SIZE) {
+        goto overflow;
+    }
+    if (msg_header->size > cmc->buffer_size - sizeof(VDAgentMessage)) {
         spice_debug("not enough data yet. %d", cmc->buffer_size);
         return;
     }
@@ -1013,6 +1028,12 @@ static void reds_on_main_agent_monitors_config(
     spice_debug("%s: %d", __func__, monitors_config->num_of_monitors);
     red_dispatcher_client_monitors_config(monitors_config);
     reds_client_monitors_config_cleanup();
+    return;
+
+overflow:
+    spice_warning("received invalid MonitorsConfig request from client, disconnecting");
+    red_channel_client_disconnect(main_channel_client_get_base(mcc));
+    reds_client_monitors_config_cleanup();
 }
 
 void reds_on_main_agent_data(MainChannelClient *mcc, void *message, size_t size)