1183901 - DistributedContentList doesn't QI to nsWrapperCache, nor trace the wrapper

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: test, requires dom.webcomponents.enabled=true

#0  0x00007fffed488658 in nsWrapperCache::SetWrapperFlags(unsigned int) (this=0x0, aFlagsToSet=1) at /home/smaug/mozilla/hg/push-m-i/js/xpconnect/wrappers/../../../dom/base/nsWrapperCache.h:312
#1  0x00007fffed4885c4 in nsWrapperCache::SetPreservingWrapper(bool) (this=0x0, aPreserve=true) at /home/smaug/mozilla/hg/push-m-i/js/xpconnect/wrappers/../../../dom/base/nsWrapperCache.h:181
#2  0x00007fffeeed48e3 in mozilla::dom::DOMProxyHandler::EnsureExpandoObject(JSContext*, JS::Handle<JSObject*>) (cx=0x7fffb72ecc40, obj=...)
    at /home/smaug/mozilla/hg/push-m-i/dom/bindings/DOMJSProxyHandler.cpp:140
#3  0x00007fffeeedaa87 in mozilla::dom::DOMProxyHandler::defineProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JSPropertyDescriptor>, JS::ObjectOpResult&, bool*) const (this=0x7ffff52acb00 <mozilla::dom::NodeListBinding::DOMProxyHandler::getInstance()::instance>, cx=0x7fffb72ecc40, proxy=..., id=..., desc=..., result=..., defined=0x7fffffff667f)
    at /home/smaug/mozilla/hg/push-m-i/dom/bindings/DOMJSProxyHandler.cpp:184
#4  0x00007fffee42a509 in mozilla::dom::NodeListBinding::DOMProxyHandler::defineProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JSPropertyDescriptor>, JS::ObjectOpResult&, bool*) const (this=0x7ffff52acb00 <mozilla::dom::NodeListBinding::DOMProxyHandler::getInstance()::instance>, cx=0x7fffb72ecc40, proxy=..., id=..., desc=..., opresult=..., defined=0x7fffffff667f) at ./NodeListBinding.cpp:341
#5  0x00007fffee187c90 in mozilla::dom::DOMProxyHandler::defineProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JSPropertyDescriptor>, JS::ObjectOpResult&) const (this=0x7ffff52acb00 <mozilla::dom::NodeListBinding::DOMProxyHandler::getInstance()::instance>, cx=0x7fffb72ecc40, proxy=..., id=..., desc=..., result=...) at ../../dist/include/mozilla/dom/DOMJSProxyHandler.h:109
#6  0x00007ffff27fac0c in js::Proxy::defineProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JSPropertyDescriptor>, JS::ObjectOpResult&) (cx=0x7fffb72ecc40, proxy=..., id=..., desc=..., result=...) at /home/smaug/mozilla/hg/push-m-i/js/src/proxy/Proxy.cpp:147
#7  0x00007ffff27fdc1d in js::proxy_DefineProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JSPropertyDescriptor>, JS::ObjectOpResult&) (cx=0x7fffb72ecc40, obj=..., id=..., desc=..., result=...) at /home/smaug/mozilla/hg/push-m-i/js/src/proxy/Proxy.cpp:566

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

(note, I'm adding assertions to catch this kind of errors

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8633799 [details] [diff] [review]
patch

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

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Commit message would be
"Bug 1183901, properly support WrapperCache on DistributedContentList, r=wchen"

Which older supported branches are affected by this flaw?
I think all supported, BUT - only in case webcomponents are enabled, which means 
the right pref set Certified on b2g. So, not in Android/Desktop by default, and on b2g only certified apps

If not all supported branches, which bug introduced the flaw?
Bug 854736, which is FF28

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
the patch should apply to supported branches, if needed

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

Comment 4

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

sec-approval+. We should take this on affected branches as well.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8633799 [details] [diff] [review]
patch

[Approval Request Comment]
https://bugzilla.mozilla.org/show_bug.cgi?id=1183901#c3

The patch seems to apply cleanly to esr38 too.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e65e02c86544

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

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

(In reply to Olli Pettay [:smaug] (about to have some vacation, expect slower than usual reviews) from comment #3)
> Which older supported branches are affected by this flaw?
> I think all supported, BUT - only in case webcomponents are enabled, which
> means 
> the right pref set Certified on b2g. So, not in Android/Desktop by default,
> and on b2g only certified apps

(In reply to Al Billings [:abillings] from comment #4)
> sec-approval+. We should take this on affected branches as well.

Given that desktop/android are not impacted in their default configuration, do we really need to push a fix to all affected branches? Can we let this ride the trains?

Comment 9

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

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #8)

> Given that desktop/android are not impacted in their default configuration,
> do we really need to push a fix to all affected branches? Can we let this
> ride the trains?

That's a good point. Let's let it ride the trains.

Comment 10

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

Marking 40, 41 and ESR38 and ESR31 as wontfix.

Comment 11

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

Comment on attachment 8633799 [details] [diff] [review]
patch

Per comment 9, we're not going to uplift this patch.

Comment 12

[Andrew McCreight [:mccr8]]

Comment on attachment 8633799 [details] [diff] [review]
patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 854736
User impact if declined: crashes in certified apps that use WebComponents
Testing completed: just tree herder
Risk to taking this patch (and alternatives if risky): low, this is pretty standard stuff
String or UUID changes made by this patch: none

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8633799 [details] [diff] [review]
patch

This is sec-high, so it has auto-approval for uplift to affected active B2G branches.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f4eaa0dd7621
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/f4eaa0dd7621
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/a527e842db12

Comment 15

[Daniel Veditz [:dveditz]]

"disabled" is a less alarming branch status than "wontfix" for a sec-high bug (if it's true as it is here, of course!)

Comment 16

[Bogdan Maris, Desktop Test Engineering]

I used the test from comment 0 with old Nightly debug asan from 2015-07-14 and I receive something like this (http://pastebin.com/a6XEMhwC), and the browser closes or tab crashes. Is this enough to confirm that the initial issue reproduces?

Also verified that no crash or errors are displayed in terminal when using latest Nightly asan build from 2015-10-28 and Firefox 42.0RC across platforms (Ubuntu 14.04 64-bit, Mac OS X 10.11 and Windows 10 64-bit).

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

It is enough, and https://bugzilla.mozilla.org/show_bug.cgi?id=1183604 should make us assert hard if there are similar mistakes elsewhere.