Re: [PATCH for-8.2] ui/vnc-clipboard: fix inflate_buffer

[Marc-André Lureau]

Hi

On Thu, Nov 23, 2023 at 12:46 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> Am 23.11.23 um 07:52 schrieb Marc-André Lureau:
> > Hi
> >
> > On Wed, Nov 22, 2023 at 5:25 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
> >>
> >> Am 22.11.23 um 14:06 schrieb Marc-André Lureau:
> >>> Hi
> >>>
> >>> On Wed, Nov 22, 2023 at 5:00 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
> >>>>
> >>>> Commit d921fea338 ("ui/vnc-clipboard: fix infinite loop in
> >>>> inflate_buffer (CVE-2023-3255)") removed this hunk, but it is still
> >>>> required, because it can happen that stream.avail_in becomes zero
> >>>> before coming across a return value of Z_STREAM_END in the loop.
> >>>
> >>> Isn't this an error from the client side then?
> >>>
> >>
> >> In my test just now I get Z_BUF_ERROR twice and after the second one,
> >> stream.avail_in is zero. Maybe if you'd call inflate() again, you'd get
> >> Z_STREAM_END, but no such call is made, because we exit the loop.
> >
> > It should exit the loop after calling inflate() again though.
> >
> > Or do you mean that it goes to Z_BUF_ERROR a second time with
> > stream.avail_in == 0, thus exit the loop quickly after ?
>
> Yes, sorry if I wasn't clear. After the second inflate() call,
> stream.avail_in == 0. See below.
>
> >
> > That could mean that the input buffer is not complete.
> >
> > "Note that Z_BUF_ERROR is not fatal, and inflate() can be called again
> > with more input..."
> >
> > Something is fishy.. Is it easy to reproduce?
> >
>
> Yes, always. For example when entering "foobar" in the clipboard on the
> host side:
>
> > Thread 1 "qemu-system-x86" hit Breakpoint 2, inflate_buffer 
> > (in=0x555557a2980c "x^b```O\313\317OJ,b", in_len=19, size=0x7fffffffd058) 
> > at ../ui/vnc-clipboard.c:44
> > 44        ret = inflateInit(&stream);
> > (gdb) n
> > [Thread 0x7ffec7c166c0 (LWP 20918) exited]
> > 45        if (ret != Z_OK) {
> > (gdb) p stream
> > $18 = {next_in = 0x555557a2980c "x^b```O\313\317OJ,b", avail_in = 19, 
> > total_in = 0, next_out = 0x555557173d20 "\303\337:\002PU", avail_out = 8, 
> > total_out = 0, msg = 0x0, state = 0x555557654200,
> >   zalloc = 0x7ffff7bc1560, zfree = 0x7ffff7bc1570, opaque = 0x0, data_type 
> > = 0, adler = 1, reserved = 0}
> > (gdb) c
> > Continuing.
> > [New Thread 0x7ffec7c166c0 (LWP 21011)]
> >
> > Thread 1 "qemu-system-x86" hit Breakpoint 3, inflate_buffer 
> > (in=0x555557a2980c "x^b```O\313\317OJ,b", in_len=19, size=0x7fffffffd058) 
> > at ../ui/vnc-clipboard.c:50
> > 50            ret = inflate(&stream, Z_FINISH);
> > (gdb) n
> > [Thread 0x7ffec7c166c0 (LWP 21011) exited]
> > 51            switch (ret) {
> > (gdb) p ret
> > $19 = -5
> > (gdb) p stream
> > $20 = {next_in = 0x555557a29818 "b", avail_in = 7, total_in = 12, next_out 
> > = 0x555557173d28 "", avail_out = 0, total_out = 8, msg = 0x0, state = 
> > 0x555557654200, zalloc = 0x7ffff7bc1560, zfree = 0x7ffff7bc1570,
> >   opaque = 0x0, data_type = 5, adler = 72352174, reserved = 0}
> > (gdb) c
> > Continuing.
> > [New Thread 0x7ffec7c166c0 (LWP 21131)]
> >
> > Thread 1 "qemu-system-x86" hit Breakpoint 3, inflate_buffer 
> > (in=0x555557a2980c "x^b```O\313\317OJ,b", in_len=19, size=0x7fffffffd058) 
> > at ../ui/vnc-clipboard.c:50
> > 50            ret = inflate(&stream, Z_FINISH);
> > (gdb) n
> > [Thread 0x7ffec7c166c0 (LWP 21131) exited]
> > 51            switch (ret) {
> > (gdb) p ret
> > $21 = -5
> > (gdb) p stream
> > $22 = {next_in = 0x555557a2981f "", avail_in = 0, total_in = 19, next_out = 
> > 0x555557173d2b "", avail_out = 5, total_out = 11, msg = 0x0, state = 
> > 0x555557654200, zalloc = 0x7ffff7bc1560, zfree = 0x7ffff7bc1570,
> >   opaque = 0x0, data_type = 128, adler = 190907009, reserved = 0}
> > (gdb) p out + 4
> > $23 = (uint8_t *) 0x555557173d24 "foobar"
> > (gdb) p *size
> > $24 = 0
>
> Now we exit the loop and without the hunk this patch re-adds, we don't
> update *size and don't return 'out'.
>

It seems like a bug in tigervnc then. For some reason, the compressed
data doesn't trigger Z_STREAM_END on the decompression side. Have you
investigated or reported an issue to them?

thanks

-- 
Marc-André Lureau