Skip to content
This repository has been archived by the owner on Jan 13, 2022. It is now read-only.

splitPacketCount vulnerability #143

Open
C0lumbo opened this issue Mar 2, 2020 · 0 comments
Open

splitPacketCount vulnerability #143

C0lumbo opened this issue Mar 2, 2020 · 0 comments

Comments

@C0lumbo
Copy link

C0lumbo commented Mar 2, 2020

A corrupt packet with a high value for splitPacketCount can cause either a crash (bad_array_new_length exception) or an infinite loop in the call to newChannel->splitPacketList.Preallocate.

Such a corrupted packet could arise by chance or be crafted by a malicious user to cause a server or peer to crash. After running into the issue in the wild, I found that the issue was briefly raised on the RakNet forums here but no fix was deployed: http://www.raknet.com/forum/index.php?topic=5253.msg21668#msg21668

My local fix was to change this block in ReliabilityLayer::CreateInternalPacketFromBitStream from:

	if (readSuccess==false ||
		internalPacket->dataBitLength==0 ||
		internalPacket->reliability>=NUMBER_OF_RELIABILITIES ||
		internalPacket->orderingChannel>=32 || 
		(hasSplitPacket && (internalPacket->splitPacketIndex >= internalPacket->splitPacketCount)))
	{
		// If this assert hits, encoding is garbage
		RakAssert("Encoding is garbage" && 0);
		ReleaseToInternalPacketPool( internalPacket );
		return 0;
	}

to

	const int maxPacketSize = 1024 * 1024 * 4;
	const int maxPacketSplit = (maxPacketSize + (MINIMUM_MTU_SIZE - 1)) / MINIMUM_MTU_SIZE;
	if (readSuccess==false ||
		internalPacket->dataBitLength==0 ||
		internalPacket->reliability>=NUMBER_OF_RELIABILITIES ||
		internalPacket->orderingChannel>=32 || 
		internalPacket->splitPacketCount > maxPacketSplit ||
		(hasSplitPacket && (internalPacket->splitPacketIndex >= internalPacket->splitPacketCount)))
	{
		// If this assert hits, encoding is garbage
		RakAssert("Encoding is garbage" && 0);
		ReleaseToInternalPacketPool( internalPacket );
		return 0;
	}

However, my fix limits packet sends to 4 megabytes, which is more than enough for my projects, but is not suitable for RakNet as a whole.

Perhaps a better fix is to define the maximum packet size in RakNetDefines.h to allow users to easily override. The maximum should also be enforced in RakPeer::Send and similar functions.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant