Prevent invalid PixelBuffer accesses

There has been multiple attempts at tricking decoders to exceed
the boundaries of the active pixel buffer. Add extra checks to
prevent such invalid access.

1 files changed, 126 insertions, 73 deletions

diff --git a/common/rfb/PixelBuffer.cxx b/common/rfb/PixelBuffer.cxx
index 7f3df6cb..f444aa02 100644
--- a/common/rfb/PixelBuffer.cxx
+++ b/common/rfb/PixelBuffer.cxx
@@ -42,17 +42,32 @@ PixelBuffer::~PixelBuffer() {}
 
 
 void
-PixelBuffer::getImage(void* imageBuf, const Rect& r, int outStride) const {
+PixelBuffer::getImage(void* imageBuf, const Rect& r, int outStride) const
+{
   int inStride;
-  const U8* data = getBuffer(r, &inStride);
-  // We assume that the specified rectangle is pre-clipped to the buffer
-  int bytesPerPixel = format.bpp/8;
-  int inBytesPerRow = inStride * bytesPerPixel;
-  if (!outStride) outStride = r.width();
-  int outBytesPerRow = outStride * bytesPerPixel;
-  int bytesPerMemCpy = r.width() * bytesPerPixel;
-  U8* imageBufPos = (U8*)imageBuf;
-  const U8* end = data + (inBytesPerRow * r.height());
+  const U8* data;
+  int bytesPerPixel, inBytesPerRow, outBytesPerRow, bytesPerMemCpy;
+  U8* imageBufPos;
+  const U8* end;
+
+  if (!r.enclosed_by(getRect()))
+    throw rfb::Exception("Source rect %dx%d at %d,%d exceeds framebuffer %dx%d",
+                         r.width(), r.height(),
+                         r.tl.x, r.tl.y, width_, height_);
+
+  data = getBuffer(r, &inStride);
+
+  bytesPerPixel = format.bpp/8;
+  inBytesPerRow = inStride * bytesPerPixel;
+
+  if (!outStride)
+    outStride = r.width();
+  outBytesPerRow = outStride * bytesPerPixel;
+  bytesPerMemCpy = r.width() * bytesPerPixel;
+
+  imageBufPos = (U8*)imageBuf;
+  end = data + (inBytesPerRow * r.height());
+
   while (data < end) {
     memcpy(imageBufPos, data, bytesPerMemCpy);
     imageBufPos += outBytesPerRow;
@@ -71,6 +86,11 @@ void PixelBuffer::getImage(const PixelFormat& pf, void* imageBuf,
     return;
   }
 
+  if (!r.enclosed_by(getRect()))
+    throw rfb::Exception("Source rect %dx%d at %d,%d exceeds framebuffer %dx%d",
+                         r.width(), r.height(),
+                         r.tl.x, r.tl.y, width_, height_);
+
   if (stride == 0)
     stride = r.width();
 
@@ -101,26 +121,19 @@ void ModifiablePixelBuffer::fillRect(const Rect& r, const void* pix)
   int stride;
   U8 *buf;
   int w, h, b;
-  Rect drect;
-
-  drect = r;
-  if (!drect.enclosed_by(getRect())) {
-    vlog.error("Destination rect %dx%d at %d,%d exceeds framebuffer %dx%d",
-               drect.width(), drect.height(), drect.tl.x, drect.tl.y, width_, height_);
-    drect = drect.intersect(getRect());
-  }
 
-  if (drect.is_empty())
-    return;
+  if (!r.enclosed_by(getRect()))
+    throw rfb::Exception("Destination rect %dx%d at %d,%d exceeds framebuffer %dx%d",
+                         r.width(), r.height(), r.tl.x, r.tl.y, width_, height_);
 
-  w = drect.width();
-  h = drect.height();
+  w = r.width();
+  h = r.height();
   b = format.bpp/8;
 
   if (h == 0)
     return;
 
-  buf = getBufferRW(drect, &stride);
+  buf = getBufferRW(r, &stride);
 
   if (b == 1) {
     while (h--) {
@@ -147,44 +160,70 @@ void ModifiablePixelBuffer::fillRect(const Rect& r, const void* pix)
     }
   }
 
-  commitBufferRW(drect);
+  commitBufferRW(r);
 }
 
 void ModifiablePixelBuffer::imageRect(const Rect& r,
                                       const void* pixels, int srcStride)
 {
-  int bytesPerPixel = getPF().bpp/8;
+  U8* dest;
   int destStride;
-  U8* dest = getBufferRW(r, &destStride);
-  int bytesPerDestRow = bytesPerPixel * destStride;
-  if (!srcStride) srcStride = r.width();
-  int bytesPerSrcRow = bytesPerPixel * srcStride;
-  int bytesPerFill = bytesPerPixel * r.width();
-  const U8* src = (const U8*)pixels;
-  U8* end = dest + (bytesPerDestRow * r.height());
+  int bytesPerPixel, bytesPerDestRow, bytesPerSrcRow, bytesPerFill;
+  const U8* src;
+  U8* end;
+
+  if (!r.enclosed_by(getRect()))
+    throw rfb::Exception("Destination rect %dx%d at %d,%d exceeds framebuffer %dx%d",
+                         r.width(), r.height(),
+                         r.tl.x, r.tl.y, width_, height_);
+
+  bytesPerPixel = getPF().bpp/8;
+
+  dest = getBufferRW(r, &destStride);
+
+  bytesPerDestRow = bytesPerPixel * destStride;
+
+  if (!srcStride)
+    srcStride = r.width();
+  bytesPerSrcRow = bytesPerPixel * srcStride;
+  bytesPerFill = bytesPerPixel * r.width();
+
+  src = (const U8*)pixels;
+  end = dest + (bytesPerDestRow * r.height());
+
   while (dest < end) {
     memcpy(dest, src, bytesPerFill);
     dest += bytesPerDestRow;
     src += bytesPerSrcRow;
   }
+
   commitBufferRW(r);
 }
 
 void ModifiablePixelBuffer::maskRect(const Rect& r,
                                      const void* pixels, const void* mask_)
 {
-  Rect cr = getRect().intersect(r);
-  if (cr.is_empty()) return;
   int stride;
-  U8* data = getBufferRW(cr, &stride);
-  U8* mask = (U8*) mask_;
-  int w = cr.width();
-  int h = cr.height();
-  int bpp = getPF().bpp;
-  int pixelStride = r.width();
-  int maskStride = (r.width() + 7) / 8;
-
-  Point offset = Point(cr.tl.x-r.tl.x, cr.tl.y-r.tl.y);
+  U8* data;
+  U8* mask;
+  int w, h, bpp, pixelStride, maskStride;
+  Point offset;
+
+  if (!r.enclosed_by(getRect()))
+    throw rfb::Exception("Destination rect %dx%d at %d,%d exceeds framebuffer %dx%d",
+                         r.width(), r.height(),
+                         r.tl.x, r.tl.y, width_, height_);
+
+  data = getBufferRW(r, &stride);
+  mask = (U8*) mask_;
+
+  w = r.width();
+  h = r.height();
+  bpp = getPF().bpp;
+  pixelStride = r.width();
+  maskStride = (r.width() + 7) / 8;
+
+  offset = Point(r.tl.x-r.tl.x, r.tl.y-r.tl.y);
   mask += offset.y * maskStride;
   for (int y = 0; y < h; y++) {
     int cy = offset.y + y;
@@ -209,23 +248,32 @@ void ModifiablePixelBuffer::maskRect(const Rect& r,
     mask += maskStride;
   }
 
-  commitBufferRW(cr);
+  commitBufferRW(r);
 }
 
 void ModifiablePixelBuffer::maskRect(const Rect& r,
                                      Pixel pixel, const void* mask_)
 {
-  Rect cr = getRect().intersect(r);
-  if (cr.is_empty()) return;
   int stride;
-  U8* data = getBufferRW(cr, &stride);
-  U8* mask = (U8*) mask_;
-  int w = cr.width();
-  int h = cr.height();
-  int bpp = getPF().bpp;
-  int maskStride = (r.width() + 7) / 8;
-
-  Point offset = Point(cr.tl.x-r.tl.x, cr.tl.y-r.tl.y);
+  U8* data;
+  U8* mask;
+  int w, h, bpp, maskStride;
+  Point offset;
+
+  if (!r.enclosed_by(getRect()))
+    throw rfb::Exception("Destination rect %dx%d at %d,%d exceeds framebuffer %dx%d",
+                         r.width(), r.height(),
+                         r.tl.x, r.tl.y, width_, height_);
+
+  data = getBufferRW(r, &stride);
+  mask = (U8*) mask_;
+
+  w = r.width();
+  h = r.height();
+  bpp = getPF().bpp;
+  maskStride = (r.width() + 7) / 8;
+
+  offset = Point(r.tl.x-r.tl.x, r.tl.y-r.tl.y);
   mask += offset.y * maskStride;
   for (int y = 0; y < h; y++) {
     for (int x = 0; x < w; x++) {
@@ -249,7 +297,7 @@ void ModifiablePixelBuffer::maskRect(const Rect& r,
     mask += maskStride;
   }
 
-  commitBufferRW(cr);
+  commitBufferRW(r);
 }
 
 void ModifiablePixelBuffer::copyRect(const Rect &rect,
@@ -262,26 +310,16 @@ void ModifiablePixelBuffer::copyRect(const Rect &rect,
   Rect drect, srect;
 
   drect = rect;
-  if (!drect.enclosed_by(getRect())) {
-    vlog.error("Destination rect %dx%d at %d,%d exceeds framebuffer %dx%d",
-               drect.width(), drect.height(), drect.tl.x, drect.tl.y, width_, height_);
-    drect = drect.intersect(getRect());
-  }
-
-  if (drect.is_empty())
-    return;
+  if (!drect.enclosed_by(getRect()))
+    throw rfb::Exception("Destination rect %dx%d at %d,%d exceeds framebuffer %dx%d",
+                         drect.width(), drect.height(),
+                         drect.tl.x, drect.tl.y, width_, height_);
 
   srect = drect.translate(move_by_delta.negate());
-  if (!srect.enclosed_by(getRect())) {
-    vlog.error("Source rect %dx%d at %d,%d exceeds framebuffer %dx%d",
-               srect.width(), srect.height(), srect.tl.x, srect.tl.y, width_, height_);
-    srect = srect.intersect(getRect());
-    // Need to readjust the destination now that the area has changed
-    drect = srect.translate(move_by_delta);
-  }
-
-  if (srect.is_empty())
-    return;
+  if (!srect.enclosed_by(getRect()))
+    throw rfb::Exception("Source rect %dx%d at %d,%d exceeds framebuffer %dx%d",
+                         srect.width(), srect.height(),
+                         srect.tl.x, srect.tl.y, width_, height_);
 
   srcData = getBuffer(srect, &srcStride);
   dstData = getBufferRW(drect, &dstStride);
@@ -331,6 +369,11 @@ void ModifiablePixelBuffer::imageRect(const PixelFormat& pf, const Rect &dest,
   rdr::U8* dstBuffer;
   int dstStride;
 
+  if (!dest.enclosed_by(getRect()))
+    throw rfb::Exception("Destination rect %dx%d at %d,%d exceeds framebuffer %dx%d",
+                         dest.width(), dest.height(),
+                         dest.tl.x, dest.tl.y, width_, height_);
+
   if (stride == 0)
     stride = dest.width();
 
@@ -355,6 +398,11 @@ FullFramePixelBuffer::~FullFramePixelBuffer() {}
 
 rdr::U8* FullFramePixelBuffer::getBufferRW(const Rect& r, int* stride_)
 {
+  if (!r.enclosed_by(getRect()))
+    throw rfb::Exception("Pixel buffer request %dx%d at %d,%d exceeds framebuffer %dx%d",
+                         r.width(), r.height(),
+                         r.tl.x, r.tl.y, width_, height_);
+
   *stride_ = stride;
   return &data[(r.tl.x + (r.tl.y * stride)) * format.bpp/8];
 }
@@ -365,6 +413,11 @@ void FullFramePixelBuffer::commitBufferRW(const Rect& r)
 
 const rdr::U8* FullFramePixelBuffer::getBuffer(const Rect& r, int* stride_) const
 {
+  if (!r.enclosed_by(getRect()))
+    throw rfb::Exception("Pixel buffer request %dx%d at %d,%d exceeds framebuffer %dx%d",
+                         r.width(), r.height(),
+                         r.tl.x, r.tl.y, width_, height_);
+
   *stride_ = stride;
   return &data[(r.tl.x + (r.tl.y * stride)) * format.bpp/8];
 }