Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
solardiz opened this issue Feb 18, 2018 · 7 comments

Comments

Projects
None yet
4 participants
@solardiz
Copy link

commented Feb 18, 2018

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

This comment has been minimized.

Copy link
Author

commented Feb 18, 2018

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

@solardiz

This comment has been minimized.

Copy link
Author

commented Feb 18, 2018

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

@kbabioch

This comment has been minimized.

Copy link

commented Feb 19, 2018

This has been assigned: CVE-2018-7225

@solardiz

This comment has been minimized.

Copy link
Author

commented Feb 22, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

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.

ppisar added a commit to ppisar/libvncserver that referenced this issue Feb 26, 2018

Validate client cut text length
Client-provided unsigned 32-bit cut text length is passed to various
functions that expects argument of a different type.

E.g. "RFB 003.003\n\001\006\0\0\0\xff\xff\xff\xff" string sent to the
RFB server leads to 4294967295 msg.cct.length value that in turn is
interpreted as -1 by rfbReadExact() and thus uninitialized str buffer
with potentially sensitive data is passed to subsequent functions.

This patch fixes it by checking for a maximal value that still can be
processed correctly. It also corrects accepting length value of zero
(malloc(0) is interpreted on differnet systems differently).

Whether a client can make the server allocate up to 2 GB and cause
a denial of service on memory-tight systems is kept without answer.
A possible solution would be adding an arbitrary memory limit that is
deemed safe.

CVE-2018-7225
<LibVNC#218>
@solardiz

This comment has been minimized.

Copy link
Author

commented Feb 26, 2018

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.

ppisar added a commit to ppisar/libvncserver that referenced this issue Mar 6, 2018

Validate client cut text length
Client-provided unsigned 32-bit cut text length is passed to various
functions that expects argument of a different type.

E.g. "RFB 003.003\n\001\006\0\0\0\xff\xff\xff\xff" string sent to the
RFB server leads to 4294967295 msg.cct.length value that in turn is
interpreted as -1 by rfbReadExact() and thus uninitialized str buffer
with potentially sensitive data is passed to subsequent functions.

This patch fixes it by checking for a maximal value that still can be
processed correctly. It also corrects accepting length value of zero
(malloc(0) is interpreted on differnet systems differently).

Whether a client can make the server allocate up to 2 GB and cause
a denial of service on memory-tight systems is kept without answer.
A possible solution would be adding an arbitrary memory limit that is
deemed safe.

CVE-2018-7225
<LibVNC#218>

ppisar added a commit to ppisar/libvncserver that referenced this issue Mar 8, 2018

Limit client cut text length to 1 MB
This patch constrains a client cut text length to 1 MB. Otherwise
a client could make server allocate 2 GB of memory and that seems to
be to much to classify it as a denial of service.

The limit also prevents from an integer overflow followed by copying
an uninitilized memory when processing msg.cct.length value larger
than SIZE_MAX or INT_MAX - sz_rfbClientCutTextMsg.

This patch also corrects accepting length value of zero (malloc(0) is
interpreted on differnet systems differently).

CVE-2018-7225
<LibVNC#218>

@bk138 bk138 self-assigned this Mar 9, 2018

@bk138 bk138 added this to the Release 0.9.12 milestone Mar 9, 2018

@bk138

This comment has been minimized.

Copy link
Member

commented Mar 24, 2018

Closed by b0c7739

@bk138 bk138 closed this Mar 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.