Skip to content
  • Frediano Ziglio's avatar
    Fix flexible array buffer overflow · bb15d481
    Frediano Ziglio authored
    
    
    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: default avatarFrediano Ziglio <fziglio@redhat.com>
    Signed-off-by: default avatarChristophe Fergeau <cfergeau@redhat.com>
    bb15d481