Closed Bug 1094930 Opened 5 years ago Closed 5 years ago

compartment mismatch in nsDocument::RegisterElement

Categories

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

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- disabled
firefox35 --- disabled
firefox36 --- fixed
firefox-esr31 --- disabled
b2g-v2.2 --- fixed

People

(Reporter: gkrizsanits, Assigned: gkrizsanits)

Details

(Keywords: sec-high)

Attachments

(3 files, 2 obsolete files)

Attached patch test (obsolete) — Splinter Review
Test case attached.

line that caused the assertion:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#6033

I guess the problem is that aOptions.mPrototype is already an unwrapped version (do we unwrap automatically the transparent wrappers for dictionary object parsing?) so we should enter its compartment before the JS_GetGlobalForObject call.

stack:

#0  0x00007ffff1bde69d in js::CompartmentChecker::fail (c1=0x3c8a1a0, 
    c2=0x3d614e0) at /home/gabor/development/imports/js/src/jscntxtinlines.h:52
#1  0x00007ffff1bde730 in js::CompartmentChecker::check (this=0x7fffffff6a10, 
    c=0x3d614e0) at /home/gabor/development/imports/js/src/jscntxtinlines.h:73
#2  0x00007ffff1bde767 in js::CompartmentChecker::check (this=0x7fffffff6a10, 
    obj=0x7fffbec09e80)
    at /home/gabor/development/imports/js/src/jscntxtinlines.h:84
#3  0x00007ffff2051ae2 in js::assertSameCompartment<JSObject*> (cx=0x706ff0, 
    t1=@0x7fffffff6a30: 0x7fffbec09e80)
    at /home/gabor/development/imports/js/src/jscntxtinlines.h:158
#4  0x00007ffff200fea2 in JS_GetGlobalForObject (cx=0x706ff0, 
    obj=0x7fffbec09e80)
    at /home/gabor/development/imports/js/src/jsapi.cpp:1382
#5  0x00007fffeeb83613 in nsDocument::RegisterElement (this=0x3c74ea0, 
    aCx=0x706ff0, aType=..., aOptions=..., aRetval=..., rv=...)
    at /home/gabor/development/imports/dom/base/nsDocument.cpp:6038
#6  0x00007fffeef6c290 in mozilla::dom::DocumentBinding::registerElement (
    cx=0x706ff0, obj=..., self=0x3c74ea0, args=...)
    at /home/gabor/development/imports/obj-x86_64-unknown-linux-gnu/dom/bindings/DocumentBinding.cpp:2450
#7  0x00007fffef6bad18 in mozilla::dom::GenericBindingMethod (cx=0x706ff0, 
    argc=2, vp=0x7a30a0)
    at /home/gabor/development/imports/dom/bindings/BindingUtils.cpp:2437
Attachment #8518229 - Attachment is patch: true
> I guess the problem is that aOptions.mPrototype is already an unwrapped version 

More precisely, it's in the compartment aCx was in when this function was called.  In this case probably the sandbox compartment since we're operating over Xrays.

But then we explicitly entered a different compartment like so:

6010   JS::Rooted<JSObject*> global(aCx, sgo->GetGlobalJSObject());
6011 
6012   JSAutoCompartment ac(aCx, global);

So if we want to work with things that were passed to us, we need to wrap them into that compartment as we go.

Are you planning to fix this?

I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=27260 on the spec here being totally not defined.
So in terms of fixing this...

The relevant line of code is:

    if (JS_GetGlobalForObject(aCx, protoObject) != global) {

clearly if we wrap protoObject into our compartment, the global of the result will be "global".  So what is this check trying to accomplish, exactly?  I don't see anything about such a check in the custom elements spec...
Flags: needinfo?(wchen)
Btw, a simple testcase to trigger this:

<iframe></iframe>
<script>
  var proto = {};
  var f = document.registerElement.call(frames[0].document, "x-foo", { prototype: proto });
</script>
(In reply to Boris Zbarsky [:bz] from comment #1) 
> Are you planning to fix this?
> 

I plan to, but if someone wants to steal it I'm happy for that too.

(In reply to Boris Zbarsky [:bz] from comment #2)
>     if (JS_GetGlobalForObject(aCx, protoObject) != global) {
> 
> clearly if we wrap protoObject into our compartment, the global of the
> result will be "global".  So what is this check trying to accomplish,
> exactly?  I don't see anything about such a check in the custom elements
> spec...

My guess is that it wants to check if the proto is from the same compartment
as the document, no? So if it's wrapped, we should unwrap, enter its compartment,
and check it against 'global'.
> My guess is that it wants to check if the proto is from the same compartment
> as the document, no? 

Why does that matter?  I see no basis in the spec for such a restriction.
(In reply to Boris Zbarsky [:bz] from comment #2)
> clearly if we wrap protoObject into our compartment, the global of the
> result will be "global".  So what is this check trying to accomplish,
> exactly?  I don't see anything about such a check in the custom elements
> spec...

This is just a bug in the implementation. The intent was to try and make sure the registered prototype is in the same compartment as the document, but you're right that the spec doesn't have such a restriction.
Flags: needinfo?(wchen)
In that case this code gets somewhat more trickier, forcing us to do some compartment dance, which I'm not sure how nice is to do at this level :(

Anyway, this is how I think then it would make sense to implement it. This is all undefined behavior according to the spec ofc., so I wonder what you guys think.

I've run into a an odd thing by the way. The original test case I attached to failed with this version because
  nsAutoPtr<LifecycleCallbacks> callbacksHolder(new LifecycleCallbacks());
  JS::RootedValue rootedv(aCx, JS::ObjectValue(*protoObject));
  if (!callbacksHolder->Init(aCx, rootedv))
this init call is trying to get a property on a same origin xray that comes with the sandbox by default, and since createdCallback is a function, the xray silently don't let access to it. Shouldn't this generated code for dictionary objects just handle this somehow? This will be painful for add-on developers at some point otherwise I'm afraid. (I flagged Bobby for this part...)
Assignee: nobody → gkrizsanits
Flags: needinfo?(bobbyholley)
Attachment #8518962 - Flags: feedback?(wchen)
Attachment #8518962 - Flags: feedback?(bzbarsky)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7)
> Created attachment 8518962 [details] [diff] [review]
> Allow registerElement to take protos from other scopes
> 
> In that case this code gets somewhat more trickier, forcing us to do some
> compartment dance, which I'm not sure how nice is to do at this level :(
> 
> Anyway, this is how I think then it would make sense to implement it. This
> is all undefined behavior according to the spec ofc., so I wonder what you
> guys think.
> 
> I've run into a an odd thing by the way. The original test case I attached
> to failed with this version because
>   nsAutoPtr<LifecycleCallbacks> callbacksHolder(new LifecycleCallbacks());
>   JS::RootedValue rootedv(aCx, JS::ObjectValue(*protoObject));
>   if (!callbacksHolder->Init(aCx, rootedv))
> this init call is trying to get a property on a same origin xray that comes
> with the sandbox by default, and since createdCallback is a function, the
> xray silently don't let access to it. Shouldn't this generated code for
> dictionary objects just handle this somehow?

How? By just unwrapping and invoking the untrusted functions? Fundamentally, I don't think we should do that over Xrays.

> This will be painful for add-on
> developers at some point otherwise I'm afraid. (I flagged Bobby for this
> part...)

We should brainstorm, sure. But not in this bug?
Flags: needinfo?(bobbyholley)
Comment on attachment 8518962 [details] [diff] [review]
Allow registerElement to take protos from other scopes

I haven't read through the details carefully yet, but some thoughts:

1)  For now, I think we should just check for proto inheriting from the SVGElement prototype of the document.  So no need to "enter the compartment of the proto".  Once the spec actually defines this, we can change as needed.

> this init call is trying to get a property on a same origin xray that comes with the sandbox by default

We should, imo, init the lifecycle callbacks while in the callee compartment (the one we were in when the function was entered).
Attachment #8518962 - Flags: feedback?(bzbarsky) → feedback+
Attachment #8518962 - Attachment is obsolete: true
Attachment #8518962 - Flags: feedback?(wchen)
Attachment #8519907 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #9)
> We should, imo, init the lifecycle callbacks while in the callee compartment
> (the one we were in when the function was entered).

The first half of the diff looks ugly, but all it does is scoping the first JSAutoCompartment (and defining a few things sooner that is used outside the scope)
Attachment #8519909 - Flags: review?(bzbarsky)
Attached patch part3: testSplinter Review
Attachment #8518229 - Attachment is obsolete: true
Attachment #8519910 - Flags: review?(bzbarsky)
Attachment #8519909 - Attachment description: part2: registerElement2 → part2: lifecycle callbacks init from the callee compartment
Boris I flagged you for the reviews but I guess William can do them as well if you are too flooded.
Keywords: sec-high
Comment on attachment 8519907 [details] [diff] [review]
part1: Allow registerElement to take protos from other scopes

>+    // We are already operating on the documents (/globals) compartment. Let's

"document's" (and "global's").

>+      // XXX: I think if the documents compartment does not have same

No XXX or think.  If the document's compartment can't access the proto, we should totally throw.  Adjust the comment as needed.

>+    // XXX: This check will go through a wrapper, but as we checked above
>+    // it should be transparent. So this should be fine.

It might not be transparent, but an Xray.  In any case, this is fine for now pending the spec getting sorted out here.

r=me
Attachment #8519907 - Flags: review?(bzbarsky) → review+
Comment on attachment 8519907 [details] [diff] [review]
part1: Allow registerElement to take protos from other scopes

Oh, and this comment:

>     // If a prototype is provided, we must check to ensure that it is from the
>     // same browsing context as us.

should go away.  It's just wrong in all sorts of ways.  ;)
Comment on attachment 8519909 [details] [diff] [review]
part2: lifecycle callbacks init from the callee compartment

Fwiw, this is one case that would have really benefited from an additional diff -w.  Way easier to review.

>+  {

Document that this is a scope for "ac" because we want to do some work in the compartment of "global" and then some more work in our original compartment.

r=me
Attachment #8519909 - Flags: review?(bzbarsky) → review+
Comment on attachment 8519910 [details] [diff] [review]
part3: test

r=me
Attachment #8519910 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #16)
> Fwiw, this is one case that would have really benefited from an additional
> diff -w.  Way easier to review.

Oh... I was not aware of that option. Thanks, I will do that next time. I tried a few things to make the patch look less ugly but failed with it...
https://hg.mozilla.org/mozilla-central/rev/dfc02afe5934
https://hg.mozilla.org/mozilla-central/rev/b3433590e13f
https://hg.mozilla.org/mozilla-central/rev/0888cdfdcfa9

This bug should have had sec-approval prior to landing AFAICT.
https://wiki.mozilla.org/Security/Bug_Approval_Process
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(gkrizsanits)
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(In reply to [Away 18-Nov to 23-Nov] Ryan VanderMeulen [:RyanVM UTC-5] from comment #21)
> This bug should have had sec-approval prior to landing AFAICT.
> https://wiki.mozilla.org/Security/Bug_Approval_Process

The mitigation factor is that this whole function is hidden behind a pref. So this should only affect developers experimenting with web components as far as I know. So I think sec-mod would be enough for it, but it's not my call and I should have been more careful. Once I sent it to try I guess it was too late already. So how do we progress from here?
Flags: needinfo?(gkrizsanits)
Comment on attachment 8519907 [details] [diff] [review]
part1: Allow registerElement to take protos from other scopes

I'm waaay too late for this, but want to raise some attention with it so we
can decide what to do here.

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

Not sure if it's possible to exploit but I'm afraid so. We call the callbacks from the document's compartment while the proto object is wrapped in the caller compartment... Then again, one must explicitly enable webcomponents with a pref flag to get this vulnerability.

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

Partially. It does not give ideas how to exploit it, but does show where the problem can be found.

Which older supported branches are affected by this flaw?

It seems pretty old to me. It's hard to tell but for sure ff 30 (Bug 856140) was already affected in the same way as today, but even ff 22 (Bug 783129) might be affected a little (should hit the assertion but I see no callback in that version so it should be harder to exploit)...

If not all supported branches, which bug introduced the flaw?

See previous answer.

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

Should be backportable, and it seems straightforward.

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

I don't expect it to cause any regression. Unless someone expected the callbacks to be called from the document's compartment instead from the callers (2nd patch, note: if we backport we should do it for both)
Attachment #8519907 - Flags: sec-approval?
I think we need to clarify our process here around security bugs in disabled-by-default functionality.  Because I'm not sure whether it makes sense to hold it to the same standard as other security bugs in terms of disclosure.
Yeah, if this is disabled everywhere, it shouldn't need sec-approval.  It would be nice for those of us who read a bunch of sec bugs if that was indicated it was disabled in the white board, or by setting the disabled affected flags.

Though, are we shipping this enabled on B2G?  I thought they were trying to get that going...
Can someone confirm that this is disabled everywhere but trunk?
Apparently it is enabled only on certified apps on B2G.
It's disabled by default on trunk too.  Like Andrew said, the only exception is certified apps on B2G.
Attachment #8519907 - Flags: sec-approval? → sec-approval+
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.