Bug 851353 (CVE-2013-1730)

compartment mismatch in nsXBLBinding::DoInitJSClass

RESOLVED FIXED in Firefox 24, Firefox OS v1.1hd

Status

()

Core
XBL
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: sachin shinde, Assigned: mrbkap)

Tracking

({sec-high})

21 Branch
mozilla26
x86_64
Linux
sec-high
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox23 wontfix, firefox24+ verified, firefox25+ verified, firefox26+ verified, firefox27 fixed, firefox-esr17 verified, b2g1824+ fixed, b2g-v1.1hd fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [asan][adv-main24+])

Attachments

(7 attachments, 6 obsolete attachments)

683 bytes, text/html
Details
1.58 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
6.29 KB, patch
mrbkap
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
5.28 KB, patch
Details | Diff | Splinter Review
5.23 KB, patch
Details | Diff | Splinter Review
848 bytes, text/html
Details
5.84 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
this is testcase crashes on lates asan build
Do you have the ASAN report?
Flags: needinfo?(sachinshinde11)
Flags: needinfo?(sachinshinde11) → needinfo?(mwobensmith)
Whiteboard: [asan]
*** 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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mwobensmith)
Component: Untriaged → XBL
Product: Firefox → Core
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).
Component: XBL → General
Summary: firefox crash at address 0 → compartment mismatch in nsXBLBinding::DoInitJSClass
Attachment #725161 - Attachment description: firefox_2.html → firefox_2.html (may crash!)
Attachment #725161 - Attachment mime type: text/plain → text/html
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
Component: General → XBL
Keywords: sec-high
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.
I can take a look.
Assignee: nobody → bobbyholley+bmo
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?
Depends on: 733636
Flags: needinfo?(bzbarsky)
About why we never reparented the element?  I have no idea....
Flags: needinfo?(bzbarsky)
(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.
Flags: needinfo?(peterv)
Created attachment 732054 [details] [diff] [review]
Crashtest. v1 r=me
Attachment #732054 - Flags: review+
(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.
Flags: needinfo?(peterv)
(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.
Flags: needinfo?(peterv)
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.
Assignee: bobbyholley+bmo → peterv
Any updates here, Peter?

Updated

4 years ago
Blocks: 893527
Blocks: 896900
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?
Flags: needinfo?(mrbkap)
(Assignee)

Comment 17

4 years ago
I will try to get to this this week, but I can't guarantee it.
Flags: needinfo?(mrbkap)
(Assignee)

Updated

4 years ago
Assignee: peterv → mrbkap
(Assignee)

Comment 18

4 years ago
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.
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?
Flags: needinfo?(mrbkap)
(Assignee)

Comment 20

4 years ago
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?
Flags: needinfo?(mrbkap)
(Assignee)

Comment 21

4 years ago
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.
Attachment #787243 - Flags: feedback?(bzbarsky)
(Assignee)

Comment 22

4 years ago
Created attachment 787244 [details] [diff] [review]
Add crashtest.

This crashtest works reliably for me.
Attachment #787244 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 23

4 years ago
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.
(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 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.
Attachment #787244 - Flags: review?(bobbyholley+bmo) → review+
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?
Attachment #787243 - Flags: feedback?(bzbarsky) → feedback+
(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.
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.
(Assignee)

Comment 29

4 years ago
(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.
(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.
(Assignee)

Comment 31

4 years ago
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.
Attachment #787711 - Flags: review?
(Assignee)

Updated

4 years ago
Attachment #787711 - Flags: review? → review?(bzbarsky)
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.
Attachment #787711 - Flags: review?(bzbarsky) → review+
Oh, you said that in comment 31.  Yeah, inline getter and boolean on nsIDocument is way better, imo.
(Assignee)

Comment 34

4 years ago
Created attachment 787779 [details] [diff] [review]
Rolled-up patch.

This addresses all of bz's comments and is ready for checkin.
Attachment #787243 - Attachment is obsolete: true
Attachment #787711 - Attachment is obsolete: true
Attachment #787779 - Flags: review+
(Assignee)

Comment 35

4 years ago
Created attachment 787781 [details] [diff] [review]
Add crashtest.
Attachment #732054 - Attachment is obsolete: true
Attachment #787244 - Attachment is obsolete: true
Attachment #787781 - Flags: review+
(Assignee)

Comment 36

4 years ago
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).
Attachment #787779 - Flags: sec-approval?
> 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 on attachment 787779 [details] [diff] [review]
Rolled-up patch.

Might be nice to put it with the other bool members...
(Assignee)

Comment 39

4 years ago
Created attachment 787814 [details] [diff] [review]
With that
Attachment #787779 - Attachment is obsolete: true
Attachment #787779 - Flags: sec-approval?
Attachment #787814 - Flags: review+
(Assignee)

Comment 40

4 years ago
Comment on attachment 787814 [details] [diff] [review]
With that

Please see comment 36.
Attachment #787814 - Flags: sec-approval?
Comment on attachment 787814 [details] [diff] [review]
With that

sec-approval+ for checkin after 8/20 (to bring it further into the cycle).
Attachment #787814 - Flags: sec-approval? → sec-approval+
Whiteboard: [asan] → [asan][checkin after 8/20 for trunk]
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?
status-b2g18: --- → affected
status-firefox23: --- → wontfix
status-firefox24: --- → affected
status-firefox25: --- → affected
status-firefox26: --- → affected
status-firefox-esr17: --- → affected
tracking-b2g18: --- → ?
tracking-firefox24: --- → ?
tracking-firefox25: --- → ?
tracking-firefox26: --- → +
tracking-firefox-esr17: --- → ?
(Assignee)

Comment 43

4 years ago
(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.
Flags: needinfo?(peterv)
(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.
Ok. Blake, go ahead and land this on trunk now.
Whiteboard: [asan][checkin after 8/20 for trunk] → [asan]
This can be landed now.
Flags: needinfo?(mrbkap)
(Assignee)

Comment 47

4 years ago
Thanks for reminding me!

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb294710d8a
Flags: needinfo?(mrbkap)

Updated

4 years ago
tracking-b2g18: ? → 24+
tracking-firefox24: ? → +
tracking-firefox25: ? → +
tracking-firefox-esr17: ? → 24+
also now in https://hg.mozilla.org/mozilla-central/rev/ebb294710d8a

we still need to add the crashtest right?
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
status-firefox26: affected → fixed
Target Milestone: --- → mozilla26
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.
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.
Flags: needinfo?(mrbkap)
(Assignee)

Comment 51

4 years ago
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.
(Assignee)

Comment 52

4 years ago
Created attachment 793876 [details] [diff] [review]
Patch for beta
(Assignee)

Comment 53

4 years ago
Comment on attachment 793875 [details] [diff] [review]
Patch for Aurora

I'm not sure if comment 50 counts as approval...
Attachment #793875 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

4 years ago
Attachment #793876 - Flags: approval-mozilla-beta?
(Assignee)

Comment 54

4 years ago
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.
Flags: needinfo?(mrbkap)
Keywords: checkin-needed
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
status-firefox24: affected → fixed
status-firefox25: affected → fixed
Flags: needinfo?(bbajaj)
Keywords: checkin-needed

Updated

4 years ago
Attachment #793875 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

4 years ago
Attachment #793876 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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!
Flags: needinfo?(bbajaj)
Duplicate of this bug: 893527
Is an ESR patch being created soon?
Whiteboard: [asan] → [asan][adv-main24+]
Alias: CVE-2013-1730
Flags: needinfo?(mrbkap)
(Assignee)

Comment 59

4 years ago
Created attachment 796819 [details] [diff] [review]
Patch for esr17
(Assignee)

Updated

4 years ago
Attachment #796819 - Flags: review+
Attachment #796819 - Flags: approval-mozilla-esr17?
(Assignee)

Comment 60

4 years ago
Comment on attachment 796819 [details] [diff] [review]
Patch for esr17

This doesn't compile, sorry.
Attachment #796819 - Attachment is obsolete: true
Attachment #796819 - Flags: review+
Attachment #796819 - Flags: approval-mozilla-esr17?
(Assignee)

Comment 61

4 years ago
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?
status-firefox-esr17: affected → unaffected
tracking-firefox-esr17: 24+ → ---
Flags: needinfo?(mrbkap) → needinfo?(bobbyholley+bmo)
(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.
Flags: needinfo?(bobbyholley+bmo)
(Assignee)

Comment 63

4 years ago
(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.
(Assignee)

Comment 64

4 years ago
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.
(Assignee)

Comment 65

4 years ago
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).
Attachment #798113 - Flags: review?(bzbarsky)
Comment on attachment 798113 [details] [diff] [review]
Patch for esr17

r=me

I like it that our more modern code is shorter!  ;)
Attachment #798113 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

4 years ago
Attachment #798113 - Flags: approval-mozilla-esr17?
Attachment #798113 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-esr17/rev/4d0feff4b2fb
status-firefox-esr17: unaffected → fixed
Keywords: checkin-needed
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.
status-firefox24: fixed → verified
status-firefox25: fixed → verified
status-firefox26: fixed → verified
status-firefox-esr17: fixed → verified
ni Blake to help with a backport patch that applies on mozilla-b2g18 .
Flags: needinfo?(mrbkap)
(Assignee)

Comment 70

4 years ago
Comment on attachment 798113 [details] [diff] [review]
Patch for esr17

This patch miraculously applies to b2g18 as-is.
Attachment #798113 - Flags: approval-mozilla-b2g18?
(Assignee)

Updated

4 years ago
Flags: needinfo?(mrbkap)

Updated

4 years ago
Attachment #798113 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
(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.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/2354421ea73f
status-b2g18: affected → fixed
status-b2g-v1.1hd: --- → affected
status-b2g-v1.2: --- → fixed
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/2354421ea73f
status-b2g-v1.1hd: affected → fixed
(Assignee)

Comment 74

4 years ago
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).
(Assignee)

Updated

4 years ago
Flags: in-testsuite? → in-testsuite+
fixed on central https://hg.mozilla.org/mozilla-central/rev/47416c6a473a
status-firefox27: --- → fixed
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
(Reporter)

Comment 77

4 years ago
If you do document.open() it clears out the dom tree and then you can see document.body == null.
(Reporter)

Comment 78

4 years ago
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()
(Assignee)

Comment 79

4 years ago
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.
Group: core-security
You need to log in before you can comment on or make changes to this bug.