[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Sun, 25 Mar 2018 19:26:15 +0200
From: Solar Designer <solar@...nwall.com>
To: oss-security@...ts.openwall.com
Subject: Re: LibVNCServer rfbserver.c: rfbProcessClientNormalMessage() case rfbClientCutText doesn't sanitize msg.cct.length
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
Powered by blists - more mailing lists
Please check out the
Open Source Software Security Wiki, which is counterpart to this
mailing list.
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
