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

[Fiona Ebner]

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'.

Best Regards,
Fiona