Closed
Bug 1204099
Opened 9 years ago
Closed 8 years ago
Implement more stringent RTP payload type validation
Categories
(Core :: WebRTC: Signaling, defect, P4)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla50
backlog | parking-lot |
People
(Reporter: bwc, Assigned: pellenbogen)
Details
(Whiteboard: [good first bug])
Attachments
(1 file, 6 obsolete files)
Once bug 1203246 lands, we will check that payload types in SDP for audio/video are less than 128. We probably should check that the payload type does not fall into a reserved range too. These are 1, 2, 19, and 72-76.
Reporter | ||
Updated•9 years ago
|
backlog: --- → parking-lot
Reporter | ||
Updated•9 years ago
|
Rank: 50
Comment 1•9 years ago
|
||
Were you thinking in mochitest SDP verification or the C++ unit tests?
Flags: needinfo?(docfaraday)
Whiteboard: [good first bug]
Reporter | ||
Comment 2•9 years ago
|
||
This will be validation in JsepSessionImpl (in the SDP validation code).
Flags: needinfo?(docfaraday)
Comment 3•8 years ago
|
||
Hi, would like to work on this bug, please assign it to me, can you also provide more info.Thank you :)
Flags: needinfo?(drno)
Updated•8 years ago
|
Assignee: nobody → pe5
Flags: needinfo?(drno)
Comment 4•8 years ago
|
||
I think we should at same time avoid payload type range 64-95 as per https://tools.ietf.org/html/rfc5761#section-4 as we usually do rtcp-mux and need to be able to demux RTP and RTCP.
Comment 5•8 years ago
|
||
(In reply to Nils Ohlmeier [:drno] from comment #4) > I think we should at same time avoid payload type range 64-95 as per > https://tools.ietf.org/html/rfc5761#section-4 as we usually do rtcp-mux and > need to be able to demux RTP and RTCP. absolutely.
Assignee | ||
Comment 6•8 years ago
|
||
This is my first stab at a fix. I have 3 things I'm unsure of: 1) Is JsepSessionImpl::ValidateRemoteDescription the right place to do this. 2) Should some of this code be factored into SdpHelper? 3) Probably more importantly, is a static array of bools the right way to encode the allowable media values? Probably not, but this was what came to mind first.
Flags: needinfo?(docfaraday)
Reporter | ||
Comment 7•8 years ago
|
||
(In reply to Paul Ellenbogen [:pellenbogen] from comment #6) > Created attachment 8764423 [details] [diff] [review] > 1204099_01.diff > > This is my first stab at a fix. I have 3 things I'm unsure of: > > 1) Is JsepSessionImpl::ValidateRemoteDescription the right place to do this. I would do it in JsepSessionImpl::ParseSdp. Maybe someday we'll support JS rewriting the payload types in the SDP, and on that day we would want the sanity checks to apply. Also, we're already iterating over the payload types there. > 2) Should some of this code be factored into SdpHelper? It is possible some of this logic might belong there. I could definitely see putting the function that checks a single payload type there. > 3) Probably more importantly, is a static array of bools the right way to > encode the allowable media values? Probably not, but this was what came to > mind first. The idiom I've used for this sort of thing is a function that builds/returns a std::bitset, and call out to this function to build a static, eg: static std::bitset<128> GetForbiddenPayloadTypes() { std::bitset<128> forbidden; forbidden[1] = true; forbidden[2] = true; ... return forbidden; } ... // In validation code static std::bitset<128> forbidden = GetForbiddenPayloadTypes();
Flags: needinfo?(docfaraday)
Assignee | ||
Comment 8•8 years ago
|
||
Including your suggestions makes things a lot cleaner. Somehow I missed a comment in the file that basically says exactly where to do the check.
Attachment #8764423 -
Attachment is obsolete: true
Flags: needinfo?(docfaraday)
Reporter | ||
Comment 9•8 years ago
|
||
Comment on attachment 8764716 [details] [diff] [review] 1204099_02.diff Review of attachment 8764716 [details] [diff] [review]: ----------------------------------------------------------------- r+ with fixes ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp @@ +1657,5 @@ > // Error has already been set > return rv; > } > > + const std::bitset<128> forbidden = GetForbiddenSdpPayloadTypes(); static const should make this a little more efficient. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.h @@ +340,5 @@ > SipccSdpParser mParser; > SdpHelper mSdpHelper; > }; > > +static std::bitset<128> GetForbiddenSdpPayloadTypes(); No need to declare this here, we aren't exporting this symbol anyhow.
Attachment #8764716 -
Flags: review+
Assignee | ||
Comment 10•8 years ago
|
||
Makes sense. If this patch is okay, 2 necessary things are 1) Determine which values should be illegal 2) Write unit tests? (Not sure if this is necessary) For #1, including the values which trigger bug 1278975 might be a good idea.
Attachment #8764716 -
Attachment is obsolete: true
Flags: needinfo?(docfaraday)
Reporter | ||
Comment 11•8 years ago
|
||
It couldn't hurt to have a unit test of some sort. Let's also expand the forbidden range to 64-95 as mentioned in comment 4.
Assignee | ||
Comment 12•8 years ago
|
||
I think that should cover the cases from bug 1278975 too, if I found the right checking logic in the webrtc code: https://dxr.mozilla.org/mozilla-central/rev/d1102663db10b3d4b9358f3cf4e16b7c56902352/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc#51
Attachment #8764729 -
Attachment is obsolete: true
Assignee | ||
Comment 13•8 years ago
|
||
I have added the unit test in this patch.
Attachment #8764737 -
Attachment is obsolete: true
Flags: needinfo?(docfaraday)
Reporter | ||
Comment 14•8 years ago
|
||
Comment on attachment 8768485 [details] [diff] [review] 1204099_05.diff Review of attachment 8768485 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nits. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp @@ +41,5 @@ > + std::bitset<128> forbidden(0); > + forbidden[1] = true; > + forbidden[2] = true; > + forbidden[19] = true; > + forbidden[64] = true; Use a loop here. ::: media/webrtc/signaling/test/jsep_session_unittest.cpp @@ +2129,5 @@ > + std::string offer = CreateOffer(); > + UniquePtr<Sdp> munge(Parse(offer)); > + SdpMediaSection& mediaSection = munge->GetMediaSection(0); > + mediaSection.AddCodec("75", "DummyFormatVal", 8000, 1); > + std::ostringstream os; Just use Sdp::ToString here.
Attachment #8768485 -
Flags: review+
Assignee | ||
Comment 15•8 years ago
|
||
Attachment #8768485 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(docfaraday)
Comment 16•8 years ago
|
||
Looks like we need a try run and then we are good to land this.
Assignee | ||
Comment 17•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/65326/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/65326/
Assignee | ||
Updated•8 years ago
|
Attachment #8772550 -
Flags: review?(docfaraday)
Assignee | ||
Updated•8 years ago
|
Attachment #8770238 -
Attachment is obsolete: true
Reporter | ||
Comment 18•8 years ago
|
||
Comment on attachment 8772550 [details] Bug 1204099: RTP payload type validation. https://reviewboard.mozilla.org/r/65326/#review62334 Looks good!
Attachment #8772550 -
Flags: review?(docfaraday) → review+
Assignee | ||
Comment 19•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1a47a17a89f
Assignee | ||
Comment 20•8 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9595f6da48e2
Assignee | ||
Comment 21•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1663b5cf0c20
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 22•8 years ago
|
||
Pushed by ihsiao@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9fa64e0224e2 check payload type. r=bwc
Keywords: checkin-needed
Comment 23•8 years ago
|
||
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=32326078&repo=mozilla-inbound#L3350
Flags: needinfo?(pe5)
Comment 24•8 years ago
|
||
Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/37e87a7c3771 Backed out changeset 9fa64e0224e2 for bustage JsepSessionImpl.cpp
Assignee | ||
Comment 25•8 years ago
|
||
Comment on attachment 8772550 [details] Bug 1204099: RTP payload type validation. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65326/diff/1-2/
Attachment #8772550 -
Attachment description: Bug 1204099: check payload type → Bug 1204099: RTP payload type validation.
Assignee | ||
Comment 26•8 years ago
|
||
My mistake, I didn't update mozreview with the latest version of the code. This version of the code already ran on the try servers https://treeherder.mozilla.org/#/jobs?repo=try&revision=1663b5cf0c20&selectedJob=24171853
Flags: needinfo?(pe5)
Keywords: checkin-needed
Comment 27•8 years ago
|
||
Pushed by ihsiao@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e461c427a87e RTP payload type validation. r=docfaraday
Keywords: checkin-needed
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e461c427a87e
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•