Matthias Maier - Ported to 0.13.3 From 111ab38611cef5012f1565a65fa2d8a8a05cce37 Mon Sep 17 00:00:00 2001 From: Frediano Ziglio Date: Mon, 15 May 2017 15:57:28 +0100 Subject: [PATCH 1/3] 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 --- diff --git a/server/reds.c b/server/reds.c index 92feea1..286993b 100644 --- a/server/reds.c +++ b/server/reds.c @@ -1077,19 +1077,35 @@ static void reds_client_monitors_config_cleanup(RedsState *reds) static void reds_on_main_agent_monitors_config(RedsState *reds, 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; } @@ -1097,6 +1113,12 @@ static void reds_on_main_agent_monitors_config(RedsState *reds, spice_debug("%s: %d", __func__, monitors_config->num_of_monitors); reds_client_monitors_config(reds, monitors_config); reds_client_monitors_config_cleanup(reds); + return; + +overflow: + spice_warning("received invalid MonitorsConfig request from client, disconnecting"); + red_channel_client_disconnect(RED_CHANNEL_CLIENT(mcc)); + reds_client_monitors_config_cleanup(reds); } void reds_on_main_agent_data(RedsState *reds, MainChannelClient *mcc, void *message, size_t size) From 571cec91e71c2aae0d5f439ea2d8439d0c3d75eb Mon Sep 17 00:00:00 2001 From: Frediano Ziglio Date: Mon, 15 May 2017 15:57:28 +0100 Subject: [PATCH 2/3] reds: Avoid integer overflows handling monitor configuration Avoid VDAgentMessage::size integer overflows. Signed-off-by: Frediano Ziglio --- server/reds.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/reds.c b/server/reds.c index ec2b6f47..656f518f 100644 --- a/server/reds.c +++ b/server/reds.c @@ -1131,6 +1131,9 @@ static void reds_on_main_agent_monitors_config(RedsState *reds, spice_debug("not enough data yet. %zd", cmc->offset); return; } + if (msg_header->size < sizeof(VDAgentMonitorsConfig)) { + goto overflow; + } monitors_config = (VDAgentMonitorsConfig *)(cmc->buffer + sizeof(*msg_header)); spice_debug("monitors_config->num_of_monitors: %d", monitors_config->num_of_monitors); reds_client_monitors_config(reds, monitors_config); -- 2.13.0 Matthias Maier - Ported to 0.13.3 From fbbcdad773e2791cfb988f4748faa41943551ca6 Mon Sep 17 00:00:00 2001 From: Frediano Ziglio Date: Mon, 15 May 2017 15:57:28 +0100 Subject: [PATCH 3/3] 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 --- diff --git a/server/reds.c b/server/reds.c index ec89105..fd1457f 100644 --- a/server/reds.c +++ b/server/reds.c @@ -1084,6 +1084,7 @@ static void reds_on_main_agent_monitors_config(RedsState *reds, 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 @@ -1113,6 +1114,12 @@ static void reds_on_main_agent_monitors_config(RedsState *reds, 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); reds_client_monitors_config(reds, monitors_config); reds_client_monitors_config_cleanup(reds); M