Merge pull request #41 from newsoft/master

Fixing 2 security issues

2 files changed, 52 insertions, 9 deletions

diff --git a/libvncserver/rfbserver.c b/libvncserver/rfbserver.c
index f64ea4e..21f9eff 100644
--- a/libvncserver/rfbserver.c
+++ b/libvncserver/rfbserver.c
@@ -1259,13 +1259,20 @@ 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]);
@@ -1274,6 +1281,10 @@ rfbBool rfbFilenameTranslate2UNIX(rfbClientPtr cl, char *path, char *unixPath)
       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);
@@ -1318,7 +1329,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);
 
@@ -1659,7 +1671,8 @@ 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)))
+            goto fail;
         cl->fileTransfer.fd=open(filename1, O_RDONLY, 0744);
 
         /*
@@ -1774,7 +1787,8 @@ rfbBool rfbProcessFileTransfer(rfbClientPtr cl, uint8_t contentType, uint8_t con
         }
         sizeHtmp = Swap32IfLE(sizeHtmp);
         
-        rfbFilenameTranslate2UNIX(cl, buffer, filename1);
+        if (!rfbFilenameTranslate2UNIX(cl, buffer, filename1, sizeof(filename1)))
+            goto fail;
 
         /* If the file exists... We can send a rfbFileChecksums back to the client before we send an rfbFileAcceptHeader */
         /* TODO: Delta Transfer */
@@ -1903,7 +1917,8 @@ 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)))
+                goto fail;
             retval = mkdir(filename1, 0755);
             if (DB) rfbLog("rfbProcessFileTransfer() rfbCommand: rfbCDirCreate(\"%s\"->\"%s\") %s\n", buffer, filename1, (retval==-1?"Failed":"Success"));
             /*
@@ -1912,7 +1927,8 @@ 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)))
+                goto fail;
             if (stat(filename1,&statbuf)==0)
             {
                 if (S_ISDIR(statbuf.st_mode))
@@ -1930,8 +1946,10 @@ 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)))
+                    goto fail;
+                if (!rfbFilenameTranslate2UNIX(cl, p+1,    filename2, sizeof(filename2)))
+                    goto fail;
                 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"));
                 /*
@@ -1951,6 +1969,10 @@ rfbBool rfbProcessFileTransfer(rfbClientPtr cl, uint8_t contentType, uint8_t con
     /* NOTE: don't forget to free(buffer) if you return early! */
     if (buffer!=NULL) free(buffer);
     return TRUE;
+
+fail:
+    if (buffer!=NULL) free(buffer);
+    return FALSE;
 }
 
 /*
diff --git a/libvncserver/scale.c b/libvncserver/scale.c
index 6f91391..d5b3f8b 100644
--- a/libvncserver/scale.c
+++ b/libvncserver/scale.c
@@ -66,6 +66,12 @@
         (double) ((int) (x)) : (double) ((int) (x) + 1) )
 #define FLOOR(x) ( (double) ((int) (x)) )
 
+static inline int pad4(int value)
+{
+    int remainder = value & 3;
+    if (!remainder) return value;
+    return value + 4 - remainder;
+}
 
 int ScaleX(rfbScreenInfoPtr from, rfbScreenInfoPtr to, int x)
 {
@@ -281,14 +287,29 @@ rfbScreenInfoPtr rfbScaledScreenAllocate(rfbClientPtr cl, int width, int height)
     ptr = malloc(sizeof(rfbScreenInfo));
     if (ptr!=NULL)
     {
+        int allocSize;
+
         /* copy *everything* (we don't use most of it, but just in case) */
         memcpy(ptr, cl->screen, sizeof(rfbScreenInfo));
+
+        /* SECURITY: make sure that no integer overflow will occur afterwards.
+         * Note: this is defensive coding, as the check should have already been
+         * performed during initial, non-scaled screen setup.
+         */
+        allocSize = pad4(width * (ptr->bitsPerPixel/8)); /* per protocol, width<2**16 and bpp<256 */
+        if (height == 0 || allocSize >= SIZE_MAX / height)
+        {
+          free(ptr);
+          return NULL; /* malloc() will allocate an incorrect buffer size - early abort */
+        }
+
+        /* Resume copy everything */
         ptr->width = width;
         ptr->height = height;
         ptr->paddedWidthInBytes = (ptr->bitsPerPixel/8)*ptr->width;
 
         /* Need to by multiples of 4 for Sparc systems */
-        ptr->paddedWidthInBytes += (ptr->paddedWidthInBytes % 4);
+        ptr->paddedWidthInBytes = pad4(ptr->paddedWidthInBytes);
 
         /* Reset the reference count to 0! */
         ptr->scaledScreenRefCount = 0;