SECURITY: malloc((uint64_t)length + 1) is unsafe, especially on 32-bit systems

[solardiz]

The fixes for #247 are incomplete, as I explained in:

https://www.openwall.com/lists/oss-security/2018/12/10/8

"Upstream's fix appears to be to add casts to (uint64_t) before adding 1 in those many malloc() calls. On platforms with larger than 32-bit size_t, this should be sufficient against integer overflows since the sizes are read from 32-bit protocol fields, but it isn't sufficient to prevent maliciously large memory allocation on the client by a rogue server. On a platform with 32-bit size_t, this isn't even sufficient to prevent the integer overflows."

Edit: I've just realized the fixes were the issue reporter's, and were merely accepted upstream. But that doesn't change anything with respect to the project needing even further fixes.

[bk138]

Thanks for reporting. What fix do you propose?

[bk138]

One more question @solardiz: How did you get to know that a uint_64t, when availabale, is actually 32 bits on 32-bit systems?

[solardiz]

Considering that (part of) the fix should be sanity-limiting the allocation size (since it's not great to let the remote system cause us to allocate almost 4 GiB), we can do just that and with a sane limit (applied prior to any typecasts or math) it'll also prevent 32-bit integer overflows and thus be a complete fix. I don't currently have time to propose a specific patch. Maybe @PaulCher will, as it's his incomplete fixes that we'll be fixing now?

I sure hope uint64_t is indeed 64-bit on any system. I didn't imply otherwise. The problem is that malloc is defined to accept a size_t, and that is often 32-bit. So we may have a 64-bit value of 2^32 that becomes a 32-bit value of 0 as it's passed to malloc.

[PaulCher]

My bad, I completely forgot about 32-bit platforms... Actually before proposing the patch I took a glance at how previous integer overflow issues were fixed in libvnc, so there must be even more.

Well, I looked up how they deal with this kind of problems in other VNC projects. In ultravnc every temporary allocations are limited to 100MB (function CheckBufferSize ClientConnection.cpp:6809). ~100 megabytes seems acceptable the frame buffer according to the modern screen sizes, do you agree? Testing is required to ensure that this memory limit will not break any functionality.

Probably for things like "reason of connection loss" more strict limitations can be enforced. As @solardiz stated in another issue Qemu is using limit of 1MB for cut text buffer.

[bk138]

@solardiz thanks for the explanation, makes sense!

[solardiz]

I think this issue isn't fully fixed yet. Please re-open.

There are more instances of the vulnerable pattern in the tree:

$ grep -rn 'malloc.*uint64_t' .
./libvncclient/rfbproto.c:1228:  client->desktopName = malloc((uint64_t)client->si.nameLength + 1);
./libvncclient/rfbproto.c:2226:    buffer = malloc((uint64_t)msg.sct.length+1);
./libvncserver/rfbserver.c:1468:        buffer=malloc((uint64_t)length+1);

Also, while 045a044 does avoid the integer overflow in MallocFrameBuffer (assuming the comment is correct, which I didn't verify), it allows for allocation of a potentially very large framebuffer, allowing a server to crash the client system. I think its check of allocSize >= SIZE_MAX should be changed to a check against some sane limit, e.g. something in the range of 100 MiB to 1 GiB.

The above list of remaining issues is probably non-exhaustive.

[ppisar]

Regarding the rfbProcessFileTransferReadBuffer(), what's the reasonable file size?

I can imagine people could want to transfer 4-GiB files (even larger but that's not supported by the protocol).

Even if we choose a smaller limit, a user can connect multiple times, ask for a large file transfer and that will exhaust the physical memory. We could implement some per-user limit, but some VNC authentication methods do not have a concept of user accounts. I think this is out of scope of the VNC server. This can be easily implemented on an operating system level by setting the process memory limits (ulimit -m -v). Then the malloc() fails and the server refuses the transfer.

Even if we choose some arbitrary limit, being it a compile-time option will make hard times for users willing to change the limit (they can have a system with smaller or larger memory). Shouldn't the file size limit be a run-time option?

[bk138]

Closing this now, futher sanity checking for plausible allocation sizes should be collected in #275

[mdeslaur]

The following CVEs were assigned for the incomplete fixes:

---

CVE-2018-20748:
LibVNC before 0.9.12 contains multiple heap out-of-bounds write vulnerabilities in
libvncclient/rfbproto.c. The fix for CVE-2018-20019 was incomplete.

c5ba3fe
e34bcbb
c2c4b81
a64c3b3

---

CVE-2018-20749:
LibVNC before 0.9.12 contains a heap out-of-bounds write vulnerability in
libvncserver/rfbserver.c. The fix for CVE-2018-15127 was incomplete.

15bb719

---

CVE-2018-20750:
LibVNC through 0.9.12 contains a heap out-of-bounds write vulnerability in
libvncserver/rfbserver.c. The fix for CVE-2018-15127 was incomplete.

09e8fc0