1325173 - RtpStreamId is truncated when read from RTP extension header

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P2)

Description

[Nico Grunbaum [:ng, @chew:mozilla.org]]

When deserializing the RtpStreamId header extension the length field should be interpreted as (length of id)-1 as per rfc5285 section 4.2[0]. E.g. a header length value of 2 indicates the id is 3 bytes. It is currently interpreted as (length of id).


Deserialization occurs in RtpHeaderParser::ParseOneByteExtensionHeader[1]

[0] https://tools.ietf.org/html/rfc5285#section-4.2
[1] (revision permalink) https://dxr.mozilla.org/mozilla-central/rev/c36fbe84042debef0a5d58b7fc88185b401762ce/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_utility.cc#493

Comment 1

[Nico Grunbaum [:ng, @chew:mozilla.org]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a1e0fad1138

Comment 2

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1325173 - read full RtpStreamId when parsing RTP header extensions.

Review commit: https://reviewboard.mozilla.org/r/100250/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100250/

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8820860 [details]
Bug 1325173 - read full RtpStreamId when parsing RTP header extensions.

https://reviewboard.mozilla.org/r/100250/#review100860

drive-by - I wrote this code
Add a comment that length is L+1

Comment 4

[Nils Ohlmeier [:drno]]

Comment on attachment 8820860 [details]
Bug 1325173 - read full RtpStreamId when parsing RTP header extensions.

https://reviewboard.mozilla.org/r/100250/#review100862

::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_utility.cc:501
(Diff revision 1)
> -          char* ptrRID = new char[len+1];
> -          memcpy(ptrRID, ptr, len);
> -          ptrRID[len] = '\0';
> +          char* ptrRID = new char[len + 2];
> +          memcpy(ptrRID, ptr, len + 1);
> +          ptrRID[len + 1] = '\0';

I think we should have a length check here as well. The maximum size is already limited through the size of |len|. But if |len| happens to be zero this read one random byte from after the RTP header extension.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Nils Ohlmeier [:drno] from comment #4)

> I think we should have a length check here as well. The maximum size is
> already limited through the size of |len|. But if |len| happens to be zero
> this read one random byte from after the RTP header extension.

If len=0 that means the RID is 1 byte.  So it will allocate 2 bytes, copy one byte (correct), and add a null terminator.  That's correct.

Comment 6

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8820860 [details]
Bug 1325173 - read full RtpStreamId when parsing RTP header extensions.

https://reviewboard.mozilla.org/r/100250/#review100862

> I think we should have a length check here as well. The maximum size is already limited through the size of |len|. But if |len| happens to be zero this read one random byte from after the RTP header extension.

A len of zero indicates that |RtpStreamId| is 1. All possible values over the range of the masked len are valid. I don't think a check is necessary.

Comment 7

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8820860 [details]
Bug 1325173 - read full RtpStreamId when parsing RTP header extensions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100250/diff/1-2/

Comment 8

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8820860 [details]
Bug 1325173 - read full RtpStreamId when parsing RTP header extensions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100250/diff/2-3/

Comment 9

[Iñaki Baz Castillo]

I know this bug is about parsing RID header extension, but in Firefox stable (51.0.1 OSX) I'm experimenting a similar issue when parsing RID header extension in RTP packets generated by Firefox. The len field is 0 (so it means 1 according to the 4 bytes format) but data[0] is always '\0'.

Comment 10

[Iñaki Baz Castillo]

Sorry, forget my comment. There is a bug but it's different (I'll report it in a new issue).

Comment 11

[Nils Ohlmeier [:drno]]

Nico, do you remember why we haven't landed this yet?

Comment 12

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Nils, I don't remember. If there is a reason I am sure we will remember shortly after it lands. I will rebase it, send it back through try, and land it Monday.

Comment 13

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1325173 - read full RtpStreamId when parsing RTP header extensions.

Review commit: https://reviewboard.mozilla.org/r/117460/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117460/

Comment 14

[Nils Ohlmeier [:drno]]

Comment on attachment 8843851 [details]
Bug 1325173 - read full RtpStreamId when parsing RTP header extensions.

https://reviewboard.mozilla.org/r/117460/#review119298

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98a314e77d6c
read full RtpStreamId when parsing RTP header extensions. r=drno

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/98a314e77d6c