Security: libvncserver/rfbserver.c: rfbProcessClientNormalMessage() case rfbClientCutText doesn't sanitize msg.cct.length

[solardiz]

While I consider this a security-relevant issue, I feel there's no overall benefit from reporting it under an embargo, so here goes.

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) {
            if (n != 0)
                rfbLogPerror("rfbProcessClientNormalMessage: read");
            free(str);
            rfbCloseClient(cl);
            return;
        }
        rfbStatRecordMessageRcvd(cl, msg.type, sz_rfbClientCutTextMsg+msg.cct.length, sz_rfbClientCutTextMsg+msg.cct.length);
        if(!cl->viewOnly) {
            cl->screen->setXCutText(str, msg.cct.length, cl);
        }
        free(str);

        return;

This passes the client-provided 32-bit message length field's value directly into malloc(), reads up to this many bytes from the client, and then passes the full value to the library-user-provided setXCutText() callback (where the value might be higher than the number of bytes actually read - with uninitialized and potentially sensitive data afterwards - and it might also be too high for the callback's implementation to handle safely). There may also be integer overflow in the addition of sz_rfbClientCutTextMsg (which is 8) to the value in the call to rfbStatRecordMessageRcvd(); I did not look into what consequences this might have.

I first found the issue during Openwall's security audit of the Virtuozzo 7 product, which uses a RHEL7-derived package of LibVNCServer-0.9.9 from its prl-vzvncserver component. A corresponding Virtuozzo 7 fix is:

https://src.openvz.org/projects/OVZ/repos/prl-vzvncserver/commits/1204a8872d90c78a2be404dd4b025124bb01b2c5

which hardens prl-vzvncserver's setXCutText() callback - but the rest of the issue needs to be fixed in libvncserver itself, hence the (belated) report in here.

We would like to thank the Virtuozzo company for funding the effort.

Included below is the relevant excerpt from our Virtuozzo 7 report:

--- cut --- 01090, PSBM-58099: prl-vzvncserver and LibVNCServer integer overflows, unlimited memory allocations, and unchecked malloc() Severity: medium Thread: 20161226 "prl-vzvncserver"

A particular combination of these 3 problems is demonstrated by sending the output of echo -e "RFB 003.003\n\001\006\0\0\0\xff\xff\xff\xff" to prl-vzvncserver's TCP port, when prl-vzvncserver is running without password. (When running with password, authentication would be needed before the specific vulnerable code can be reached, and the string to send would accordingly be longer.) This first causes LibVNCServer to allocate 4 GiB of address space and then to hand out this uninitialized memory to the prl-vzvncserver/console.c: vcSetXCutTextProc() callback, which would attempt to make another similar allocation and make a copy of the data. Unfortunately, this LibVNCServer API, as well as many others, is defined to use "int" rather than "size_t" for data sizes, and indeed prl-vzvncserver uses "int" too. For this particular request, this results in a zero byte allocation with malloc(), which succeeds, and then in a memcpy() of (size_t)-1 bytes to it. With a range of other similar requests, malloc() may instead be made to fail (for trying to allocate a ridiculous amount of address space, sign-extended to 64-bit), in which case the memcpy() more reliably fails on a NULL pointer dereference. Either way, the service crashes. Finally, it is possible to have the process actually write to (and thus allocate for real) almost 4 GiB of memory with one request, by making the length field just below 2 GiB. If no data is sent, then 2 GiB would be written from the uninitialized memory (likely mostly read-as-zero) to the memory allocated by prl-vzvncserver's callback. If the data is actually sent, then first it is written to memory by LibVNCServer and then is copied by the callback, for 4 GiB total. Exploitability of this specific issue into something worse than these varying possibilities is highly doubtful (although exploitation of unlimited size memcpy() is not unheard of), but all 3 of these issues are prevalent in prl-vzvncserver and LibVNCServer code in general, so maybe the impact of another similar issue would more obviously be worse. We recommend that sanity checks be introduced into LibVNCServer so that it doesn't try to allocate unreasonable amounts of memory and pass unsafe sizes to callbacks. We also recommend prl-vzvncserver to sanity-check its inputs (including received from LibVNCServer) and in this way avoid integer overflows and unreasonably large allocations. Finally, it is good practice to check whether a malloc() succeeded before writing to the memory. The function vcSetXCutTextProc() came from LibVNCServer-0.9.9/vncterm/VNConsole.c, so its shortcomings also need to be reported to LibVNCServer upstream.

Fix: Some aspects of this issue, most importantly covering prl-vzvncserver's vcSetXCutTextProc() callback, have been addressed with commit 1204a8872d90c78a2be404dd4b025124bb01b2c5 on 20170130.

Related: https://googleprojectzero.blogspot.com/2015/03/taming-wild-copy-parallel-thread.html http://www.giac.org/paper/gcih/361/port-80-apache-http-daemon-exploit/103818 --- cut ---

LibVNCServer-0.9.9/vncterm/VNConsole.c mentioned above appears to be no longer part of libvncserver, hence is not otherwise included in description of this issue.

[solardiz]

Oh, I see that vncterm exists as a separate repo, so I might report its issues in there: https://github.com/LibVNC/vncterm

[solardiz]

Also reported this LibVNCServer issue in http://www.openwall.com/lists/oss-security/2018/02/18/1

[kbabioch]

This has been assigned: CVE-2018-7225

[solardiz]

I think part of the fix should be not invoking the setXCutText() callback at all when rfbReadExact() reads other than exactly msg.cct.length bytes from the client. Invoking the callback with the actual read count would avoid the uninitialized memory issue, but would be weird/unneeded.

There should also be a sane limit on the value of msg.cct.length that this code would agree to process, so that unreasonably large memory allocation and integer overflow risk (including inside a realistic implementation of the callback) are avoided.

Oh, and 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).

[ppisar]

The statement about rfbReadExact() is not true. It returns 0 in case not all requested data could be read. And the rfbClientCutText case correctly closses a connection and returns in that case.

[solardiz]

Oh, you're right. So it's just the conversion to int in the rfbReadExact() call, combined with len > 0 loop condition in rfbReadExactTimeout(), which caused the issue I had observed initially.

Yet maybe rfbReadExactTimeout() semantics should be adjusted so that it'd return failure when called with negative len.

[bk138]

Closed by b0c77391e6bd0a2305bbc9b37a2499af74ddd9ee