1325513 - RTP header extensions potentially read out of bounds

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Nils Ohlmeier [:drno]]

The code from webrtc.org which parses one byte RTP header extensions looks like it could potentially read out of bounds.

The len of the header extension get read here http://searchfox.org/mozilla-central/rev/ac40ca3ec39efe85bfb111274c10ee4ceea5bb7a/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_utility.cc#389
But there is no check if there is actually sufficient data in the remaining buffer which matches the len value.

Then all the header parsers in this switch statement http://searchfox.org/mozilla-central/rev/ac40ca3ec39efe85bfb111274c10ee4ceea5bb7a/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_utility.cc#404 appear to read as many bytes from memory as |len| indicates. The biggest problem is probably the RID header parser which actually reads up to 16 bytes.

With a regular RTP packet this probably does not cause any problems as this would only read from the RTP audio or video payload, which the other side send as part of the RTP packet. 
But if someone sends a specially crafted RTP packet which has for example a RID extension header with a len of 15 in it, but the RTP packet actually ends right after the len field of the RID extension header the memcpy would potentially read 15 bytes from memory which happens to come after the received RTP packet.

Note: this code is identical even with the latest webrtc.org master branch. So Google Chrome is affected by this problem as well. Also any implementation which use a copy of webrtc.org, so we need to co-ordinate with webrtc.org on landing and disclosing.

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: Check RTP extension header length

Comment 2

[Nico Grunbaum [:ng, @chew:mozilla.org]]

This appears to have been added with the import in Bug 749889, and appears in Firefox 16.

Comment 3

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

Other than RID, the most any of the others can read past the end is 2 bytes. And I don't think any of these are currently exposed to the JS, so while it may read out of bounds, it would take major effort to even get a hint as to what the out-of-bound bytes are.  And RID values on packets aren't exposed either.  So other than a (very unlikely) crash, there's almost no risk here.  I would not worry about this being disclosed or landed in webrtc.org publicly if my analysis holds.

Comment 4

[Nils Ohlmeier [:drno]]

Justin do you agree with Randell's assessment in comment #3?
Can I just land this in Firefox and then submit it to webrtc.org or should we take any precautions?

Comment 5

[Julien Cristau [:jcristau] (back April 22)]

Mass wontfix for bugs affecting firefox 52.

Comment 6

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

Nils, let's land this and file a bug upstream.  Or email justin or someone else at google, take your pick.

Comment 7

[Nils Ohlmeier [:drno]]

Attachment: Check RTP extension header length

Comment 8

[Nils Ohlmeier [:drno]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55959691c22c30f86152689931f446adbcf2e310
Bug 1325513: Check RTP extension header length. r=jesup

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/55959691c22c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Too late for Fx53 at this point, but please nominate this for Aurora and ESR52 approval when you get a chance.

Comment 11

[Nils Ohlmeier [:drno]]

Comment on attachment 8855420 [details] [diff] [review]
Check RTP extension header length

Approval Request Comment
[Feature/Bug causing the regression]: Long standing bug from upstream lib
[User impact if declined]: Small risk of reading out of bounds memory.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: NO
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only adds extra out of bounds safety check.
[String changes made/needed]: N/A

Comment 12

[Nils Ohlmeier [:drno]]

Comment on attachment 8855420 [details] [diff] [review]
Check RTP extension header length

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Don't leave ESR users exposed to this potential security problem for a long time :-) 
User impact if declined: Small risk of reading limited amount of out of bounds memory in a WebRTC call.
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): Pretty low risk as it only adds an additional out bounds safety check.
String or UUID changes made by this patch: N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8855420 [details] [diff] [review]
Check RTP extension header length

54 went to Beta today.

Comment 14

[Gerry Chang [:gchang]]

Comment on attachment 8855420 [details] [diff] [review]
Check RTP extension header length

Fix a security issue. Beta54+. Should be in 54 beta 1.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3cf4e805acc2

Comment 16

[Daniel Veditz [:dveditz]]

Nils: I don't see an answer to the needinfo on Justin -- are we sure upstream is aware of this bug? If they've got one please add a link to it in the "see also" field

Comment 17

[Nils Ohlmeier [:drno]]

(In reply to Daniel Veditz [:dveditz] from comment #16)
> Nils: I don't see an answer to the needinfo on Justin -- are we sure
> upstream is aware of this bug? If they've got one please add a link to it in
> the "see also" field

I'm in the process of reporting the issue plus the patch upstream. I'll update this bug once I have more information (hopefully within a few days).

Comment 18

[Nils Ohlmeier [:drno]]

I opened https://bugs.chromium.org/p/webrtc/issues/detail?id=7603 on the webrtc.org/Chrome side.
Waiting for instructions/help how to submit the patch in a secure way on their end.

Comment 19

[Justin Uberti]

Thanks for reporting this, and sorry for dropping the ball here. 

Seems like the risk is fairly low, we can discuss how to notify the webrtc userbase in 7603.

Comment 20

[Nils Ohlmeier [:drno]]

Turned out that I overlooked that this problem got already fixed in the upstream release 52. Firefox uses the upstream release version 49 so far.

Comment 21

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8855420 [details] [diff] [review]
Check RTP extension header length

bounds check for rtp, looks safe enough, esr52.2+

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/218e0963406f

Comment 23

[Al Billings [:abillings - ex-MoCo]]

(In reply to Nils Ohlmeier [:drno] from comment #20)
> Turned out that I overlooked that this problem got already fixed in the
> upstream release 52. Firefox uses the upstream release version 49 so far.

Does this mean we can disclose? I'm working on advisories.

Comment 24

[Nils Ohlmeier [:drno]]

(In reply to Al Billings [:abillings] from comment #23)
> (In reply to Nils Ohlmeier [:drno] from comment #20)
> > Turned out that I overlooked that this problem got already fixed in the
> > upstream release 52. Firefox uses the upstream release version 49 so far.
> 
> Does this mean we can disclose? I'm working on advisories.

Yes we can disclose.
This has been fixed in Chrome since version 52. And everybody who uses webrtc.org directly had plenty of opportunity to update as well.

Comment 25

[Daniel Veditz [:dveditz]]

Nils: do you have the ability to grant me access to the upstream bug, or can ask someone to add me? Did Google assign a CVE to this one or release a security advisory/release-note?

Comment 26

[Nils Ohlmeier [:drno]]

Sorry I did not realize that the chromium bug did not got opened up when it turned out to be fixed already.

Here is the original bug report, which is public as far as I can tell, from when it got fixed in Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=620277