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

Status: VERIFIED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Max Jonas Werner [:Makkes]]

Attachment: webrtc-dc-testcourt.html

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

[Loic]

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

Comment 2

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

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.

Comment 3

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

The second crash (in the timer code) is known and being investigated by myself and Michael Tuexen

Comment 4

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

Attachment: webrtc-dc-testcourt.html (updated for Trickle ICE and SIPCC changes)

Updated test.  Assumes it only needs one ICE candidate....

Comment 5

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

Attachment: webrtc-dc-testcourt.html

Updated to avoid garbage collection destroying datachannels since they have no references to them.  Also fix logging of ondatachannel

Comment 6

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

Attachment: Fix handling of increasing number of SCTP channels used by DataChannels

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

Comment 7

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

Attachment: Simple patch that ups the number of static DataChannels and disables expansion

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)

Comment 8

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

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.

Comment 9

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

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

Comment 10

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

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

Comment 11

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

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.

Comment 12

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

Attachment: Simple patch that ups the number of static DataChannels and disables expansion

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

Comment 13

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

Comment on attachment 8502693 [details] [diff] [review]
Simple patch that ups the number of static DataChannels and disables expansion


See previous request

Comment 14

[Daniel Veditz [:dveditz]]

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.

Comment 15

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

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.

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 17

[Andrew McCreight [:mccr8]]

This is okay to check in now, from a sec-approval standpoint.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/c072fade4e36
Will land branches after we're green

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c072fade4e36

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Looks like Randell landed this on Aurora/Beta.

https://hg.mozilla.org/releases/mozilla-aurora/rev/054875648c9a
https://hg.mozilla.org/releases/mozilla-beta/rev/cc85ed51d280
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/cc85ed51d280

Comment 22

[Ryan VanderMeulen [:RyanVM]]

And esr31:
https://hg.mozilla.org/releases/mozilla-esr31/rev/3c5b07daee61

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e652d8489ee8
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/11f0341ce66e

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/e652d8489ee8

Comment 25

[Matt Wobensmith [:mwobensmith][:matt:]]

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.