Closed
Bug 1279004
Opened 9 years ago
Closed 9 years ago
Don't decode SRTCP packets with the wrong SSRC
Categories
(Core :: WebRTC: Audio/Video, defect, P2)
Core
WebRTC: Audio/Video
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.
Assignee | ||
Comment 1•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/58578/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58578/
Attachment #8761344 -
Flags: review?(rjesup)
Comment 2•9 years ago
|
||
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+
Assignee | ||
Comment 3•9 years ago
|
||
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/
Assignee | ||
Comment 4•9 years ago
|
||
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
Comment 6•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•