PeerConnection.createDataChannel crashes remote end in mozilla::detail::PrimitiveIntrinsics<int>::sub(long*, long)

VERIFIED FIXED in Firefox 34

Status

()

defect
--
critical
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: Makkes, Assigned: jesup)

Tracking

({crash, csectype-bounds, sec-critical})

Trunk
mozilla36
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox33 wontfix, firefox34+ verified, firefox35+ verified, firefox36+ verified, firefox-esr3134+ fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [adv-main34+][adv-esr31.3+], crash signature)

Attachments

(4 attachments, 2 obsolete attachments)

Reporter

Description

5 years ago
Posted file webrtc-dc-testcourt.html (obsolete) —
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140924083558

Steps to reproduce:

1. Load the attached page webrtc-dc-testcourt.html in two Firefox instances
2. In instance A click "Create Offer"
3. Copy the resulting offer from the lower textarea of instance A into the upper textarea of instance B and on instance B click "Accept offer"
4. Copy the resulting answer from the lower textarea of instance B into the upper textarea of instance A and on instance A click "Accept answer"
5. On instance A you should see "open event: dc onopen" and no instance B you should see "info event: pc ondatachannel"
6. On instance A click "Create additional DC"
7. Instance B should log "info event: pc ondatachannel"


Actual results:

When repeating step 6 multiple times (seems to differ between 8 and 10), instance B crashes.


Expected results:

AFAIK it should be possible to open at least 16 Data Channels per PeerConnection (netwerk/sctp/datachannel/DataChannelProtocol.h:#define WEBRTC_DATACHANNEL_STREAMS_DEFAULT 16)

What should actually never happen is that one end crashes completely.

Comment 1

5 years ago
I tried between FF32 (instance A) and FF35 (instance B) because in FF35, creating offer sends no offer and the error message "error event: pc#createOffer onerror {"name":"INVALID_STATE","message":"Cannot create offer in state HAVE_LOCAL_OFFER"}"

Anyway, I crashed FF35:
https://crash-stats.mozilla.com/report/index/90f8377d-38fd-4ce0-a695-5aea82141008

But after clsoing instance B and playing with instance A, A crashed too:
https://crash-stats.mozilla.com/report/index/12297c63-1671-4723-8f90-9d2a82141008
Severity: normal → critical
Crash Signature: [@ mozilla::detail::PrimitiveIntrinsics<int>::sub(long*, long) ]
Summary: PeerConnection.createDataChannel crashes remote end → PeerConnection.createDataChannel crashes remote end in mozilla::detail::PrimitiveIntrinsics<int>::sub(long*, long)
Until i've validated the state of the crash, I'm marking this as a sec bug.  It's crashing (in a debug nightly build) in mStreams[stream] = ....; where stream >= mStreams.Length().  (mStreams is a nsTArray)

Something regressed the mirroring of input and output stream sizes - this worked and I thought we had a test for it (perhaps our test misses this usecase).

Working on a fix (shouldn't be hard; wallpaper for existing builds should be trivial).  Note: the test fails on recent builds because it violates the spec (calls createOffer() after calling createOffer()).  I will upload a tweaked version.

I'm currently presuming this bug has been there for a Long Time, as this code hasn't changed recently.
Assignee: nobody → rjesup
Group: core-security
Status: UNCONFIRMED → ASSIGNED
Component: WebRTC → WebRTC: Networking
Ever confirmed: true
The second crash (in the timer code) is known and being investigated by myself and Michael Tuexen
Updated test.  Assumes it only needs one ICE candidate....
Attachment #8501609 - Attachment is obsolete: true
Updated to avoid garbage collection destroying datachannels since they have no references to them.  Also fix logging of ondatachannel
Attachment #8501911 - Attachment is obsolete: true
this fixes the extending the number of datachannels completely, including a hidden bug when an extension was done and the re-send of the Open failed with EAGAIN - it resent it later and successfully opened the channel, but didn't change the channel state to OPEN or fire onopen.  Note that dynamic extension of negotiated channels is no longer part of the DataChannel spec, though we still support it for now
this patch is much simpler and avoids having to fix the extension bugs by only increasing the number of statically allocated streams/DataChannels (256 in this patch, but that's pretty arbitrary and costs 4 or 8 (64-bit arch) bytes per channel per peerconnection with datachannels active).  This is safer to uplift however. (there is a small amount of cleanup here, and also a safety-valve check on incoming stream numbers in Open requests)
Blocks: 1080312
sec-critical since it does mStreams[index] where index > mStream.Length() on an nsTArray<nsRefPtr<DataChannel>> and the index could be an arbitrary (uint16_t) attack-value.
Attachment #8502193 - Flags: review?(tuexen)
Comment on attachment 8502191 [details] [diff] [review]
Fix handling of increasing number of SCTP channels used by DataChannels

also r? for this one since we may want to land this on Inbound as a stepping stone to max-size (65534) DataChannel indexes
Attachment #8502191 - Flags: review?(tuexen)

Updated

5 years ago
Attachment #8502191 - Flags: review?(tuexen) → review+

Updated

5 years ago
Attachment #8502193 - Flags: review?(tuexen) → review+
Comment on attachment 8502193 [details] [diff] [review]
Simple patch that ups the number of static DataChannels and disables expansion

[Security approval request comment]
How easily could an exploit be constructed based on the patch? 
Not easily, especially if couched as a stepping stone to spec compliance and the LOG() for out-of-bounds indexes removed (and landed on a "spec-compliance and Chrome interop" bug).  Even if recognized, it would be pretty tricky to exploit -- it would try to assign a DataChannel to an nsRefPtr past the end of the TArray, so the attack would have to be by either poking a pointer to a DataChannel into a NULL, or causing a DataChannel pointer to replace (and Release()) some other nsRefPtr<> if can hit by out-of-bounds indexing.  While not impossible, this would seem pretty hard to leverage.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? 
The LOG() message points a little at it.

Which older supported branches are affected by this flaw?
All older branches; it certainly appears to be in 32 and we haven't changed that code since Mar 2013 - when we switched from 3-way handshake to 0-RTT declarative opens.

If not all supported branches, which bug introduced the flaw? 
Bug 855623 probably.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  
Trivial, as the code is basically all the same.

How likely is this patch to cause regressions; how much testing does it need?  
This patch basically just turns off a moderately complex set of code and bumps the static array size (and adds a safety check that no one feeds us an invalid Open index/stream id), so the only testing would be that it smoothly stops handing out channels when you hit the limit, which is easy to test with the testcase here.  I'll write an automated test as well, but we may want to hold off on committing it.  I have tested it locally, no problems
Attachment #8502193 - Flags: sec-approval?
No longer blocks: 1080312
See Also: → 1080312
Note: the 'simple' version here will break pre-negotiated channels above the new limit.  Those worked safely before, since they didn't send an Open Request packet after increasing the number of channels, instead the client at the receiving side is expected to also createDataChannel() with the higher id, causing that end to also increase the number of channels.  Thus, no chance of storing the mStreams[stream] with a value past the end.  This does depend on being able to increase the number of channels outbound, which the simple patch removed.

The alternative to retain current pre-negotiated behavior would be to either take the more complex patch that fixes it in general, or up it to 256 but disable non-prenegotiated increases only.
Alternate fixed-size allocation patch that allows pre-negotiated channels to use any channel id to avoid a regression.  Tested that automatic increases do not happen (once you click your way to 256 channels...), and that large-id pre-negotiated channels work
Attachment #8502193 - Attachment is obsolete: true
Attachment #8502193 - Flags: sec-approval?
Attachment #8502693 - Flags: review?(tuexen)
Comment on attachment 8502693 [details] [diff] [review]
Simple patch that ups the number of static DataChannels and disables expansion


See previous request
Attachment #8502693 - Flags: sec-approval?
Attachment #8502693 - Flags: approval-mozilla-esr31?
Attachment #8502693 - Flags: approval-mozilla-aurora?
Attachment #8502193 - Attachment is obsolete: false

Updated

5 years ago
Attachment #8502693 - Flags: review?(tuexen) → review+
We already have release candidate builds for Firefox 33 and nothing here seems to say this is a chemspill-level corner-cutting bug. Let's make sure it's fixed right and get it into Firefox 34 when we can have the reassurance of some beta testing on the fix.
I'm going to give this sec-approval+ for checkin on 10/28 or later (two weeks into the next cycle since we ship on 10/14). Once it is on trunk, we should put it on Aurora and Beta so it gets the advantage of our beta releases.
Whiteboard: [checkin 10/28]
Attachment #8502693 - Flags: sec-approval? → sec-approval+
Comment on attachment 8502693 [details] [diff] [review]
Simple patch that ups the number of static DataChannels and disables expansion

Adding beta approval flag as on Oct 28, 34 will be Beta and 35 will be Aurora.
Attachment #8502693 - Flags: approval-mozilla-beta?
This is okay to check in now, from a sec-approval standpoint.
Flags: needinfo?(rjesup)
Comment on attachment 8502693 [details] [diff] [review]
Simple patch that ups the number of static DataChannels and disables expansion

Approving branch landings now too, since this can go to central today.
Attachment #8502693 - Flags: approval-mozilla-esr31?
Attachment #8502693 - Flags: approval-mozilla-esr31+
Attachment #8502693 - Flags: approval-mozilla-beta?
Attachment #8502693 - Flags: approval-mozilla-beta+
Attachment #8502693 - Flags: approval-mozilla-aurora?
Attachment #8502693 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c072fade4e36
Will land branches after we're green
Flags: needinfo?(rjesup)
https://hg.mozilla.org/mozilla-central/rev/c072fade4e36
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [checkin 10/28]
Target Milestone: --- → mozilla36
Whiteboard: [adv-main34+][adv-esr31.3+]
Confirmed crash in Fx34, 2014-09-15.
Verified fixed in shipping Fx34.0.5.
Verified fixed in Fx35, release candidate, 2015-01-06.
Verified fixed in Fx36, 2015-01-06.

Test case does not work on Fx31esr, so not able to verify fix on that branch.
Depends on: 1157887
No longer depends on: 1157887

Updated

4 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.