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

[Frediano Ziglio]

 server/main_channel.c |    3 +++
 server/reds.c         |   11 ++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

New commits:
commit 5f96b596353d73bdf4bb3cd2de61e48a7fd5b4c3
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Tue Nov 29 16:46:56 2016 +0000

    main-channel: Prevent overflow reading messages from client
    
    Caller is supposed the function return a buffer able to store
    size bytes.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/main_channel.c b/server/main_channel.c
index 0ecc9df..1fc3915 100644
--- a/server/main_channel.c
+++ b/server/main_channel.c
@@ -1026,6 +1026,9 @@ static uint8_t *main_channel_alloc_msg_rcv_buf(RedChannelClient *rcc,
 
     if (type == SPICE_MSGC_MAIN_AGENT_DATA) {
         return reds_get_agent_data_buffer(mcc, size);
+    } else if (size > sizeof(main_chan->recv_buf)) {
+        /* message too large, caller will log a message and close the connection */
+        return NULL;
     } else {
         return main_chan->recv_buf;
     }
commit f66dc643635518e53dfbe5262f814a64eec54e4a
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Tue Dec 13 14:40:10 2016 +0000

    Prevent integer overflows in capability checks
    
    The limits for capabilities are specified using 32 bit unsigned integers.
    This could cause possible integer overflows causing buffer overflows.
    For instance the sum of num_common_caps and num_caps can be 0 avoiding
    additional checks.
    As the link message is now capped to 4096 and the capabilities are
    contained in the link message limit the capabilities to 1024
    (capabilities are expressed in number of uint32_t items).
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/reds.c b/server/reds.c
index 86a33d5..9150454 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2110,6 +2110,14 @@ static void reds_handle_read_link_done(void *opaque)
     link_mess->num_channel_caps = GUINT32_FROM_LE(link_mess->num_channel_caps);
     link_mess->num_common_caps = GUINT32_FROM_LE(link_mess->num_common_caps);
 
+    /* Prevent DoS. Currently we defined only 13 capabilities,
+     * I expect 1024 to be valid for quite a lot time */
+    if (link_mess->num_channel_caps > 1024 || link_mess->num_common_caps > 1024) {
+        reds_send_link_error(link, SPICE_LINK_ERR_INVALID_DATA);
+        reds_link_free(link);
+        return;
+    }
+
     num_caps = link_mess->num_common_caps + link_mess->num_channel_caps;
     caps = (uint32_t *)((uint8_t *)link_mess + link_mess->caps_offset);
 
commit 1c6517973095a67c8cb57f3550fc1298404ab556
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Tue Dec 13 14:39:48 2016 +0000

    Prevent possible DoS attempts during protocol handshake
    
    The limit for link message is specified using a 32 bit unsigned integer.
    This could cause possible DoS due to excessive memory allocations and
    some possible crashes.
    For instance a value >= 2^31 causes a spice_assert to be triggered in
    async_read_handler (reds-stream.c) due to an integer overflow at this
    line:
    
       int n = async->end - async->now;
    
    This could be easily triggered with a program like
    
      #!/usr/bin/env python
    
      import socket
      import time
      from struct import pack
    
      server = '127.0.0.1'
      port = 5900
    
      s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
      s.connect((server, port))
      data = pack('<4sIII', 'REDQ', 2, 2, 0xaaaaaaaa)
      s.send(data)
    
      time.sleep(1)
    
    without requiring any authentication (the same can be done
    with TLS).
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/reds.c b/server/reds.c
index f40b65c..86a33d5 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2202,7 +2202,8 @@ static void reds_handle_read_header_done(void *opaque)
 
     reds->peer_minor_version = header->minor_version;
 
-    if (header->size < sizeof(SpiceLinkMess)) {
+    /* the check for 4096 is to avoid clients to cause arbitrary big memory allocations */
+    if (header->size < sizeof(SpiceLinkMess) || header->size > 4096) {
         reds_send_link_error(link, SPICE_LINK_ERR_INVALID_DATA);
         spice_warning("bad size %u", header->size);
         reds_link_free(link);