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)
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•8 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•8 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•8 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•8 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•8 years ago
|
||
bugherder |
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.
Description
•