Fix vulnerability related to ACK/NAK handling (CVSS score: 7.2)

Source/DS_RangeList.h

@@ -40,9 +40,9 @@ namespace DataStructures
     {
         if (a<b.minIndex)
             return -1;
-        if (a==b.minIndex)
-            return 0;
-        return 1;
+		if (a>b.maxIndex)
+			return 1;
+        return 0;
     }
 
 	template <class range_type>
@@ -53,12 +53,16 @@ namespace DataStructures
 		~RangeList();
 		void Insert(range_type index);
 		void Clear(void);
+		bool IsWithinRange(range_type value) const;
 		unsigned Size(void) const;
 		unsigned RangeSum(void) const;
 		RakNet::BitSize_t Serialize(RakNet::BitStream *in, RakNet::BitSize_t maxBits, bool clearSerialized);
 		bool Deserialize(RakNet::BitStream *out);
 
 		DataStructures::OrderedList<range_type, RangeNode<range_type> , RangeNodeComp<range_type> > ranges;
+
+	private:
+		bool DeserializeSingleRange(RakNet::BitStream *out, range_type& min, range_type& max);
 	};
 
 	template <class range_type>
@@ -118,28 +122,51 @@ namespace DataStructures
 		unsigned short count;
 		out->AlignReadToByteBoundary();
 		out->Read(count);
-		unsigned short i;
+		range_type absMin;
 		range_type min,max;
-		unsigned char maxEqualToMin=0;
 
-		for (i=0; i < count; i++)
+		if (count == 0) {
+			return true;
+		}
+
+		if (!DeserializeSingleRange(out, min, max)) {
+			return false;
+		}
+		ranges.InsertAtEnd(RangeNode<range_type>(min, max), _FILE_AND_LINE_);
+
+		for (unsigned short i = 1; i < count; i++)
 		{
-			out->Read(maxEqualToMin);
-			if (out->Read(min)==false)
+			absMin = max;
+
+			if (!DeserializeSingleRange(out, min, max)) {
 				return false;
-			if (maxEqualToMin==false)
-			{
-				if (out->Read(max)==false)
-					return false;
-				if (max<min)
-					return false;
 			}
-			else
-				max=min;
+			if (min <= absMin) {
+				return false;
+			}
+			ranges.InsertAtEnd(RangeNode<range_type>(min, max), _FILE_AND_LINE_);
+		}
+		return true;
+	}
 
+	template <class range_type>
+	bool RangeList<range_type>::DeserializeSingleRange(RakNet::BitStream *out, range_type& min, range_type& max)
+	{
+		unsigned char maxEqualToMin;
 
-			ranges.InsertAtEnd(RangeNode<range_type>(min,max), _FILE_AND_LINE_);
+		out->Read(maxEqualToMin);
+		if (out->Read(min) == false)
+			return false;
+		if (maxEqualToMin == false)
+		{
+			if (out->Read(max) == false)
+				return false;
+			if (max < min)
+				return false;
 		}
+		else
+			max = min;
+
 		return true;
 	}
 
@@ -223,6 +250,15 @@ namespace DataStructures
 		ranges.Clear(true, _FILE_AND_LINE_);
 	}
 
+	template <class range_type>
+	bool RangeList<range_type>::IsWithinRange(range_type value) const
+	{
+		bool objectExists;
+		// not interested in the return value
+		(void)ranges.GetIndexFromKey(value, &objectExists);
+		return objectExists;
+	}
+
 	template <class range_type>
 	unsigned RangeList<range_type>::Size(void) const
 	{

Source/ReliabilityLayer.cpp

@@ -733,6 +733,24 @@ bool ReliabilityLayer::HandleSocketReceiveFromConnectedPlayer(
 
 			return false;
 		}
+
+		unsigned int k = 0;
+		while (k < unreliableWithAckReceiptHistory.Size()) {
+			if (incomingAcks.IsWithinRange(unreliableWithAckReceiptHistory[k].datagramNumber)) {
+				InternalPacket *ackReceipt = AllocateFromInternalPacketPool();
+				AllocInternalPacketData(ackReceipt, 5, false, _FILE_AND_LINE_);
+				ackReceipt->dataBitLength = BYTES_TO_BITS(5);
+				ackReceipt->data[0] = (MessageID)ID_SND_RECEIPT_ACKED;
+				memcpy(ackReceipt->data + sizeof(MessageID), &unreliableWithAckReceiptHistory[k].sendReceiptSerial, sizeof(uint32_t));
+				outputQueue.Push(ackReceipt, _FILE_AND_LINE_);
+				// Remove, swap with last
+				unreliableWithAckReceiptHistory.RemoveAtIndex(k);
+			}
+			else {
+				k++;
+			}
+		}
+
 		for (i=0; i<incomingAcks.ranges.Size();i++)
 		{
             if (incomingAcks.ranges[i].minIndex>incomingAcks.ranges[i].maxIndex || (incomingAcks.ranges[i].maxIndex == (uint24_t)(0xFFFFFFFF)))
@@ -745,30 +763,14 @@ bool ReliabilityLayer::HandleSocketReceiveFromConnectedPlayer(
 			}
 			for (datagramNumber=incomingAcks.ranges[i].minIndex; datagramNumber >= incomingAcks.ranges[i].minIndex && datagramNumber <= incomingAcks.ranges[i].maxIndex; datagramNumber++)
 			{
-				CCTimeType whenSent;
-
-				if (unreliableWithAckReceiptHistory.Size()>0)
-				{
-					unsigned int k=0;
-					while (k < unreliableWithAckReceiptHistory.Size())
-					{
-						if (unreliableWithAckReceiptHistory[k].datagramNumber == datagramNumber)
-						{
-							InternalPacket *ackReceipt = AllocateFromInternalPacketPool();
-							AllocInternalPacketData(ackReceipt, 5,  false, _FILE_AND_LINE_ );
-							ackReceipt->dataBitLength=BYTES_TO_BITS(5);
-							ackReceipt->data[0]=(MessageID)ID_SND_RECEIPT_ACKED;
-							memcpy(ackReceipt->data+sizeof(MessageID), &unreliableWithAckReceiptHistory[k].sendReceiptSerial, sizeof(uint32_t));
-							outputQueue.Push(ackReceipt, _FILE_AND_LINE_ );
-
-							// Remove, swap with last
-							unreliableWithAckReceiptHistory.RemoveAtIndex(k);
-						}
-						else
-							k++;
-					}
+				const DatagramSequenceNumberType offsetIntoList = datagramNumber - datagramHistoryPopCount;
+				if (offsetIntoList >= datagramHistory.Size()) {
+					// reached the end of the datagramHistory list - hence, we are done
+					receivePacketCount++;
+					return true;
 				}
 
+				CCTimeType whenSent;
 				MessageNumberNode *messageNumberNode = GetMessageNumberNodeByDatagramIndex(datagramNumber, &whenSent);
 				if (messageNumberNode)
 				{
@@ -835,6 +837,12 @@ bool ReliabilityLayer::HandleSocketReceiveFromConnectedPlayer(
 				// REMOVEME
 				//				printf("%p NAK %i\n", this, dhf.datagramNumber.val);
 
+				const DatagramSequenceNumberType offsetIntoList = messageNumber - datagramHistoryPopCount;
+				if (offsetIntoList >= datagramHistory.Size()) {
+					// reached the end of the datagramHistory list - hence, we are done
+					receivePacketCount++;
+					return true;
+				}
 
 				CCTimeType timeSent;
 				MessageNumberNode *messageNumberNode = GetMessageNumberNodeByDatagramIndex(messageNumber, &timeSent);