failed assertion `payloadOffset < this->payloadLength': payload offset bigger than payload size

[ibc]

2017-11-09T16:20:22.837Z mediasoup:WARN:mediasoup-worker [id:dnlijfix#1] RTC::RTCP::Sdes::Parse() | not enough space for SDES item, discarded
2017-11-09T16:20:23.102Z mediasoup:WARN:mediasoup-worker [id:dnlijfix#1] RTC::RTCP::Sdes::Parse() | not enough space for SDES item, discarded
2017-11-09T16:20:23.539Z mediasoup:mediasoup-worker [id:dnlijfix#1] RTC::Producer::ReceiveRtpPacket() | key frame received [ssrc:93142492, seq:18331, profile:default]
mediasoup-worker's stderr: ABORT[id:dnlijfix#1] RTC::RtpPacket::ShiftPayload() | failed assertion `payloadOffset < this->payloadLength': payload offset bigger than payload size
2017-11-09T16:20:24.980Z mediasoup:ERROR:Worker child process exited [id:dnlijfix#1, code:null, signal:SIGABRT]

[ibc]

It happens in VP8.cpp, here:

// Modify the RtpPacket payload in order to always have two byte pictureId.
if (!payloadDescriptor->hasTwoBytePictureId)
{
	// Shift the RTP payload one byte from the begining of the pictureId field.
	packet->ShiftPayload(2, 1, true /*expand*/);
	// Set the two byte pictureId marker bit.
	data[2] = 0x80;
	// Update the payloadDescriptor.
	payloadDescriptor->hasTwoBytePictureId = true;
}

This is probably produced by Edge. AFAIR it sends rare VP8 payloads, without some fields.

So basically the caller must check the payload size before calling ShiftPayload().

[ibc]

Opss, no, what it happen (AFAIR) is that Edge does NOT include PictureId in VP8 payloads.

This is, the fact that a VP8 payload does not have hasTwoBytePictureId does not mean that it has 1 byte PictureId, but the code above wrongly assumes it.

I assume that, within VP8::Parse(), we need to set a flag telling whether there is PictureId or not, regardless it's 1 or 2 bytes length. If false, no PictureId read/rewrite should be done.

[ibc]

It happened again in the demo server (which was updated):

mediasoup-worker's stderr: ABORT[id:wenesnle#1] RTC::RtpPacket::ShiftPayload() | failed assertion `payloadOffset < this->payloadLength': payload offset bigger than payload size

[ibc]

Checking i is not enough. It may happen that the VP8 payload has i but has no PictureId:

if (payloadDescriptor->i)
{
	if (len < ++offset + 1)
		return nullptr;

	byte = data[offset];

	if ((byte >> 7) & 0x01)
	{
		if (len < ++offset + 1)
			return nullptr;

		payloadDescriptor->hasTwoBytePictureId = true;

		payloadDescriptor->pictureId = (byte & 0x7F) << 8;
		payloadDescriptor->pictureId += data[offset];
	}
	else
	{
		payloadDescriptor->pictureId = byte & 0x7F;
	}
}

so this check will return true anyway:

if (payloadDescriptor->i && !payloadDescriptor->hasTwoBytePictureId)

[ibc]

I've commited a fix: 1528cfe

However, I think there are more vulnerabilities as, Encode() checks whether a flag i or l is set and assumes that, if so, there was PictureId field (fixed) and tl0PictureIndex. But that's not true so it must be fixed or a malicious packet with l but no tl0PictureIndex would produce a crash.

[ibc]

To clarify, the fact that the original payload has bit flags set (i, l, t, k, y, etc) does not mean that there is real space for the associated fields, so we need more boolean getters such as hasTl0PictureIndex, hasTlIndex, hasKeyIndex, etc. And check those getters rather than the original (and not reliable) flags in the payload.

[jmillan]

Fix + tests will be ready tomorrow.

[jmillan]

Comment: Whenever returning nullptr from VP8::Parse, then no VP8:Encode is called for such RTP packet.

I'm working on this right now.

[ibc]

OK, then the check should also verify whether this->payloadDescriptor is set (rather than checking its ->i field, which may be randomly set to 1 or 0 depending on next memory/bytes in the payload).

[jmillan]

If this this->payloadDescriptor is not set VP8::Encode will not be called.

I'm working on it.

[ibc]

AFAIU the crash happens when the Producer mangles the packet so before any Encode() call.

[jmillan]

Here was the problem to this issue: 725ea8b

If the extended flag is not set then we must ignore the payload descriptor. This was a hole in the parser.

Some VP8 tests have been added too.

[jmillan]

Also, the suggested hasX have been implemented. We do not rely on the flags anymore but on the actual data 8ba457e