801227 - WebRTC crash [@mozilla::MediaManager::GetUserMedia]

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

This testcase simulates an abrupt abort during the setup of a valid WebRTC session. The result is a use after free.

alloc
mozilla::MediaManager::GetUserMedia mozalloc.h:200

free
mozilla::MediaManager::OnNavigation MediaManager.cpp:827

reuse
mozilla::GetUserMediaStreamRunnable::Run MediaManager.cpp:257

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

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

Adding anant, author of this code

Comment 3

[Anant Narayanan [:anant]]

Try run is ongoing at https://tbpl.mozilla.org/?tree=Try&rev=caf3a0111ea0

Once we have an ASAN build from that run, we can verify that it fixes this issue.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This is btw a regression. My debug build from Oct 12th didn't crash while a more recent one does.

Comment 5

[Jason Smith [:jsmith]]

Regression window wanted isn't warranted here - we've already got a test case that reproduces the issue consistently and a fix has already put out in that try build to see if it fixes the issue.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: crashtest v1

This is the reduced crashtest for the attached testcase. Please do not check-in before the actual code lands. It's also based on my patch on bug 791278 to not cause broken hunks.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: crashtest v1

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jason Smith [:jsmith] from comment #5)
> Regression window wanted isn't warranted here - we've already got a test
> case that reproduces the issue consistently and a fix has already put out in
> that try build to see if it fixes the issue.

This is not a justification that we are not interested in the regressing bug. It's always helpful to know which changeset has caused the regression. Only that way we can explicitly test the code path.

Comment 9

[Anant Narayanan [:anant]]

Attachment: Patch v1

Is it possible to fetch a ASAN build from that try run to see if it fixes the issue? I'm uploading the patch in any case, so you can also run a local build.

Comment 10

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

Yes, just click on a B and "Go to build directory".

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-caf3a0111ea0/

Comment 11

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> (In reply to Jason Smith [:jsmith] from comment #5)
> > Regression window wanted isn't warranted here - we've already got a test
> > case that reproduces the issue consistently and a fix has already put out in
> > that try build to see if it fixes the issue.
> 
> This is not a justification that we are not interested in the regressing
> bug. It's always helpful to know which changeset has caused the regression.
> Only that way we can explicitly test the code path.

No. Usually we use these keywords as an informational element to get enough information to solve the problem. We can't look at every single bug (as there's too many against our existing resources), so we can't be perfect with having every little detail, but if there's enough information to solve the problem, then that's acceptable. Abusing the flag though is going to do nothing but cause confusion between where the regression window is actually warranted (e.g. cases where root cause is not clear and foggy) vs. others where there's enough clarity.

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Does this affect FF 18?

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Yes, it does even with a bit different stack: 

bp-75c780f4-4225-4964-afb7-2fcd22121019

0 	XUL 	mozilla::MediaPipelineReceiveAudio::Init 	nsDOMMediaStream.h:37
1 	XUL 	mozilla::MediaPipelineReceiveAudio::MediaPipelineReceiveAudio 	MediaPipeline.h:265
2 	XUL 	vcmRxStartICE 	MediaPipeline.h:266
3 	XUL 	lsm_rx_start 	lsm.c:1042
4 	XUL 	lsm_update_media 	lsm.c:3905
5 	XUL 	cc_call_state 	lsm.c:3951
6 	XUL 	fsmdef_ev_setlocaldesc 	fsmdef.c:3160
7 	XUL 	sm_process_event 	sm.c:83
8 	XUL 	fim_process_event 	fim.c:671
9 	XUL 	gsm_process_msg 	gsm.c:167
10 	XUL 	GSMTask 	gsm.c:359
11 	libsystem_c.dylib 	libsystem_c.dylib@0x4e8be 	
12 	libsystem_c.dylib 	libsystem_c.dylib@0x51b74 	
13 	XUL 	XUL@0x14c9aff

Comment 14

[Anant Narayanan [:anant]]

David, PeerConnection functionality is available in FF 18 but is behind a pref, so it will affect only a small set of developers who have the pref turned on. We would still like to uplift this to Aurora though.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Is the patch ready to land? If yes, we should also include the crashtest.

Comment 16

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

Let's refresh this patch (other changes have landed), reverify the problem exists, and ask for sec-approval (since apparently the bug existed, even if in a slightly different form, in 18)

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

Anant are you able to refresh per comment 16?

Comment 18

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

Attachment: Updated testcase for api change (createAnswer)

Comment 19

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

Attachment: Patch v2 (unbitrotted)

Comment 20

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

Comment on attachment 680452 [details] [diff] [review]
Patch v2 (unbitrotted)

Since I had to significantly un-bitrot it, r? to anant

Comment 21

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

We need someone to check this with an ASAN build, and also verify that the warnings we get mean it's a valid test or not: 
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "this._dompc._pc is null" {file: "file:///home/jesup/src/mozilla/inbound/obj-x86_64-unknown-linux-gnu-debug/dist/bin/components/PeerConnection.js" line: 612}]' when calling method: [IPeerConnectionObserver::onStateChange]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
************************************************************

Comment 23

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

Henrik, can you verify if this covers your issues?  I'll push a try build later

Comment 24

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

Henrik, can you verify if this covers your issues?  I'll push a try build later

Comment 25

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

The warnings are probably expected, as once the inner window is destroyed pc_ is set to null.

Comment 26

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

https://tbpl.mozilla.org/?tree=Try&rev=3f0fff5a6fdf

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Looks like the try build fixes the problem. I can't get my crashtest to crash Firefox anymore.

Comment 28

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

henrik: just to verify, did you use the updated testcase?  There was an API change to createAnswer() that caused the previous test to no longer do what it had been doing.

In any case, I think it's good to commit this given your results

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

No, I haven't used this updated testcase. Could you transform it into a crashtest? It would have been more obvious for me if you had replaced my created crashtest.

Comment 30

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

The crashtest is not affected by the API change; the testcase was.   So your test was valid.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The updated testcase still crashes Minefield after a couple of seconds:

Report: bp-77daa719-5d51-48af-84e8-e40402121112

Could be something different.

Comment 32

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

Comment on attachment 680452 [details] [diff] [review]
Patch v2 (unbitrotted)

[Security approval request comment]
How easily can the security issue be deduced from the patch?
  Not especially easily.  That there's exposure of the listeners list is visible, however, and they may deduce that's what to look at.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
  Yes, as it clearly shows the use-after-free

Which older supported branches are affected by this flaw?
  I believe 18, 17, 16, esr17, though the code has been changed a moderate amount surrounding this.  Note again that GetUserMedia is preffed off in all these releases.  I suggest it be turned off at config level for esr17 as a precaution and since it doesn't add anything to an ESR release (if it has not been turned off already).

If not all supported branches, which bug introduced the flaw?
   Bug 752353 (initial DOM checkin, not built be default) and bug 691234 (enable gUM DOM on Desktop).

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
   No, we don't have backports, but they should be fairly easy to produce with some changes to the GetActiveWindows portion of the patch (either use more of the pre-bitrot version, or port more of the updated code to encapsulate the ActiveWindows code behind methods).

How likely is this patch to cause regressions; how much testing does it need?
   Unlikely to cause regressions, as it simply moves checks of window liveness to when it's used instead of when the runnable is created.  Testing would be basic functionality.

Comment 33

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

Comment on attachment 680452 [details] [diff] [review]
Patch v2 (unbitrotted)

sec-approval+

Comment 34

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

(In reply to Henrik Skupin (:whimboo) from comment #31)
> The updated testcase still crashes Minefield after a couple of seconds:
> 
> Report: bp-77daa719-5d51-48af-84e8-e40402121112
> 
> Could be something different.

The stack appears to be useless, however, the crash is likely a security-safe null-ptr deref (0x000000d), so it's not *likely* to be the same bug.  Any chance of reproducing with real symbols?  (gdb?)  Given the crash appearing to be a null-ptr deref, I think we're still safe to land on m-c.

I cannot get the updated testcase to crash for me on Linux (debug), loaded from a local file.  What build did you test from the try run, exactly?

Comment 35

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d28022bc22a

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5d28022bc22a

Comment 37

[Jason Smith [:jsmith]]

Crash test is here, but an hg link showing it checked in. Do we need to check it in or is there no link here?

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I will do that once I have verified that this patch fixed the problem and the crashtest is valid.

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 672323 [details] [diff] [review]
crashtest v1

This crashtest will not work because it's spinning an infinite loop. I most likely have to implement it inside an iframe so we only reload it once. I will have an updated version tomorrow.

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So the iframe method doesn't work because we end-up in another crash. See bug 812122. I will try to use localStorage for it, so we reload the page itself and not a sub page which seems to be the issue of the new bug.

Comment 41

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: crashtest v2

Updated crashtest which makes use of the localStorage. I have pushed it to try:
https://tbpl.mozilla.org/?tree=Try&rev=7234c950ce7d

Comment 42

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

Comment on attachment 681920 [details] [diff] [review]
crashtest v2

Also verified it crashes without the fix, and doesn't with the fix (linux64 debug)

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 681920 [details] [diff] [review]
crashtest v2

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c09ed66c045

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c09ed66c045