Re: LibVNCServer rfbserver.c: rfbProcessClientNormalMessage() case rfbClientCutText doesn't sanitize msg.cct.length

[Solar Designer <solar () openwall com>]

On Thu, Feb 22, 2018 at 06:23:29PM +0100, Solar Designer wrote:
> On Sun, Feb 18, 2018 at 07:09:45PM +0100, Solar Designer wrote:
> > https://github.com/LibVNC/libvncserver/issues/218
>
> > libvncserver/rfbserver.c: rfbProcessClientNormalMessage() contains the
> > following code:
> >
> >     case rfbClientCutText:
> >
> >         if ((n = rfbReadExact(cl, ((char *)&msg) + 1,
> >                            sz_rfbClientCutTextMsg - 1)) <= 0) {
> >             if (n != 0)
> >                 rfbLogPerror("rfbProcessClientNormalMessage: read");
> >             rfbCloseClient(cl);
> >             return;
> >         }
> >
> >         msg.cct.length = Swap32IfLE(msg.cct.length);
> >
> >         str = (char *)malloc(msg.cct.length);
> >         if (str == NULL) {
> >                 rfbLogPerror("rfbProcessClientNormalMessage: not enough memory");
> >                 rfbCloseClient(cl);
> >                 return;
> >         }
> >
> >         if ((n = rfbReadExact(cl, str, msg.cct.length)) <= 0) {
>
> As I just wrote in a comment to the GitHub issue above:
>
> There's another issue I had missed: the first rfbReadExact() reading the
> msg header is only checked for <= 0, but that doesn't catch a partial
> read e.g. on a prematurely closed connection.  The same issue is present
> all over the codebase.  I guess "Exact" in the name was understood
> literally, but the function doesn't guarantee that when a lower-level
> read() or the like returns 0, such as when there's no more data to read.
> Maybe the function itself should be adjusted to match the semantics the
> callers expects from it (set errno to a value of its choosing and return
> -1 on a partial read? it already does that on a timeout, so this change
> wouldn't make it more inconsistent).

As Petr Pisar pointed out on the GitHub issue, I was wrong about that
"another issue" above.  rfbReadExact() returns 0 on a partial read, so
the "<= 0" checks do correctly detect this failure mode.  So, no,
luckily the issue is not "present all over the codebase."

The cause of the behavior I had observed, where rfbReadExact() was not
"<= 0" on a partial read in my original test case, was different: it was
implicit conversion of msg.cct.length to int (resulting in a negative
value) when making the call to rfbReadExact().  In that case,
rfbReadExactTimeout() and thus rfbReadExact() return 1:

int
rfbReadExactTimeout(rfbClientPtr cl, char* buf, int len, int timeout)
{
[...]
    while (len > 0) {
[...]
    }
[...]
    return 1;
}

As I wrote in a comment to the GitHub issue, as a hardening measure
"maybe rfbReadExactTimeout() semantics should be adjusted so that it'd
return failure when called with negative len."

Meanwhile, Petr fixed the original issue I had reported, by limiting the
cut text length to 1 MiB (the same limit that QEMU uses):

https://github.com/LibVNC/libvncserver/commit/b0c77391e6bd0a2305bbc9b37a2499af74ddd9ee

Alexander