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

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

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
>


-- 
Marc-André Lureau

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

-- 
Marc-André Lureau

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


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

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