pxcloud/spice-common
5
0
Fork 0
mirror of https://gitlab.uni-freiburg.de/opensourcevdi/spice-common synced 2026-02-01 17:50:07 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity
812 Commits 2 Branches 3 Tags 1.4 MiB
5b6878e72c
Commit Graph

7 Commits

Author SHA1 Message Date
Frediano Ziglio
bb15d4815a Fix flexible array buffer overflow 
This is kind of a DoS, possibly flexible array in the protocol
causes the network size check to be ignored due to integer overflows.

The size of flexible array is computed as (message_end - position),
then this size is added to the number of bytes before the array and
this number is used to check if we overflow initial message.

An example is:

    message {
        uint32 dummy[2];
        uint8 data[] @end;
    } LenMessage;

which generated this (simplified remove useless code) code:

    { /* data */
        data__nelements = message_end - (start + 8);

        data__nw_size = data__nelements;
    }

    nw_size = 8 + data__nw_size;

    /* Check if message fits in reported side */
    if (nw_size > (uintptr_t) (message_end - start)) {
        return NULL;
    }

Following code:
- data__nelements == message_end - (start + 8)
- data__nw_size == data__nelements == message_end - (start + 8)
- nw_size == 8 + data__nw_size == 8 + message_end - (start + 8) ==
  8 + message_end - start - 8 == message_end -start
- the check for overflow is (nw_size > (message_end - start)) but
  nw_size == message_end - start so the check is doing
  ((message_end - start) > (message_end - start)) which is always false.

If message_end - start < 8 then data__nelements (number of element
on the array above) computation generate an integer underflow that
later create a buffer overflow.

Add a check to make sure that the array starts before the message ends
to avoid the overflow.

Difference is:
    diff -u save/generated_client_demarshallers1.c common/generated_client_demarshallers1.c
    --- save/generated_client_demarshallers1.c	2018-06-22 22:13:48.626793919 +0100
    +++ common/generated_client_demarshallers1.c	2018-06-22 22:14:03.408163291 +0100
    @@ -225,6 +225,9 @@
         uint64_t data__nelements;

         { /* data */
    +        if (SPICE_UNLIKELY((start + 0) > message_end)) {
    +            goto error;
    +        }
             data__nelements = message_end - (start + 0);

             data__nw_size = data__nelements;
    @@ -243,6 +246,9 @@
         *free_message = nofree;
         return data;

    +   error:
    +    free(data);
    +    return NULL;
     }

     static uint8_t * parse_msg_set_ack(uint8_t *message_start, uint8_t *message_end, SPICE_GNUC_UNUSED int minor, size_t *size, message_destructor_t *free_message)
    @@ -301,6 +307,9 @@
         SpiceMsgPing *out;

         { /* data */
    +        if (SPICE_UNLIKELY((start + 12) > message_end)) {
    +            goto error;
    +        }
             data__nelements = message_end - (start + 12);

             data__nw_size = data__nelements;
    @@ -5226,6 +5235,9 @@
             uint64_t cursor_data__nw_size;
             uint64_t cursor_data__nelements;
             { /* data */
    +            if (SPICE_UNLIKELY((start2 + 22) > message_end)) {
    +                goto error;
    +            }
                 cursor_data__nelements = message_end - (start2 + 22);

                 cursor_data__nw_size = cursor_data__nelements;
    @@ -5305,6 +5317,9 @@
             uint64_t cursor_data__nw_size;
             uint64_t cursor_data__nelements;
             { /* data */
    +            if (SPICE_UNLIKELY((start2 + 22) > message_end)) {
    +                goto error;
    +            }
                 cursor_data__nelements = message_end - (start2 + 22);

                 cursor_data__nw_size = cursor_data__nelements;
    @@ -5540,6 +5555,9 @@
         SpiceMsgPlaybackPacket *out;

         { /* data */
    +        if (SPICE_UNLIKELY((start + 4) > message_end)) {
    +            goto error;
    +        }
             data__nelements = message_end - (start + 4);

             data__nw_size = data__nelements;
    @@ -5594,6 +5612,9 @@
         SpiceMsgPlaybackMode *out;

         { /* data */
    +        if (SPICE_UNLIKELY((start + 8) > message_end)) {
    +            goto error;
    +        }
             data__nelements = message_end - (start + 8);

             data__nw_size = data__nelements;
    diff -u save/generated_client_demarshallers.c common/generated_client_demarshallers.c
    --- save/generated_client_demarshallers.c	2018-06-22 22:13:48.626793919 +0100
    +++ common/generated_client_demarshallers.c	2018-06-22 22:14:03.004153195 +0100
    @@ -225,6 +225,9 @@
         uint64_t data__nelements;

         { /* data */
    +        if (SPICE_UNLIKELY((start + 0) > message_end)) {
    +            goto error;
    +        }
             data__nelements = message_end - (start + 0);

             data__nw_size = data__nelements;
    @@ -243,6 +246,9 @@
         *free_message = nofree;
         return data;

    +   error:
    +    free(data);
    +    return NULL;
     }

     static uint8_t * parse_msg_set_ack(uint8_t *message_start, uint8_t *message_end, SPICE_GNUC_UNUSED int minor, size_t *size, message_destructor_t *free_message)
    @@ -301,6 +307,9 @@
         SpiceMsgPing *out;

         { /* data */
    +        if (SPICE_UNLIKELY((start + 12) > message_end)) {
    +            goto error;
    +        }
             data__nelements = message_end - (start + 12);

             data__nw_size = data__nelements;
    @@ -6574,6 +6583,9 @@
             }

             { /* data */
    +            if (SPICE_UNLIKELY((start2 + 2 + cursor_u__nw_size) > message_end)) {
    +                goto error;
    +            }
                 cursor_data__nelements = message_end - (start2 + 2 + cursor_u__nw_size);

                 cursor_data__nw_size = cursor_data__nelements;
    @@ -6670,6 +6682,9 @@
             }

             { /* data */
    +            if (SPICE_UNLIKELY((start2 + 2 + cursor_u__nw_size) > message_end)) {
    +                goto error;
    +            }
                 cursor_data__nelements = message_end - (start2 + 2 + cursor_u__nw_size);

                 cursor_data__nw_size = cursor_data__nelements;
    @@ -6907,6 +6922,9 @@
         SpiceMsgPlaybackPacket *out;

         { /* data */
    +        if (SPICE_UNLIKELY((start + 4) > message_end)) {
    +            goto error;
    +        }
             data__nelements = message_end - (start + 4);

             data__nw_size = data__nelements;
    @@ -6961,6 +6979,9 @@
         SpiceMsgPlaybackMode *out;

         { /* data */
    +        if (SPICE_UNLIKELY((start + 6) > message_end)) {
    +            goto error;
    +        }
             data__nelements = message_end - (start + 6);

             data__nw_size = data__nelements;
    @@ -7559,6 +7580,9 @@
         SpiceMsgTunnelSocketData *out;

         { /* data */
    +        if (SPICE_UNLIKELY((start + 2) > message_end)) {
    +            goto error;
    +        }
             data__nelements = message_end - (start + 2);

             data__nw_size = data__nelements;
    @@ -7840,6 +7864,9 @@
         }

         { /* compressed_data */
    +        if (SPICE_UNLIKELY((start + 1 + u__nw_size) > message_end)) {
    +            goto error;
    +        }
             compressed_data__nelements = message_end - (start + 1 + u__nw_size);

             compressed_data__nw_size = compressed_data__nelements;
    diff -u save/generated_server_demarshallers.c common/generated_server_demarshallers.c
    --- save/generated_server_demarshallers.c	2018-06-22 22:13:48.627793944 +0100
    +++ common/generated_server_demarshallers.c	2018-06-22 22:14:05.231208847 +0100
    @@ -306,6 +306,9 @@
         uint64_t data__nelements;

         { /* data */
    +        if (SPICE_UNLIKELY((start + 0) > message_end)) {
    +            goto error;
    +        }
             data__nelements = message_end - (start + 0);

             data__nw_size = data__nelements;
    @@ -324,6 +327,9 @@
         *free_message = nofree;
         return data;

    +   error:
    +    free(data);
    +    return NULL;
     }

     static uint8_t * parse_msgc_disconnecting(uint8_t *message_start, uint8_t *message_end, SPICE_GNUC_UNUSED int minor, size_t *size, message_destructor_t *free_message)
    @@ -1259,6 +1265,9 @@
         SpiceMsgcRecordPacket *out;

         { /* data */
    +        if (SPICE_UNLIKELY((start + 4) > message_end)) {
    +            goto error;
    +        }
             data__nelements = message_end - (start + 4);

             data__nw_size = data__nelements;
    @@ -1313,6 +1322,9 @@
         SpiceMsgcRecordMode *out;

         { /* data */
    +        if (SPICE_UNLIKELY((start + 6) > message_end)) {
    +            goto error;
    +        }
             data__nelements = message_end - (start + 6);

             data__nw_size = data__nelements;
    @@ -1841,6 +1853,9 @@
         SpiceMsgcTunnelSocketData *out;

         { /* data */
    +        if (SPICE_UNLIKELY((start + 2) > message_end)) {
    +            goto error;
    +        }
             data__nelements = message_end - (start + 2);

             data__nw_size = data__nelements;
    @@ -2057,6 +2072,9 @@
         }

         { /* compressed_data */
    +        if (SPICE_UNLIKELY((start + 1 + u__nw_size) > message_end)) {
    +            goto error;
    +        }
             compressed_data__nelements = message_end - (start + 1 + u__nw_size);

             compressed_data__nw_size = compressed_data__nelements;

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
 2018-08-16 15:02:24 +01:00
Frediano Ziglio
f4a0fec5e9 tests: Join test-overflow and test-marshallers 
test-overflow was doing a specific test on demarshalling code.
Joining the 2 tests also allows to remove the dependency from the main
protocol allowing to run the test independently from generation setting.
Using Meson when building either SPICE server or spice-gtk, we only
generate the specific marshallers/demarshallers for that given case.
With this commit the test is built in any case.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
 2018-07-12 05:50:50 +01:00
Frediano Ziglio
2b5cadcd8a test-marshallers: Check for "zero" attribute 
Check the previous fix for "zero" attribute works correctly
(commit bc9df58162, "marshal: Fix a bug
with zero attribute").

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
 2018-07-03 12:25:31 +01:00
Eduardo Lima (Etrunko)
20bda4dba9 Fix demarshaller code generator 
Even though commit df4ec5c318 commented
out most of smartcard code which triggered this error, it still might
happen if a new message is added with an array member.

The reason is a missing declaration of mem_size, which is fixed simply
by checking if the attribute 'nocopy' is present.

The error log follows:

generated_server_demarshallers.c: In function ‘parse_msgc_smartcard_reader_add’:
generated_server_demarshallers.c:1985:30: error: ‘mem_size’ undeclared (first use in this function); did you mean ‘nw_size’?
     data = (uint8_t *)malloc(mem_size);
                              ^~~~~~~~
                              nw_size

This patch also updates test-marshallers so that this bug is triggered.

The diff between generated demarshallers with the patch applied follows:

--- tests/generated_test_demarshallers.c.old    2018-05-17 14:35:29.234056487 -0300
+++ tests/generated_test_demarshallers.c        2018-05-17 14:35:40.554031295 -0300
@@ -286,6 +286,7 @@ static uint8_t * parse_msg_main_ArrayMes
     uint8_t *start = message_start;
     uint8_t *data = NULL;
     uint64_t nw_size;
+    uint64_t mem_size;
     uint8_t *in, *end;
     uint64_t name__nw_size;
     uint64_t name__nelements;
@@ -298,6 +299,7 @@ static uint8_t * parse_msg_main_ArrayMes
     }

     nw_size = 0 + name__nw_size;
+    mem_size = sizeof(SpiceMsgMainArrayMessage);

     /* Check if message fits in reported side */
     if (nw_size > (uintptr_t) (message_end - start)) {

Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
 2018-05-17 21:02:58 +01:00
Frediano Ziglio
c2f2096d90 test-marshallers: Test demarshalling 
Generate demarshallers code and check it too.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
 2017-09-25 16:13:27 +01:00
Frediano Ziglio
c25c75b552 test-marshallers: Use unaligned structure 
Allows to test for bad performance on some systems.
For instance on ARMv6/ARMv7 which does not support by default
64 bit unaligned read/write this can be checked on Linux
using /proc/cpu/alignment file.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
 2017-09-25 16:13:18 +01:00
Christophe Fergeau
523875d8c5 Add marshaller test case 
This allows to test the spice_marshaller_linearize() fix which was sent
recently.
 2015-09-15 17:48:59 +02:00