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

[Jonathon Jongsma]

On Wed, 2018-07-11 at 13:21 -0400, Frediano Ziglio wrote:
> > 
> > On Tue, 2018-07-10 at 14:35 +0100, Frediano Ziglio wrote:
> > > Check we have enough data before reading.
> > 
> > Check *that* we have...
> > 
> 
> updated, thanks
> 
> > > 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>
> > 
> 
> does
> 
>        g_warning("missing header in LZ4 data");
> 
> look fine?
> 

Yeah, that sounds fine.

Jonathon


> Frediano
> 
> > 
> > > +    }
> > >      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
> > >