CVE-2017-11721: Improper Restriction of Operations within the Bounds of a Memory Buffer (CWE-119)
value: Buffer overflow in ioquake3 before 2017-08-02 allows remote attackers to cause a denial of service (application crash) or possibly have unspecified other impact via a crafted packet.
CWE description: The product performs operations on a memory buffer, but it reads from or writes to a memory location outside the buffer's intended boundary. This may result in read or write operations on unexpected memory locations that could be linked to other variables, data structures, or internal program data.
CVSS v3 base score: 9.8
Published: 2017-08-03T08:29Z
Repository: https://github.com/ioquake/ioq3
Language: C
Files changed: code/qcommon/huffman.c, code/qcommon/msg.c, code/qcommon/qcommon.h
Commit date: 2017-08-02 14:55:22 -0500
Commit message: Fix/improve buffer overflow in MSG_ReadBits/MSG_WriteBits

Prevent reading past end of message in MSG_ReadBits. If read past
end of msg->data buffer (16348 bytes) the engine could SEGFAULT.
Make MSG_WriteBits use an exact buffer overflow check instead of
possibly failing with a few bytes left.

Diff: diff --git a/code/qcommon/huffman.c b/code/qcommon/huffman.c
index c1b9f242..1f424984 100644
--- a/code/qcommon/huffman.c
+++ b/code/qcommon/huffman.c
@@ -277,13 +277,18 @@ int Huff_Receive (node_t *node, int *ch, byte *fin) {
 	}
 	return (*ch = node->symbol);
 }
 
 /* Get a symbol */
-void Huff_offsetReceive (node_t *node, int *ch, byte *fin, int *offset) {
+void Huff_offsetReceive (node_t *node, int *ch, byte *fin, int *offset, int maxoffset) {
 	bloc = *offset;
 	while (node && node->symbol == INTERNAL_NODE) {
+		if (bloc >= maxoffset) {
+			*ch = 0;
+			*offset = maxoffset + 1;
+			return;
+		}
 		if (get_bit(fin)) {
 			node = node->right;
 		} else {
 			node = node->left;
 		}
@@ -296,40 +301,44 @@ void Huff_offsetReceive (node_t *node, int *ch, byte *fin, int *offset) {
 	*ch = node->symbol;
 	*offset = bloc;
 }
 
 /* Send the prefix code for this node */
-static void send(node_t *node, node_t *child, byte *fout) {
+static void send(node_t *node, node_t *child, byte *fout, int maxoffset) {
 	if (node->parent) {
-		send(node->parent, node, fout);
+		send(node->parent, node, fout, maxoffset);
 	}
 	if (child) {
+		if (bloc >= maxoffset) {
+			bloc = maxoffset + 1;
+			return;
+		}
 		if (node->right == child) {
 			add_bit(1, fout);
 		} else {
 			add_bit(0, fout);
 		}
 	}
 }
 
 /* Send a symbol */
-void Huff_transmit (huff_t *huff, int ch, byte *fout) {
+void Huff_transmit (huff_t *huff, int ch, byte *fout, int maxoffset) {
 	int i;
 	if (huff->loc[ch] == NULL) { 
 		/* node_t hasn't been transmitted, send a NYT, then the symbol */
-		Huff_transmit(huff, NYT, fout);
+		Huff_transmit(huff, NYT, fout, maxoffset);
 		for (i = 7; i >= 0; i--) {
 			add_bit((char)((ch >> i) & 0x1), fout);
 		}
 	} else {
-		send(huff->loc[ch], NULL, fout);
+		send(huff->loc[ch], NULL, fout, maxoffset);
 	}
 }
 
-void Huff_offsetTransmit (huff_t *huff, int ch, byte *fout, int *offset) {
+void Huff_offsetTransmit (huff_t *huff, int ch, byte *fout, int *offset, int maxoffset) {
 	bloc = *offset;
-	send(huff->loc[ch], NULL, fout);
+	send(huff->loc[ch], NULL, fout, maxoffset);
 	*offset = bloc;
 }
 
 void Huff_Decompress(msg_t *mbuf, int offset) {
 	int			ch, cch, i, j, size;
@@ -411,11 +420,11 @@ void Huff_Compress(msg_t *mbuf, int offset) {
 
 	bloc = 16;
 
 	for (i=0; i<size; i++ ) {
 		ch = buffer[i];
-		Huff_transmit(&huff, ch, seq);						/* Transmit symbol */
+		Huff_transmit(&huff, ch, seq, size<<3);						/* Transmit symbol */
 		Huff_addRef(&huff, (byte)ch);								/* Do update */
 	}
 
 	bloc += 8;												// next byte
 
diff --git a/code/qcommon/msg.c b/code/qcommon/msg.c
index 20dec91d..eb767be4 100644
--- a/code/qcommon/msg.c
+++ b/code/qcommon/msg.c
@@ -105,13 +105,11 @@ bit functions
 void MSG_WriteBits( msg_t *msg, int value, int bits ) {
 	int	i;
 
 	oldsize += bits;
 
-	// this isn't an exact overflow check, but close enough
-	if ( msg->maxsize - msg->cursize < 4 ) {
-		msg->overflowed = qtrue;
+	if ( msg->overflowed ) {
 		return;
 	}
 
 	if ( bits == 0 || bits < -31 || bits > 32 ) {
 		Com_Error( ERR_DROP, "MSG_WriteBits: bad bits %i", bits );
@@ -120,10 +118,15 @@ void MSG_WriteBits( msg_t *msg, int value, int bits ) {
 	if ( bits < 0 ) {
 		bits = -bits;
 	}
 
 	if ( msg->oob ) {
+		if ( msg->cursize + ( bits >> 3 ) > msg->maxsize ) {
+			msg->overflowed = qtrue;
+			return;
+		}
+
 		if ( bits == 8 ) {
 			msg->data[msg->cursize] = value;
 			msg->cursize += 1;
 			msg->bit += 8;
 		} else if ( bits == 16 ) {
@@ -142,20 +145,29 @@ void MSG_WriteBits( msg_t *msg, int value, int bits ) {
 	} else {
 		value &= (0xffffffff >> (32 - bits));
 		if ( bits&7 ) {
 			int nbits;
 			nbits = bits&7;
+			if ( msg->bit + nbits > msg->maxsize << 3 ) {
+				msg->overflowed = qtrue;
+				return;
+			}
 			for( i = 0; i < nbits; i++ ) {
 				Huff_putBit( (value & 1), msg->data, &msg->bit );
 				value = (value >> 1);
 			}
 			bits = bits - nbits;
 		}
 		if ( bits ) {
 			for( i = 0; i < bits; i += 8 ) {
-				Huff_offsetTransmit( &msgHuff.compressor, (value & 0xff), msg->data, &msg->bit );
+				Huff_offsetTransmit( &msgHuff.compressor, (value & 0xff), msg->data, &msg->bit, msg->maxsize << 3 );
 				value = (value >> 8);
+
+				if ( msg->bit > msg->maxsize << 3 ) {
+					msg->overflowed = qtrue;
+					return;
+				}
 			}
 		}
 		msg->cursize = (msg->bit >> 3) + 1;
 	}
 }
@@ -165,20 +177,29 @@ int MSG_ReadBits( msg_t *msg, int bits ) {
 	int			get;
 	qboolean	sgn;
 	int			i, nbits;
 //	FILE*	fp;
 
+	if ( msg->readcount > msg->cursize ) {
+		return 0;
+	}
+
 	value = 0;
 
 	if ( bits < 0 ) {
 		bits = -bits;
 		sgn = qtrue;
 	} else {
 		sgn = qfalse;
 	}
 
 	if (msg->oob) {
+		if (msg->readcount + (bits>>3) > msg->cursize) {
+			msg->readcount = msg->cursize + 1;
+			return 0;
+		}
+
 		if(bits==8)
 		{
 			value = msg->data[msg->readcount];
 			msg->readcount += 1;
 			msg->bit += 8;
@@ -202,21 +223,30 @@ int MSG_ReadBits( msg_t *msg, int bits ) {
 			Com_Error(ERR_DROP, "can't read %d bits", bits);
 	} else {
 		nbits = 0;
 		if (bits&7) {
 			nbits = bits&7;
+			if (msg->bit + nbits > msg->cursize << 3) {
+				msg->readcount = msg->cursize + 1;
+				return 0;
+			}
 			for(i=0;i<nbits;i++) {
 				value |= (Huff_getBit(msg->data, &msg->bit)<<i);
 			}
 			bits = bits - nbits;
 		}
 		if (bits) {
 //			fp = fopen("c:\\netchan.bin", "a");
 			for(i=0;i<bits;i+=8) {
-				Huff_offsetReceive (msgHuff.decompressor.tree, &get, msg->data, &msg->bit);
+				Huff_offsetReceive (msgHuff.decompressor.tree, &get, msg->data, &msg->bit, msg->cursize<<3);
 //				fwrite(&get, 1, 1, fp);
 				value |= (get<<(i+nbits));
+
+				if (msg->bit > msg->cursize<<3) {
+					msg->readcount = msg->cursize + 1;
+					return 0;
+				}
 			}
 //			fclose(fp);
 		}
 		msg->readcount = (msg->bit>>3)+1;
 	}
diff --git a/code/qcommon/qcommon.h b/code/qcommon/qcommon.h
index 124c393a..07eebe71 100644
--- a/code/qcommon/qcommon.h
+++ b/code/qcommon/qcommon.h
@@ -1194,13 +1194,13 @@ typedef struct {
 void	Huff_Compress(msg_t *buf, int offset);
 void	Huff_Decompress(msg_t *buf, int offset);
 void	Huff_Init(huffman_t *huff);
 void	Huff_addRef(huff_t* huff, byte ch);
 int		Huff_Receive (node_t *node, int *ch, byte *fin);
-void	Huff_transmit (huff_t *huff, int ch, byte *fout);
-void	Huff_offsetReceive (node_t *node, int *ch, byte *fin, int *offset);
-void	Huff_offsetTransmit (huff_t *huff, int ch, byte *fout, int *offset);
+void	Huff_transmit (huff_t *huff, int ch, byte *fout, int maxoffset);
+void	Huff_offsetReceive (node_t *node, int *ch, byte *fin, int *offset, int maxoffset);
+void	Huff_offsetTransmit (huff_t *huff, int ch, byte *fout, int *offset, int maxoffset);
 void	Huff_putBit( int bit, byte *fout, int *offset);
 int		Huff_getBit( byte *fout, int *offset);
 
 // don't use if you don't know what you're doing.
 int		Huff_getBloc(void);

Vulnerable code: void Huff_offsetReceive (node_t *node, int *ch, byte *fin, int *offset) {
static void send(node_t *node, node_t *child, byte *fout) {
		send(node->parent, node, fout);
void Huff_transmit (huff_t *huff, int ch, byte *fout) {
		Huff_transmit(huff, NYT, fout);
		send(huff->loc[ch], NULL, fout);
void Huff_offsetTransmit (huff_t *huff, int ch, byte *fout, int *offset) {
	send(huff->loc[ch], NULL, fout);
		Huff_transmit(&huff, ch, seq);						/* Transmit symbol */
	// this isn't an exact overflow check, but close enough
	if ( msg->maxsize - msg->cursize < 4 ) {
		msg->overflowed = qtrue;
				Huff_offsetTransmit( &msgHuff.compressor, (value & 0xff), msg->data, &msg->bit );
				Huff_offsetReceive (msgHuff.decompressor.tree, &get, msg->data, &msg->bit);
void	Huff_transmit (huff_t *huff, int ch, byte *fout);
void	Huff_offsetReceive (node_t *node, int *ch, byte *fin, int *offset);
void	Huff_offsetTransmit (huff_t *huff, int ch, byte *fout, int *offset);
Fixed code: void Huff_offsetReceive (node_t *node, int *ch, byte *fin, int *offset, int maxoffset) {
		if (bloc >= maxoffset) {
			*ch = 0;
			*offset = maxoffset + 1;
			return;
		}
static void send(node_t *node, node_t *child, byte *fout, int maxoffset) {
		send(node->parent, node, fout, maxoffset);
		if (bloc >= maxoffset) {
			bloc = maxoffset + 1;
			return;
		}
void Huff_transmit (huff_t *huff, int ch, byte *fout, int maxoffset) {
		Huff_transmit(huff, NYT, fout, maxoffset);
		send(huff->loc[ch], NULL, fout, maxoffset);
void Huff_offsetTransmit (huff_t *huff, int ch, byte *fout, int *offset, int maxoffset) {
	send(huff->loc[ch], NULL, fout, maxoffset);
		Huff_transmit(&huff, ch, seq, size<<3);						/* Transmit symbol */
	if ( msg->overflowed ) {
		if ( msg->cursize + ( bits >> 3 ) > msg->maxsize ) {
			msg->overflowed = qtrue;
			return;
		}

			if ( msg->bit + nbits > msg->maxsize << 3 ) {
				msg->overflowed = qtrue;
				return;
			}
				Huff_offsetTransmit( &msgHuff.compressor, (value & 0xff), msg->data, &msg->bit, msg->maxsize << 3 );

				if ( msg->bit > msg->maxsize << 3 ) {
					msg->overflowed = qtrue;
					return;
				}
	if ( msg->readcount > msg->cursize ) {
		return 0;
	}

		if (msg->readcount + (bits>>3) > msg->cursize) {
			msg->readcount = msg->cursize + 1;
			return 0;
		}

			if (msg->bit + nbits > msg->cursize << 3) {
				msg->readcount = msg->cursize + 1;
				return 0;
			}
				Huff_offsetReceive (msgHuff.decompressor.tree, &get, msg->data, &msg->bit, msg->cursize<<3);

				if (msg->bit > msg->cursize<<3) {
					msg->readcount = msg->cursize + 1;
					return 0;
				}
void	Huff_transmit (huff_t *huff, int ch, byte *fout, int maxoffset);
void	Huff_offsetReceive (node_t *node, int *ch, byte *fin, int *offset, int maxoffset);
void	Huff_offsetTransmit (huff_t *huff, int ch, byte *fout, int *offset, int maxoffset);