Hello community,
here is the log from the commit of package tigervnc for openSUSE:Factory
checked in at 2020-01-12 23:17:04
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Comparing /work/SRC/openSUSE:Factory/tigervnc (Old)
and /work/SRC/openSUSE:Factory/.tigervnc.new.6675 (New)
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Package is "tigervnc"
Sun Jan 12 23:17:04 2020 rev:62 rq:762160 version:1.10.0
Changes:
--------
--- /work/SRC/openSUSE:Factory/tigervnc/tigervnc.changes 2020-01-07
23:51:28.883973005 +0100
+++ /work/SRC/openSUSE:Factory/.tigervnc.new.6675/tigervnc.changes
2020-01-12 23:17:52.122671546 +0100
@@ -1,0 +2,28 @@
+Tue Jan 7 15:43:09 UTC 2020 - Stefan Dirsch <sndir...@suse.com>
+
+- TigerVNC security fix:
+ 0001-Make-ZlibInStream-more-robust-against-failures.patch
+ 0002-Encapsulate-PixelBuffer-internal-details.patch
+ 0003-Restrict-PixelBuffer-dimensions-to-safe-values.patch
+ 0004-Add-write-protection-to-OffsetPixelBuffer.patch
+ 0005-Handle-empty-Tight-gradient-rects.patch
+ 0006-Add-unit-test-for-PixelFormat-sanity-checks.patch
+ 0007-Fix-depth-sanity-test-in-PixelFormat.patch
+ 0008-Add-sanity-checks-for-PixelFormat-shift-values.patch
+ 0009-Remove-unused-FixedMemOutStream.patch
+ 0010-Use-size_t-for-lengths-in-stream-objects.patch
+ 0011-Be-defensive-about-overflows-in-stream-objects.patch
+ 0012-Add-unit-tests-for-PixelFormat.is888-detection.patch
+ 0013-Handle-pixel-formats-with-odd-shift-values.patch
+ * stack use-after-return due to incorrect usage of stack memory
+ in ZRLEDecoder (CVE-2019-15691, bsc#1159856)
+ * improper value checks in CopyRectDecode may lead to heap
+ buffer overflow (CVE-2019-15692, bsc#1160250)
+ * heap buffer overflow in TightDecoder::FilterGradient
+ (CVE-2019-15693, bsc#1159858)
+ * improper error handling in processing MemOutStream may lead
+ to heap buffer overflow (CVE-2019-15694, bsc#1160251
+ * stack buffer overflow, which could be triggered from
+ CMsgReader::readSetCurso (CVE-2019-15695, bsc#1159860)
+
+-------------------------------------------------------------------
New:
----
0001-Make-ZlibInStream-more-robust-against-failures.patch
0002-Encapsulate-PixelBuffer-internal-details.patch
0003-Restrict-PixelBuffer-dimensions-to-safe-values.patch
0004-Add-write-protection-to-OffsetPixelBuffer.patch
0005-Handle-empty-Tight-gradient-rects.patch
0006-Add-unit-test-for-PixelFormat-sanity-checks.patch
0007-Fix-depth-sanity-test-in-PixelFormat.patch
0008-Add-sanity-checks-for-PixelFormat-shift-values.patch
0009-Remove-unused-FixedMemOutStream.patch
0010-Use-size_t-for-lengths-in-stream-objects.patch
0011-Be-defensive-about-overflows-in-stream-objects.patch
0012-Add-unit-tests-for-PixelFormat.is888-detection.patch
0013-Handle-pixel-formats-with-odd-shift-values.patch
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Other differences:
------------------
++++++ tigervnc.spec ++++++
--- /var/tmp/diff_new_pack.PFiKfk/_old 2020-01-12 23:17:53.354672002 +0100
+++ /var/tmp/diff_new_pack.PFiKfk/_new 2020-01-12 23:17:53.354672002 +0100
@@ -1,7 +1,7 @@
#
# spec file for package tigervnc
#
-# Copyright (c) 2019 SUSE LLC
+# Copyright (c) 2020 SUSE LINUX GmbH, Nuernberg, Germany.
#
# All modifications and additions to the file contributed by third parties
# remain the property of their copyright owners, unless otherwise agreed
@@ -141,6 +141,19 @@
Patch10: n_correct_path_in_desktop_file.patch
Patch11: U_viewer-reset-ctrl-alt-to-menu-state-on-focus.patch
Patch12: tigervnc-fix-saving-of-bad-server-certs.patch
+Patch21: 0001-Make-ZlibInStream-more-robust-against-failures.patch
+Patch22: 0002-Encapsulate-PixelBuffer-internal-details.patch
+Patch23: 0003-Restrict-PixelBuffer-dimensions-to-safe-values.patch
+Patch24: 0004-Add-write-protection-to-OffsetPixelBuffer.patch
+Patch25: 0005-Handle-empty-Tight-gradient-rects.patch
+Patch26: 0006-Add-unit-test-for-PixelFormat-sanity-checks.patch
+Patch27: 0007-Fix-depth-sanity-test-in-PixelFormat.patch
+Patch28: 0008-Add-sanity-checks-for-PixelFormat-shift-values.patch
+Patch29: 0009-Remove-unused-FixedMemOutStream.patch
+Patch30: 0010-Use-size_t-for-lengths-in-stream-objects.patch
+Patch31: 0011-Be-defensive-about-overflows-in-stream-objects.patch
+Patch32: 0012-Add-unit-tests-for-PixelFormat.is888-detection.patch
+Patch33: 0013-Handle-pixel-formats-with-odd-shift-values.patch
%description
TigerVNC is an implementation of VNC (Virtual Network Computing), a
@@ -260,6 +273,19 @@
%patch10 -p1
%patch11 -p1
%patch12 -p1
+%patch21 -p1
+%patch22 -p1
+%patch23 -p1
+%patch24 -p1
+%patch25 -p1
+%patch26 -p1
+%patch27 -p1
+%patch28 -p1
+%patch29 -p1
+%patch30 -p1
+%patch31 -p1
+%patch32 -p1
+%patch33 -p1
pushd unix/xserver
patch -p1 < ../xserver120.patch
++++++ 0001-Make-ZlibInStream-more-robust-against-failures.patch ++++++
>From d61a767d6842b530ffb532ddd5a3d233119aad40 Mon Sep 17 00:00:00 2001
From: Pierre Ossman <oss...@cendio.se>
Date: Tue, 10 Sep 2019 11:05:48 +0200
Subject: [PATCH] Make ZlibInStream more robust against failures
Move the checks around to avoid missing cases where we might access
memory that is no longer valid. Also avoid touching the underlying
stream implicitly (e.g. via the destructor) as it might also no
longer be valid.
A malicious server could theoretically use this for remote code
execution in the client.
Issue found by Pavel Cheremushkin from Kaspersky Lab
---
common/rdr/ZlibInStream.cxx | 13 +++++++------
common/rdr/ZlibInStream.h | 2 +-
common/rfb/CMsgReader.cxx | 3 ++-
common/rfb/SMsgReader.cxx | 3 ++-
common/rfb/TightDecoder.cxx | 3 ++-
common/rfb/zrleDecode.h | 3 ++-
6 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/common/rdr/ZlibInStream.cxx b/common/rdr/ZlibInStream.cxx
index 4053bd19..a361010c 100644
--- a/common/rdr/ZlibInStream.cxx
+++ b/common/rdr/ZlibInStream.cxx
@@ -52,16 +52,16 @@ int ZlibInStream::pos()
return offset + ptr - start;
}
-void ZlibInStream::removeUnderlying()
+void ZlibInStream::flushUnderlying()
{
ptr = end = start;
- if (!underlying) return;
while (bytesIn > 0) {
decompress(true);
end = start; // throw away any data
}
- underlying = 0;
+
+ setUnderlying(NULL, 0);
}
void ZlibInStream::reset()
@@ -90,7 +90,7 @@ void ZlibInStream::init()
void ZlibInStream::deinit()
{
assert(zs != NULL);
- removeUnderlying();
+ setUnderlying(NULL, 0);
inflateEnd(zs);
delete zs;
zs = NULL;
@@ -100,8 +100,6 @@ int ZlibInStream::overrun(int itemSize, int nItems, bool
wait)
{
if (itemSize > bufSize)
throw Exception("ZlibInStream overrun: max itemSize exceeded");
- if (!underlying)
- throw Exception("ZlibInStream overrun: no underlying stream");
if (end - ptr != 0)
memmove(start, ptr, end - ptr);
@@ -127,6 +125,9 @@ int ZlibInStream::overrun(int itemSize, int nItems, bool
wait)
bool ZlibInStream::decompress(bool wait)
{
+ if (!underlying)
+ throw Exception("ZlibInStream overrun: no underlying stream");
+
zs->next_out = (U8*)end;
zs->avail_out = start + bufSize - end;
diff --git a/common/rdr/ZlibInStream.h b/common/rdr/ZlibInStream.h
index 6bd4da4c..86ba1ff1 100644
--- a/common/rdr/ZlibInStream.h
+++ b/common/rdr/ZlibInStream.h
@@ -38,7 +38,7 @@ namespace rdr {
virtual ~ZlibInStream();
void setUnderlying(InStream* is, int bytesIn);
- void removeUnderlying();
+ void flushUnderlying();
int pos();
void reset();
diff --git a/common/rfb/CMsgReader.cxx b/common/rfb/CMsgReader.cxx
index a9e12d70..52d40ce7 100644
--- a/common/rfb/CMsgReader.cxx
+++ b/common/rfb/CMsgReader.cxx
@@ -242,7 +242,8 @@ void CMsgReader::readExtendedClipboard(rdr::S32 len)
num++;
}
- zis.removeUnderlying();
+ zis.flushUnderlying();
+ zis.setUnderlying(NULL, 0);
handler->handleClipboardProvide(flags, lengths, buffers);
diff --git a/common/rfb/SMsgReader.cxx b/common/rfb/SMsgReader.cxx
index ab42e59a..dc7ddea6 100644
--- a/common/rfb/SMsgReader.cxx
+++ b/common/rfb/SMsgReader.cxx
@@ -293,7 +293,8 @@ void SMsgReader::readExtendedClipboard(rdr::S32 len)
num++;
}
- zis.removeUnderlying();
+ zis.flushUnderlying();
+ zis.setUnderlying(NULL, 0);
handler->handleClipboardProvide(flags, lengths, buffers);
diff --git a/common/rfb/TightDecoder.cxx b/common/rfb/TightDecoder.cxx
index 5b7c553d..ebc98b06 100644
--- a/common/rfb/TightDecoder.cxx
+++ b/common/rfb/TightDecoder.cxx
@@ -341,7 +341,8 @@ void TightDecoder::decodeRect(const Rect& r, const void*
buffer,
zis[streamId].readBytes(netbuf, dataSize);
- zis[streamId].removeUnderlying();
+ zis[streamId].flushUnderlying();
+ zis[streamId].setUnderlying(NULL, 0);
delete ms;
bufptr = netbuf;
diff --git a/common/rfb/zrleDecode.h b/common/rfb/zrleDecode.h
index 32b5c92b..f4325385 100644
--- a/common/rfb/zrleDecode.h
+++ b/common/rfb/zrleDecode.h
@@ -174,7 +174,8 @@ void ZRLE_DECODE (const Rect& r, rdr::InStream* is,
}
}
- zis->removeUnderlying();
+ zis->flushUnderlying();
+ zis->setUnderlying(NULL, 0);
}
#undef ZRLE_DECODE
--
2.16.4
++++++ 0002-Encapsulate-PixelBuffer-internal-details.patch ++++++
>From 53f913a76196c7357d4858bfbf2c33caa9181bae Mon Sep 17 00:00:00 2001
From: Pierre Ossman <oss...@cendio.se>
Date: Tue, 10 Sep 2019 15:18:30 +0200
Subject: [PATCH] Encapsulate PixelBuffer internal details
Don't allow subclasses to just override dimensions or buffer details
directly and instead force them to go via methods. This allows us
to do sanity checks on the new values and catch bugs and attacks.
---
common/rfb/Cursor.cxx | 3 +-
common/rfb/EncodeManager.cxx | 5 +-
common/rfb/PixelBuffer.cxx | 103 +++++++++++++++++++++-------------
common/rfb/PixelBuffer.h | 17 ++++--
unix/x0vncserver/XPixelBuffer.cxx | 9 +--
unix/xserver/hw/vnc/XserverDesktop.cc | 24 ++++----
unix/xserver/hw/vnc/XserverDesktop.h | 2 +-
vncviewer/PlatformPixelBuffer.cxx | 9 ++-
win/rfb_win32/DIBSectionBuffer.cxx | 41 ++++++--------
9 files changed, 111 insertions(+), 102 deletions(-)
diff --git a/common/rfb/Cursor.cxx b/common/rfb/Cursor.cxx
index d7b536de..3ca69f7c 100644
--- a/common/rfb/Cursor.cxx
+++ b/common/rfb/Cursor.cxx
@@ -272,8 +272,7 @@ void RenderedCursor::update(PixelBuffer* framebuffer,
assert(cursor);
format = framebuffer->getPF();
- width_ = framebuffer->width();
- height_ = framebuffer->height();
+ setSize(framebuffer->width(), framebuffer->height());
rawOffset = pos.subtract(cursor->hotspot());
clippedRect = Rect(0, 0, cursor->width(), cursor->height())
diff --git a/common/rfb/EncodeManager.cxx b/common/rfb/EncodeManager.cxx
index 735be072..54f7102b 100644
--- a/common/rfb/EncodeManager.cxx
+++ b/common/rfb/EncodeManager.cxx
@@ -1049,11 +1049,8 @@ void EncodeManager::OffsetPixelBuffer::update(const
PixelFormat& pf,
int stride_)
{
format = pf;
- width_ = width;
- height_ = height;
// Forced cast. We never write anything though, so it should be safe.
- data = (rdr::U8*)data_;
- stride = stride_;
+ setBuffer(width, height, (rdr::U8*)data_, stride_);
}
// Preprocessor generated, optimised methods
diff --git a/common/rfb/PixelBuffer.cxx b/common/rfb/PixelBuffer.cxx
index 7f4c1ad3..0aa67744 100644
--- a/common/rfb/PixelBuffer.cxx
+++ b/common/rfb/PixelBuffer.cxx
@@ -35,8 +35,14 @@ static LogWriter vlog("PixelBuffer");
// -=- Generic pixel buffer class
PixelBuffer::PixelBuffer(const PixelFormat& pf, int w, int h)
- : format(pf), width_(w), height_(h) {}
-PixelBuffer::PixelBuffer() : width_(0), height_(0) {}
+ : format(pf), width_(0), height_(0)
+{
+ setSize(w, h);
+}
+
+PixelBuffer::PixelBuffer() : width_(0), height_(0)
+{
+}
PixelBuffer::~PixelBuffer() {}
@@ -53,7 +59,7 @@ PixelBuffer::getImage(void* imageBuf, const Rect& r, int
outStride) const
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_);
+ r.tl.x, r.tl.y, width(), height());
data = getBuffer(r, &inStride);
@@ -89,7 +95,7 @@ void PixelBuffer::getImage(const PixelFormat& pf, void*
imageBuf,
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_);
+ r.tl.x, r.tl.y, width(), height());
if (stride == 0)
stride = r.width();
@@ -100,6 +106,12 @@ void PixelBuffer::getImage(const PixelFormat& pf, void*
imageBuf,
stride, srcStride);
}
+void PixelBuffer::setSize(int width, int height)
+{
+ width_ = width;
+ height_ = height;
+}
+
// -=- Modifiable generic pixel buffer class
ModifiablePixelBuffer::ModifiablePixelBuffer(const PixelFormat& pf,
@@ -124,7 +136,7 @@ void ModifiablePixelBuffer::fillRect(const Rect& r, const
void* pix)
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_);
+ r.width(), r.height(), r.tl.x, r.tl.y, width(),
height());
w = r.width();
h = r.height();
@@ -175,7 +187,7 @@ void ModifiablePixelBuffer::imageRect(const Rect& r,
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_);
+ r.tl.x, r.tl.y, width(), height());
bytesPerPixel = getPF().bpp/8;
@@ -214,13 +226,13 @@ void ModifiablePixelBuffer::copyRect(const Rect &rect,
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_);
+ drect.tl.x, drect.tl.y, width(), height());
srect = drect.translate(move_by_delta.negate());
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_);
+ srect.tl.x, srect.tl.y, width(), height());
bytesPerPixel = format.bpp/8;
@@ -275,7 +287,7 @@ void ModifiablePixelBuffer::imageRect(const PixelFormat&
pf, const Rect &dest,
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_);
+ dest.tl.x, dest.tl.y, width(), height());
if (stride == 0)
stride = dest.width();
@@ -304,7 +316,7 @@ 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_);
+ r.tl.x, r.tl.y, width(), height());
*stride_ = stride;
return &data[(r.tl.x + (r.tl.y * stride)) * (format.bpp/8)];
@@ -319,55 +331,68 @@ const rdr::U8* FullFramePixelBuffer::getBuffer(const
Rect& r, int* stride_) cons
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_);
+ r.tl.x, r.tl.y, width(), height());
*stride_ = stride;
return &data[(r.tl.x + (r.tl.y * stride)) * (format.bpp/8)];
}
+void FullFramePixelBuffer::setBuffer(int width, int height,
+ rdr::U8* data_, int stride_)
+{
+ ModifiablePixelBuffer::setSize(width, height);
+ stride = stride_;
+ data = data_;
+}
+
+void FullFramePixelBuffer::setSize(int w, int h)
+{
+ // setBuffer() should be used
+ throw rfb::Exception("Invalid call to FullFramePixelBuffer::setSize()");
+}
+
// -=- Managed pixel buffer class
// Automatically allocates enough space for the specified format & area
ManagedPixelBuffer::ManagedPixelBuffer()
- : datasize(0)
+ : data_(NULL), datasize(0)
{
- checkDataSize();
-};
+}
ManagedPixelBuffer::ManagedPixelBuffer(const PixelFormat& pf, int w, int h)
- : FullFramePixelBuffer(pf, w, h, NULL, w), datasize(0)
+ : FullFramePixelBuffer(pf, 0, 0, NULL, 0), data_(NULL), datasize(0)
{
- checkDataSize();
-};
-
-ManagedPixelBuffer::~ManagedPixelBuffer() {
- if (data) delete [] data;
-};
+ setSize(w, h);
+}
+ManagedPixelBuffer::~ManagedPixelBuffer()
+{
+ if (data_)
+ delete [] data_;
+}
-void
-ManagedPixelBuffer::setPF(const PixelFormat &pf) {
- format = pf; checkDataSize();
-};
-void
-ManagedPixelBuffer::setSize(int w, int h) {
- width_ = w; height_ = h; stride = w; checkDataSize();
-};
+void ManagedPixelBuffer::setPF(const PixelFormat &pf)
+{
+ format = pf;
+ setSize(width(), height());
+}
+void ManagedPixelBuffer::setSize(int w, int h)
+{
+ unsigned long new_datasize = w * h * (format.bpp/8);
-inline void
-ManagedPixelBuffer::checkDataSize() {
- unsigned long new_datasize = width_ * height_ * (format.bpp/8);
+ new_datasize = w * h * (format.bpp/8);
if (datasize < new_datasize) {
- if (data) {
- delete [] data;
- datasize = 0; data = 0;
+ if (data_) {
+ delete [] data_;
+ data_ = NULL;
+ datasize = 0;
}
if (new_datasize) {
- data = new U8[new_datasize];
- if (!data)
- throw Exception("rfb::ManagedPixelBuffer unable to allocate buffer");
+ data_ = new U8[new_datasize];
datasize = new_datasize;
}
}
-};
+
+ setBuffer(w, h, data_, w);
+}
diff --git a/common/rfb/PixelBuffer.h b/common/rfb/PixelBuffer.h
index d89793f5..3e4018f9 100644
--- a/common/rfb/PixelBuffer.h
+++ b/common/rfb/PixelBuffer.h
@@ -90,7 +90,12 @@ namespace rfb {
protected:
PixelBuffer();
+ virtual void setSize(int width, int height);
+
+ protected:
PixelFormat format;
+
+ private:
int width_, height_;
};
@@ -154,7 +159,12 @@ namespace rfb {
protected:
FullFramePixelBuffer();
+ virtual void setBuffer(int width, int height, rdr::U8* data, int stride);
+ private:
+ virtual void setSize(int w, int h);
+
+ private:
rdr::U8* data;
int stride;
};
@@ -172,12 +182,9 @@ namespace rfb {
virtual void setPF(const PixelFormat &pf);
virtual void setSize(int w, int h);
- // Return the total number of bytes of pixel data in the buffer
- int dataLen() const { return width_ * height_ * (format.bpp/8); }
-
- protected:
+ private:
+ rdr::U8* data_; // Mirrors FullFramePixelBuffer::data
unsigned long datasize;
- void checkDataSize();
};
};
diff --git a/unix/x0vncserver/XPixelBuffer.cxx
b/unix/x0vncserver/XPixelBuffer.cxx
index 4769b651..f0b06967 100644
--- a/unix/x0vncserver/XPixelBuffer.cxx
+++ b/unix/x0vncserver/XPixelBuffer.cxx
@@ -50,13 +50,8 @@ XPixelBuffer::XPixelBuffer(Display *dpy, ImageFactory
&factory,
ffs(m_image->xim->blue_mask) - 1);
// Set up the remaining data of the parent class.
- width_ = rect.width();
- height_ = rect.height();
- data = (rdr::U8 *)m_image->xim->data;
-
- // Calculate the distance in pixels between two subsequent scan
- // lines of the framebuffer. This may differ from image width.
- stride = m_image->xim->bytes_per_line * 8 / m_image->xim->bits_per_pixel;
+ setBuffer(rect.width(), rect.height(), (rdr::U8 *)m_image->xim->data,
+ m_image->xim->bytes_per_line * 8 / m_image->xim->bits_per_pixel);
// Get initial screen image from the X display.
m_image->get(DefaultRootWindow(m_dpy), m_offsetLeft, m_offsetTop);
diff --git a/unix/xserver/hw/vnc/XserverDesktop.cc
b/unix/xserver/hw/vnc/XserverDesktop.cc
index 4edffec7..5c8b4cef 100644
--- a/unix/xserver/hw/vnc/XserverDesktop.cc
+++ b/unix/xserver/hw/vnc/XserverDesktop.cc
@@ -75,7 +75,7 @@ XserverDesktop::XserverDesktop(int screenIndex_,
void* fbptr, int stride)
: screenIndex(screenIndex_),
server(0), listeners(listeners_),
- directFbptr(true),
+ shadowFramebuffer(NULL),
queryConnectId(0), queryConnectTimer(this)
{
format = pf;
@@ -97,8 +97,8 @@ XserverDesktop::~XserverDesktop()
delete listeners.back();
listeners.pop_back();
}
- if (!directFbptr)
- delete [] data;
+ if (shadowFramebuffer)
+ delete [] shadowFramebuffer;
delete server;
}
@@ -116,22 +116,18 @@ void XserverDesktop::setFramebuffer(int w, int h, void*
fbptr, int stride_)
{
ScreenSet layout;
- width_ = w;
- height_ = h;
-
- if (!directFbptr) {
- delete [] data;
- directFbptr = true;
+ if (shadowFramebuffer) {
+ delete [] shadowFramebuffer;
+ shadowFramebuffer = NULL;
}
if (!fbptr) {
- fbptr = new rdr::U8[w * h * (format.bpp/8)];
+ shadowFramebuffer = new rdr::U8[w * h * (format.bpp/8)];
+ fbptr = shadowFramebuffer;
stride_ = w;
- directFbptr = false;
}
- data = (rdr::U8*)fbptr;
- stride = stride_;
+ setBuffer(w, h, (rdr::U8*)fbptr, stride_);
vncSetGlueContext(screenIndex);
layout = ::computeScreenLayout(&outputIdMap);
@@ -492,7 +488,7 @@ void XserverDesktop::handleClipboardData(const char* data_)
void XserverDesktop::grabRegion(const rfb::Region& region)
{
- if (directFbptr)
+ if (shadowFramebuffer == NULL)
return;
std::vector<rfb::Rect> rects;
diff --git a/unix/xserver/hw/vnc/XserverDesktop.h
b/unix/xserver/hw/vnc/XserverDesktop.h
index 6c670689..cc50f9e9 100644
--- a/unix/xserver/hw/vnc/XserverDesktop.h
+++ b/unix/xserver/hw/vnc/XserverDesktop.h
@@ -118,7 +118,7 @@ private:
int screenIndex;
rfb::VNCServer* server;
std::list<network::SocketListener*> listeners;
- bool directFbptr;
+ rdr::U8* shadowFramebuffer;
uint32_t queryConnectId;
network::Socket* queryConnectSocket;
diff --git a/vncviewer/PlatformPixelBuffer.cxx
b/vncviewer/PlatformPixelBuffer.cxx
index ff1935e7..61f7b743 100644
--- a/vncviewer/PlatformPixelBuffer.cxx
+++ b/vncviewer/PlatformPixelBuffer.cxx
@@ -36,7 +36,7 @@ static rfb::LogWriter vlog("PlatformPixelBuffer");
PlatformPixelBuffer::PlatformPixelBuffer(int width, int height) :
FullFramePixelBuffer(rfb::PixelFormat(32, 24, false, true,
255, 255, 255, 16, 8, 0),
- width, height, NULL, 0),
+ 0, 0, NULL, 0),
Surface(width, height)
#if !defined(WIN32) && !defined(__APPLE__)
, shminfo(NULL), xim(NULL)
@@ -56,14 +56,13 @@ PlatformPixelBuffer::PlatformPixelBuffer(int width, int
height) :
vlog.debug("Using standard XImage");
}
- data = (rdr::U8*)xim->data;
- stride = xim->bytes_per_line / (getPF().bpp/8);
+ setBuffer(width, height, (rdr::U8*)xim->data,
+ xim->bytes_per_line / (getPF().bpp/8));
// On X11, the Pixmap backing this Surface is uninitialized.
clear(0, 0, 0);
#else
- FullFramePixelBuffer::data = (rdr::U8*)Surface::data;
- stride = width;
+ setBuffer(width, height, (rdr::U8*)Surface::data, width);
#endif
}
diff --git a/win/rfb_win32/DIBSectionBuffer.cxx
b/win/rfb_win32/DIBSectionBuffer.cxx
index e2b0d641..e00cf233 100644
--- a/win/rfb_win32/DIBSectionBuffer.cxx
+++ b/win/rfb_win32/DIBSectionBuffer.cxx
@@ -52,39 +52,28 @@ void DIBSectionBuffer::setPF(const PixelFormat& pf) {
if (!pf.trueColour)
throw rfb::Exception("palette format not supported");
format = pf;
- recreateBuffer();
+ setSize(width(), height());
}
-void DIBSectionBuffer::setSize(int w, int h) {
- if (width_ == w && height_ == h) {
- vlog.debug("size unchanged by setSize()");
- return;
- }
- width_ = w;
- height_ = h;
- recreateBuffer();
-}
-
-
inline void initMaxAndShift(DWORD mask, int* max, int* shift) {
for ((*shift) = 0; (mask & 1) == 0; (*shift)++) mask >>= 1;
(*max) = (rdr::U16)mask;
}
-void DIBSectionBuffer::recreateBuffer() {
+void DIBSectionBuffer::setSize(int w, int h) {
HBITMAP new_bitmap = 0;
rdr::U8* new_data = 0;
- if (width_ && height_ && (format.depth != 0)) {
+ if (w && h && (format.depth != 0)) {
BitmapInfo bi;
memset(&bi, 0, sizeof(bi));
UINT iUsage = DIB_RGB_COLORS;
bi.bmiHeader.biSize = sizeof(BITMAPINFOHEADER);
bi.bmiHeader.biBitCount = format.bpp;
- bi.bmiHeader.biSizeImage = (format.bpp / 8) * width_ * height_;
+ bi.bmiHeader.biSizeImage = (format.bpp / 8) * w * h;
bi.bmiHeader.biPlanes = 1;
- bi.bmiHeader.biWidth = width_;
- bi.bmiHeader.biHeight = -height_;
+ bi.bmiHeader.biWidth = w;
+ bi.bmiHeader.biHeight = -h;
bi.bmiHeader.biCompression = (format.bpp > 8) ? BI_BITFIELDS : BI_RGB;
bi.mask.red = format.pixelFromRGB((rdr::U16)~0, 0, 0);
bi.mask.green = format.pixelFromRGB(0, (rdr::U16)~0, 0);
@@ -115,12 +104,12 @@ void DIBSectionBuffer::recreateBuffer() {
if (device) {
BitmapDC src_dev(device, bitmap);
BitmapDC dest_dev(device, new_bitmap);
- BitBlt(dest_dev, 0, 0, width_, height_, src_dev, 0, 0, SRCCOPY);
+ BitBlt(dest_dev, 0, 0, w, h, src_dev, 0, 0, SRCCOPY);
} else {
WindowDC wndDC(window);
BitmapDC src_dev(wndDC, bitmap);
BitmapDC dest_dev(wndDC, new_bitmap);
- BitBlt(dest_dev, 0, 0, width_, height_, src_dev, 0, 0, SRCCOPY);
+ BitBlt(dest_dev, 0, 0, w, h, src_dev, 0, 0, SRCCOPY);
}
}
@@ -128,17 +117,17 @@ void DIBSectionBuffer::recreateBuffer() {
// Delete the old bitmap
DeleteObject(bitmap);
bitmap = 0;
- data = 0;
+ setBuffer(0, 0, NULL, 0);
}
if (new_bitmap) {
int bpp, depth;
int redMax, greenMax, blueMax;
int redShift, greenShift, blueShift;
+ int new_stride;
// Set up the new bitmap
bitmap = new_bitmap;
- data = new_data;
// Determine the *actual* DIBSection format
DIBSECTION ds;
@@ -147,14 +136,16 @@ void DIBSectionBuffer::recreateBuffer() {
// Correct the "stride" of the DIB
// *** This code DWORD aligns each row - is that right???
- stride = width_;
- int bytesPerRow = stride * format.bpp/8;
+ new_stride = w;
+ int bytesPerRow = new_stride * format.bpp/8;
if (bytesPerRow % 4) {
bytesPerRow += 4 - (bytesPerRow % 4);
- stride = (bytesPerRow * 8) / format.bpp;
- vlog.info("adjusting DIB stride: %d to %d", width_, stride);
+ new_stride = (bytesPerRow * 8) / format.bpp;
+ vlog.info("adjusting DIB stride: %d to %d", w, new_stride);
}
+ setBuffer(w, h, new_data, new_stride);
+
// Calculate the PixelFormat for the DIB
bpp = depth = ds.dsBm.bmBitsPixel;
--
2.16.4
++++++ 0003-Restrict-PixelBuffer-dimensions-to-safe-values.patch ++++++
>From 996356b6c65ca165ee1ea46a571c32a1dc3c3821 Mon Sep 17 00:00:00 2001
From: Pierre Ossman <oss...@cendio.se>
Date: Tue, 10 Sep 2019 15:21:03 +0200
Subject: [PATCH] Restrict PixelBuffer dimensions to safe values
We do a lot of calculations based on pixel coordinates and we need
to make sure they do not overflow. Restrict the maximum dimensions
we support rather than try to switch over all calculations to use
64 bit integers.
This prevents attackers from from injecting code by specifying a
huge framebuffer size and relying on the values overflowing to
access invalid areas of the heap.
This primarily affects the client which gets both the screen
dimensions and the pixel contents from the remote side. But the
server might also be affected as a client can adjust the screen
dimensions, as can applications inside the session.
Issue found by Pavel Cheremushkin from Kaspersky Lab.
---
common/rfb/PixelBuffer.cxx | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/common/rfb/PixelBuffer.cxx b/common/rfb/PixelBuffer.cxx
index 0aa67744..fe406b96 100644
--- a/common/rfb/PixelBuffer.cxx
+++ b/common/rfb/PixelBuffer.cxx
@@ -31,6 +31,14 @@ using namespace rdr;
static LogWriter vlog("PixelBuffer");
+// We do a lot of byte offset calculations that assume the result fits
+// inside a signed 32 bit integer. Limit the maximum size of pixel
+// buffers so that these calculations never overflow.
+
+const int maxPixelBufferWidth = 16384;
+const int maxPixelBufferHeight = 16384;
+const int maxPixelBufferStride = 16384;
+
// -=- Generic pixel buffer class
@@ -108,6 +116,11 @@ void PixelBuffer::getImage(const PixelFormat& pf, void*
imageBuf,
void PixelBuffer::setSize(int width, int height)
{
+ if ((width < 0) || (width > maxPixelBufferWidth))
+ throw rfb::Exception("Invalid PixelBuffer width of %d pixels requested",
width);
+ if ((height < 0) || (height > maxPixelBufferHeight))
+ throw rfb::Exception("Invalid PixelBuffer height of %d pixels requested",
height);
+
width_ = width;
height_ = height;
}
@@ -340,6 +353,15 @@ const rdr::U8* FullFramePixelBuffer::getBuffer(const Rect&
r, int* stride_) cons
void FullFramePixelBuffer::setBuffer(int width, int height,
rdr::U8* data_, int stride_)
{
+ if ((width < 0) || (width > maxPixelBufferWidth))
+ throw rfb::Exception("Invalid PixelBuffer width of %d pixels requested",
width);
+ if ((height < 0) || (height > maxPixelBufferHeight))
+ throw rfb::Exception("Invalid PixelBuffer height of %d pixels requested",
height);
+ if ((stride_ < 0) || (stride_ > maxPixelBufferStride) || (stride_ < width))
+ throw rfb::Exception("Invalid PixelBuffer stride of %d pixels requested",
stride_);
+ if ((width != 0) && (height != 0) && (data_ == NULL))
+ throw rfb::Exception("PixelBuffer requested without a valid memory area");
+
ModifiablePixelBuffer::setSize(width, height);
stride = stride_;
data = data_;
--
2.16.4
++++++ 0004-Add-write-protection-to-OffsetPixelBuffer.patch ++++++
>From 9f615301aba1cc54a749950bf9462c5a85217bc4 Mon Sep 17 00:00:00 2001
From: Pierre Ossman <oss...@cendio.se>
Date: Tue, 10 Sep 2019 15:25:30 +0200
Subject: [PATCH] Add write protection to OffsetPixelBuffer
No one should every try to write to this buffer. Enforce that by
throwing an exception if any one tries to get a writeable pointer
to the data.
---
common/rfb/EncodeManager.cxx | 6 ++++++
common/rfb/EncodeManager.h | 3 +++
2 files changed, 9 insertions(+)
diff --git a/common/rfb/EncodeManager.cxx b/common/rfb/EncodeManager.cxx
index 54f7102b..92ac5676 100644
--- a/common/rfb/EncodeManager.cxx
+++ b/common/rfb/EncodeManager.cxx
@@ -28,6 +28,7 @@
#include <rfb/SMsgWriter.h>
#include <rfb/UpdateTracker.h>
#include <rfb/LogWriter.h>
+#include <rfb/Exception.h>
#include <rfb/RawEncoder.h>
#include <rfb/RREEncoder.h>
@@ -1053,6 +1054,11 @@ void EncodeManager::OffsetPixelBuffer::update(const
PixelFormat& pf,
setBuffer(width, height, (rdr::U8*)data_, stride_);
}
+rdr::U8* EncodeManager::OffsetPixelBuffer::getBufferRW(const Rect& r, int*
stride)
+{
+ throw rfb::Exception("Invalid write attempt to OffsetPixelBuffer");
+}
+
// Preprocessor generated, optimised methods
#define BPP 8
diff --git a/common/rfb/EncodeManager.h b/common/rfb/EncodeManager.h
index bdae9063..f8201c34 100644
--- a/common/rfb/EncodeManager.h
+++ b/common/rfb/EncodeManager.h
@@ -148,6 +148,9 @@ namespace rfb {
void update(const PixelFormat& pf, int width, int height,
const rdr::U8* data_, int stride);
+
+ private:
+ virtual rdr::U8* getBufferRW(const Rect& r, int* stride);
};
OffsetPixelBuffer offsetPixelBuffer;
--
2.16.4
++++++ 0005-Handle-empty-Tight-gradient-rects.patch ++++++
>From b4ada8d0c6dac98c8b91fc64d112569a8ae5fb95 Mon Sep 17 00:00:00 2001
From: Pierre Ossman <oss...@cendio.se>
Date: Tue, 10 Sep 2019 15:36:42 +0200
Subject: [PATCH] Handle empty Tight gradient rects
We always assumed there would be one pixel per row so a rect with
a zero width would result in us writing to unknown memory.
This could theoretically be used by a malicious server to inject
code in to the viewer process.
Issue found by Pavel Cheremushkin from Kaspersky Lab.
---
common/rfb/tightDecode.h | 37 +++++++++++++++++++++----------------
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/common/rfb/tightDecode.h b/common/rfb/tightDecode.h
index b6e86ed5..8f77aebd 100644
--- a/common/rfb/tightDecode.h
+++ b/common/rfb/tightDecode.h
@@ -56,15 +56,17 @@ TightDecoder::FilterGradient24(const rdr::U8 *inbuf,
int rectWidth = r.width();
for (y = 0; y < rectHeight; y++) {
- /* First pixel in a row */
- for (c = 0; c < 3; c++) {
- pix[c] = inbuf[y*rectWidth*3+c] + prevRow[c];
- thisRow[c] = pix[c];
- }
- pf.bufferFromRGB((rdr::U8*)&outbuf[y*stride], pix, 1);
+ for (x = 0; x < rectWidth; x++) {
+ /* First pixel in a row */
+ if (x == 0) {
+ for (c = 0; c < 3; c++) {
+ pix[c] = inbuf[y*rectWidth*3+c] + prevRow[c];
+ thisRow[c] = pix[c];
+ }
+ pf.bufferFromRGB((rdr::U8*)&outbuf[y*stride], pix, 1);
+ continue;
+ }
- /* Remaining pixels of a row */
- for (x = 1; x < rectWidth; x++) {
for (c = 0; c < 3; c++) {
est[c] = prevRow[x*3+c] + pix[c] - prevRow[(x-1)*3+c];
if (est[c] > 0xff) {
@@ -103,17 +105,20 @@ void TightDecoder::FilterGradient(const rdr::U8* inbuf,
int rectWidth = r.width();
for (y = 0; y < rectHeight; y++) {
- /* First pixel in a row */
- pf.rgbFromBuffer(pix, &inbuf[y*rectWidth], 1);
- for (c = 0; c < 3; c++)
- pix[c] += prevRow[c];
+ for (x = 0; x < rectWidth; x++) {
+ /* First pixel in a row */
+ if (x == 0) {
+ pf.rgbFromBuffer(pix, &inbuf[y*rectWidth], 1);
+ for (c = 0; c < 3; c++)
+ pix[c] += prevRow[c];
- memcpy(thisRow, pix, sizeof(pix));
+ memcpy(thisRow, pix, sizeof(pix));
- pf.bufferFromRGB((rdr::U8*)&outbuf[y*stride], pix, 1);
+ pf.bufferFromRGB((rdr::U8*)&outbuf[y*stride], pix, 1);
+
+ continue;
+ }
- /* Remaining pixels of a row */
- for (x = 1; x < rectWidth; x++) {
for (c = 0; c < 3; c++) {
est[c] = prevRow[x*3+c] + pix[c] - prevRow[(x-1)*3+c];
if (est[c] > 255) {
--
2.16.4
++++++ 0006-Add-unit-test-for-PixelFormat-sanity-checks.patch ++++++
>From 014c5012377519d7f0add23ebac077ccd882aa9f Mon Sep 17 00:00:00 2001
From: Pierre Ossman <oss...@cendio.se>
Date: Tue, 10 Sep 2019 15:59:51 +0200
Subject: [PATCH] Add unit test for PixelFormat sanity checks
---
common/rfb/PixelFormat.cxx | 3 +-
tests/unit/CMakeLists.txt | 3 ++
tests/unit/pixelformat.cxx | 114 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 119 insertions(+), 1 deletion(-)
create mode 100644 tests/unit/pixelformat.cxx
diff --git a/common/rfb/PixelFormat.cxx b/common/rfb/PixelFormat.cxx
index 883b0410..0be4d1da 100644
--- a/common/rfb/PixelFormat.cxx
+++ b/common/rfb/PixelFormat.cxx
@@ -81,7 +81,8 @@ PixelFormat::PixelFormat(int b, int d, bool e, bool t,
redMax(rm), greenMax(gm), blueMax(bm),
redShift(rs), greenShift(gs), blueShift(bs)
{
- assert(isSane());
+ if (!isSane())
+ throw Exception("invalid pixel format");
updateState();
}
diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt
index c847238d..acc3adcd 100644
--- a/tests/unit/CMakeLists.txt
+++ b/tests/unit/CMakeLists.txt
@@ -8,3 +8,6 @@ target_link_libraries(convertlf rfb)
add_executable(hostport hostport.cxx)
target_link_libraries(hostport rfb)
+
+add_executable(pixelformat pixelformat.cxx)
+target_link_libraries(pixelformat rfb)
diff --git a/tests/unit/pixelformat.cxx b/tests/unit/pixelformat.cxx
new file mode 100644
index 00000000..4eb45281
--- /dev/null
+++ b/tests/unit/pixelformat.cxx
@@ -0,0 +1,114 @@
+/* Copyright 2019 Pierre Ossman <oss...@cendio.se> for Cendio AB
+ *
+ * This is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This software is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this software; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307,
+ * USA.
+ */
+
+#include <stdio.h>
+
+#include <rfb/PixelFormat.h>
+#include <rfb/Exception.h>
+
+static void doTest(bool should_fail, int b, int d, bool e, bool t,
+ int rm, int gm, int bm, int rs, int gs, int bs)
+{
+ rfb::PixelFormat* pf;
+
+ printf("PixelFormat(%d, %d, %s, %s, %d, %d, %d, %d, %d, %d): ",
+ b, d, e ? "true" : "false", t ? "true": "false",
+ rm, gm, bm, rs, gs, bs);
+
+ try {
+ pf = new rfb::PixelFormat(b, d, e, t, rm, gm, bm, rs, gs, bs);
+ } catch(rfb::Exception &e) {
+ if (should_fail)
+ printf("OK");
+ else
+ printf("FAILED");
+ printf("\n");
+ fflush(stdout);
+ return;
+ }
+
+ delete pf;
+
+ if (should_fail)
+ printf("FAILED");
+ else
+ printf("OK");
+ printf("\n");
+ fflush(stdout);
+}
+
+int main(int argc, char** argv)
+{
+ /* Normal true color formats */
+
+ doTest(false, 32, 24, false, true, 255, 255, 255, 0, 8, 16);
+ doTest(false, 32, 24, false, true, 255, 255, 255, 24, 16, 8);
+
+ doTest(false, 16, 16, false, true, 15, 31, 15, 0, 5, 11);
+
+ doTest(false, 8, 8, false, true, 3, 7, 3, 0, 2, 5);
+
+ /* Excessive bpp */
+
+ doTest(false, 32, 16, false, true, 15, 31, 15, 0, 5, 11);
+
+ doTest(false, 16, 16, false, true, 15, 31, 15, 0, 5, 11);
+
+ doTest(false, 32, 8, false, true, 3, 7, 3, 0, 2, 5);
+
+ doTest(false, 16, 8, false, true, 3, 7, 3, 0, 2, 5);
+
+ /* Colour map */
+
+ doTest(false, 8, 8, false, false, 0, 0, 0, 0, 0, 0);
+
+ /* Invalid bpp */
+
+ doTest(true, 64, 24, false, true, 255, 255, 255, 0, 8, 16);
+
+ doTest(true, 18, 16, false, true, 15, 31, 15, 0, 5, 11);
+
+ doTest(true, 3, 3, false, true, 1, 1, 1, 0, 1, 2);
+
+ /* Invalid depth */
+
+ doTest(true, 16, 24, false, true, 15, 31, 15, 0, 5, 11);
+
+ doTest(true, 8, 24, false, true, 3, 7, 3, 0, 2, 5);
+ doTest(true, 8, 16, false, true, 3, 7, 3, 0, 2, 5);
+
+ doTest(true, 32, 24, false, false, 0, 0, 0, 0, 0, 0);
+
+ /* Invalid max values */
+
+ doTest(true, 32, 24, false, true, 254, 255, 255, 0, 8, 16);
+ doTest(true, 32, 24, false, true, 255, 253, 255, 0, 8, 16);
+ doTest(true, 32, 24, false, true, 255, 255, 252, 0, 8, 16);
+
+ doTest(true, 32, 24, false, true, 511, 127, 127, 0, 16, 20);
+ doTest(true, 32, 24, false, true, 127, 511, 127, 0, 4, 20);
+ doTest(true, 32, 24, false, true, 127, 127, 511, 0, 4, 8);
+
+ /* Overlapping channels */
+
+ doTest(true, 32, 24, false, true, 255, 255, 255, 0, 7, 16);
+ doTest(true, 32, 24, false, true, 255, 255, 255, 0, 8, 15);
+ doTest(true, 32, 24, false, true, 255, 255, 255, 0, 16, 7);
+
+ return 0;
+}
--
2.16.4
++++++ 0007-Fix-depth-sanity-test-in-PixelFormat.patch ++++++
>From f1b9b868ec943d51ef631f53a095d48d3f178f4f Mon Sep 17 00:00:00 2001
From: Pierre Ossman <oss...@cendio.se>
Date: Tue, 10 Sep 2019 16:01:44 +0200
Subject: [PATCH] Fix depth sanity test in PixelFormat
---
common/rfb/PixelFormat.cxx | 2 +-
tests/unit/pixelformat.cxx | 4 ++++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/common/rfb/PixelFormat.cxx b/common/rfb/PixelFormat.cxx
index 0be4d1da..2d8142d1 100644
--- a/common/rfb/PixelFormat.cxx
+++ b/common/rfb/PixelFormat.cxx
@@ -679,7 +679,7 @@ bool PixelFormat::isSane(void)
return false;
totalBits = bits(redMax) + bits(greenMax) + bits(blueMax);
- if (totalBits > bpp)
+ if (totalBits > depth)
return false;
if (((redMax << redShift) & (greenMax << greenShift)) != 0)
diff --git a/tests/unit/pixelformat.cxx b/tests/unit/pixelformat.cxx
index 4eb45281..7b6087f7 100644
--- a/tests/unit/pixelformat.cxx
+++ b/tests/unit/pixelformat.cxx
@@ -104,6 +104,10 @@ int main(int argc, char** argv)
doTest(true, 32, 24, false, true, 127, 511, 127, 0, 4, 20);
doTest(true, 32, 24, false, true, 127, 127, 511, 0, 4, 8);
+ /* Insufficient depth */
+
+ doTest(true, 32, 16, false, true, 255, 255, 255, 0, 8, 16);
+
/* Overlapping channels */
doTest(true, 32, 24, false, true, 255, 255, 255, 0, 7, 16);
--
2.16.4
++++++ 0008-Add-sanity-checks-for-PixelFormat-shift-values.patch ++++++
>From cd1d650c532a46e95a1229dffaf281c76a50cdfe Mon Sep 17 00:00:00 2001
From: Pierre Ossman <oss...@cendio.se>
Date: Tue, 10 Sep 2019 16:07:50 +0200
Subject: [PATCH] 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.
---
common/rfb/PixelFormat.cxx | 7 +++++++
tests/unit/pixelformat.cxx | 6 ++++++
2 files changed, 13 insertions(+)
diff --git a/common/rfb/PixelFormat.cxx b/common/rfb/PixelFormat.cxx
index 2d8142d1..789c43ed 100644
--- 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 7b6087f7..46fecfb4 100644
--- 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);
--
2.16.4
++++++ 0009-Remove-unused-FixedMemOutStream.patch ++++++
>From 4ff58f0acaeb566b79ae12cf013b376eaaaab834 Mon Sep 17 00:00:00 2001
From: Pierre Ossman <oss...@cendio.se>
Date: Mon, 23 Sep 2019 10:09:31 +0200
Subject: [PATCH] Remove unused FixedMemOutStream
---
common/rdr/FixedMemOutStream.h | 52 ------------------------------------------
1 file changed, 52 deletions(-)
delete mode 100644 common/rdr/FixedMemOutStream.h
diff --git a/common/rdr/FixedMemOutStream.h b/common/rdr/FixedMemOutStream.h
deleted file mode 100644
index e4ec52cb..00000000
--- a/common/rdr/FixedMemOutStream.h
+++ /dev/null
@@ -1,52 +0,0 @@
-/* Copyright (C) 2002-2005 RealVNC Ltd. All Rights Reserved.
- *
- * This is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This software is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this software; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307,
- * USA.
- */
-
-//
-// A FixedMemOutStream writes to a buffer of a fixed length.
-//
-
-#ifndef __RDR_FIXEDMEMOUTSTREAM_H__
-#define __RDR_FIXEDMEMOUTSTREAM_H__
-
-#include <rdr/OutStream.h>
-#include <rdr/Exception.h>
-
-namespace rdr {
-
- class FixedMemOutStream : public OutStream {
-
- public:
-
- FixedMemOutStream(void* buf, int len) {
- ptr = start = (U8*)buf;
- end = start + len;
- }
-
- int length() { return ptr - start; }
- void reposition(int pos) { ptr = start + pos; }
- const void* data() { return (const void*)start; }
-
- private:
-
- int overrun(int itemSize, int nItems) { throw EndOfStream(); }
- U8* start;
- };
-
-}
-
-#endif
--
2.16.4
++++++ 0010-Use-size_t-for-lengths-in-stream-objects.patch ++++++
++++ 1406 lines (skipped)
++++++ 0011-Be-defensive-about-overflows-in-stream-objects.patch ++++++
>From 75e6e0653a48baf474fd45d78b1da53e2f324642 Mon Sep 17 00:00:00 2001
From: Pierre Ossman <oss...@cendio.se>
Date: Tue, 24 Sep 2019 09:41:07 +0200
Subject: [PATCH] Be defensive about overflows in stream objects
We use a lot of lengths given to us over the network, so be more
paranoid about them causing an overflow as otherwise an attacker
might trick us in to overwriting other memory.
This primarily affects the client which often gets lengths from the
server, but there are also some scenarios where the server might
theoretically be vulnerable.
Issue found by Pavel Cheremushkin from Kaspersky Lab.
---
common/rdr/FdInStream.cxx | 8 +++++---
common/rdr/FdOutStream.cxx | 7 ++++---
common/rdr/FileInStream.cxx | 8 +++++---
common/rdr/HexInStream.cxx | 8 +++++---
common/rdr/HexOutStream.cxx | 6 ++++--
common/rdr/InStream.h | 24 +++++++++++++-----------
common/rdr/MemOutStream.h | 4 ++++
common/rdr/OutStream.h | 24 +++++++++++++-----------
common/rdr/RandomStream.cxx | 6 ++++--
common/rdr/TLSInStream.cxx | 10 ++++++----
common/rdr/TLSOutStream.cxx | 6 ++++--
common/rdr/ZlibInStream.cxx | 6 ++++--
common/rdr/ZlibOutStream.cxx | 6 ++++--
13 files changed, 75 insertions(+), 48 deletions(-)
diff --git a/common/rdr/FdInStream.cxx b/common/rdr/FdInStream.cxx
index 9e84ab7a..1730d6d1 100644
--- a/common/rdr/FdInStream.cxx
+++ b/common/rdr/FdInStream.cxx
@@ -136,7 +136,7 @@ size_t FdInStream::overrun(size_t itemSize, size_t nItems,
bool wait)
ptr = start;
size_t bytes_to_read;
- while (end < start + itemSize) {
+ while ((size_t)(end - start) < itemSize) {
bytes_to_read = start + bufSize - end;
if (!timing) {
// When not timing, we must be careful not to read too much
@@ -152,8 +152,10 @@ size_t FdInStream::overrun(size_t itemSize, size_t nItems,
bool wait)
end += n;
}
- if (itemSize * nItems > (size_t)(end - ptr))
- nItems = (end - ptr) / itemSize;
+ size_t nAvail;
+ nAvail = (end - ptr) / itemSize;
+ if (nAvail < nItems)
+ return nAvail;
return nItems;
}
diff --git a/common/rdr/FdOutStream.cxx b/common/rdr/FdOutStream.cxx
index 1757dc35..f5d07e4b 100644
--- a/common/rdr/FdOutStream.cxx
+++ b/common/rdr/FdOutStream.cxx
@@ -149,9 +149,10 @@ size_t FdOutStream::overrun(size_t itemSize, size_t nItems)
}
}
- // Can we fit all the items asked for?
- if (itemSize * nItems > (size_t)(end - ptr))
- nItems = (end - ptr) / itemSize;
+ size_t nAvail;
+ nAvail = (end - ptr) / itemSize;
+ if (nAvail < nItems)
+ return nAvail;
return nItems;
}
diff --git a/common/rdr/FileInStream.cxx b/common/rdr/FileInStream.cxx
index 94f5db88..bdb05a3a 100644
--- a/common/rdr/FileInStream.cxx
+++ b/common/rdr/FileInStream.cxx
@@ -68,7 +68,7 @@ size_t FileInStream::overrun(size_t itemSize, size_t nItems,
bool wait)
ptr = b;
- while (end < b + itemSize) {
+ while ((size_t)(end - b) < itemSize) {
size_t n = fread((U8 *)end, b + sizeof(b) - end, 1, file);
if (n == 0) {
if (ferror(file))
@@ -80,8 +80,10 @@ size_t FileInStream::overrun(size_t itemSize, size_t nItems,
bool wait)
end += b + sizeof(b) - end;
}
- if (itemSize * nItems > (size_t)(end - ptr))
- nItems = (end - ptr) / itemSize;
+ size_t nAvail;
+ nAvail = (end - ptr) / itemSize;
+ if (nAvail < nItems)
+ return nAvail;
return nItems;
}
diff --git a/common/rdr/HexInStream.cxx b/common/rdr/HexInStream.cxx
index 8f939889..a6bc92cd 100644
--- a/common/rdr/HexInStream.cxx
+++ b/common/rdr/HexInStream.cxx
@@ -91,7 +91,7 @@ size_t HexInStream::overrun(size_t itemSize, size_t nItems,
bool wait) {
offset += ptr - start;
ptr = start;
- while (end < ptr + itemSize) {
+ while ((size_t)(end - ptr) < itemSize) {
size_t n = in_stream.check(2, 1, wait);
if (n == 0) return 0;
const U8* iptr = in_stream.getptr();
@@ -110,8 +110,10 @@ size_t HexInStream::overrun(size_t itemSize, size_t
nItems, bool wait) {
end += length;
}
- if (itemSize * nItems > (size_t)(end - ptr))
- nItems = (end - ptr) / itemSize;
+ size_t nAvail;
+ nAvail = (end - ptr) / itemSize;
+ if (nAvail < nItems)
+ return nAvail;
return nItems;
}
diff --git a/common/rdr/HexOutStream.cxx b/common/rdr/HexOutStream.cxx
index 7232514c..eac2eff8 100644
--- a/common/rdr/HexOutStream.cxx
+++ b/common/rdr/HexOutStream.cxx
@@ -102,8 +102,10 @@ HexOutStream::overrun(size_t itemSize, size_t nItems) {
writeBuffer();
- if (itemSize * nItems > (size_t)(end - ptr))
- nItems = (end - ptr) / itemSize;
+ size_t nAvail;
+ nAvail = (end - ptr) / itemSize;
+ if (nAvail < nItems)
+ return nAvail;
return nItems;
}
diff --git a/common/rdr/InStream.h b/common/rdr/InStream.h
index 14ecf093..f71a4d9e 100644
--- a/common/rdr/InStream.h
+++ b/common/rdr/InStream.h
@@ -43,12 +43,15 @@ namespace rdr {
inline size_t check(size_t itemSize, size_t nItems=1, bool wait=true)
{
- if (ptr + itemSize * nItems > end) {
- if (ptr + itemSize > end)
- return overrun(itemSize, nItems, wait);
+ size_t nAvail;
+
+ if (itemSize > (size_t)(end - ptr))
+ return overrun(itemSize, nItems, wait);
+
+ nAvail = (end - ptr) / itemSize;
+ if (nAvail < nItems)
+ return nAvail;
- nItems = (end - ptr) / itemSize;
- }
return nItems;
}
@@ -93,13 +96,12 @@ namespace rdr {
// readBytes() reads an exact number of bytes.
void readBytes(void* data, size_t length) {
- U8* dataPtr = (U8*)data;
- U8* dataEnd = dataPtr + length;
- while (dataPtr < dataEnd) {
- size_t n = check(1, dataEnd - dataPtr);
- memcpy(dataPtr, ptr, n);
+ while (length > 0) {
+ size_t n = check(1, length);
+ memcpy(data, ptr, n);
ptr += n;
- dataPtr += n;
+ data = (U8*)data + n;
+ length -= n;
}
}
diff --git a/common/rdr/MemOutStream.h b/common/rdr/MemOutStream.h
index 4a815b30..b56bac3a 100644
--- a/common/rdr/MemOutStream.h
+++ b/common/rdr/MemOutStream.h
@@ -23,6 +23,7 @@
#ifndef __RDR_MEMOUTSTREAM_H__
#define __RDR_MEMOUTSTREAM_H__
+#include <rdr/Exception.h>
#include <rdr/OutStream.h>
namespace rdr {
@@ -65,6 +66,9 @@ namespace rdr {
if (len < (size_t)(end - start) * 2)
len = (end - start) * 2;
+ if (len < (size_t)(end - start))
+ throw Exception("Overflow in MemOutStream::overrun()");
+
U8* newStart = new U8[len];
memcpy(newStart, start, ptr - start);
ptr = newStart + (ptr - start);
diff --git a/common/rdr/OutStream.h b/common/rdr/OutStream.h
index 11aafd2d..0f60ccc1 100644
--- a/common/rdr/OutStream.h
+++ b/common/rdr/OutStream.h
@@ -46,12 +46,15 @@ namespace rdr {
inline size_t check(size_t itemSize, size_t nItems=1)
{
- if (ptr + itemSize * nItems > end) {
- if (ptr + itemSize > end)
- return overrun(itemSize, nItems);
+ size_t nAvail;
+
+ if (itemSize > (size_t)(end - ptr))
+ return overrun(itemSize, nItems);
+
+ nAvail = (end - ptr) / itemSize;
+ if (nAvail < nItems)
+ return nAvail;
- nItems = (end - ptr) / itemSize;
- }
return nItems;
}
@@ -91,13 +94,12 @@ namespace rdr {
// writeBytes() writes an exact number of bytes.
void writeBytes(const void* data, size_t length) {
- const U8* dataPtr = (const U8*)data;
- const U8* dataEnd = dataPtr + length;
- while (dataPtr < dataEnd) {
- size_t n = check(1, dataEnd - dataPtr);
- memcpy(ptr, dataPtr, n);
+ while (length > 0) {
+ size_t n = check(1, length);
+ memcpy(ptr, data, n);
ptr += n;
- dataPtr += n;
+ data = (U8*)data + n;
+ length -= n;
}
}
diff --git a/common/rdr/RandomStream.cxx b/common/rdr/RandomStream.cxx
index d5f1cc85..1be9b251 100644
--- a/common/rdr/RandomStream.cxx
+++ b/common/rdr/RandomStream.cxx
@@ -126,8 +126,10 @@ size_t RandomStream::overrun(size_t itemSize, size_t
nItems, bool wait) {
*(U8*)end++ = (int) (256.0*rand()/(RAND_MAX+1.0));
}
- if (itemSize * nItems > (size_t)(end - ptr))
- nItems = (end - ptr) / itemSize;
+ size_t nAvail;
+ nAvail = (end - ptr) / itemSize;
+ if (nAvail < nItems)
+ return nAvail;
return nItems;
}
diff --git a/common/rdr/TLSInStream.cxx b/common/rdr/TLSInStream.cxx
index d0f94263..3e1172f1 100644
--- a/common/rdr/TLSInStream.cxx
+++ b/common/rdr/TLSInStream.cxx
@@ -43,7 +43,7 @@ ssize_t TLSInStream::pull(gnutls_transport_ptr_t str, void*
data, size_t size)
return -1;
}
- if (in->getend() - in->getptr() < (ptrdiff_t)size)
+ if ((size_t)(in->getend() - in->getptr()) < size)
size = in->getend() - in->getptr();
in->readBytes(data, size);
@@ -92,15 +92,17 @@ size_t TLSInStream::overrun(size_t itemSize, size_t nItems,
bool wait)
end -= ptr - start;
ptr = start;
- while (end < start + itemSize) {
+ while ((size_t)(end - start) < itemSize) {
size_t n = readTLS((U8*) end, start + bufSize - end, wait);
if (!wait && n == 0)
return 0;
end += n;
}
- if (itemSize * nItems > (size_t)(end - ptr))
- nItems = (end - ptr) / itemSize;
+ size_t nAvail;
+ nAvail = (end - ptr) / itemSize;
+ if (nAvail < nItems)
+ return nAvail;
return nItems;
}
diff --git a/common/rdr/TLSOutStream.cxx b/common/rdr/TLSOutStream.cxx
index 30c456fe..7d7c3b56 100644
--- a/common/rdr/TLSOutStream.cxx
+++ b/common/rdr/TLSOutStream.cxx
@@ -100,8 +100,10 @@ size_t TLSOutStream::overrun(size_t itemSize, size_t
nItems)
flush();
- if (itemSize * nItems > (size_t)(end - ptr))
- nItems = (end - ptr) / itemSize;
+ size_t nAvail;
+ nAvail = (end - ptr) / itemSize;
+ if (nAvail < nItems)
+ return nAvail;
return nItems;
}
diff --git a/common/rdr/ZlibInStream.cxx b/common/rdr/ZlibInStream.cxx
index e2f971c7..9fcfaf6b 100644
--- a/common/rdr/ZlibInStream.cxx
+++ b/common/rdr/ZlibInStream.cxx
@@ -113,8 +113,10 @@ size_t ZlibInStream::overrun(size_t itemSize, size_t
nItems, bool wait)
return 0;
}
- if (itemSize * nItems > (size_t)(end - ptr))
- nItems = (end - ptr) / itemSize;
+ size_t nAvail;
+ nAvail = (end - ptr) / itemSize;
+ if (nAvail < nItems)
+ return nAvail;
return nItems;
}
diff --git a/common/rdr/ZlibOutStream.cxx b/common/rdr/ZlibOutStream.cxx
index 26a11315..7a0d692c 100644
--- a/common/rdr/ZlibOutStream.cxx
+++ b/common/rdr/ZlibOutStream.cxx
@@ -130,8 +130,10 @@ size_t ZlibOutStream::overrun(size_t itemSize, size_t
nItems)
}
}
- if (itemSize * nItems > (size_t)(end - ptr))
- nItems = (end - ptr) / itemSize;
+ size_t nAvail;
+ nAvail = (end - ptr) / itemSize;
+ if (nAvail < nItems)
+ return nAvail;
return nItems;
}
--
2.16.4
++++++ 0012-Add-unit-tests-for-PixelFormat.is888-detection.patch ++++++
>From 91bdaa6c87a7f311163b5f1e4bbcd9de584968cd Mon Sep 17 00:00:00 2001
From: Pierre Ossman <oss...@cendio.se>
Date: Wed, 2 Oct 2019 16:05:34 +0200
Subject: [PATCH] Add unit tests for PixelFormat.is888() detection
---
tests/unit/pixelformat.cxx | 60 +++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 59 insertions(+), 1 deletion(-)
diff --git a/tests/unit/pixelformat.cxx b/tests/unit/pixelformat.cxx
index 46fecfb4..cfae2f9d 100644
--- a/tests/unit/pixelformat.cxx
+++ b/tests/unit/pixelformat.cxx
@@ -52,8 +52,31 @@ static void doTest(bool should_fail, int b, int d, bool e,
bool t,
fflush(stdout);
}
-int main(int argc, char** argv)
+static void do888Test(bool expected, int b, int d, bool e, bool t,
+ int rm, int gm, int bm, int rs, int gs, int bs)
+{
+ rfb::PixelFormat* pf;
+
+ printf("PixelFormat(%d, %d, %s, %s, %d, %d, %d, %d, %d, %d): ",
+ b, d, e ? "true" : "false", t ? "true": "false",
+ rm, gm, bm, rs, gs, bs);
+
+ pf = new rfb::PixelFormat(b, d, e, t, rm, gm, bm, rs, gs, bs);
+
+ if (pf->is888() == expected)
+ printf("OK");
+ else
+ printf("FAILED");
+ printf("\n");
+ fflush(stdout);
+
+ delete pf;
+}
+
+static void sanityTests()
{
+ printf("Sanity checks:\n\n");
+
/* Normal true color formats */
doTest(false, 32, 24, false, true, 255, 255, 255, 0, 8, 16);
@@ -120,5 +143,40 @@ int main(int argc, char** argv)
doTest(true, 32, 24, false, true, 255, 255, 255, 0, 8, 15);
doTest(true, 32, 24, false, true, 255, 255, 255, 0, 16, 7);
+ printf("\n");
+}
+
+void is888Tests()
+{
+ printf("Simple format detection:\n\n");
+
+ /* Positive cases */
+
+ do888Test(true, 32, 24, false, true, 255, 255, 255, 0, 8, 16);
+ do888Test(true, 32, 24, false, true, 255, 255, 255, 24, 16, 8);
+ do888Test(true, 32, 24, false, true, 255, 255, 255, 24, 8, 0);
+
+ /* Low depth */
+
+ do888Test(false, 32, 16, false, true, 15, 31, 15, 0, 8, 16);
+ do888Test(false, 32, 8, false, true, 3, 7, 3, 0, 8, 16);
+
+ /* Low bpp and depth */
+
+ do888Test(false, 16, 16, false, true, 15, 31, 15, 0, 5, 11);
+ do888Test(false, 8, 8, false, true, 3, 7, 3, 0, 2, 5);
+
+ /* Colour map */
+
+ do888Test(false, 8, 8, false, false, 0, 0, 0, 0, 0, 0);
+
+ printf("\n");
+}
+
+int main(int argc, char** argv)
+{
+ sanityTests();
+ is888Tests();
+
return 0;
}
--
2.16.4
++++++ 0013-Handle-pixel-formats-with-odd-shift-values.patch ++++++
>From 05e28490873a861379c943bf616614b78b558b89 Mon Sep 17 00:00:00 2001
From: Pierre Ossman <oss...@cendio.se>
Date: Wed, 2 Oct 2019 16:06:08 +0200
Subject: [PATCH] Handle pixel formats with odd shift values
Our fast paths assume that each channel fits in to a separate byte.
That means the shift needs to be a multiple of 8. Start actually
checking this so that a client cannot trip us up and possibly cause
incorrect code exection.
Issue found by Pavel Cheremushkin from Kaspersky Lab.
---
common/rfb/PixelFormat.cxx | 6 ++++++
tests/unit/pixelformat.cxx | 6 ++++++
2 files changed, 12 insertions(+)
diff --git a/common/rfb/PixelFormat.cxx b/common/rfb/PixelFormat.cxx
index 789c43ed..1b4ab1ba 100644
--- a/common/rfb/PixelFormat.cxx
+++ b/common/rfb/PixelFormat.cxx
@@ -206,6 +206,12 @@ bool PixelFormat::is888(void) const
return false;
if (blueMax != 255)
return false;
+ if ((redShift & 0x7) != 0)
+ return false;
+ if ((greenShift & 0x7) != 0)
+ return false;
+ if ((blueShift & 0x7) != 0)
+ return false;
return true;
}
diff --git a/tests/unit/pixelformat.cxx b/tests/unit/pixelformat.cxx
index cfae2f9d..2e0c0bbb 100644
--- a/tests/unit/pixelformat.cxx
+++ b/tests/unit/pixelformat.cxx
@@ -170,6 +170,12 @@ void is888Tests()
do888Test(false, 8, 8, false, false, 0, 0, 0, 0, 0, 0);
+ /* Odd shifts */
+
+ do888Test(false, 32, 24, false, true, 255, 255, 255, 0, 8, 18);
+ do888Test(false, 32, 24, false, true, 255, 255, 255, 0, 11, 24);
+ do888Test(false, 32, 24, false, true, 255, 255, 255, 4, 16, 24);
+
printf("\n");
}
--
2.16.4
