[Spice-devel] [PATCH spice-common 1/2] canvas_base: Check for overflows decoding LZ4

[Jonathon Jongsma]

On Tue, 2018-07-10 at 14:35 +0100, Frediano Ziglio wrote:
> Check we have enough data before reading.

Check *that* we have...

> This could lead to read buffer overflows being undetected.
> This is not a security issue, read happens only in the client not
> causing
> any information leakage, maximum can generate a crash or some garbage
> on
> the screen.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  common/canvas_base.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/common/canvas_base.c b/common/canvas_base.c
> index 05c4e1f..ae07625 100644
> --- a/common/canvas_base.c
> +++ b/common/canvas_base.c
> @@ -537,6 +537,9 @@ static pixman_image_t *canvas_get_lz4(CanvasBase
> *canvas, SpiceImage *image)
>      width = image->descriptor.width;
>      stride_encoded = width;
>      height = image->descriptor.height;
> +    if (data + 2 > data_end) {
> +        return NULL;

All other error conditions that you handle below print a warning. Do
you want one here as well?

Acked-by: Jonathon Jongsma <jjongsma at redhat.com>


> +    }
>      top_down = !!*(data++);
>      spice_format = *(data++);
>      switch (spice_format) {
> @@ -579,16 +582,22 @@ static pixman_image_t
> *canvas_get_lz4(CanvasBase *canvas, SpiceImage *image)
>      bits = dest;
>  
>      do {
> +        if (data + 4 > data_end) {
> +            goto format_error;
> +        }
>          // Read next compressed block
>          enc_size = read_uint32_be(data);
>          data += 4;
> +        /* check overflow. This check is a bit different to avoid
> +         * possible overflows. From previous check data_end - data
> cannot overflow.
> +         * Computing data + enc_size on 32 bit could cause
> overflows. */
> +        if (enc_size < 0 || data_end - data < (unsigned int)
> enc_size) {
> +            goto format_error;
> +        }
>          dec_size = LZ4_decompress_safe_continue(stream, (const char
> *) data,
>                                                  (char *) dest,
> enc_size, available);
>          if (dec_size <= 0) {
> -            spice_warning("Error decoding LZ4 block\n");
> -            pixman_image_unref(surface);
> -            surface = NULL;
> -            break;
> +            goto format_error;
>          }
>          dest += dec_size;
>          available -= dec_size;
> @@ -599,6 +608,12 @@ static pixman_image_t *canvas_get_lz4(CanvasBase
> *canvas, SpiceImage *image)
>  
>      LZ4_freeStreamDecode(stream);
>      return surface;
> +
> +format_error:
> +    spice_warning("Error decoding LZ4 block\n");
> +    LZ4_freeStreamDecode(stream);
> +    pixman_image_unref(surface);
> +    return NULL;
>  }
>  #endif
>