Add bounds check for negative ctrlsrc->z.

third_party/bspatch/mbspatch.cc

 /*-
  * Copyright 2003,2004 Colin Percival
  * All rights reserved
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted providing that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 16 matching lines @@
  * 2005-04-26 - Define the header as a C structure, add a CRC32 checksum to
  *              the header, and make all the types 32-bit.
  *                --Benjamin Smedberg <benjamin@smedbergs.us>
  * 2007-11-14 - Added CalculateCrc() and ApplyBinaryPatch() methods.
  *                --Rahul Kuchhal
  */
 
 #include "mbspatch.h"
 
 #include <sys/stat.h>
+#include <sys/types.h>
 #include <stdlib.h>
 #include <stdio.h>
 #include <fcntl.h>
 #include <string.h>
 #include <limits.h>
 
 #ifdef _WIN32
 # include <io.h>
 # include <winsock2.h>
 #else
@@ skipping 18 matching lines @@
 
   header->slen      = ntohl(header->slen);
   header->scrc32    = ntohl(header->scrc32);
   header->dlen      = ntohl(header->dlen);
   header->cblen     = ntohl(header->cblen);
   header->difflen   = ntohl(header->difflen);
   header->extralen  = ntohl(header->extralen);
 
   struct stat hs;
   s = fstat(fd, &hs);
-  if (s)
+  if (s != 0)
     return READ_ERROR;
 
   if (memcmp(header->tag, "MBDIFF10", 8) != 0)
     return UNEXPECTED_ERROR;
 
-  if (sizeof(MBSPatchHeader) +
-      header->cblen +
-      header->difflen +
-      header->extralen != int(hs.st_size))
+  if (hs.st_size > INT_MAX)
+    return UNEXPECTED_ERROR;
+
+  size_t size = static_cast<size_t>(hs.st_size);

[kcwu, 2016/07/27 04:14:36]

size -= sizeof(MBSPatchHeader);

[rickyz (no longer on Chrome), 2016/07/27 04:24:14]

Done.

+  if (slen < header->cblen || header->cblen % sizeof(MBSPatchTriple) != 0)

[kcwu, 2016/07/27 04:14:36]

s/slen/size/

[rickyz (no longer on Chrome), 2016/07/27 04:24:14]

Done.

+    return UNEXPECTED_ERROR;
+  size -= header->cblen;
+
+  if (size < header->difflen)
+    return UNEXPECTED_ERROR;
+  size -= header->difflen;
+
+  if (size < header->extralen)
+    return UNEXPECTED_ERROR;
+  size -= header->extralen;
+
+  if (size != 0)
     return UNEXPECTED_ERROR;
 
   return OK;
 }
 
 int
 MBS_ApplyPatch(const MBSPatchHeader *header, int patchfd,
                unsigned char *fbuffer, int filefd)
 {
+  unsigned char *fbufstart = fbuffer;
   unsigned char *fbufend = fbuffer + header->slen;
 
   unsigned char *buf = (unsigned char*) malloc(header->cblen +
                                                header->difflen +
                                                header->extralen);
   if (!buf)
     return MEM_ERROR;
 
   int rv = OK;
 
   int r = header->cblen + header->difflen + header->extralen;
   unsigned char *wb = buf;
   while (r) {
     int c = read(patchfd, wb, (r > SSIZE_MAX) ? SSIZE_MAX : r);
     if (c < 0) {
       rv = READ_ERROR;
       goto end;
     }
 
     r -= c;
+    wb += c

[kcwu, 2016/07/27 04:14:36]

good catch. but missed semicolon;

[rickyz (no longer on Chrome), 2016/07/27 04:24:14]

Oops, apologies for all of the build errors, been a while since I've made a
chromium change, so don't have a build setup fully ready to go yet.

 
     if (c == 0 && r) {
       rv = UNEXPECTED_ERROR;
       goto end;
     }
   }
 
   {
     MBSPatchTriple *ctrlsrc = (MBSPatchTriple*) buf;
+    if (header->cblen % sizeof(MBSPatchTriple) != 0) {

[kcwu, 2016/07/27 04:14:36]

This is redundant? You have checked it in MBS_ReadHeader.

[rickyz (no longer on Chrome), 2016/07/27 04:24:14]

I was debating between doing validation earlier vs. having the check next to the
code that depends on it. I ended up going with the latter.

+      rv = UNEXPECTED_ERROR;
+      goto end;
+    }
+
     unsigned char *diffsrc = buf + header->cblen;
     unsigned char *extrasrc = diffsrc + header->difflen;
 
     MBSPatchTriple *ctrlend = (MBSPatchTriple*) diffsrc;
     unsigned char *diffend = extrasrc;
     unsigned char *extraend = extrasrc + header->extralen;
 
-    do {
+    while (ctrlsrc < ctrlend) {
       ctrlsrc->x = ntohl(ctrlsrc->x);
       ctrlsrc->y = ntohl(ctrlsrc->y);
       ctrlsrc->z = ntohl(ctrlsrc->z);
 
 #ifdef DEBUG_bsmedberg
       printf("Applying block:\n"
              " x: %u\n"
              " y: %u\n"
              " z: %i\n",
              ctrlsrc->x,
              ctrlsrc->y,
              ctrlsrc->z);
 #endif
 
       /* Add x bytes from oldfile to x bytes from the diff block */
 
-      if (fbuffer + ctrlsrc->x > fbufend ||
-          diffsrc + ctrlsrc->x > diffend) {
+      if (ctrlsrc->x > fbufend - fbuffer ||
+          ctrlsrc->x > diffend - diffsrc) {
         rv = UNEXPECTED_ERROR;
         goto end;
       }
       for (unsigned int i = 0; i < ctrlsrc->x; ++i) {
         diffsrc[i] += fbuffer[i];
       }
       if ((int) write(filefd, diffsrc, ctrlsrc->x) != ctrlsrc->x) {
         rv = WRITE_ERROR;
         goto end;
       }
       fbuffer += ctrlsrc->x;
       diffsrc += ctrlsrc->x;
 
       /* Copy y bytes from the extra block */
 
-      if (extrasrc + ctrlsrc->y > extraend) {
+      if (ctrlsrc->y > extraend - extrasrc) {
         rv = UNEXPECTED_ERROR;
         goto end;
       }
       if ((int) write(filefd, extrasrc, ctrlsrc->y) != ctrlsrc->y) {
         rv = WRITE_ERROR;
         goto end;
       }
       extrasrc += ctrlsrc->y;
 
       /* "seek" forwards in oldfile by z bytes */
 
-      if (fbuffer + ctrlsrc->z > fbufend) {
+      if (ctrlsrc->z < fbufstart - fbuffer ||
+          ctrlsrc->z > fbufend - fbuffer) {
         rv = UNEXPECTED_ERROR;
         goto end;
       }
       fbuffer += ctrlsrc->z;
 
       /* and on to the next control block */
 
       ++ctrlsrc;
-    } while (ctrlsrc < ctrlend);
+    }
   }
 
 end:
   free(buf);
   return rv;
 }
 
 int CalculateCrc(const unsigned char *buf, int size) {
   CrcGenerateTable();
   unsigned int crc = 0xffffffffL;
@@ skipping 69 matching lines @@
 
     ret = MBS_ApplyPatch(&header, pfd, buf, nfd);
   } while (0);
 
   free(buf);
   close(pfd);
   if (ofd >= 0) close(ofd);
   if (nfd >= 0) close(nfd);
   return ret;
 }