Closed Bug 1098583 Opened 10 years ago Closed 9 years ago

Empty datachannel label results in heap overflow

Categories

(Core :: WebRTC: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox33 --- wontfix
firefox34 --- wontfix
firefox35 + fixed
firefox36 + fixed
firefox-esr31 - wontfix
b2g-v1.4 --- wontfix
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: drno, Assigned: drno)

Details

(Keywords: sec-moderate, Whiteboard: [adv-main35+][b2g-adv-main2.2+])

Attachments

(1 file)

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.
Attachment #8522510 - Flags: review?(rjesup)
Attachment #8522510 - Flags: review?(rjesup) → review+
After talking to Dan about this setting to sec-mod.

Try run looks green.
Assignee: nobody → drno
Keywords: sec-moderate
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
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
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?
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 ;)
Taking this on ESR31 seems to be a good idea (as well as fixing it on the other branches).
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)
[Tracking Requested - why for this release]:
(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)
We have taken sec-moderates in the past.
We can consider it based on risk/reward, leaving the tracking nomination at ? until this is actually fixed in trunk.
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 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 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?
Attachment #8522510 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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 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?
(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.
Attachment #8522510 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Nils, can you verify that this has been fixed in Fx35? Thank you.
Flags: needinfo?(drno)
Whiteboard: [adv-main35+]
Whiteboard: [adv-main35+] → [adv-main35+][b2g-adv-main2.2+]
Group: core-security → core-security-release
Flags: needinfo?(drno)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.