Commit aeab07d5 authored by CentOS Sources's avatar CentOS Sources
Browse files

import tigervnc-1.9.0-14.el8_1

parent e35c07e9
From d61a767d6842b530ffb532ddd5a3d233119aad40 Mon Sep 17 00:00:00 2001
From: Pierre Ossman <ossman@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 4053bd1..a361010 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 6bd4da4..86ba1ff 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/TightDecoder.cxx b/common/rfb/TightDecoder.cxx
index 3a1254a..4273eb7 100644
--- a/common/rfb/TightDecoder.cxx
+++ b/common/rfb/TightDecoder.cxx
@@ -340,7 +340,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 0bfbbe1..a69ca67 100644
--- a/common/rfb/zrleDecode.h
+++ b/common/rfb/zrleDecode.h
@@ -178,7 +178,8 @@ void ZRLE_DECODE (const Rect& r, rdr::InStream* is,
}
}
- zis->removeUnderlying();
+ zis->flushUnderlying();
+ zis->setUnderlying(NULL, 0);
}
#undef ZRLE_DECODE
From 996356b6c65ca165ee1ea46a571c32a1dc3c3821 Mon Sep 17 00:00:00 2001
From: Pierre Ossman <ossman@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 ad58324..18f41f8 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;
}
@@ -337,6 +350,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_;
From b4ada8d0c6dac98c8b91fc64d112569a8ae5fb95 Mon Sep 17 00:00:00 2001
From: Pierre Ossman <ossman@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 b6e86ed5e..8f77aebd0 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) {
This diff is collapsed.
diff --git a/common/rfb/PixelFormat.cxx b/common/rfb/PixelFormat.cxx
index a9d015d..896f4e5 100644
--- a/common/rfb/PixelFormat.cxx
+++ b/common/rfb/PixelFormat.cxx
@@ -200,6 +200,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;
}
From 9f615301aba1cc54a749950bf9462c5a85217bc4 Mon Sep 17 00:00:00 2001
From: Pierre Ossman <ossman@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 1653cea..66ba432 100644
--- a/common/rfb/EncodeManager.cxx
+++ b/common/rfb/EncodeManager.cxx
@@ -24,6 +24,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>
@@ -895,6 +896,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 79db950..7d47420 100644
--- a/common/rfb/EncodeManager.h
+++ b/common/rfb/EncodeManager.h
@@ -124,6 +124,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;
From 75e6e0653a48baf474fd45d78b1da53e2f324642 Mon Sep 17 00:00:00 2001
From: Pierre Ossman <ossman@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 789cbec..406ece5 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 1757dc3..f5d07e4 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 94f5db8..bdb05a3 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 8f93988..a6bc92c 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 7232514..eac2eff 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 14ecf09..f71a4d9 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 4a815b3..b56bac3 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 11aafd2..0f60ccc 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 7681095..6c64ac5 100644
--- a/common/rdr/RandomStream.cxx
+++ b/common/rdr/RandomStream.cxx
@@ -123,8 +123,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 d0f9426..3e1172f 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;
<