qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH for-8.2] ui/vnc-clipboard: fix inflate_buffer
@ 2023-11-22 12:58 Fiona Ebner
  2023-11-22 13:06 ` Marc-André Lureau
  0 siblings, 1 reply; 10+ messages in thread
From: Fiona Ebner @ 2023-11-22 12:58 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-stable, marcandre.lureau, kraxel, mcascell

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(+)

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




^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH for-8.2] ui/vnc-clipboard: fix inflate_buffer
  2023-11-22 12:58 [PATCH for-8.2] ui/vnc-clipboard: fix inflate_buffer Fiona Ebner
@ 2023-11-22 13:06 ` Marc-André Lureau
  2023-11-22 13:25   ` Fiona Ebner
  0 siblings, 1 reply; 10+ messages in thread
From: Marc-André Lureau @ 2023-11-22 13:06 UTC (permalink / raw)
  To: Fiona Ebner; +Cc: qemu-devel, qemu-stable, kraxel, mcascell

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


-- 
Marc-André Lureau


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-8.2] ui/vnc-clipboard: fix inflate_buffer
  2023-11-22 13:06 ` Marc-André Lureau
@ 2023-11-22 13:25   ` Fiona Ebner
  2023-11-23  6:52     ` Marc-André Lureau
  0 siblings, 1 reply; 10+ messages in thread
From: Fiona Ebner @ 2023-11-22 13:25 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: qemu-devel, qemu-stable, kraxel, mcascell

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-8.2] ui/vnc-clipboard: fix inflate_buffer
  2023-11-22 13:25   ` Fiona Ebner
@ 2023-11-23  6:52     ` Marc-André Lureau
  2023-11-23  8:46       ` Fiona Ebner
  0 siblings, 1 reply; 10+ messages in thread
From: Marc-André Lureau @ 2023-11-23  6:52 UTC (permalink / raw)
  To: Fiona Ebner; +Cc: qemu-devel, qemu-stable, kraxel, mcascell

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
>


-- 
Marc-André Lureau


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-8.2] ui/vnc-clipboard: fix inflate_buffer
  2023-11-23  6:52     ` Marc-André Lureau
@ 2023-11-23  8:46       ` Fiona Ebner
  2023-11-27  9:15         ` Marc-André Lureau
  0 siblings, 1 reply; 10+ messages in thread
From: Fiona Ebner @ 2023-11-23  8:46 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: qemu-devel, qemu-stable, kraxel, mcascell

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-8.2] ui/vnc-clipboard: fix inflate_buffer
  2023-11-23  8:46       ` Fiona Ebner
@ 2023-11-27  9:15         ` Marc-André Lureau
  2023-11-27 10:51           ` Fiona Ebner
  0 siblings, 1 reply; 10+ messages in thread
From: Marc-André Lureau @ 2023-11-27  9:15 UTC (permalink / raw)
  To: Fiona Ebner; +Cc: qemu-devel, qemu-stable, kraxel, mcascell

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-8.2] ui/vnc-clipboard: fix inflate_buffer
  2023-11-27  9:15         ` Marc-André Lureau
@ 2023-11-27 10:51           ` Fiona Ebner
  2023-11-28 10:52             ` Marc-André Lureau
  0 siblings, 1 reply; 10+ messages in thread
From: Fiona Ebner @ 2023-11-27 10:51 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: qemu-devel, qemu-stable, kraxel, mcascell

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-8.2] ui/vnc-clipboard: fix inflate_buffer
  2023-11-27 10:51           ` Fiona Ebner
@ 2023-11-28 10:52             ` Marc-André Lureau
  2023-12-04  7:30               ` Marc-André Lureau
  0 siblings, 1 reply; 10+ messages in thread
From: Marc-André Lureau @ 2023-11-28 10:52 UTC (permalink / raw)
  To: Fiona Ebner; +Cc: qemu-devel, qemu-stable, kraxel, mcascell

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-8.2] ui/vnc-clipboard: fix inflate_buffer
  2023-11-28 10:52             ` Marc-André Lureau
@ 2023-12-04  7:30               ` Marc-André Lureau
  2023-12-06  8:59                 ` Fiona Ebner
  0 siblings, 1 reply; 10+ messages in thread
From: Marc-André Lureau @ 2023-12-04  7:30 UTC (permalink / raw)
  To: Fiona Ebner; +Cc: qemu-devel, qemu-stable, kraxel, mcascell

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-8.2] ui/vnc-clipboard: fix inflate_buffer
  2023-12-04  7:30               ` Marc-André Lureau
@ 2023-12-06  8:59                 ` Fiona Ebner
  0 siblings, 0 replies; 10+ messages in thread
From: Fiona Ebner @ 2023-12-06  8:59 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: qemu-devel, qemu-stable, kraxel, mcascell

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-12-06  9:00 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-22 12:58 [PATCH for-8.2] ui/vnc-clipboard: fix inflate_buffer Fiona Ebner
2023-11-22 13:06 ` Marc-André Lureau
2023-11-22 13:25   ` Fiona Ebner
2023-11-23  6:52     ` Marc-André Lureau
2023-11-23  8:46       ` Fiona Ebner
2023-11-27  9:15         ` Marc-André Lureau
2023-11-27 10:51           ` Fiona Ebner
2023-11-28 10:52             ` Marc-André Lureau
2023-12-04  7:30               ` Marc-André Lureau
2023-12-06  8:59                 ` Fiona Ebner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).