Closed Bug 1161719 Opened 5 years ago Closed 5 years ago

Possible OOB read in |MediaPipelineFilter::FilterSenderReport|

Categories

(Core :: WebRTC: Signaling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
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

People

(Reporter: erahm, Assigned: bwc)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file)

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
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: nobody → docfaraday
Status: NEW → ASSIGNED
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?
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
IIUC, this goes back to Fx39? Should we consider backporting?
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)
Whiteboard: [CID 1294186][CID 1288629] → [CID 1294186][CID 1288629][adv-main40+]
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.