RTP header extensions potentially read out of bounds

RESOLVED FIXED in Firefox -esr52

Status

()

P1
normal
Rank:
15
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: drno, Assigned: drno)

Tracking

({csectype-disclosure, sec-moderate})

Trunk
mozilla55
csectype-disclosure, sec-moderate
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox-esr5254+ fixed, firefox53 wontfix, firefox54 fixed, firefox55 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main54+][adv-esr52.2+] Embargo until we know the status of upstream)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
backlog: --- → webrtc/webaudio+
Rank: 15
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
Priority: -- → P1
(Assignee)

Comment 1

2 years ago
Created attachment 8821391 [details] [diff] [review]
Check RTP extension header length
This appears to have been added with the import in Bug 749889, and appears in Firefox 16.
Group: core-security → media-core-security
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.
Assignee: nobody → drno
Keywords: csectype-disclosure, sec-moderate
(Assignee)

Comment 4

2 years ago
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?
Flags: needinfo?(juberti)
Mass wontfix for bugs affecting firefox 52.
status-firefox52: affected → wontfix
Nils, let's land this and file a bug upstream.  Or email justin or someone else at google, take your pick.
Flags: needinfo?(drno)
(Assignee)

Comment 7

2 years ago
Created attachment 8855420 [details] [diff] [review]
Check RTP extension header length
Attachment #8821391 - Attachment is obsolete: true
Flags: needinfo?(drno)
Attachment #8855420 - Flags: review?(rjesup)
Attachment #8855420 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/55959691c22c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Too late for Fx53 at this point, but please nominate this for Aurora and ESR52 approval when you get a chance.
status-firefox50: affected → wontfix
status-firefox51: affected → wontfix
status-firefox53: affected → wontfix
status-firefox54: --- → affected
status-firefox-esr52: --- → affected
tracking-firefox-esr52: --- → ?
Flags: needinfo?(drno)
(Assignee)

Comment 11

2 years ago
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
Flags: needinfo?(drno)
Attachment #8855420 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 12

2 years ago
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.
Attachment #8855420 - Flags: approval-mozilla-esr52?
Comment on attachment 8855420 [details] [diff] [review]
Check RTP extension header length

54 went to Beta today.
Attachment #8855420 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8855420 [details] [diff] [review]
Check RTP extension header length

Fix a security issue. Beta54+. Should be in 54 beta 1.
Attachment #8855420 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: media-core-security → core-security-release
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
status-firefox-esr45: --- → wontfix
tracking-firefox-esr52: ? → 54+
Flags: needinfo?(drno)
Whiteboard: Embargo until we know the status of upstream
(Assignee)

Comment 17

2 years ago
(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).
(Assignee)

Comment 18

2 years ago
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.
Flags: needinfo?(drno)

Comment 19

2 years ago
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.
Flags: needinfo?(juberti)
(Assignee)

Comment 20

2 years ago
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 on attachment 8855420 [details] [diff] [review]
Check RTP extension header length

bounds check for rtp, looks safe enough, esr52.2+
Attachment #8855420 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Flags: qe-verify-
Whiteboard: Embargo until we know the status of upstream → [post-critsmash-triage] Embargo until we know the status of upstream
(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.
Flags: needinfo?(drno)
(Assignee)

Comment 24

2 years ago
(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.
Flags: needinfo?(drno)
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?
Flags: needinfo?(drno)
(Assignee)

Comment 26

2 years ago
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
Flags: needinfo?(drno)
Whiteboard: [post-critsmash-triage] Embargo until we know the status of upstream → [post-critsmash-triage][adv-main54+][adv-esr52.2+] Embargo until we know the status of upstream
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.