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

[Marc-André Lureau]

Hi

On Mon, Nov 27, 2023 at 2:52 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> Am 27.11.23 um 10:15 schrieb Marc-André Lureau:
> >
> > 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?
> >
>
> This was with noVNC. A colleague tested with TigerVNC. I haven't stepped
> through with GDB there, but it might be similar. No, I haven't
> reported/investigated for the VNC clients yet. Unfortunately, I've got
> my hands full with other things at the moment, so it will be a while
> until I can do that.
>
> Even if it's a bug in the clients, this was working before d921fea338
> ("ui/vnc-clipboard: fix infinite loop in inflate_buffer
> (CVE-2023-3255)") so I still feel like it might be worth handling in QEMU.
>
> But is it really a client error? What I don't understand is why the
> return value of inflate() is Z_BUF_ERROR even though all the input was
> handled.
>
> From https://www.zlib.net/manual.html
>
> "inflate() returns [...] Z_BUF_ERROR if no progress was possible or if
> there was not enough room in the output buffer when Z_FINISH is used."

At the end of the input stream, subsequent calls could not make
progress. We never reached Z_STREAM_END

It seems to me the callers do not flush the streams with Z_FINISH
(https://github.com/TigerVNC/tigervnc/blob/master/common/rdr/ZlibOutStream.cxx),
and this is what marks the end of a zlib stream ultimately...

>
> > 51            ret = inflate(&stream, Z_FINISH);
> > (gdb) p stream
> > $23 = {next_in = 0x555557652708 "", avail_in = 5, total_in = 12, next_out = 
> > 0x555557627378 "", avail_out = 8, total_out = 8, msg = 0x0, state = 
> > 0x5555578df5c0, zalloc = 0x7ffff7bc1560, zfree = 0x7ffff7bc1570,
> >   opaque = 0x0, data_type = 5, adler = 71434672, reserved = 0}
> > (gdb) n
> > 52            switch (ret) {
> > (gdb) p stream
> > $24 = {next_in = 0x55555765270d "", avail_in = 0, total_in = 17, next_out = 
> > 0x555557627379 "", avail_out = 7, total_out = 9, msg = 0x0, state = 
> > 0x5555578df5c0, zalloc = 0x7ffff7bc1560, zfree = 0x7ffff7bc1570,
> >   opaque = 0x0, data_type = 128, adler = 99746224, reserved = 0}
> > (gdb) p ret
> > $25 = -5
> > (gdb) p out + 4
> > $26 = (uint8_t *) 0x555557627374 "fish"
>
> Progress was made and there was enough space for the output (avail_out =
> 7 after the call), so it really shouldn't return Z_BUF_ERROR, right?
>
> zlib version is 1:1.2.13.dfsg-1 (Debian 12 Bookworm)


It's hard to make the best decision.

We could return the uncompressed data so far, that would fix the
regression. But potentially, we have incomplete data returned. Clients
should be fixed to include Z_STREAM_END marker (using Z_FINISH).


-- 
Marc-André Lureau