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

RESOLVED FIXED in Firefox 42, Firefox OS v2.1S

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Blocks: 1 bug, {sec-high})

36 Branch
mozilla42
sec-high
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox39 disabled, firefox40+ disabled, firefox41+ disabled, firefox42+ fixed, firefox-esr31- disabled, firefox-esr3840+ disabled, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-v2.2r fixed, b2g-master fixed)

Details

(Whiteboard: [adv-main42-])

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
Created attachment 8633775 [details]
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
(Assignee)

Updated

2 years ago
Blocks: 811542
Keywords: sec-high
(Assignee)

Updated

2 years ago
Assignee: nobody → bugs
(Assignee)

Comment 1

2 years ago
(note, I'm adding assertions to catch this kind of errors
(Assignee)

Comment 2

2 years ago
Created attachment 8633799 [details] [diff] [review]
patch
Attachment #8633799 - Flags: review?(wchen)

Updated

2 years ago
Attachment #8633799 - Flags: review?(wchen) → review+
(Assignee)

Comment 3

2 years ago
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.
Attachment #8633799 - Flags: sec-approval?
Blocks: 1183604
sec-approval+. We should take this on affected branches as well.
status-firefox39: --- → wontfix
status-firefox40: --- → affected
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox-esr31: --- → affected
status-firefox-esr38: --- → affected
tracking-firefox40: --- → +
tracking-firefox41: --- → +
tracking-firefox42: --- → +
tracking-firefox-esr31: --- → ?
tracking-firefox-esr38: --- → 40+
Attachment #8633799 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 5

2 years ago
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.
Attachment #8633799 - Flags: approval-mozilla-esr38?
Attachment #8633799 - Flags: approval-mozilla-beta?
Attachment #8633799 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e65e02c86544
https://hg.mozilla.org/mozilla-central/rev/e65e02c86544
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-b2g-master: --- → fixed
status-firefox42: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(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?
Flags: needinfo?(abillings)
(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.
Flags: needinfo?(abillings)
Marking 40, 41 and ESR38 and ESR31 as wontfix.
status-firefox40: affected → wontfix
status-firefox41: affected → wontfix
status-firefox-esr31: affected → wontfix
status-firefox-esr38: affected → wontfix
tracking-firefox-esr31: ? → -
Comment on attachment 8633799 [details] [diff] [review]
patch

Per comment 9, we're not going to uplift this patch.
Attachment #8633799 - Flags: approval-mozilla-esr38?
Attachment #8633799 - Flags: approval-mozilla-esr38-
Attachment #8633799 - Flags: approval-mozilla-beta?
Attachment #8633799 - Flags: approval-mozilla-beta-
Attachment #8633799 - Flags: approval-mozilla-aurora?
Attachment #8633799 - Flags: approval-mozilla-aurora-
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
Attachment #8633799 - Flags: approval-mozilla-b2g37?
Attachment #8633799 - Flags: approval-mozilla-b2g34?
Attachment #8633799 - Flags: approval-mozilla-b2g32?
Attachment #8633799 - Flags: approval‑mozilla‑b2g37_v2_2r?
status-b2g-v2.0: --- → wontfix
status-b2g-v2.0M: --- → wontfix
status-b2g-v2.1: --- → wontfix
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-v2.2r: --- → affected
Comment on attachment 8633799 [details] [diff] [review]
patch

This is sec-high, so it has auto-approval for uplift to affected active B2G branches.
Attachment #8633799 - Flags: approval‑mozilla‑b2g37_v2_2r?
Attachment #8633799 - Flags: approval-mozilla-b2g37?
Attachment #8633799 - Flags: approval-mozilla-b2g34?
Attachment #8633799 - Flags: approval-mozilla-b2g32?
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
status-b2g-v2.1S: affected → fixed
status-b2g-v2.2: affected → fixed
status-b2g-v2.2r: affected → fixed
"disabled" is a less alarming branch status than "wontfix" for a sec-high bug (if it's true as it is here, of course!)
status-firefox39: wontfix → disabled
status-firefox40: wontfix → disabled
status-firefox41: wontfix → disabled
status-firefox-esr31: wontfix → disabled
status-firefox-esr38: wontfix → disabled
Group: dom-core-security → release-core-security
Whiteboard: [adv-main42-]
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).
Flags: needinfo?(bugs)
(Assignee)

Comment 17

2 years ago
It is enough, and https://bugzilla.mozilla.org/show_bug.cgi?id=1183604 should make us assert hard if there are similar mistakes elsewhere.
Flags: needinfo?(bugs)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.