Last Comment Bug 851353 - (CVE-2013-1730) compartment mismatch in nsXBLBinding::DoInitJSClass
(CVE-2013-1730)
: compartment mismatch in nsXBLBinding::DoInitJSClass
Status: RESOLVED FIXED
[asan][adv-main24+]
: sec-high
Product: Core
Classification: Components
Component: XBL (show other bugs)
: 21 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla26
Assigned To: Blake Kaplan (:mrbkap)
:
Mentors:
: 893527 (view as bug list)
Depends on: 733636
Blocks: 893527 896900
  Show dependency treegraph
 
Reported: 2013-03-14 16:39 PDT by sachin shinde
Modified: 2014-11-19 20:11 PST (History)
24 users (show)
abillings: sec‑bounty+
mrbkap: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
verified
+
verified
+
verified
fixed
verified
24+
fixed
fixed
fixed


Attachments
firefox_2.html (may crash!) (683 bytes, text/html)
2013-03-14 16:39 PDT, sachin shinde
no flags Details
Crashtest. v1 r=me (1.50 KB, patch)
2013-04-01 14:20 PDT, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
Details | Diff | Splinter Review
Naive solution (3.05 KB, patch)
2013-08-07 17:43 PDT, Blake Kaplan (:mrbkap)
bzbarsky: feedback+
Details | Diff | Splinter Review
Add crashtest. (1.57 KB, patch)
2013-08-07 17:44 PDT, Blake Kaplan (:mrbkap)
bobbyholley: review+
Details | Diff | Splinter Review
Addressing bz's review comments (7.35 KB, patch)
2013-08-08 11:55 PDT, Blake Kaplan (:mrbkap)
bzbarsky: review+
Details | Diff | Splinter Review
Rolled-up patch. (6.17 KB, patch)
2013-08-08 14:32 PDT, Blake Kaplan (:mrbkap)
mrbkap: review+
Details | Diff | Splinter Review
Add crashtest. (1.58 KB, patch)
2013-08-08 14:33 PDT, Blake Kaplan (:mrbkap)
mrbkap: review+
Details | Diff | Splinter Review
With that (6.29 KB, patch)
2013-08-08 16:04 PDT, Blake Kaplan (:mrbkap)
mrbkap: review+
abillings: sec‑approval+
Details | Diff | Splinter Review
Patch for Aurora (5.28 KB, patch)
2013-08-22 00:07 PDT, Blake Kaplan (:mrbkap)
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Patch for beta (5.23 KB, patch)
2013-08-22 00:08 PDT, Blake Kaplan (:mrbkap)
bajaj.bhavana: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Patch for esr17 (5.39 KB, patch)
2013-08-28 11:55 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
testcase showing comment 63 (848 bytes, text/html)
2013-08-30 19:18 PDT, Blake Kaplan (:mrbkap)
no flags Details
Patch for esr17 (5.84 KB, patch)
2013-08-30 19:19 PDT, Blake Kaplan (:mrbkap)
bzbarsky: review+
lukasblakk+bugs: approval‑mozilla‑esr17+
bajaj.bhavana: approval‑mozilla‑b2g18+
Details | Diff | Splinter Review

Description sachin shinde 2013-03-14 16:39:57 PDT
Created attachment 725161 [details]
firefox_2.html (may crash!)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:14.0) Gecko/20100101 Firefox/14.0.1
Build ID: 20120713224749

Steps to reproduce:

please see the attachments
Comment 1 sachin shinde 2013-03-14 16:41:29 PDT
this is testcase crashes on lates asan build
Comment 2 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2013-03-15 06:10:06 PDT
Do you have the ASAN report?
Comment 3 Matt Wobensmith [:mwobensmith][:matt:] 2013-03-25 15:26:23 PDT
*** Compartment mismatch 0x13e239040 vs. 0x141c8d040
ASAN:SIGSEGV
=================================================================
==12922== ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000107a2cee9 sp 0x7fff5fbef750 bp 0x7fff5fbef750 T0)
AddressSanitizer can not provide additional info.
    #0 0x107a2cee8 in js::CompartmentChecker::fail(JSCompartment*, JSCompartment*) (in XUL) + 56
    #1 0x107a2ce70 in js::CompartmentChecker::check(JSCompartment*) (in XUL) + 160
    #2 0x107a2583d in void js::assertSameCompartment<JSObject*>(JSContext*, JSObject* const&) (in XUL) + 189
    #3 0x1079fdf5d in JS_GetObjectId(JSContext*, JSObject*, jsid*) (in XUL) + 157
    #4 0x104ec28ea in nsXBLBinding::DoInitJSClass(JSContext*, JSObject*, JSObject*, nsCString const&, nsXBLPrototypeBinding*, JSObject**) (in XUL) + 554
    #5 0x104eecc36 in nsXBLProtoImpl::InitTargetObjects(nsXBLPrototypeBinding*, nsIScriptContext*, nsIContent*, nsIXPConnectJSObjectHolder**, JSObject**) (in XUL) + 694
    #6 0x104eec67c in nsXBLProtoImpl::InstallImplementation(nsXBLPrototypeBinding*, nsIContent*) (in XUL) + 556
    #7 0x104efd9bf in nsXBLService::LoadBindings(nsIContent*, nsIURI*, nsIPrincipal*, bool, nsXBLBinding**, bool*) (in XUL) + 1375
    #8 0x10426dee9 in nsCSSFrameConstructor::AddFrameConstructionItemsInternal(nsFrameConstructorState&, nsIContent*, nsIFrame*, nsIAtom*, int, bool, nsStyleContext*, unsigned int, nsCSSFrameConstructor::FrameConstructionItemList&) (in XUL) + 793
    #9 0x1042701e5 in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsIFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) (in XUL) + 1925
    #10 0x1042794a7 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsIFrame*, nsFrameItems&) (in XUL) + 4311
    #11 0x10427d2f7 in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsIFrame*, nsFrameItems&) (in XUL) + 1063
    #12 0x104296855 in nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsIFrame*, nsFrameItems&) (in XUL) + 453
    #13 0x10427042c in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsIFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) (in XUL) + 2508
    #14 0x104274a7d in nsCSSFrameConstructor::ConstructBlock(nsFrameConstructorState&, nsStyleDisplay const*, nsIContent*, nsIFrame*, nsIFrame*, nsStyleContext*, nsIFrame**, nsFrameItems&, bool, PendingBinding*) (in XUL) + 1149
    #15 0x1042733e3 in nsCSSFrameConstructor::ConstructDocElementFrame(mozilla::dom::Element*, nsILayoutHistoryState*, nsIFrame**) (in XUL) + 5347
    #16 0x104282536 in nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) (in XUL) + 1958
    #17 0x10439ee89 in PresShell::ContentInserted(nsIDocument*, nsIContent*, nsIContent*, int) (in XUL) + 473
    #18 0x1049fab9a in nsNodeUtils::ContentInserted(nsINode*, nsIContent*, int) (in XUL) + 698
    #19 0x1049d4ea9 in nsINode::doInsertChildAt(nsIContent*, unsigned int, bool, nsAttrAndChildArray&) (in XUL) + 825
    #20 0x1049d6e2a in nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) (in XUL) + 5002
    #21 0x10679e02e in mozilla::dom::NodeBinding::appendChild(JSContext*, JS::Handle<JSObject*>, nsINode*, unsigned int, JS::Value*) (in XUL) + 366
    #22 0x10679b279 in mozilla::dom::NodeBinding::genericMethod(JSContext*, unsigned int, JS::Value*) (in XUL) + 553
    #23 0x107b9c45a in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (in XUL) + 218
    #24 0x107b8df42 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (in XUL) + 738
    #25 0x107d32b21 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) (in XUL) + 65
    #26 0x107b8eb70 in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (in XUL) + 720
    #27 0x107c522ea in js::BaseProxyHandler::call(JSContext*, JSObject*, unsigned int, JS::Value*) (in XUL) + 250
    #28 0x107dc472f in js::Wrapper::call(JSContext*, JSObject*, unsigned int, JS::Value*) (in XUL) + 271
    #29 0x107dc7c31 in js::CrossCompartmentWrapper::call(JSContext*, JSObject*, unsigned int, JS::Value*) (in XUL) + 561
    #30 0x107c6ab98 in js::Proxy::call(JSContext*, JSObject*, unsigned int, JS::Value*) (in XUL) + 344
    #31 0x107c6f99b in proxy_Call(JSContext*, unsigned int, JS::Value*) (in XUL) + 59
    #32 0x107b9c45a in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (in XUL) + 218
    #33 0x107b8e2a2 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (in XUL) + 1602
    #34 0x107b8641f in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (in XUL) + 61951
    #35 0x107fc27ec in js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) (in XUL) + 860
    #36 0x107fc2d7b in CheckStackAndEnterMethodJIT(JSContext*, js::StackFrame*, void*, bool) (in XUL) + 299
    #37 0x107fc2ba6 in js::mjit::JaegerShot(JSContext*, bool) (in XUL) + 454
    #38 0x107b76f92 in js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (in XUL) + 946
    #39 0x107b8e067 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (in XUL) + 1031
    #40 0x107d32b21 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) (in XUL) + 65
    #41 0x107b8eb70 in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (in XUL) + 720
    #42 0x107a189ea in JS_CallFunctionValue(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::Value*) (in XUL) + 650
    #43 0x104f79fec in nsJSContext::CallEventHandler(nsISupports*, JSObject*, JSObject*, nsIArray*, nsIVariant**) (in XUL) + 1772
    #44 0x104fde7d1 in nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) (in XUL) + 1105
    #45 0x104fcce11 in nsGlobalWindow::RunTimeout(nsTimeout*) (in XUL) + 1857
    #46 0x104fde148 in nsGlobalWindow::TimerCallback(nsITimer*, void*) (in XUL) + 152
    #47 0x1069aa6f5 in nsTimerImpl::Fire() (in XUL) + 2277
    #48 0x1069aae86 in nsTimerEvent::Run() (in XUL) + 486
    #49 0x10699ddab in nsThread::ProcessNextEvent(bool, bool*) (in XUL) + 2139
    #50 0x1068e48ae in NS_ProcessPendingEvents_P(nsIThread*, unsigned int) (in XUL) + 254
    #51 0x10619a893 in nsBaseAppShell::NativeEventCallback() (in XUL) + 451
    #52 0x10611a83a in nsAppShell::ProcessGeckoEvents(void*) (in XUL) + 490
    #53 0x7fff938cfb30 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ (in CoreFoundation) + 16
    #54 0x7fff938cf454 in __CFRunLoopDoSources0 (in CoreFoundation) + 244
    #55 0x7fff938f27f4 in __CFRunLoopRun (in CoreFoundation) + 788
    #56 0x7fff938f20e1 in CFRunLoopRunSpecific (in CoreFoundation) + 289
    #57 0x7fff95d18eb3 in RunCurrentEventLoopInMode (in HIToolbox) + 208
    #58 0x7fff95d18b93 in ReceiveNextEventCommon (in HIToolbox) + 165
    #59 0x7fff95d18ae2 in BlockUntilNextEventMatchingListInMode (in HIToolbox) + 61
    #60 0x7fff8f29e562 in _DPSNextEvent (in AppKit) + 684
    #61 0x7fff8f29de21 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 127
    #62 0x106119085 in -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in XUL) + 245
    #63 0x7fff8f2951d2 in -[NSApplication run] (in AppKit) + 516
    #64 0x10611b419 in nsAppShell::Run() (in XUL) + 185
    #65 0x105cf6b07 in nsAppStartup::Run() (in XUL) + 311
    #66 0x103bbe0ff in XREMain::XRE_mainRun() (in XUL) + 4287
    #67 0x103bbf0b7 in XREMain::XRE_main(int, char**, nsXREAppData const*) (in XUL) + 599
    #68 0x103bbf562 in XRE_main (in XUL) + 146
    #69 0x100002d50 in 0x200002d50
    #70 0x1000023fa in 0x2000023fa
    #71 0x100001853 in 0x200001853
    #72 0x1 in 0x0000000100000001 (in firefox-bin)
Stats: 784M malloced (575M for red zones) by 1113439 calls
Stats: 116M realloced by 54557 calls
Stats: 713M freed by 881304 calls
Stats: 669M really freed by 793567 calls
Stats: 438M (112372 full pages) mmaped in 723 calls
  mmaps   by size class: 7:307125; 8:112585; 9:30690; 10:13286; 11:9435; 12:4480; 13:2880; 14:800; 15:816; 16:1128; 17:452; 18:36; 19:39; 20:22; 21:3; 22:4; 23:2; 24:2;
  mallocs by size class: 7:676464; 8:219539; 9:76161; 10:57438; 11:44653; 12:15244; 13:9520; 14:4729; 15:4647; 16:2691; 17:2081; 18:137; 19:79; 20:32; 21:6; 22:10; 23:3; 24:5;
  frees   by size class: 7:531389; 8:159446; 9:61629; 10:51213; 11:42023; 12:13260; 13:8865; 14:4325; 15:4469; 16:2412; 17:2053; 18:113; 19:58; 20:28; 21:5; 22:9; 23:2; 24:5;
  rfrees  by size class: 7:472614; 8:142432; 9:56914; 10:48374; 11:40134; 12:11945; 13:8279; 14:4182; 15:4160; 16:2288; 17:2033; 18:107; 19:58; 20:27; 21:5; 22:9; 23:2; 24:4;
Stats: malloc large: 9791 small slow: 16939
==12922== ABORTING
Comment 4 Daniel Veditz [:dveditz] 2013-03-26 18:22:52 PDT
I hope your User-Agent is spoofed and that you're not _really_ using Firefox 14 here (not sure we even had ASan builds of Fx14). Did you build this yourself? If so from which changeset, if not where did you get the build?

Reasonably sure "XBL" isn't the problem here since you're not using it directly. Most likely we've got a problem in the compartments implementation. Bill's recent "Zones" fix (bug 759585) changed a bunch of stuff there, but that landed a few days after you filed this bug so you couldn't be affected by that.

Could possibly be the XBL widget behind the <video> control doing something with compartments it shouldn't be doing.

Not concerned with the null deref since that appears to be us terminating the process on purpose after failing an assert, but failing a compartment assertion is generally not good. Since it is an assert this probably reproduces on a plain debug build (will have to try that next).
Comment 5 Daniel Veditz [:dveditz] 2013-03-27 00:59:49 PDT
Yup, self-kill on a non-ASan recent debug build:

WARNING: NS_ENSURE_TRUE(currentURI) failed: file dev/nightly/content/base/src/ThirdPartyUtil.cpp, line 96
*** Compartment mismatch 00000000229E5D50 vs. 000000001FBCA570
Comment 6 Bill McCloskey (:billm) 2013-03-27 11:18:14 PDT
In bug 782818, we made compartment mismatches fatal on nightly and aurora. However, they are not fatal on beta/release, so this is worse than a null deref.

I tried this in a nightly from 1/1/2013 and it still asserts, so this is not a new regression.
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2013-04-01 09:52:40 PDT
I can take a look.
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2013-04-01 10:46:33 PDT
Ok, so there are two issues here.

The first is that old footgun where nsContentUtils::WrapNative doesn't wrap its return value by default. Andrew was taking a look at flipping this default a while back, but I think it stalled at some point. I could spot fix this issue here, but I think we should fix this pattern everywhere at once, rather than just waiting for fuzzers to find the callsites that we already know to be broken. Marking the dep.

However, the issue of _why_ the object is cross-compartment is interesting. We're applying an XBL binding to aBoundElement (a XULElement), and so we grab the global associated with the content's document via aContent->OwnerDoc()->GetScopeObject(). However, this is _not_ the scope of aBoundElement->GetWrapper() because of a recent call to document.open().

We decided a while back to just leave new-binding reflectors in their old scope during document.open(), because we could always find them via the wrapper cache. The assumption was that we'd always end up reparenting them in CloneAndAdopt if they ever got inserted into the new document.

However, that doesn't seem to be happening here. aBoundElement->IsInDoc() returns true, as it ought to since we seem to be applying bindings here. So why did we never trigger the transplant here? Even if we fix the compartment issue here, this seems likely to cause problems quite soon after.

bz, what do you think?
Comment 9 Boris Zbarsky [:bz] 2013-04-01 10:50:15 PDT
About why we never reparented the element?  I have no idea....
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2013-04-01 11:02:05 PDT
(In reply to Boris Zbarsky (:bz) from comment #9)
> About why we never reparented the element?  I have no idea....

OK, I be peterv does.
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2013-04-01 14:20:59 PDT
Created attachment 732054 [details] [diff] [review]
Crashtest. v1 r=me
Comment 12 Peter Van der Beken [:peterv] - away till Aug 1st 2013-05-06 06:13:13 PDT
(In reply to Bobby Holley (:bholley) from comment #8)
> We decided a while back to just leave new-binding reflectors in their old
> scope during document.open(), because we could always find them via the
> wrapper cache. The assumption was that we'd always end up reparenting them
> in CloneAndAdopt if they ever got inserted into the new document.

document.open doesn't create a new document, so there's no reason to call CloneAndAdopt in this case.
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2013-05-06 13:39:21 PDT
(In reply to Peter Van der Beken [:peterv] from comment #12)
> document.open doesn't create a new document, so there's no reason to call
> CloneAndAdopt in this case.

Yes, but it ejects all the content from the document, meaning aBoundElement->IsInDoc() would start returning false. But it's returning true, which means that it has now somehow made its way into the document. So what's the deal?

If mrbkap would be a better person to investigate this, I could flag him instead.
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2013-05-20 12:51:47 PDT
Peter and I talked about this on IRC two weeks ago, I think. The basic issue was that, because the identity of the OwnerDoc doesn't change, we don't realize that we need to reparent the wrapper to a new scope when inserting it back into the document.

I just looked over CloneAndAdopt for a bit and didn't see any obvious place where this logic would be happening. I think Peter's probably the best person to write this patch.
Comment 15 Andrew McCreight [:mccr8] 2013-06-06 13:45:06 PDT
Any updates here, Peter?
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2013-07-29 16:23:20 PDT
Discussed this with jst and mccr8 over lunch, and agreed that we need to get some immediate traction on this. Blake, are you able to take a look this week?
Comment 17 Blake Kaplan (:mrbkap) 2013-07-30 14:00:46 PDT
I will try to get to this this week, but I can't guarantee it.
Comment 18 Blake Kaplan (:mrbkap) 2013-08-06 14:05:10 PDT
I spent a few hours last week trying to reproduce this bug and failed in all aspects (I went so far as to build a mac build from April 1, 2013 and that didn't reproduce either with the testcase here or bholley's crashtest). I don't understand how the testcase ever worked, actually, given that it seems to depend on the pattern:

document.open(); setTimeout(function, 0);

and as far as I can tell, function is guaranteed to not be called unless we reuse the inner window, in which case we don't get a new global JS object anyway. Given that, I don't understand how to patch things, either. Peter might have a better idea, though.
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2013-08-06 17:42:38 PDT
I just built an osx64 10.7 build of http://hg.mozilla.org/mozilla-central/rev/0b7c27024048 , and got an instant compartment mismatch running the crashtest (I also removed all the other crashtests from content/xbl/crashtests/crashtests.list FWIW).

My mozconfig was as follows:

mk_add_options MOZ_OBJDIR=/files/mozilla/build/z
mk_add_options MOZ_MAKE_FLAGS="-j4 -s"
ac_add_options --enable-debug
ac_add_options --disable-optimize
ac_add_options --enable-accessibility
CC=clang
CXX=clang++
AUTOCLOBBER=1

Blake, can you confirm whether you're able to reproduce that or not?
Comment 20 Blake Kaplan (:mrbkap) 2013-08-07 16:11:36 PDT
I have a testcase that can reproduce this now. Apparently we don't generate a document body after a document.open until you do document.write?
Comment 21 Blake Kaplan (:mrbkap) 2013-08-07 17:43:24 PDT
Created attachment 787243 [details] [diff] [review]
Naive solution

This is the most naive solution possible: for ever append, make sure
that the incoming object's JS object is up to date. This fixes the
testcase, but we should probably find a better solution that only
penalizes users of document.open.
Comment 22 Blake Kaplan (:mrbkap) 2013-08-07 17:44:16 PDT
Created attachment 787244 [details] [diff] [review]
Add crashtest.

This crashtest works reliably for me.
Comment 23 Blake Kaplan (:mrbkap) 2013-08-07 18:06:45 PDT
One solution that I'm prototyping now (but will have to test tomorrow is this):

Once we do document.open, all nodes in the old document become detached from the tree and all references to the document become proxies thanks to the magic of brain transplants. Therefore, any attempt to reattach a node from the old DOM into the document again will cause the DOM node's JS object to be wrapped into the new scope. If we set a bit on the old scope, we can detect when this happens and brain transplant the DOM node in PrepareForWrapping. This would let us assume everywhere else in the DOM code that the wrappers are up to date.
Comment 24 Bobby Holley (:bholley) (busy with Stylo) 2013-08-07 18:14:22 PDT
(In reply to Blake Kaplan (:mrbkap) from comment #23)
> One solution that I'm prototyping now (but will have to test tomorrow is
> this):
> 
> Once we do document.open, all nodes in the old document become detached from
> the tree and all references to the document become proxies thanks to the
> magic of brain transplants. Therefore, any attempt to reattach a node from
> the old DOM into the document again will cause the DOM node's JS object to
> be wrapped into the new scope.

Unless there was already a CCW for that wrapper in the target scope, in which case we hit the cache, right? That seems like a major problem.
Comment 25 Bobby Holley (:bholley) (busy with Stylo) 2013-08-07 18:16:55 PDT
Comment on attachment 787244 [details] [diff] [review]
Add crashtest.

Review of attachment 787244 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/crashtests/851353-1.html
@@ +2,5 @@
> +<html class="reftest-wait">
> +<head>
> +    <meta charset="UTF-8">
> +</head>
> +<body onload='start()'>

So, this doesn't race because the script is contained in the body element itself, right?

@@ +4,5 @@
> +    <meta charset="UTF-8">
> +</head>
> +<body onload='start()'>
> +    <script>
> +        function start(){

Nit - space.
Comment 26 Boris Zbarsky [:bz] 2013-08-07 20:29:43 PDT
Comment on attachment 787243 [details] [diff] [review]
Naive solution

I'm really not happy with the extra check as written on every append.

Is the issue that document.open() leaves the old nodes in the old scope but with the same owner document, so we never end up reparenting them if they get inserted into the new DOM?

The simple way to restrict the penalty to document.open() cases is to have a flag on nsIDocument that gets set if open() was ever called on it and restrict the checking to that case, I'd think.

Note also that nsINode inherits from nsWrapperCache, so there is no reason to QI to it. Furthermore, it seems like we should be able to compare compartments of the GetWrapper() and GetGlobalJSObject() without doing any slow AutoJSContext bits, and only start doing expensive stuff if the compartments don't match, right?
Comment 27 Bobby Holley (:bholley) (busy with Stylo) 2013-08-07 20:33:08 PDT
(In reply to Boris Zbarsky (:bz) (vacation Aug 10-19) from comment #26)
> Is the issue that document.open() leaves the old nodes in the old scope but
> with the same owner document, so we never end up reparenting them if they
> get inserted into the new DOM?

Yes.

> The simple way to restrict the penalty to document.open() cases is to have a
> flag on nsIDocument that gets set if open() was ever called on it and
> restrict the checking to that case, I'd think.

Clever idea.

Is there some way we can set their OwnerDoc to null when they get ejected in the document.open stuff? That seems more satisfying, but I don't know if it borks any invariants.
Comment 28 Boris Zbarsky [:bz] 2013-08-07 20:45:29 PDT
OwnerDoc() has the invariant of being never null, ever.  Not even after CC unlinking.  You can't break that.

.ownerDocument has the invariant of never being null for non-Document Node instances in the spec too.
Comment 29 Blake Kaplan (:mrbkap) 2013-08-08 00:01:07 PDT
(In reply to Bobby Holley (:bholley) from comment #24)
> Unless there was already a CCW for that wrapper in the target scope, in
> which case we hit the cache, right? That seems like a major problem.

My idea was to use the prewrap function (WrapperFactory::PrepareForWrapping) which is used to find the identity object for the cache lookup and therefore guaranteed to be called, whether or not the wrapper already exists.
Comment 30 Bobby Holley (:bholley) (busy with Stylo) 2013-08-08 09:25:48 PDT
(In reply to Blake Kaplan (:mrbkap) from comment #29)
> (In reply to Bobby Holley (:bholley) from comment #24)
> > Unless there was already a CCW for that wrapper in the target scope, in
> > which case we hit the cache, right? That seems like a major problem.
> 
> My idea was to use the prewrap function (WrapperFactory::PrepareForWrapping)
> which is used to find the identity object for the cache lookup and therefore
> guaranteed to be called, whether or not the wrapper already exists.

Oh, I see. This also relies on the fact that the new scope is totally brand new (in that it can't have a pre-existing CCW to the node). I think I like the first patch (combined with bz's optimization) better.
Comment 31 Blake Kaplan (:mrbkap) 2013-08-08 11:55:41 PDT
Created attachment 787711 [details] [diff] [review]
Addressing bz's review comments

This interdiff applies on top of attachment 787243 [details] [diff] [review] and fixes all of bz's
review comments. I hesitated between putting mDidDocumentOpen on
nsIDocument and making the getter inline and the current solution. Let
me know if you prefer that.
Comment 32 Boris Zbarsky [:bz] 2013-08-08 13:32:00 PDT
Comment on attachment 787711 [details] [diff] [review]
Addressing bz's review comments

Please just put the boolean member on nsIDocument instead of adding a common-case virtual function call here.

r=me with that.
Comment 33 Boris Zbarsky [:bz] 2013-08-08 13:34:20 PDT
Oh, you said that in comment 31.  Yeah, inline getter and boolean on nsIDocument is way better, imo.
Comment 34 Blake Kaplan (:mrbkap) 2013-08-08 14:32:59 PDT
Created attachment 787779 [details] [diff] [review]
Rolled-up patch.

This addresses all of bz's comments and is ready for checkin.
Comment 35 Blake Kaplan (:mrbkap) 2013-08-08 14:33:32 PDT
Created attachment 787781 [details] [diff] [review]
Add crashtest.
Comment 36 Blake Kaplan (:mrbkap) 2013-08-08 14:55:54 PDT
Comment on attachment 787779 [details] [diff] [review]
Rolled-up patch.

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

I'd give this medium to easy difficulty. The patch points pretty directly to document.open and appending nodes. The jump to using XBL (and namely video or marquee elements) would take a fair amount of digging, though.

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

The test paints a bulls-eye for sure.

Which older supported branches are affected by this flaw?

This goes back at least as far as Paris bindings and maybe even to slim wrappers, but I'm not sure.

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

Backports should be pretty easy for this bug.

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

Because this patch involves wrapper reparenting, it comes with a fair amount of risk. However, we are very unlikely to actually hit this code in the wild (document.open probably doesn't mix with the more "proper" DOM methods that often, especially in this case where we're pulling a node out of the document.open'd document).
Comment 37 Andrew McCreight [:mccr8] 2013-08-08 15:09:38 PDT
> This goes back at least as far as Paris bindings and maybe even to slim wrappers, but I'm not sure.
This (slim wrappers) means possibly to all supported branches.
Comment 38 Boris Zbarsky [:bz] 2013-08-08 15:13:18 PDT
Comment on attachment 787779 [details] [diff] [review]
Rolled-up patch.

Might be nice to put it with the other bool members...
Comment 39 Blake Kaplan (:mrbkap) 2013-08-08 16:04:22 PDT
Created attachment 787814 [details] [diff] [review]
With that
Comment 40 Blake Kaplan (:mrbkap) 2013-08-08 16:06:40 PDT
Comment on attachment 787814 [details] [diff] [review]
With that

Please see comment 36.
Comment 41 Al Billings [:abillings] 2013-08-08 16:44:19 PDT
Comment on attachment 787814 [details] [diff] [review]
With that

sec-approval+ for checkin after 8/20 (to bring it further into the cycle).
Comment 42 Al Billings [:abillings] 2013-08-08 16:47:05 PDT
I want specific Release Management comments about whether we want to take this on Aurora, Beta, or ESR given the risks.

Blake, we may need to quantify what that risk means. What happens if something goes wrong?
Comment 43 Blake Kaplan (:mrbkap) 2013-08-12 10:29:56 PDT
(In reply to Al Billings [:abillings] from comment #42)
> Blake, we may need to quantify what that risk means. What happens if
> something goes wrong?

The patch here will cause us to reparent more wrappers, albeit in a very rare situation. Wrapper reparenting has the chance of triggering rare bugs, most likely crashes, so adding more calls into the wrapper reparenting code carries some non-trivial amount of risk. That being said, I think the reward outweighs the risk here.
Comment 44 Alex Keybl [:akeybl] 2013-08-12 15:43:20 PDT
(In reply to Al Billings [:abillings] from comment #42)
> I want specific Release Management comments about whether we want to take
> this on Aurora, Beta, or ESR given the risks.
> 
> Blake, we may need to quantify what that risk means. What happens if
> something goes wrong?

Given client risk, I'd like to land early in the cycle (now) if we want to uplift to Beta 24. If we'd like this to land in Firefox 25 first, I'm OK with landing now but I can't weigh the risk of finding this critical problem.
Comment 45 Al Billings [:abillings] 2013-08-12 16:28:41 PDT
Ok. Blake, go ahead and land this on trunk now.
Comment 46 Andrew McCreight [:mccr8] 2013-08-15 13:49:11 PDT
This can be landed now.
Comment 47 Blake Kaplan (:mrbkap) 2013-08-15 14:40:49 PDT
Thanks for reminding me!

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb294710d8a
Comment 48 Carsten Book [:Tomcat] 2013-08-16 02:12:23 PDT
also now in https://hg.mozilla.org/mozilla-central/rev/ebb294710d8a

we still need to add the crashtest right?
Comment 49 Andrew McCreight [:mccr8] 2013-08-16 07:53:49 PDT
The crash test shouldn't get landed until the fix has made it out everywhere, which in the case of B2G it isn't entirely clear what that will mean.
Comment 50 bhavana bajaj [:bajaj] 2013-08-21 10:11:02 PDT
If we are ok with nightly testing we should go ahead and uplift this asap given comment 44, else this will have to go in Fx25.

Our next beta for Fx24 goes to build tomorrow morning PT, so please make sure to land by then.
Comment 51 Blake Kaplan (:mrbkap) 2013-08-22 00:07:37 PDT
Created attachment 793875 [details] [diff] [review]
Patch for Aurora

To note: neither the aurora nor beta patches change the IID of nsIDocument, which is OK because the function is inline.
Comment 52 Blake Kaplan (:mrbkap) 2013-08-22 00:08:16 PDT
Created attachment 793876 [details] [diff] [review]
Patch for beta
Comment 53 Blake Kaplan (:mrbkap) 2013-08-22 00:09:23 PDT
Comment on attachment 793875 [details] [diff] [review]
Patch for Aurora

I'm not sure if comment 50 counts as approval...
Comment 54 Blake Kaplan (:mrbkap) 2013-08-22 00:11:02 PDT
If the checkin-needed angels are content with comment 50 meaning approval, then this is ready to go. Otherwise, this needs stamps before being checked in.
Comment 55 Ryan VanderMeulen [:RyanVM] 2013-08-22 07:59:21 PDT
Works for me, though I'm sure bajaj will want to retroactively mark the branch-specific patches as approved for posterity's sake :)

https://hg.mozilla.org/releases/mozilla-aurora/rev/3ec2bbe316c2
https://hg.mozilla.org/releases/mozilla-beta/rev/0082355f37ca
Comment 56 bhavana bajaj [:bajaj] 2013-08-22 09:21:53 PDT
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #55)
> Works for me, though I'm sure bajaj will want to retroactively mark the
> branch-specific patches as approved for posterity's sake :)
> 
> https://hg.mozilla.org/releases/mozilla-aurora/rev/3ec2bbe316c2
> https://hg.mozilla.org/releases/mozilla-beta/rev/0082355f37ca

:) Thanks Ryan, done!
Comment 57 Andrew McCreight [:mccr8] 2013-08-22 16:35:22 PDT
*** Bug 893527 has been marked as a duplicate of this bug. ***
Comment 58 Al Billings [:abillings] 2013-08-26 16:21:43 PDT
Is an ESR patch being created soon?
Comment 59 Blake Kaplan (:mrbkap) 2013-08-28 11:55:55 PDT
Created attachment 796819 [details] [diff] [review]
Patch for esr17
Comment 60 Blake Kaplan (:mrbkap) 2013-08-28 16:25:26 PDT
Comment on attachment 796819 [details] [diff] [review]
Patch for esr17

This doesn't compile, sorry.
Comment 61 Blake Kaplan (:mrbkap) 2013-08-28 17:38:55 PDT
So, I am unconvinced that esr is affected by this. For the testcase here, which uses <video> (or <audio>), we definitely don't have to worry for esr17 because we're guaranteed to get a fat wrapper for those nodes, and they'll be correctly moved during a document.open.

The case that worries me is if we replace the <video> or <audio> with e.g. a <span> which gets a slim wrapper and then reattach the node. I don't think we'll update the wrapper cached slim wrapper, which could cause problems (though, again, I'm not sure about this). bholley, what do you think?
Comment 62 Bobby Holley (:bholley) (busy with Stylo) 2013-08-28 19:09:46 PDT
(In reply to Blake Kaplan (:mrbkap) from comment #61)
> So, I am unconvinced that esr is affected by this. For the testcase here,
> which uses <video> (or <audio>), we definitely don't have to worry for esr17
> because we're guaranteed to get a fat wrapper for those nodes, and they'll
> be correctly moved during a document.open.
> 
> The case that worries me is if we replace the <video> or <audio> with e.g. a
> <span> which gets a slim wrapper and then reattach the node. I don't think
> we'll update the wrapper cached slim wrapper, which could cause problems
> (though, again, I'm not sure about this). bholley, what do you think?

Yeah, I forgot about slim wrappers. Regardless of the actual XBL compartment mismatch here, the hazard is there for any non-XPCWN node. So I think we should land the backport.
Comment 63 Blake Kaplan (:mrbkap) 2013-08-30 19:14:55 PDT
(In reply to Bobby Holley (:bholley) from comment #62)
> Yeah, I forgot about slim wrappers. Regardless of the actual XBL compartment
> mismatch here, the hazard is there for any non-XPCWN node. So I think we
> should land the backport.

I found, much to my surprise, that it's actually possible to end up with a fat WN in the wrong compartment. I don't know if it's possible to actually cause a compartment mismatch, though. It's impossible to use XBL in content and any JS that accesses the object will simply get a CCW around the object. That being said, we should probably fix this anyway.
Comment 64 Blake Kaplan (:mrbkap) 2013-08-30 19:18:16 PDT
Created attachment 798112 [details]
testcase showing comment 63

In order to really see this testcase in action, put a breakpoint in math_sin, and once there in CheckForOutdatedParent.
Comment 65 Blake Kaplan (:mrbkap) 2013-08-30 19:19:29 PDT
Created attachment 798113 [details] [diff] [review]
Patch for esr17

This backport wasn't trivial. Asking for a second review (most of the differences were in CheckForOutdatedParent, everything else was simply merge conflicts).
Comment 66 Boris Zbarsky [:bz] 2013-08-30 19:26:22 PDT
Comment on attachment 798113 [details] [diff] [review]
Patch for esr17

r=me

I like it that our more modern code is shorter!  ;)
Comment 67 Ryan VanderMeulen [:RyanVM] 2013-09-03 05:46:09 PDT
https://hg.mozilla.org/releases/mozilla-esr17/rev/4d0feff4b2fb
Comment 68 Matt Wobensmith [:mwobensmith][:matt:] 2013-09-11 18:53:17 PDT
I was able to observe the crash in an ASan build of FF21 from 2013-01-21. 

Confirmed fixed in FF17esr, FF23, FF24, FF25 - all ASan builds from 2013-09-11.
Comment 69 bhavana bajaj [:bajaj] 2013-10-15 14:44:39 PDT
ni Blake to help with a backport patch that applies on mozilla-b2g18 .
Comment 70 Blake Kaplan (:mrbkap) 2013-10-15 17:23:28 PDT
Comment on attachment 798113 [details] [diff] [review]
Patch for esr17

This patch miraculously applies to b2g18 as-is.
Comment 71 bhavana bajaj [:bajaj] 2013-10-15 18:29:09 PDT
(In reply to Blake Kaplan (:mrbkap) from comment #70)
> Comment on attachment 798113 [details] [diff] [review]
> Patch for esr17
> 
> This patch miraculously applies to b2g18 as-is.

:) awesome ..Approved, landing away.
Comment 72 Ryan VanderMeulen [:RyanVM] 2013-10-16 06:07:32 PDT
https://hg.mozilla.org/releases/mozilla-b2g18/rev/2354421ea73f
Comment 74 Blake Kaplan (:mrbkap) 2013-10-16 16:30:29 PDT
I accidentally checked in the testcase for this bug https://hg.mozilla.org/integration/mozilla-inbound/rev/47416c6a473a sorry! (abillings says that it's probably ok).
Comment 75 Carsten Book [:Tomcat] 2013-10-17 05:12:27 PDT
fixed on central https://hg.mozilla.org/mozilla-central/rev/47416c6a473a
Comment 77 sachin shinde 2013-11-04 22:17:14 PST
If you do document.open() it clears out the dom tree and then you can see document.body == null.
Comment 78 sachin shinde 2013-11-04 22:38:19 PST
On Internet Explorer If there is any script AFTER document.open()(even settimeout) it never gets executed.my gut feelings is setTimeout causing the issue since it may be opened/queued in a different thread and thing that script gets executed after document.open()
Comment 79 Blake Kaplan (:mrbkap) 2013-11-05 00:16:09 PST
I'm still a little perplexed as to how the original testcase worked (I never actually saw it work, to be honest). You'll notice that my testcase got around the problem in comment 78 by using a second document that doesn't get document.open()ed.

Note You need to log in before you can comment on or make changes to this bug.