Closed Bug 821842 Opened 7 years ago Closed 7 years ago

compartment mismatch in nsXBLBinding::DoInitJSClass during print preview

Categories

(Core :: XBL, defect)

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 817342
Tracking Status
firefox17 --- wontfix
firefox18 --- wontfix
firefox19 --- affected
firefox20 --- affected
firefox-esr10 --- unaffected
firefox-esr17 --- affected
b2g18 --- affected
b2g18-v1.0.1 --- affected

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

(Keywords: sec-moderate, Whiteboard: [sg:dupe 817342])

Attachments

(1 file, 1 obsolete file)

I looked at all of the compartment checker crashes, and aside from various URL binding crashes, this was the only other one I saw. The stacks look like this:

0 js::CompartmentChecker::fail 	js/src/jscntxtinlines.h:204
1 JS_GetObjectId 	js/src/jsapi.cpp:3361
2 nsXBLBinding::DoInitJSClass 	content/xbl/src/nsXBLBinding.cpp:1382
3 nsRuleNode::ComputeUserInterfaceData 	layout/style/nsRuleNode.cpp:4062
4 XPCJSContextStack::Push 	js/xpconnect/src/XPCJSContextStack.cpp:102
5 XPCJSRuntime::Get 	js/xpconnect/src/xpcprivate.h:667
6 nsXPConnect::Pop 	js/xpconnect/src/nsXPConnect.cpp:2127
7 nsContentUtils::WrapNative 	content/base/src/nsContentUtils.cpp:6076
8 nsXBLProtoImpl::InitTargetObjects 	content/xbl/src/nsXBLProtoImpl.cpp:133
9 nsCOMPtr<nsIRDFXMLSerializer>::nsCOMPtr<nsIRDFXMLSerializer> 	obj-firefox/dist/include/nsCOMPtr.h:538
10 nsXBLProtoImpl::InstallImplementation 	content/xbl/src/nsXBLProtoImpl.cpp:70
11 nsBindingManager::SetBinding 	content/xbl/src/nsBindingManager.cpp:551
12 nsXBLBinding::InstallImplementation 	content/xbl/src/nsXBLBinding.cpp:1064
13 nsXBLService::LoadBindings 	content/xbl/src/nsXBLService.cpp:563
14 je_malloc 	

The three instances of this crash that I found:
https://crash-stats.mozilla.com/report/index/088f9c7e-cabc-4587-933d-3ae042121214
https://crash-stats.mozilla.com/report/index/df0c92cb-916e-4b6c-869d-513e62121214
https://crash-stats.mozilla.com/report/index/7142209e-e469-4ce5-ab54-f9c0b2121214
That stack looks completely bogus...

Assuming that the top several frames are right, though, looks like JS_GetPrototype does NOT assert on compartment mismatches (should it?) so presumably the args to DoInitJSClass were already in the wrong compartments.  The only aller is nsXBLPrototypeBinding::InitClass.  Afaict nothing really guarantees that the args to InitClass are same-compartment; either the callers need to ensure that, or InitClass itself does.
Here's another crash with a saner looking stack:
https://crash-stats.mozilla.com/report/index/c25eea71-cf54-4057-9e08-3b68a2121214

0 js::CompartmentChecker::fail js/src/jscntxtinlines.h:204
1 JS_GetObjectId js/src/jsapi.cpp:3361
2 nsXBLBinding::DoInitJSClass content/xbl/src/nsXBLBinding.cpp:1382
3 nsXBLProtoImpl::InitTargetObjects content/xbl/src/nsXBLProtoImpl.cpp:133
4 nsXBLProtoImpl::InstallImplementation content/xbl/src/nsXBLProtoImpl.cpp:70
5 nsXBLService::LoadBindings content/xbl/src/nsXBLService.cpp:563

As you say, the only caller of DoInitJSClass is InitClass, so PGO probably just mashed the latter into the former. nsXBLProtoImpl::InitTargetObjects is in fact a caller of InitClass, and shows up in all 4 stacks, so presumably that is the place that is causing problems.

The relevant code on that path [1] looks like:

rv = nsContentUtils::WrapNative(jscontext, global, aBoundElement, &v,
                                  getter_AddRefs(wrapper));
[check for failure]
rv = aBinding->InitClass(mClassName, jscontext, global, JSVAL_TO_OBJECT(v),
                           aTargetClassObject);

I would have thought that wrapping it would result in something in the same compartment, but I don't really know wrapping stuff very well. I didn't notice any auto compartment stuff happening before that anywhere on the call stack, so maybe that's the problem?

[1] http://dxr.mozilla.org/mozilla-central/content/xbl/src/nsXBLProtoImpl.cpp.html#l91
Ugh, this is bug 733636. This implicitly passes aAllowWrapping=false.
Though actually we probably don't want a security wrapper here, and should just enter the compartment of the global, and assert that the result of WrapNative is same-compartment.

Can we expose assertSameCompartment outside the js engine? I always end up doing MOZ_ASSERT(js::IsObjectInContextCompartment(...)), which is nowhere near as nice. And we don't get to turn those on for release builds like we're doing with assertSameCompartment.
Yeah, I was just thinking that would be useful for bug 791896.
This appears to be fairly common now that the other compartment checker bug was fixed. There were 15 on the 12-16 build.
(In reply to Bobby Holley (:bholley) from comment #4)
> Can we expose assertSameCompartment outside the js engine?

I filed bug 822425 for this.

I'm looking at a broader fix for this in bug 733636. We don't want to explicitly mark the dependency because we want to be more subtle about setting the ENABLE_SECURITY flag to true...
Assignee: nobody → continuation
Whiteboard: [to be fixed by bug bug 733636]
Attached patch manually eliminate default arg (obsolete) — Splinter Review
I'll just post ongoing work in this bug instead of the public one. Hopefully we can come up with something that is less obvious. Anyways, this eliminates the implicit argument
In the meantime, this is crashing users in opt builds.  Are we planning to ship opt compartment checks in the final release of 20?
No, Bill set them up with some kind of thing where they autodisable in beta and release, or something like that.
The regression range in bug 825401 suggests that this could be a regression from CPG, which would make sense.
Attachment #693121 - Attachment is obsolete: true
I audited every callsite of nsContentUtils::WrapNative.

I only left the previous false behavior for two places:
- nsXBLProtoImplAnonymousMethod::Execute: this immediately enters the compartment of the return value
- nsJSContext::JSObjectFromInterface: this has two callers. One does a JS_WrapObject, which I think makes it okay, and the other enters the compartment.

For the rest of the sites, the most common reason they look okay is because they involve either wrapping a new object, or an object that isn't wrapper cached. There were a number of places that I couldn't figure out why they are okay, so maybe they weren't. I guess it doesn't matter too much, though.
Setting the allowWrapping flag to "true" in nsXBLProtoImpl::InitTargetObjects seems to break video controls. For instance, parser/htmlparser/tests/mochitest/test_bug709083.html:

WARNING: Not same origin error!: file /Users/amccreight/mz/cent/dom/base/nsJSEnvironment.cpp, line 389
TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - Script error. at chrome://global/content/bindings/videocontrols.xml:0

This is unfortunate because this is the particular place we're getting these cross-compartment errors. I'll investigate.
Hmmm, probably has to do with the fact that aAllowSecurityWrappers in this case applies _both_ to cross-compartment wrappers _and_ same-compartment security wrappers, and the code here is probably getting confused by the resulting SCSW SOW.
Judging by the crash report comments (almost all of them refer to print preview), the stacks, and dholbert's test cases, this seems to be related to print preview.
For instance, in this stack you can see nsDocumentViewer::PrintPreview deep in the stack:

https://crash-stats.mozilla.com/report/index/6477eed7-341f-4dc2-8973-d10862130103
Summary: compartment mismatch in nsXBLBinding::DoInitJSClass → compartment mismatch in nsXBLBinding::DoInitJSClass during print preview
No longer blocks: 825401
I strongly suspect this is a dupe of bug 817342. All the comments involve print preview, which is the same problem as that bug. Additionally, dholbert came up with steps to cause the same stack as this bug, involving print preview. When those STR are run in a debug, rather than Nightly build, they produce the same stacks as bug 817342. My guess is that we have stricter compartment checks in debug builds than we do in Nightly builds, so it simply ends up dying earlier.

Downgrading this to sec-moderate because it seems to involve running addon JS during a print preview, which would probably be difficult to exploit.

Once the patch for that bug lands, it will be easy to confirm that this signature has gone away.

I don't want to explicitly dupe this bug to that one, both so we can track if it goes away, and to hide the connection to compartment problems a bit.
Whiteboard: [to be fixed by bug bug 733636] → [sg:dupe 817342]
No longer blocks: 825380
Duplicate of this bug: 825380
It is still possible that we're hitting multiple problems here, and something like bug 733636 will also be required, so we'll have to see.
There have been no crashes with this signature since the 1/10 build. The patch for bug 817342 landed first in the 1/11 Nightly, so it seems safe to dupe this.
No longer blocks: compartment-mismatch
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 817342
Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected
Group: core-security
You need to log in before you can comment on or make changes to this bug.