Closed Bug 1204099 Opened 4 years ago Closed 3 years ago

Implement more stringent RTP payload type validation

Categories

(Core :: WebRTC: Signaling, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox43 --- affected
firefox50 --- fixed
Blocking Flags:
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.
backlog: --- → parking-lot
Rank: 50
Were you thinking in mochitest SDP verification or the C++ unit tests?
Flags: needinfo?(docfaraday)
Whiteboard: [good first bug]
This will be validation in JsepSessionImpl (in the SDP validation code).
Flags: needinfo?(docfaraday)
Hi, would like to work on this bug, please assign it to me, can you also provide more info.Thank you :)
Flags: needinfo?(drno)
Assignee: nobody → pe5
Flags: needinfo?(drno)
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.
(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.
Attached patch 1204099_01.diff (obsolete) — Splinter Review
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)
(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)
Attached patch 1204099_02.diff (obsolete) — Splinter Review
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)
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+
Attached patch 1204099_03.diff (obsolete) — Splinter Review
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)
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.
Attached patch 1204099_04.diff (obsolete) — Splinter Review
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
Attached patch 1204099_05.diff (obsolete) — Splinter Review
I have added the unit test in this patch.
Attachment #8764737 - Attachment is obsolete: true
Flags: needinfo?(docfaraday)
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+
Attached patch 1204099_06.diff (obsolete) — Splinter Review
Attachment #8768485 - Attachment is obsolete: true
Flags: needinfo?(docfaraday)
Looks like we need a try run and then we are good to land this.
Attachment #8772550 - Flags: review?(docfaraday)
Attachment #8770238 - Attachment is obsolete: true
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+
Keywords: checkin-needed
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37e87a7c3771
Backed out changeset 9fa64e0224e2 for bustage JsepSessionImpl.cpp
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.
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
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e461c427a87e
RTP payload type validation. r=docfaraday
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e461c427a87e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.