From 3554c19d1c65c2bc4ae8cadb7296256c03215257 Mon Sep 17 00:00:00 2001

From: Christophe Fergeau <cfergeau@redhat.com>

Date: Fri, 23 Aug 2013 11:29:44 +0200

Subject: [PATCH] Fix buffer overflow when decrypting client SPICE ticket

reds_handle_ticket uses a fixed size 'password' buffer for the decrypted

password whose size is SPICE_MAX_PASSWORD_LENGTH. However,

RSA_private_decrypt which we call for the decryption expects the

destination buffer to be at least RSA_size(link->tiTicketing.rsa)

bytes long. On my spice-server build, SPICE_MAX_PASSWORD_LENGTH

is 60 while RSA_size() is 128, so we end up overflowing 'password'

when using long passwords (this was reproduced using the string:

'fullscreen=1proxy=#enter proxy here; e.g spice_proxy = http://[proxy]:[port]'

as a password).

When the overflow occurs, QEMU dies with:

*** stack smashing detected ***: qemu-system-x86_64 terminated

This commit ensures we use a corectly sized 'password' buffer,

and that it's correctly nul-terminated so that we can use strcmp

instead of strncmp. To keep using strncmp, we'd need to figure out

which one of 'password' and 'taTicket.password' is the smaller buffer,

and use that size.

This fixes rhbz#999839

---

 server/reds.c | 44 ++++++++++++++++++++++++++++++++------------

 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/server/reds.c b/server/reds.c

index 0f81a32..7b7f262 100644

--- a/server/reds.c

+++ b/server/reds.c

@@ -1932,39 +1932,59 @@ static void reds_handle_link(RedLinkInfo *link)

 static void reds_handle_ticket(void *opaque)

 {

     RedLinkInfo *link = (RedLinkInfo *)opaque;

-    char password[SPICE_MAX_PASSWORD_LENGTH];

+    char *password;

     time_t ltime;

+    int password_size;

     //todo: use monotonic time

     time(<ime);

-    RSA_private_decrypt(link->tiTicketing.rsa_size,

-                        link->tiTicketing.encrypted_ticket.encrypted_data,

-                        (unsigned char *)password, link->tiTicketing.rsa, RSA_PKCS1_OAEP_PADDING);

+    if (RSA_size(link->tiTicketing.rsa) < SPICE_MAX_PASSWORD_LENGTH) {

+        spice_warning("RSA modulus size is smaller than SPICE_MAX_PASSWORD_LENGTH (%d < %d), "

+                      "SPICE ticket sent from client may be truncated",

+                      RSA_size(link->tiTicketing.rsa), SPICE_MAX_PASSWORD_LENGTH);

+    }

+

+    password = g_malloc0(RSA_size(link->tiTicketing.rsa) + 1);

+    password_size = RSA_private_decrypt(link->tiTicketing.rsa_size,

+                                        link->tiTicketing.encrypted_ticket.encrypted_data,

+                                        (unsigned char *)password,

+                                        link->tiTicketing.rsa,

+                                        RSA_PKCS1_OAEP_PADDING);

+    if (password_size == -1) {

+        spice_warning("failed to decrypt RSA encrypted password: %s",

+                      ERR_error_string(ERR_get_error(), NULL));

+        goto error;

+    }

+    password[password_size] = '\0';

     if (ticketing_enabled && !link->skip_auth) {

         int expired =  taTicket.expiration_time < ltime;

         if (strlen(taTicket.password) == 0) {

-            reds_send_link_result(link, SPICE_LINK_ERR_PERMISSION_DENIED);

             spice_warning("Ticketing is enabled, but no password is set. "

-                        "please set a ticket first");

-            reds_link_free(link);

-            return;

+                          "please set a ticket first");

+            goto error;

         }

-        if (expired || strncmp(password, taTicket.password, SPICE_MAX_PASSWORD_LENGTH) != 0) {

+        if (expired || strcmp(password, taTicket.password) != 0) {

             if (expired) {

                 spice_warning("Ticket has expired");

             } else {

                 spice_warning("Invalid password");

             }

-            reds_send_link_result(link, SPICE_LINK_ERR_PERMISSION_DENIED);

-            reds_link_free(link);

-            return;

+            goto error;

         }

     }

     reds_handle_link(link);

+    goto end;

+

+error:

+    reds_send_link_result(link, SPICE_LINK_ERR_PERMISSION_DENIED);

+    reds_link_free(link);

+

+end:

+    g_free(password);

 }

 static inline void async_read_clear_handlers(AsyncRead *obj)

-- 

1.8.3.1