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

[Mauro Matteo Cascella]

On Tue, May 23, 2023 at 3:03 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> 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.

Oh, you are right! Then yes, feel free to drop the two 'Fixes' lines.
This is more of a hardening bug than a real security issue. I'll
reject the newly assigned CVE.

Thanks,

> 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 :|
>
--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0