An overflow and crash risk in RTC::Transport::SendRtcp

[billzmh]

Your environment

• Operating system: Linux
• Node version:
• npm version:
• gcc/clang version:
• mediasoup version: v3 recent version
• mediasoup-client version:

Issue description

In RTC::Transport::SendRtcp(), when serializing the compound RTCP packet for mapProducers, there's a overflow and crash risk: packet->GetSize() will return default 0 forever if packet->Serialize(RTC::RTCP::Buffer) was not called before it :

	void Transport::SendRtcp(uint64_t nowMs)
	{
		MS_TRACE();

		std::unique_ptr<RTC::RTCP::CompoundPacket> packet{ nullptr };

		for (auto& kv : this->mapConsumers)
		{
			auto* consumer = kv.second;

			for (auto* rtpStream : consumer->GetRtpStreams())
			{
				// Reset the Compound packet.
				packet.reset(new RTC::RTCP::CompoundPacket());

				consumer->GetRtcp(packet.get(), rtpStream, nowMs);

				// Send the RTCP compound packet if there is a sender report.
				if (packet->HasSenderReport())
				{
					packet->Serialize(RTC::RTCP::Buffer);
					SendRtcpCompoundPacket(packet.get());
				}
			}
		}

		// Reset the Compound packet.
		packet.reset(new RTC::RTCP::CompoundPacket());

		for (auto& kv : this->mapProducers)
		{
			auto* producer = kv.second;

			producer->GetRtcp(packet.get(), nowMs);

			// One more RR would exceed the MTU, send the compound packet now.
			/*
			 * The if condition below will not be met forever, because the GetSize() always return 0.-------------------
			*/
			if (packet->GetSize() + sizeof(RTCP::ReceiverReport::Header) > RTC::MtuSize)
			{
				//The following code will not be executed.---------------------------
				packet->Serialize(RTC::RTCP::Buffer);
				SendRtcpCompoundPacket(packet.get());

				// Reset the Compound packet.
				packet.reset(new RTC::RTCP::CompoundPacket());
			}
		}

		if (packet->GetReceiverReportCount() != 0u)
		{
			/*
			 * Finally, if the mapProducers's elements amout is enough large, all producers's data serialized into 
			 * the buffer together, then it will overflow.
			*/
			packet->Serialize(RTC::RTCP::Buffer);
			SendRtcpCompoundPacket(packet.get());
		}
	}

Because packet->GetSize() returned the size's value only, but didn't calculated the size's value:

	namespace RTCP
	{
		class CompoundPacket
		{
		public:
			CompoundPacket() = default;

		public:
			const uint8_t* GetData() const
			{
				return this->header;
			}
			size_t GetSize() const
			{
				return this->size;
			}
			......
			......   

The size's value was calculated in CompoundPacket::Serialize() :

namespace RTC
{
	namespace RTCP
	{
		/* Instance methods. */

		void CompoundPacket::Serialize(uint8_t* data)
		{
			MS_TRACE();

			this->header = data;

			// Calculate the total required size for the entire message.
			if (HasSenderReport())
			{
				this->size = this->senderReportPacket.GetSize();

				if (this->receiverReportPacket.GetCount() != 0u)
				{
					this->size += ReceiverReport::HeaderSize * this->receiverReportPacket.GetCount();
				}
			}
			// If no sender nor receiver reports are present send an empty Receiver Report
			// packet as the head of the compound packet.
			else
			{
				this->size = this->receiverReportPacket.GetSize();
			}
			......
			......

If the mapProducers element amount is enough large, then packet->Serialize(RTC::RTCP::Buffer); will OVERFLOW: the RTC::RTCP::Buffer can't hold all serialized data and some adjoining important data will be damaged by OVERFLOW.

I modified it like below, calling packet->Serialize(RTC::RTCP::Buffer) before calling packet->GetSize():

	void Transport::SendRtcp(uint64_t nowMs)
	{
		MS_TRACE();

		std::unique_ptr<RTC::RTCP::CompoundPacket> packet{ nullptr };

		for (auto& kv : this->mapConsumers)
		{
			auto* consumer = kv.second;

			for (auto* rtpStream : consumer->GetRtpStreams())
			{
				// Reset the Compound packet.
				packet.reset(new RTC::RTCP::CompoundPacket());

				consumer->GetRtcp(packet.get(), rtpStream, nowMs);

				// Send the RTCP compound packet if there is a sender report.
				if (packet->HasSenderReport())
				{
					packet->Serialize(RTC::RTCP::Buffer);
					SendRtcpCompoundPacket(packet.get());
				}
			}
		}

		// Reset the Compound packet.
		packet.reset(new RTC::RTCP::CompoundPacket());

		for (auto& kv : this->mapProducers)
		{
			auto* producer = kv.second;

			producer->GetRtcp(packet.get(), nowMs);
			packet->Serialize(RTC::RTCP::Buffer); // Move Here----------------------------

			// One more RR would exceed the MTU, send the compound packet now.
			if (packet->GetSize() + sizeof(RTCP::ReceiverReport::Header) > RTC::MtuSize)
			{
				SendRtcpCompoundPacket(packet.get());

				// Reset the Compound packet.
				packet.reset(new RTC::RTCP::CompoundPacket());
			}
		}

		if (packet->GetReceiverReportCount() != 0u)
		{
			packet->Serialize(RTC::RTCP::Buffer);
			SendRtcpCompoundPacket(packet.get());
		}
	}

[jmillan]

Good finding @billzmh,

There's an ongoing fix for this. Thanks for the report.

[billzmh]

Good luck! Please tell me if u fixed it, thanks!