Closed
Bug 1098583
Opened 10 years ago
Closed 9 years ago
Empty datachannel label results in heap overflow
Categories
(Core :: WebRTC: Networking, defect)
Core
WebRTC: Networking
Tracking
()
People
(Reporter: drno, Assigned: drno)
Details
(Keywords: sec-moderate, Whiteboard: [adv-main35+][b2g-adv-main2.2+])
Attachments
(1 file)
2.59 KB,
patch
|
jesup
:
review+
lsblakk
:
approval-mozilla-beta+
bajaj
:
approval-mozilla-b2g34+
|
Details | Diff | Splinter Review |
If no label gets assigned to a data channel the memset in SendOpenRequestMesssage() can write one \0 beyond the allocate memory. http://dxr.mozilla.org/mozilla-central/source/netwerk/sctp/datachannel/DataChannel.cpp#994 The attached patch fixes the problem. Marking as security until we are clear how big the risk is for an exploit. My guess it is low, given that this bug will only write one additional \0 into the heap.
Assignee | ||
Updated•10 years ago
|
status-b2g-v1.4:
--- → ?
status-b2g-v2.0:
--- → ?
status-b2g-v2.0M:
--- → ?
status-b2g-v2.1:
--- → ?
status-b2g-v2.2:
--- → ?
status-firefox33:
--- → affected
status-firefox34:
--- → affected
status-firefox35:
--- → affected
status-firefox36:
--- → affected
status-firefox-esr31:
--- → ?
Assignee | ||
Updated•10 years ago
|
Attachment #8522510 -
Flags: review?(rjesup)
Updated•10 years ago
|
Attachment #8522510 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 1•10 years ago
|
||
Try run for the patch: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e5525c19776a
Assignee | ||
Comment 2•10 years ago
|
||
After talking to Dan about this setting to sec-mod. Try run looks green.
Assignee: nobody → drno
Keywords: sec-moderate
Comment 3•10 years ago
|
||
As sec-mod, we can land this at will. We should ask for uplift to Aurora, but not beta for a sec-mod this late in the game. We can also consider it for ESR31
Assignee | ||
Comment 4•9 years ago
|
||
Not sure if the sheriffs have access to sec bugs. Randel can you land this if they don't?
Flags: needinfo?(rjesup)
Keywords: checkin-needed
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8522510 [details] [diff] [review] fix_data_channel_open_request.patch Approval Request Comment [Feature/regressing bug #]: The problem exist since a long time (at least before 33). Probably since we added data channels to WebRTC. [User impact if declined]: If a web page creates data channels without a label this potentially over rides a single byte with '\0', which could potentially result in crashes or other unwanted behavior. [Describe test coverage new/current, TBPL]: The problem was discovered through fuzzing on a local ASAN build and has been verified by running the same test again. No coverage in TBPL as instrumenting this from JS API first and then verify a single byte in memory is very hard. [Risks and why]: Low risk, as we only unified the length calculation from three places into one common place. No functional change. [String/UUID change made/needed]: N/A
Attachment #8522510 -
Flags: approval-mozilla-aurora?
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ed14c45e6e6 Yes, we have security bug access. And yes, including a commit message with your patch is appreciated when requesting checkin ;)
Flags: needinfo?(rjesup)
Keywords: checkin-needed
Comment 7•9 years ago
|
||
Taking this on ESR31 seems to be a good idea (as well as fixing it on the other branches).
Comment 8•9 years ago
|
||
As a sec-moderate bug, it's not in ESR criteria so does that mean this bug should have a higher rating?
Flags: needinfo?(abillings)
Comment 9•9 years ago
|
||
[Tracking Requested - why for this release]:
Comment 10•9 years ago
|
||
(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #8) > As a sec-moderate bug, it's not in ESR criteria so does that mean this bug > should have a higher rating? No, it means that it seemed low risk to me and a good thing to take across the board and a developer pinged me to explicitly ask if we could take it.
Flags: needinfo?(abillings)
Comment 11•9 years ago
|
||
We have taken sec-moderates in the past.
Comment 12•9 years ago
|
||
We can consider it based on risk/reward, leaving the tracking nomination at ? until this is actually fixed in trunk.
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4ed14c45e6e6 You'll probably want to nominate this for b2g34 approval as well.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 14•9 years ago
|
||
Comment on attachment 8522510 [details] [diff] [review] fix_data_channel_open_request.patch [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: leaves a bounds write violation Fix Landed on Version: 36 Risk to taking this patch (and alternatives if risky): low risk; simple rework String or UUID changes made by this patch: none
Attachment #8522510 -
Flags: approval-mozilla-esr31?
Attachment #8522510 -
Flags: approval-mozilla-b2g34?
Comment 15•9 years ago
|
||
Comment on attachment 8522510 [details] [diff] [review] fix_data_channel_open_request.patch We merged in between. cf comment #5 for uplift the request.
Attachment #8522510 -
Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Updated•9 years ago
|
Attachment #8522510 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 16•9 years ago
|
||
Since comment 5 doesn't even commit to this being an issue in ff31, and the low risk & likelihood of impact on that branch, we'll wontfix this for esr31 and it can go out in the next version, 38.
Comment 17•9 years ago
|
||
Comment on attachment 8522510 [details] [diff] [review] fix_data_channel_open_request.patch I'm interpreting comment 16 as a no on the esr31 request :)
Attachment #8522510 -
Flags: approval-mozilla-esr31?
Comment 18•9 years ago
|
||
(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #16) > Since comment 5 doesn't even commit to this being an issue in ff31, and the > low risk & likelihood of impact on that branch, we'll wontfix this for esr31 > and it can go out in the next version, 38. Ok. I will note it's almost certainly applicable to 31. I'll check. It is a low risk issue from an exploit point of view, though it is a bounds violation by one byte.
Updated•9 years ago
|
Attachment #8522510 -
Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Comment 21•9 years ago
|
||
Nils, can you verify that this has been fixed in Fx35? Thank you.
Flags: needinfo?(drno)
Updated•9 years ago
|
Whiteboard: [adv-main35+]
Updated•9 years ago
|
Whiteboard: [adv-main35+] → [adv-main35+][b2g-adv-main2.2+]
Updated•9 years ago
|
Group: core-security → core-security-release
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(drno)
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•