1161719 - Possible OOB read in |MediaPipelineFilter::FilterSenderReport|

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Eric Rahm [:erahm]]

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

Comment 1

[Byron Campen [:bwc]]

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.

Comment 2

[Byron Campen [:bwc]]

Attachment: Fix bounds check in FilterSenderReport

Comment 3

[Byron Campen [:bwc]]

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49e4aa8a2e24

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 5

[Eric Rahm [:erahm]]

Perhaps we should add a test for this in media/webrtc/signaling/test/mediapipeline_unittest.cpp?

Comment 6

[Byron Campen [:bwc]]

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

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/36f729b4ffc9

Comment 8

[Ryan VanderMeulen [:RyanVM]]

IIUC, this goes back to Fx39? Should we consider backporting?

Comment 9

[Byron Campen [:bwc]]

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.