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

[Marc-André Lureau]

Hi

On Tue, Nov 28, 2023 at 2:52 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> 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).
>

I'll queue your patch for 8.2. thanks



-- 
Marc-André Lureau