| Message ID | 20231122125826.228189-1-f.ebner@proxmox.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for-8.2] ui/vnc-clipboard: fix inflate_buffer | expand |
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? > > This fixes the host->guest direction of the clipboard with noVNC and > TigerVNC as clients. > > Fixes: d921fea338 ("ui/vnc-clipboard: fix infinite loop in inflate_buffer (CVE-2023-3255)") > Reported-by: Friedrich Weber <f.weber@proxmox.com> > Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> > --- > ui/vnc-clipboard.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/ui/vnc-clipboard.c b/ui/vnc-clipboard.c > index c759be3438..124b6fbd9c 100644 > --- a/ui/vnc-clipboard.c > +++ b/ui/vnc-clipboard.c > @@ -69,6 +69,11 @@ static uint8_t *inflate_buffer(uint8_t *in, uint32_t in_len, uint32_t *size) > } > } > > + *size = stream.total_out; > + inflateEnd(&stream); > + > + return out; > + > err_end: > inflateEnd(&stream); > err: > -- > 2.39.2 > > >
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. Would it be better/more correct to ensure that inflate is called again in such a scenario? Best Regards, Fiona
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 ? 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? > Would it be better/more correct to ensure that inflate is called again > in such a scenario? > > Best Regards, > Fiona >
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
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
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." > 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) Best Regards, Fiona
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).
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
Am 04.12.23 um 08:30 schrieb Marc-André Lureau: > Hi > > On Tue, Nov 28, 2023 at 2:52 PM Marc-André Lureau > <marcandre.lureau@gmail.com> wrote: >> >> >> 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 > Thanks to you for looking into the issue! A colleague of mine now opened a bug report for noVNC and according to the the maintainer there, the protocol does not require setting an end marker: https://github.com/novnc/noVNC/issues/1820#issuecomment-1841014968 Best Regards, Fiona
diff --git a/ui/vnc-clipboard.c b/ui/vnc-clipboard.c index c759be3438..124b6fbd9c 100644 --- a/ui/vnc-clipboard.c +++ b/ui/vnc-clipboard.c @@ -69,6 +69,11 @@ static uint8_t *inflate_buffer(uint8_t *in, uint32_t in_len, uint32_t *size) } } + *size = stream.total_out; + inflateEnd(&stream); + + return out; + err_end: inflateEnd(&stream); err:
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. This fixes the host->guest direction of the clipboard with noVNC and TigerVNC as clients. Fixes: d921fea338 ("ui/vnc-clipboard: fix infinite loop in inflate_buffer (CVE-2023-3255)") Reported-by: Friedrich Weber <f.weber@proxmox.com> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> --- ui/vnc-clipboard.c | 5 +++++ 1 file changed, 5 insertions(+)