#28 tightvnc-filetransfer 64 bit buffer overflow

[Karl J. Runge]

I received this from Peter Arrenbrecht via email:

-----------------------------------------------------------
Hi Karl

Seems there is a buffer overflow in x11vnc's tightvnc-filetransfer
code when compiling to 64 bits:

FileTransferMsg
CreateFileDownloadZeroSizeDataMsg(unsigned long mTime)
{
FileTransferMsg fileDownloadZeroSizeDataMsg;
int length = sz_rfbFileDownloadDataMsg + sizeof(unsigned long); //
<-- used to be sizeof(int) here!
rfbFileDownloadDataMsg *pFDD = NULL;
char *pFollow = NULL;

char *pData = (char*) calloc(length, sizeof(char));
memset(&fileDownloadZeroSizeDataMsg, 0, sizeof(FileTransferMsg));
if(pData == NULL) {
rfbLog("File [%s]: Method [%s]: pData is NULL\n",
__FILE__, __FUNCTION__);
return fileDownloadZeroSizeDataMsg;
}

pFDD = (rfbFileDownloadDataMsg *) pData;
pFollow = &pData[sz_rfbFileDownloadDataMsg];

pFDD->type = rfbFileDownloadData;
pFDD->compressLevel = 0;
pFDD->compressedSize = Swap16IfLE(0);
pFDD->realSize = Swap16IfLE(0);

memcpy(pFollow, &mTime, sizeof(unsigned long)); // <-- but used here

fileDownloadZeroSizeDataMsg.data = pData;
fileDownloadZeroSizeDataMsg.length = length;

return fileDownloadZeroSizeDataMsg;

}

Detected by GCC build with full warnings.

Cheers,
-parren

[Johannes Schindelin]

I cannot say that I understand why mTime is copied to the end of the structure, but I think it is a safe bet that the allocation should use sizeof(unsigned long) as suggested. I pushed this as branch 'fix-filetransfer'; please merge if you are happy with it.

[Karl J. Runge]

Thanks for looking into this. I pasted it in just so as to not lose it.

My concern is that this appears to be data that is to be sent over the wire: i.e. the tightvnc filetransfer protocol. If so, I don't think the server can send different sized packets depending on whether it is a 32 or 64-bit machine. I haven't chased thru the code path this is used from or look into the tightvnc filetransfer protocol (e.g. tightvnc's implementation.)

[Michael Gilbert]

I just ran into this myself on Gentoo Linux.

I took a look through the tightvnc source, and rfbproto.h has this struct:

typedef struct _rfbFileDownloadDataMsg {
CARD8 type;
CARD8 compressLevel;
CARD16 realSize;
CARD16 compressedSize;
/* Followed by File[copressedSize],
but if (realSize = compressedSize = 0) followed by CARD32 modTime */
} rfbFileDownloadDataMsg;

#define sz_rfbFileDownloadDataMsg 6

So, it would seem that we need a 32 bit integer. I think a simple cast to a uint32_t should do the trick.

Here's a patch for that:

http://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo-x86/net-libs/libvncserver/files/libvncserver-memcpy.patch