Re: [PATCH] ui/cursor: incomplete check for integer overflow in cursor_alloc

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, May 23, 2023 at 02:50:09PM +0200, Mauro Matteo Cascella wrote:
> On Tue, May 23, 2023 at 10:16 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Mon, May 08, 2023 at 04:18:13PM +0200, Mauro Matteo Cascella wrote:
> > > The cursor_alloc function still accepts a signed integer for both the cursor
> > > width and height. A specially crafted negative width/height could make datasize
> > > wrap around and cause the next allocation to be 0, potentially leading to a
> > > heap buffer overflow. Modify QEMUCursor struct and cursor_alloc prototype to
> > > accept unsigned ints.
> > >
> > I concur with Marc-Andre that there is no code path that can
> > actually trigger an overflow:
> >
> >
> >   hw/display/ati.c:        s->cursor = cursor_alloc(64, 64);
> >   hw/display/vhost-user-gpu.c:            s->current_cursor = cursor_alloc(64, 64);
> >   hw/display/virtio-gpu.c:            s->current_cursor = cursor_alloc(64, 64);
> >
> > Not exploitable as fixed size
> >
> >   hw/display/qxl-render.c:    c = cursor_alloc(cursor->header.width, cursor->header.height);
> >
> > Cursor header defined as:
> >
> >   typedef struct SPICE_ATTR_PACKED QXLCursorHeader {
> >       uint64_t unique;
> >       uint16_t type;
> >       uint16_t width;
> >       uint16_t height;
> >       uint16_t hot_spot_x;
> >       uint16_t hot_spot_y;
> >   } QXLCursorHeader;
> >
> > So no negative values can be passed to cursor_alloc()

> >
> > > Fixes: CVE-2023-1601
> > > Fixes: fa892e9a ("ui/cursor: fix integer overflow in cursor_alloc (CVE-2021-4206)")
> >
> > Given there is no possible codepath that can overflow, CVE-2023-1601
> > looks invalid to me. It should be clsoed as not-a-bug and these two
> > Fixes lines removed.
> 
> I think you can tweak the original PoC [1] to trigger this bug.
> Setting width/height to 0x80000000 (versus 0x8000) should do the
> trick. You should be able to overflow datasize while bypassing the
> sanity check (width > 512 || height > 512) as width/height are signed
> prior to this patch. I haven't tested it, though.

The QXLCursorHeader  width/height fields are uint16_t, so 0x80000000
will get truncated. No matter what value the guest sets, when we
interpret this in qxl_cursor when calling cursor_alloc, the value
will be in the range 0-65535, as that's the bounds of uint16_t.

We'll pass this unsigned value to cursor_alloc() which converts from
uint16_t, to (signed) int. 'int' is larger than uint16_t, so the
result will still be positive in the range 0-65535, and so the sanity
check > 512 will fire and protect us.

I still see no bug, let alone a CVE.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|