Closed Bug 1325173 Opened 7 years ago Closed 7 years ago

RtpStreamId is truncated when read from RTP extension header

Categories

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

Product:
Core
Component:
WebRTC: Networking
Version:
49 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: ng, Assigned: ng)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1325173 - read full RtpStreamId when parsing RTP header extensions.
59 bytes, text/x-review-board-request
drno
: review+
Details 
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
Rank: 25
Priority: -- → P2
Assignee: nobody → na-g
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
Attachment #8820860 - Flags: review+
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.
Attachment #8820860 - Flags: review+
(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 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.
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'.
Sorry, forget my comment. There is a bug but it's different (I'll report it in a new issue).
Nico, do you remember why we haven't landed this yet?
Flags: needinfo?(na-g)
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.
Flags: needinfo?(na-g)
Attachment #8820860 - Attachment is obsolete: true
Comment on attachment 8843851 [details]
Bug 1325173 - read full RtpStreamId when parsing RTP header extensions.

https://reviewboard.mozilla.org/r/117460/#review119298
Attachment #8843851 - Flags: review?(drno) → review+
Pushed by na-g@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/98a314e77d6c
read full RtpStreamId when parsing RTP header extensions. r=drno
https://hg.mozilla.org/mozilla-central/rev/98a314e77d6c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: