CVE-2018-8785: Out-of-bounds Write (CWE-787)
value: FreeRDP prior to version 2.0.0-rc4 contains a Heap-Based Buffer Overflow in function zgfx_decompress() that results in a memory corruption and probably even a remote code execution.
CWE description: The product writes data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 9.8
Published: 2018-11-29T18:29Z
Repository: https://github.com/FreeRDP/FreeRDP
Language: C
Files changed: include/freerdp/codec/zgfx.h, libfreerdp/codec/zgfx.c
Commit date: 2018-11-20 11:08:31 +0100
Commit message: Fixed CVE-2018-8785

Thanks to Eyal Itkin from Check Point Software Technologies.

Diff: diff --git a/include/freerdp/codec/zgfx.h b/include/freerdp/codec/zgfx.h
index b5c65319c..b0faf6e0d 100644
--- a/include/freerdp/codec/zgfx.h
+++ b/include/freerdp/codec/zgfx.h
@@ -30,38 +30,22 @@
 
 #define ZGFX_PACKET_COMPR_TYPE_RDP8		0x04
 
 #define ZGFX_SEGMENTED_MAXSIZE			65535
 
-struct _ZGFX_CONTEXT
-{
-	BOOL Compressor;
-
-	const BYTE* pbInputCurrent;
-	const BYTE* pbInputEnd;
-
-	UINT32 bits;
-	UINT32 cBitsRemaining;
-	UINT32 BitsCurrent;
-	UINT32 cBitsCurrent;
-
-	BYTE OutputBuffer[65536];
-	UINT32 OutputCount;
-
-	BYTE HistoryBuffer[2500000];
-	UINT32 HistoryIndex;
-	UINT32 HistoryBufferSize;
-};
 typedef struct _ZGFX_CONTEXT ZGFX_CONTEXT;
 
 #ifdef __cplusplus
 extern "C" {
 #endif
 
-FREERDP_API int zgfx_decompress(ZGFX_CONTEXT* zgfx, const BYTE* pSrcData, UINT32 SrcSize, BYTE** ppDstData, UINT32* pDstSize, UINT32 flags);
-FREERDP_API int zgfx_compress(ZGFX_CONTEXT* zgfx, const BYTE* pSrcData, UINT32 SrcSize, BYTE** ppDstData, UINT32* pDstSize, UINT32* pFlags);
-FREERDP_API int zgfx_compress_to_stream(ZGFX_CONTEXT* zgfx, wStream* sDst, const BYTE* pUncompressed, UINT32 uncompressedSize, UINT32* pFlags);
+FREERDP_API int zgfx_decompress(ZGFX_CONTEXT* zgfx, const BYTE* pSrcData, UINT32 SrcSize,
+                                BYTE** ppDstData, UINT32* pDstSize, UINT32 flags);
+FREERDP_API int zgfx_compress(ZGFX_CONTEXT* zgfx, const BYTE* pSrcData, UINT32 SrcSize,
+                              BYTE** ppDstData, UINT32* pDstSize, UINT32* pFlags);
+FREERDP_API int zgfx_compress_to_stream(ZGFX_CONTEXT* zgfx, wStream* sDst,
+                                        const BYTE* pUncompressed, UINT32 uncompressedSize, UINT32* pFlags);
 
 FREERDP_API void zgfx_context_reset(ZGFX_CONTEXT* zgfx, BOOL flush);
 
 FREERDP_API ZGFX_CONTEXT* zgfx_context_new(BOOL Compressor);
 FREERDP_API void zgfx_context_free(ZGFX_CONTEXT* zgfx);
@@ -69,6 +53,6 @@ FREERDP_API void zgfx_context_free(ZGFX_CONTEXT* zgfx);
 #ifdef __cplusplus
 }
 #endif
 
 #endif /* FREERDP_CODEC_ZGFX_H */
- 
+
diff --git a/libfreerdp/codec/zgfx.c b/libfreerdp/codec/zgfx.c
index 41bcb8498..f94c41b20 100644
--- a/libfreerdp/codec/zgfx.c
+++ b/libfreerdp/codec/zgfx.c
@@ -42,18 +42,38 @@
  * Minimum match length: 3 bytes
  */
 
 struct _ZGFX_TOKEN
 {
-	int prefixLength;
-	int prefixCode;
-	int valueBits;
-	int tokenType;
+	UINT32 prefixLength;
+	UINT32 prefixCode;
+	UINT32 valueBits;
+	UINT32 tokenType;
 	UINT32 valueBase;
 };
 typedef struct _ZGFX_TOKEN ZGFX_TOKEN;
 
+struct _ZGFX_CONTEXT
+{
+	BOOL Compressor;
+
+	const BYTE* pbInputCurrent;
+	const BYTE* pbInputEnd;
+
+	UINT32 bits;
+	UINT32 cBitsRemaining;
+	UINT32 BitsCurrent;
+	UINT32 cBitsCurrent;
+
+	BYTE OutputBuffer[65536];
+	UINT32 OutputCount;
+
+	BYTE HistoryBuffer[2500000];
+	UINT32 HistoryIndex;
+	UINT32 HistoryBufferSize;
+};
+
 static const ZGFX_TOKEN ZGFX_TOKEN_TABLE[] =
 {
 	// len code vbits type  vbase
 	{  1,    0,   8,   0,           0 },    // 0
 	{  5,   17,   5,   1,           0 },    // 10001
@@ -96,21 +116,30 @@ static const ZGFX_TOKEN ZGFX_TOKEN_TABLE[] =
 	{  9,  381,  23,   1,     8705696 },    // 101111101
 	{  9,  382,  24,   1,    17094304 },    // 101111110
 	{ 0 }
 };
 
-#define zgfx_GetBits(_zgfx, _nbits) \
-	while (_zgfx->cBitsCurrent < _nbits) { \
-		_zgfx->BitsCurrent <<= 8; \
-		if (_zgfx->pbInputCurrent < _zgfx->pbInputEnd) \
-			_zgfx->BitsCurrent += *(_zgfx->pbInputCurrent)++; \
-		_zgfx->cBitsCurrent += 8; \
-	} \
-	_zgfx->cBitsRemaining -= _nbits; \
-	_zgfx->cBitsCurrent -= _nbits; \
-	_zgfx->bits = _zgfx->BitsCurrent >> _zgfx->cBitsCurrent; \
+static INLINE BOOL zgfx_GetBits(ZGFX_CONTEXT* _zgfx, UINT32 _nbits)
+{
+	if (!_zgfx)
+		return FALSE;
+
+	while (_zgfx->cBitsCurrent < _nbits)
+	{
+		_zgfx->BitsCurrent <<= 8;
+
+		if (_zgfx->pbInputCurrent < _zgfx->pbInputEnd)
+			_zgfx->BitsCurrent += *(_zgfx->pbInputCurrent)++;
+
+		_zgfx->cBitsCurrent += 8;
+	}
+
+	_zgfx->cBitsRemaining -= _nbits;
+	_zgfx->cBitsCurrent -= _nbits;
+	_zgfx->bits = _zgfx->BitsCurrent >> _zgfx->cBitsCurrent;
 	_zgfx->BitsCurrent &= ((1 << _zgfx->cBitsCurrent) - 1);
+}
 
 static void zgfx_history_buffer_ring_write(ZGFX_CONTEXT* zgfx, const BYTE* src, size_t count)
 {
 	UINT32 front;
 
@@ -191,11 +220,11 @@ static void zgfx_history_buffer_ring_read(ZGFX_CONTEXT* zgfx, int offset, BYTE*
 
 static BOOL zgfx_decompress_segment(ZGFX_CONTEXT* zgfx, wStream* stream, size_t segmentSize)
 {
 	BYTE c;
 	BYTE flags;
-	int extra;
+	UINT32 extra = 0;
 	int opIndex;
 	int haveBits;
 	int inPrefix;
 	UINT32 count;
 	UINT32 distance;
@@ -315,12 +344,12 @@ static BOOL zgfx_decompress_segment(ZGFX_CONTEXT* zgfx, wStream* stream, size_t
 int zgfx_decompress(ZGFX_CONTEXT* zgfx, const BYTE* pSrcData, UINT32 SrcSize, BYTE** ppDstData,
                     UINT32* pDstSize, UINT32 flags)
 {
 	int status = -1;
 	BYTE descriptor;
-
 	wStream* stream = Stream_New((BYTE*)pSrcData, SrcSize);
+
 	if (!stream)
 		return -1;
 
 	if (Stream_GetRemainingLength(stream) < 1)
 		goto fail;
@@ -331,10 +360,11 @@ int zgfx_decompress(ZGFX_CONTEXT* zgfx, const BYTE* pSrcData, UINT32 SrcSize, BY
 	{
 		if (!zgfx_decompress_segment(zgfx, stream, Stream_GetRemainingLength(stream)))
 			goto fail;
 
 		*ppDstData = NULL;
+
 		if (zgfx->OutputCount > 0)
 			*ppDstData = (BYTE*) malloc(zgfx->OutputCount);
 
 		if (!*ppDstData)
 			goto fail;
@@ -347,10 +377,11 @@ int zgfx_decompress(ZGFX_CONTEXT* zgfx, const BYTE* pSrcData, UINT32 SrcSize, BY
 		UINT32 segmentSize;
 		UINT16 segmentNumber;
 		UINT16 segmentCount;
 		UINT32 uncompressedSize;
 		BYTE* pConcatenated;
+		size_t used = 0;
 
 		if (Stream_GetRemainingLength(stream) < 6)
 			goto fail;
 
 		Stream_Read_UINT16(stream, segmentCount); /* segmentCount (2 bytes) */
@@ -375,12 +406,19 @@ int zgfx_decompress(ZGFX_CONTEXT* zgfx, const BYTE* pSrcData, UINT32 SrcSize, BY
 			Stream_Read_UINT32(stream, segmentSize); /* segmentSize (4 bytes) */
 
 			if (!zgfx_decompress_segment(zgfx, stream, segmentSize))
 				goto fail;
 
+			if (zgfx->OutputCount > UINT32_MAX - used)
+				goto fail;
+
+			if (used + zgfx->OutputCount > uncompressedSize)
+				goto fail;
+
 			CopyMemory(pConcatenated, zgfx->OutputBuffer, zgfx->OutputCount);
 			pConcatenated += zgfx->OutputCount;
+			used += zgfx->OutputCount;
 		}
 	}
 	else
 	{
 		goto fail;

Vulnerable code: struct _ZGFX_CONTEXT
{
	BOOL Compressor;

	const BYTE* pbInputCurrent;
	const BYTE* pbInputEnd;

	UINT32 bits;
	UINT32 cBitsRemaining;
	UINT32 BitsCurrent;
	UINT32 cBitsCurrent;

	BYTE OutputBuffer[65536];
	UINT32 OutputCount;

	BYTE HistoryBuffer[2500000];
	UINT32 HistoryIndex;
	UINT32 HistoryBufferSize;
};
FREERDP_API int zgfx_decompress(ZGFX_CONTEXT* zgfx, const BYTE* pSrcData, UINT32 SrcSize, BYTE** ppDstData, UINT32* pDstSize, UINT32 flags);
FREERDP_API int zgfx_compress(ZGFX_CONTEXT* zgfx, const BYTE* pSrcData, UINT32 SrcSize, BYTE** ppDstData, UINT32* pDstSize, UINT32* pFlags);
FREERDP_API int zgfx_compress_to_stream(ZGFX_CONTEXT* zgfx, wStream* sDst, const BYTE* pUncompressed, UINT32 uncompressedSize, UINT32* pFlags);
 
	int prefixLength;
	int prefixCode;
	int valueBits;
	int tokenType;
#define zgfx_GetBits(_zgfx, _nbits) \
	while (_zgfx->cBitsCurrent < _nbits) { \
		_zgfx->BitsCurrent <<= 8; \
		if (_zgfx->pbInputCurrent < _zgfx->pbInputEnd) \
			_zgfx->BitsCurrent += *(_zgfx->pbInputCurrent)++; \
		_zgfx->cBitsCurrent += 8; \
	} \
	_zgfx->cBitsRemaining -= _nbits; \
	_zgfx->cBitsCurrent -= _nbits; \
	_zgfx->bits = _zgfx->BitsCurrent >> _zgfx->cBitsCurrent; \
	int extra;

Fixed code: FREERDP_API int zgfx_decompress(ZGFX_CONTEXT* zgfx, const BYTE* pSrcData, UINT32 SrcSize,
                                BYTE** ppDstData, UINT32* pDstSize, UINT32 flags);
FREERDP_API int zgfx_compress(ZGFX_CONTEXT* zgfx, const BYTE* pSrcData, UINT32 SrcSize,
                              BYTE** ppDstData, UINT32* pDstSize, UINT32* pFlags);
FREERDP_API int zgfx_compress_to_stream(ZGFX_CONTEXT* zgfx, wStream* sDst,
                                        const BYTE* pUncompressed, UINT32 uncompressedSize, UINT32* pFlags);

	UINT32 prefixLength;
	UINT32 prefixCode;
	UINT32 valueBits;
	UINT32 tokenType;
struct _ZGFX_CONTEXT
{
	BOOL Compressor;

	const BYTE* pbInputCurrent;
	const BYTE* pbInputEnd;

	UINT32 bits;
	UINT32 cBitsRemaining;
	UINT32 BitsCurrent;
	UINT32 cBitsCurrent;

	BYTE OutputBuffer[65536];
	UINT32 OutputCount;

	BYTE HistoryBuffer[2500000];
	UINT32 HistoryIndex;
	UINT32 HistoryBufferSize;
};

static INLINE BOOL zgfx_GetBits(ZGFX_CONTEXT* _zgfx, UINT32 _nbits)
{
	if (!_zgfx)
		return FALSE;

	while (_zgfx->cBitsCurrent < _nbits)
	{
		_zgfx->BitsCurrent <<= 8;

		if (_zgfx->pbInputCurrent < _zgfx->pbInputEnd)
			_zgfx->BitsCurrent += *(_zgfx->pbInputCurrent)++;

		_zgfx->cBitsCurrent += 8;
	}

	_zgfx->cBitsRemaining -= _nbits;
	_zgfx->cBitsCurrent -= _nbits;
	_zgfx->bits = _zgfx->BitsCurrent >> _zgfx->cBitsCurrent;
}
	UINT32 extra = 0;


		size_t used = 0;
			if (zgfx->OutputCount > UINT32_MAX - used)
				goto fail;

			if (used + zgfx->OutputCount > uncompressedSize)
				goto fail;

			used += zgfx->OutputCount;