Closed Bug 1279004 Opened 8 years ago Closed 8 years ago

Don't decode SRTCP packets with the wrong SSRC

Categories

(Core :: WebRTC: Audio/Video, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: drno, Assigned: drno)

Details

Attachments

(1 file)

According to https://tools.ietf.org/html/rfc3711#section-3.4 the first 8 bytes of a SRTCP are not encrypted. But our current code first decodes the SRTCP message, but then calls FilterSenderReport() on it
https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp?from=MediaPipeline.cpp%3A1000#1013
which only looks at the first 8 bytes.

So we spare some CPU cycles by doing decryption only after doing filtering.
Comment on attachment 8761344 [details]
Bug 1279004: do RTCP decryption after filtering.

https://reviewboard.mozilla.org/r/58578/#review55560

r+ but fix the issue opened here or in a different bug.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:997
(Diff revision 1)
> +  // We do not filter RTCP for send pipelines, since the webrtc.org code for
> +  // senders already has logic to ignore RRs that do not apply.

Perhaps so, but decrypting them isn't zero cost.  that said, this is basically ok.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:999
(Diff revision 1)
> +  if (filter_ && direction_ == RECEIVE) {
> +    if (!filter_->FilterSenderReport(data, len)) {
> +      MOZ_MTLOG(ML_NOTICE, "Dropping incoming RTCP packet; filtered out");
> +      return;
> +    }
> +  }

Per rfc 5506 3.4.3 (reduced-size RTCP) an SRTCP packet does NOT have to start with an RR or SR, but just has to have a normal RTCP packet header which has the SSRC in the unencrypted portion (which makes this moving of the code possible to begin with).  So the check for SENDER_REPORT_T in FilterSenderReport needs to change (and the function name is incorrect as well).
https://tools.ietf.org/html/rfc5506#section-3.4.3
Attachment #8761344 - Flags: review?(rjesup) → review+
Comment on attachment 8761344 [details]
Bug 1279004: do RTCP decryption after filtering.

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

> Per rfc 5506 3.4.3 (reduced-size RTCP) an SRTCP packet does NOT have to start with an RR or SR, but just has to have a normal RTCP packet header which has the SSRC in the unencrypted portion (which makes this moving of the code possible to begin with).  So the check for SENDER_REPORT_T in FilterSenderReport needs to change (and the function name is incorrect as well).
> https://tools.ietf.org/html/rfc5506#section-3.4.3

Created bug 1279153 and added a comment reference into the code to that new bug.
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d76d3fe00cc1
do RTCP decryption after filtering. r=jesup
https://hg.mozilla.org/mozilla-central/rev/d76d3fe00cc1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: