Possible OOB read in |MediaPipelineFilter::FilterSenderReport|

RESOLVED FIXED in Firefox 40, Firefox OS master

Status

()

Core
WebRTC: Signaling
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: erahm, Assigned: bwc)

Tracking

({coverity})

Trunk
mozilla40
coverity
Points:
---

Firefox Tracking Flags

(firefox38 unaffected, firefox38.0.5 unaffected, firefox39 wontfix, firefox40 fixed, firefox-esr31 unaffected, firefox-esr38 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-master fixed)

Details

(Whiteboard: [CID 1294186][CID 1288629][adv-main40+])

Attachments

(1 attachment)

Coverity indicates the possibility of an OOB read in |MediaPipelineFilter::FilterSenderReport| [1]. 

It looks like his can happen if |len >= FIRST_SSRC_OFFSET && len < FIRST_SSRC_OFFSET+sizeof(uint32_t)|.

Flagging as sec, it's not clear to me if this is an exploitable issue.

[1] https://hg.mozilla.org/mozilla-central/annotate/754579ec0e68/media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp#l93
(Assignee)

Comment 1

3 years ago
Yeah, we need to be checking < FIRST_SSRC_OFFSET + 4 here. I doubt this is exploitable in any way other than a crash, since we're just doing reads into a value that never leaks out.
(Assignee)

Comment 2

3 years ago
Created attachment 8601763 [details] [diff] [review]
Fix bounds check in FilterSenderReport
(Assignee)

Updated

3 years ago
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 years ago
Comment on attachment 8601763 [details] [diff] [review]
Fix bounds check in FilterSenderReport

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49e4aa8a2e24
Attachment #8601763 - Attachment filename: . → bounds_check_fix.patch
Attachment #8601763 - Flags: review?(rjesup)
Comment on attachment 8601763 [details] [diff] [review]
Fix bounds check in FilterSenderReport

Review of attachment 8601763 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp
@@ +78,5 @@
>  
>  bool
>  MediaPipelineFilter::FilterSenderReport(const unsigned char* data,
>                                          size_t len) const {
> +  if (len < FIRST_SSRC_OFFSET + 4) {

+ 4 is ok since the code following does data, data+1, data+2, data+3.
Attachment #8601763 - Flags: review?(rjesup) → review+
Perhaps we should add a test for this in media/webrtc/signaling/test/mediapipeline_unittest.cpp?
(Assignee)

Comment 6

3 years ago
I actually checked this in earlier this morning, I guess pulsebot can't post notifications in sec bugs.

https://hg.mozilla.org/integration/mozilla-inbound/rev/36f729b4ffc9
https://hg.mozilla.org/mozilla-central/rev/36f729b4ffc9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40

Updated

3 years ago
status-firefox40: affected → fixed
IIUC, this goes back to Fx39? Should we consider backporting?
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → unaffected
status-b2g-master: --- → fixed
status-firefox38: --- → unaffected
status-firefox38.0.5: --- → unaffected
status-firefox39: --- → affected
status-firefox-esr31: --- → unaffected
status-firefox-esr38: --- → unaffected
Flags: needinfo?(docfaraday)
(Assignee)

Comment 9

3 years ago
I'm pretty sure that RTCP malformed in this way would not even make it past decryption, honestly. It's a super-low risk fix though. I'm happy to have it just ride the train.
Flags: needinfo?(docfaraday)
status-firefox39: affected → wontfix
Whiteboard: [CID 1294186][CID 1288629] → [CID 1294186][CID 1288629][adv-main40+]

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.