Security: VNConsole.c: vcSetXCutTextProc() integer overflow and unchecked malloc()

[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.

As I reported in LibVNC/libvncserver#218, LibVNCServer's setXCutText() callback API is poorly defined (with int instead of size_t) and poorly used by LibVNCServer (without sanitization of the length, and passing in the client-specified full length even if fewer bytes were actually read from the client). I also mentioned callback implementation side issues, as illustrated by prl-vzvncserver's callback, which was based off vncterm's (hence the report in here) and which has since been fixed in prl-vzvncserver.

vncterm's implementation of the callback was and still is:

void vcSetXCutTextProc(char* str,int len, struct _rfbClientRec* cl)
{
  vncConsolePtr c=(vncConsolePtr)cl->screen->screenData;

  if(c->selection) free(c->selection);
  c->selection=(char*)malloc(len+1);
  memcpy(c->selection,str,len);
  c->selection[len]=0;
}

Besides the conversion to signed int during the call, there's also len+1 in the implementation, which may cause an integer overflow resulting in e.g. malloc(0) (which succeeds) followed by memcpy(..., ..., -1) (which writes beyond the allocated memory). And there's no check for malloc() possibly returning NULL.

I first found the issue during Openwall's security audit of the Virtuozzo 7 product, which reuses this code in its prl-vzvncserver component. A corresponding Virtuozzo 7 fix is:

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

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 ---

[solardiz]

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

[kbabioch]

This has been assigned: Use CVE-2018-7226.