876080 - Crash with expando on MediaStreamList

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase

==34094== ERROR: AddressSanitizer crashed on unknown address 0x00010e3b9450 (pc 0x00010e3b9450 sp 0x7fff5fbf5498 bp 0x7fff5fbf55b0 T0)
AddressSanitizer can not provide additional info.
    #0 0x10e3b944e in vtable for sipcc::PeerConnectionImpl (in XUL) + 15
    #1 0x1097e03cd in mozilla::dom::DOMProxyHandler::defineProperty [DOMJSProxyHandler.cpp:196]
    #2 0x10b0c47cf in js::BaseProxyHandler::set [jsproxy.cpp:262]
    #3 0x10b0e5d12 in js::Proxy::set [jsproxy.cpp:2539]
    #4 0x10b0eb4cb in proxy_SetGeneric [jsproxy.cpp:2851]
    #5 0x10afef772 in js::Interpret [jsinterp.cpp:2156]

Also crashes in non-ASan builds, but the crash location may be less predictable.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Bah.  We have:

'MediaStreamList': {
    'wrapperCache': False,
    'nativeOwnership': 'owned',

but proxy binding codegen assumes its objects are nsISupports and wrappercached for purposes of expandos... but don't enforce that at codegen time.  See also discussion in bug 871849.  I hadn't realize at the point bug 711628 happened that we had this restriction...

In any case, we need to either fix the codegen to not make assumptions like that or we need to fix this list to be sane.

Oh, and how exactly are we returning an 'owned' thing from an attribute anyway?  How does that work?  And why is it declared as "object" in the IDL, instead of the actual interface (which would have caught the problem with returning an 'owned' thing, though not the problem with the fact that a proxy can't be 'owned')?  :(

Nor marking a dep on bug 711628 for now to avoid people investigating this area too closely.

Peter, do you have time to fix this, or should I look on Tuesday?

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

How discoverable/exploitable is this? Are we looking at something with enough potential risk to users that it would drive a 21.0.1 here or is it more of a ride-along if something more serious comes up?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Discoverable, not sure.  I assume it's not too hard.

Exploitable, again not sure: we're making a virtual function call through the wrong vtable, or something, and I'm not sure how easy it is to make that misbehave.  Will depend on what's in the other vtable...

Comment 4

[Peter Van der Beken [:peterv]]

PeerConnection is not prefed on in FF 21 afaik, so if this is exploitable the easiest fix is to tell those people that enabled it to pref it off again.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I believe we're somewhat committed to shipping it in 22, but jesup should know for sure.

Are we sure MediaStreamList is the only thing affected?

Comment 6

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

We are committed to shipping PeerConnection in 22

Comment 7

[Maire Reavy [:mreavy]]

To add to jesup's comment above: We've made public statements that WebRTC is one of the big features coming in Fx22.   If we were to disable it at this point, it would trigger a press cycle and hurt the perceived forward progress of WebRTC as a web feature.

Comment 8

[bhavana bajaj [:bajaj]]

Not tracking for Fx21 given comment #4 and we are tracking and awaiting a fix in Fx22.

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 755564 [details] [diff] [review]
v1

r=me.  Thanks!

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 755564 [details] [diff] [review]
v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I don't think it's obvious.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The testcase does. I haven't included it in the patch.

Which older supported branches are affected by this flaw?

21 is affected, but the feature is disabled. Trunk, aurora and beta are all affected.

If not all supported branches, which bug introduced the flaw?

Bug 711628.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The backport should not be any different.

How likely is this patch to cause regressions; how much testing does it need?

Low risk.

Comment 13

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

Comment on attachment 755564 [details] [diff] [review]
v1

sec-approval+ for m-c. Please nominate branch patches after it goes into trunk so we don't ship this enabled.

Comment 14

[Boris Zbarsky [:bzbarsky]]

In the interest of getting this into beta4:

http://hg.mozilla.org/integration/mozilla-inbound/rev/e3931a13e31f

The patch attached in this bug applies as-is on Aurora.  I'll post a Beta version.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 755564 [details] [diff] [review]
v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 711628
User impact if declined: Easy-to-trigger crash, probably exploitable.
Testing completed (on m-c, etc.): Presumably no longer crashes on the testcase in
   this bug.
Risk to taking this patch (and alternatives if risky): Should be reasonably low.
   This object is now using the same binding setup as most things are.
String or IDL/UUID changes made by this patch:  None.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Er, except that inbound push failed.  So it's actually https://hg.mozilla.org/integration/mozilla-inbound/rev/3b8de2536ce0

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for beta 22

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b8de2536ce0

Comment 19

[Alex Keybl [:akeybl]]

Comment on attachment 755564 [details] [diff] [review]
v1

sec-critical, low risk, no reason to hold this back.

Comment 20

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/015e80be63a3
https://hg.mozilla.org/releases/mozilla-beta/rev/752541e2bfb9

Comment 21

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (X11; Linux i686; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
Build ID: 20130605070403, Firefox 22 beta 4

Verified the fix using the testcase from bug description: Firefox doesn't crash anymore.

Comment 22

[u279076]

Mihaela, please check if this is fixed in Firefox 23 and 24, thanks.

Comment 23

[Mihaela Velimiroviciu (:mihaelav)]

Verified on 23 Beta 5, build ID: 20130711122148:
Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0

...and Aurora:
Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20130708 Firefox/24.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20130711 Firefox/24.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130711 Firefox/24.0

There are no crashes when loading the testcase attaced in bug description.