Implement more stringent RTP payload type validation

RESOLVED FIXED in Firefox 50

Status

()

Core
WebRTC: Signaling
P4
normal
Rank:
50
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bwc, Assigned: pellenbogen)

Tracking

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox43 affected, firefox50 fixed)

Details

(Whiteboard: [good first bug])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

3 years ago
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

3 years ago
backlog: --- → parking-lot
(Reporter)

Updated

3 years ago
Rank: 50
Were you thinking in mochitest SDP verification or the C++ unit tests?
Flags: needinfo?(docfaraday)
Whiteboard: [good first bug]
(Reporter)

Comment 2

3 years ago
This will be validation in JsepSessionImpl (in the SDP validation code).
Flags: needinfo?(docfaraday)

Comment 3

2 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

2 years ago
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.
(Assignee)

Comment 6

2 years ago
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.
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

2 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

2 years ago
Created attachment 8764716 [details] [diff] [review]
1204099_02.diff

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

2 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

2 years ago
Created attachment 8764729 [details] [diff] [review]
1204099_03.diff

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

2 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

2 years ago
Created attachment 8764737 [details] [diff] [review]
1204099_04.diff

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

2 years ago
Created attachment 8768485 [details] [diff] [review]
1204099_05.diff

I have added the unit test in this patch.
Attachment #8764737 - Attachment is obsolete: true
Flags: needinfo?(docfaraday)
(Reporter)

Comment 14

2 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

2 years ago
Created attachment 8770238 [details] [diff] [review]
1204099_06.diff
Attachment #8768485 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Flags: needinfo?(docfaraday)
Looks like we need a try run and then we are good to land this.
(Assignee)

Updated

2 years ago
Attachment #8772550 - Flags: review?(docfaraday)
(Assignee)

Updated

2 years ago
Attachment #8770238 - Attachment is obsolete: true
(Reporter)

Comment 18

2 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)

Updated

2 years ago
Keywords: checkin-needed

Comment 22

2 years ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fa64e0224e2
check payload type. r=bwc
Keywords: checkin-needed
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=32326078&repo=mozilla-inbound#L3350
Flags: needinfo?(pe5)

Comment 24

2 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

2 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

2 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

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e461c427a87e
Status: NEW → RESOLVED
Last Resolved: 2 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.