[prev in list] [next in list] [prev in thread] [next in thread]
List: oss-security
Subject: Re: [oss-security] LibVNCServer rfbserver.c: rfbProcessClientNormalMessage() case rfbClientCutText d
From: Solar Designer <solar () openwall ! com>
Date: 2018-03-25 17:26:15
Message-ID: 20180325172614.GA26989 () openwall ! com
[Download RAW message or body]
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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic