Closed Bug 876080 Opened 9 years ago Closed 9 years ago

Crash with expando on MediaStreamList


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

Not set



Tracking Status
firefox21 - disabled
firefox22 + verified
firefox23 + verified
firefox24 + verified
firefox-esr17 --- unaffected
b2g18 --- unaffected
b2g18-v1.0.0 --- unaffected
b2g18-v1.0.1 --- unaffected


(Reporter: jruderman, Assigned: peterv)


(Keywords: crash, sec-critical, testcase, Whiteboard: Caused by bug 711628 [adv-main22-][don't mark dependency][webrtc][blocking-webrtc+][webrtc_uplift])

Crash Data


(3 files, 1 obsolete file)

Attached file 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.
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?
Assignee: nobody → bzbarsky
Flags: needinfo?(peterv)
Whiteboard: Caused by bug 711628
Whiteboard: Caused by bug 711628 → Caused by bug 711628 [don't mark dependency]
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?
Assignee: bzbarsky → peterv
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...
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.
Flags: needinfo?(peterv)
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?
We are committed to shipping PeerConnection in 22
Whiteboard: Caused by bug 711628 [don't mark dependency] → Caused by bug 711628 [don't mark dependency][webrtc]
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.
Not tracking for Fx21 given comment #4 and we are tracking and awaiting a fix in Fx22.
Whiteboard: Caused by bug 711628 [don't mark dependency][webrtc] → Caused by bug 711628 [don't mark dependency][webrtc][blocking-webrtc+]
Attached patch v1 (obsolete) — Splinter Review
Attachment #755562 - Flags: review?(bzbarsky)
Attached patch v1Splinter Review
Attachment #755562 - Attachment is obsolete: true
Attachment #755562 - Flags: review?(bzbarsky)
Attachment #755564 - Flags: review?(bzbarsky)
Comment on attachment 755564 [details] [diff] [review]

r=me.  Thanks!
Attachment #755564 - Flags: review?(bzbarsky) → review+
Whiteboard: Caused by bug 711628 [don't mark dependency][webrtc][blocking-webrtc+] → Caused by bug 711628 [don't mark dependency][webrtc][blocking-webrtc+][webrtc_uplift]
Comment on attachment 755564 [details] [diff] [review]

[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.
Attachment #755564 - Flags: sec-approval?
Comment on attachment 755564 [details] [diff] [review]

sec-approval+ for m-c. Please nominate branch patches after it goes into trunk so we don't ship this enabled.
Attachment #755564 - Flags: sec-approval? → sec-approval+
In the interest of getting this into beta4:

The patch attached in this bug applies as-is on Aurora.  I'll post a Beta version.
Flags: in-testsuite?
Target Milestone: --- → mozilla24
Comment on attachment 755564 [details] [diff] [review]

[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.
Attachment #755564 - Flags: approval-mozilla-aurora?
Er, except that inbound push failed.  So it's actually
Assignee: peterv → bzbarsky
Assignee: bzbarsky → peterv
Attachment #756640 - Flags: approval-mozilla-beta?
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 755564 [details] [diff] [review]

sec-critical, low risk, no reason to hold this back.
Attachment #755564 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #756640 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Whiteboard: Caused by bug 711628 [don't mark dependency][webrtc][blocking-webrtc+][webrtc_uplift] → Caused by bug 711628 [adv-main22-][don't mark dependency][webrtc][blocking-webrtc+][webrtc_uplift]
Mihaela, please check if this is fixed in Firefox 23 and 24, thanks.
Keywords: verifyme
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.
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.