commit 06ccdf016154fde8eccb5355613ba04c59127b2e Author: Nicolas Ruff Date: Mon Sep 1 14:36:26 2014 +0200 Fix multiple stack-based buffer overflows in file transfer feature diff --git a/libvncserver/rfbserver.c b/libvncserver/rfbserver.c index df7d74c..445331a 100644 --- a/libvncserver/rfbserver.c +++ b/libvncserver/rfbserver.c @@ -1241,21 +1241,35 @@ typedef struct { #define RFB_FILE_ATTRIBUTE_TEMPORARY 0x100 #define RFB_FILE_ATTRIBUTE_COMPRESSED 0x800 -rfbBool rfbFilenameTranslate2UNIX(rfbClientPtr cl, char *path, char *unixPath) +rfbBool rfbFilenameTranslate2UNIX(rfbClientPtr cl, /* in */ char *path, /* out */ char *unixPath, size_t unixPathMaxLen ) { int x; char *home=NULL; FILEXFER_ALLOWED_OR_CLOSE_AND_RETURN("", cl, FALSE); + /* + * Do not use strncpy() - truncating the file name would probably have undesirable side effects + * Instead check if destination buffer is big enough + */ + + if (strlen(path) >= unixPathMaxLen) + return FALSE; + /* C: */ if (path[0]=='C' && path[1]==':') + { strcpy(unixPath, &path[2]); + } else { home = getenv("HOME"); if (home!=NULL) { + /* Re-check buffer size */ + if ((strlen(path) + strlen(home) + 1) >= unixPathMaxLen) + return FALSE; + strcpy(unixPath, home); strcat(unixPath,"/"); strcat(unixPath, path); @@ -1293,7 +1307,8 @@ rfbBool rfbSendDirContent(rfbClientPtr cl, int length, char *buffer) FILEXFER_ALLOWED_OR_CLOSE_AND_RETURN("", cl, FALSE); /* Client thinks we are Winblows */ - rfbFilenameTranslate2UNIX(cl, buffer, path); + if (!rfbFilenameTranslate2UNIX(cl, buffer, path, sizeof(path))) + return FALSE; if (DB) rfbLog("rfbProcessFileTransfer() rfbDirContentRequest: rfbRDirContent: \"%s\"->\"%s\"\n",buffer, path); @@ -1570,7 +1585,11 @@ rfbBool rfbProcessFileTransfer(rfbClientPtr cl, uint8_t contentType, uint8_t con /* add some space to the end of the buffer as we will be adding a timespec to it */ if ((buffer = rfbProcessFileTransferReadBuffer(cl, length))==NULL) return FALSE; /* The client requests a File */ - rfbFilenameTranslate2UNIX(cl, buffer, filename1); + if (!rfbFilenameTranslate2UNIX(cl, buffer, filename1, sizeof(filename1))) + { + if (buffer!=NULL) free(buffer); + return FALSE; + } cl->fileTransfer.fd=open(filename1, O_RDONLY, 0744); /* @@ -1685,7 +1704,11 @@ rfbBool rfbProcessFileTransfer(rfbClientPtr cl, uint8_t contentType, uint8_t con } sizeHtmp = Swap32IfLE(sizeHtmp); - rfbFilenameTranslate2UNIX(cl, buffer, filename1); + if (!rfbFilenameTranslate2UNIX(cl, buffer, filename1, sizeof(filename1))) + { + if (buffer!=NULL) free(buffer); + return FALSE; + } /* If the file exists... We can send a rfbFileChecksums back to the client before we send an rfbFileAcceptHeader */ /* TODO: Delta Transfer */ @@ -1814,7 +1837,12 @@ rfbBool rfbProcessFileTransfer(rfbClientPtr cl, uint8_t contentType, uint8_t con if ((buffer = rfbProcessFileTransferReadBuffer(cl, length))==NULL) return FALSE; switch (contentParam) { case rfbCDirCreate: /* Client requests the creation of a directory */ - rfbFilenameTranslate2UNIX(cl, buffer, filename1); + if (!rfbFilenameTranslate2UNIX(cl, buffer, filename1, sizeof(filename1))) + { + if (buffer!=NULL) free(buffer); + return FALSE; + } + retval = mkdir(filename1, 0755); if (DB) rfbLog("rfbProcessFileTransfer() rfbCommand: rfbCDirCreate(\"%s\"->\"%s\") %s\n", buffer, filename1, (retval==-1?"Failed":"Success")); /* @@ -1823,7 +1851,12 @@ rfbBool rfbProcessFileTransfer(rfbClientPtr cl, uint8_t contentType, uint8_t con if (buffer!=NULL) free(buffer); return retval; case rfbCFileDelete: /* Client requests the deletion of a file */ - rfbFilenameTranslate2UNIX(cl, buffer, filename1); + if (!rfbFilenameTranslate2UNIX(cl, buffer, filename1, sizeof(filename1))) + { + if (buffer!=NULL) free(buffer); + return FALSE; + } + if (stat(filename1,&statbuf)==0) { if (S_ISDIR(statbuf.st_mode)) @@ -1841,8 +1874,18 @@ rfbBool rfbProcessFileTransfer(rfbClientPtr cl, uint8_t contentType, uint8_t con { /* Split into 2 filenames ('*' is a seperator) */ *p = '\0'; - rfbFilenameTranslate2UNIX(cl, buffer, filename1); - rfbFilenameTranslate2UNIX(cl, p+1, filename2); + if (!rfbFilenameTranslate2UNIX(cl, buffer, filename1, sizeof(filename1))) + { + if (buffer!=NULL) free(buffer); + return FALSE; + } + + if (!rfbFilenameTranslate2UNIX(cl, p+1, filename2, sizeof(filename2))) + { + if (buffer!=NULL) free(buffer); + return FALSE; + } + retval = rename(filename1,filename2); if (DB) rfbLog("rfbProcessFileTransfer() rfbCommand: rfbCFileRename(\"%s\"->\"%s\" -->> \"%s\"->\"%s\") %s\n", buffer, filename1, p+1, filename2, (retval==-1?"Failed":"Success")); /* commit f528072216dec01cee7ca35d94e171a3b909e677 Author: Nicolas Ruff Date: Mon Sep 1 14:51:07 2014 +0200 Fix stack-based buffer overflow in rfbFileTransferOffer message, FileTime processing diff --git a/libvncserver/rfbserver.c b/libvncserver/rfbserver.c index 445331a..23532b0 100644 --- a/libvncserver/rfbserver.c +++ b/libvncserver/rfbserver.c @@ -1683,16 +1683,17 @@ rfbBool rfbProcessFileTransfer(rfbClientPtr cl, uint8_t contentType, uint8_t con */ if ((buffer = rfbProcessFileTransferReadBuffer(cl, length))==NULL) return FALSE; - /* Parse the FileTime */ + /* Parse the FileTime + * TODO: FileTime is actually never used afterwards + */ p = strrchr(buffer, ','); if (p!=NULL) { *p = '\0'; - strcpy(szFileTime, p+1); + strncpy(szFileTime, p+1, sizeof(szFileTime)); + szFileTime[sizeof(szFileTime)-1] = '\x00'; /* ensure NULL terminating byte is present, even if copy overflowed */ } else szFileTime[0]=0; - - /* Need to read in sizeHtmp */ if ((n = rfbReadExact(cl, (char *)&sizeHtmp, 4)) <= 0) { if (n != 0)