Add sanity checks for PixelFormat shift values

Otherwise we might be tricked in to reading and writing things at
incorrect offsets for pixels which ultimately could result in an
attacker writing things to the stack or heap and executing things
they shouldn't.

This only affects the server as the client never uses the pixel
format suggested by th server.

Issue found by Pavel Cheremushkin from Kaspersky Lab.

diff --git a/common/rfb/PixelFormat.cxx b/common/rfb/PixelFormat.cxx
index 2d8142d157a774bb73c132025fd596246b9bdf7e..789c43edaa4e5496f59c28941e9e1cdc02aa2f5c 100644 (file)
--- a/common/rfb/PixelFormat.cxx
+++ b/common/rfb/PixelFormat.cxx
@@ -682,6 +682,13 @@ bool PixelFormat::isSane(void)
   if (totalBits > depth)
     return false;
 
+  if ((bits(redMax) + redShift) > bpp)
+    return false;
+  if ((bits(greenMax) + greenShift) > bpp)
+    return false;
+  if ((bits(blueMax) + blueShift) > bpp)
+    return false;
+
   if (((redMax << redShift) & (greenMax << greenShift)) != 0)
     return false;
   if (((redMax << redShift) & (blueMax << blueShift)) != 0)

diff --git a/tests/unit/pixelformat.cxx b/tests/unit/pixelformat.cxx
index 7b6087f7bc66bb96ed0593ee8786744db6fbabe1..46fecfb49fd1375922aeb9f694561b48efa4bf7a 100644 (file)
--- a/tests/unit/pixelformat.cxx
+++ b/tests/unit/pixelformat.cxx
@@ -108,6 +108,12 @@ int main(int argc, char** argv)
 
     doTest(true, 32, 16, false, true, 255, 255, 255, 0, 8, 16);
 
+    /* Invalid shift values */
+
+    doTest(true, 32, 24, false, true, 255, 255, 255, 25, 8, 16);
+    doTest(true, 32, 24, false, true, 255, 255, 255, 0, 25, 16);
+    doTest(true, 32, 24, false, true, 255, 255, 255, 0, 8, 25);
+
     /* Overlapping channels */
 
     doTest(true, 32, 24, false, true, 255, 255, 255, 0, 7, 16);