655649 - (CVE-2012-3985) Script access checks should use effective script origin, not origin

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Collin Jackson]

The HTML5 spec requires that browsers raise a SECURITY_ERR exception when members of the Window object (e.g. localStorage) are accessed by scripts that have a different effective script origin. Firefox allows access by scripts that have the same origin OR the same effective script origin.

Here's a scenario where the new behavior has an observable security impact:

https://attacker.cmu.edu/ wants to access the localStorage of https://weblogin.cmu.edu/. Assume that there exists a page at https://help.weblogin.cmu.edu/ that sets document.domain = 'cmu.edu' and that https://weblogin.cmu.edu/ sets its document.domain to 'weblogin.cmu.edu'.

In Chrome/Safari/IE8, I claim that the attacker can't do it. They can inject script into the one help.weblogin.cmu.edu page that set document.domain to 'cmu.edu' but this page can't script other help.weblogin.cmu.edu pages nor can it script weblogin.cmu.edu.

In Firefox 4, the attack works. The frame hierarchy looks like this:

https://attacker.cmu.edu/
   ---> https://help.weblogin.cmu.edu/
       ---> https://help.weblogin.cmu.edu/blah
           ---> https://weblogin.cmu.edu/

The top frame can script the second frame by setting document.domain to 'cmu.edu', the second frame can script the third due to Firefox's non-standard access check, the third frame can script the fourth by setting its document.domain to 'weblogin.cmu.edu'

There may be less contrived scenarios; this is what I could come up with after thinking about it for a few minutes. I'm setting the security flag on this bug because I haven't thought through all the ramifications of the new origin enforcement behavior, but it's fine to clear it if you feel the risk is low.

Incidentally, this attack also worked in IE7, but for a different reason. IE7 allows 'help.weblogin.cmu.edu' to set its document.domain to 'weblogin.cmu.edu' after setting it to 'cmu.edu'. This was fixed in IE8, which now prohibits setting document.domain to a longer value than its current value.

http://www.adambarth.com/papers/2009/barth-weinberger-song.pdf has a great explanation of how to do the HTML5 script access check quickly. The key is to get the access check down to a single pointer comparison in the common case.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I believe this was a purposeful decision when the current compartment system was implemented.  The reasoning was that if two pages were once same-origin then making them different origins at a later time doesn't buy you much because you could have already injected whatever you want from the one page into the other (e.g. an onmessage handler that evals the passed-in string).  The effect is that document.domain changes can _drop_ security restrictions in Gecko 2.0 but not _introduce_ them, which is what this bug seems to be about.

That reasoning does fall down in the example described in comment 0, I think: there is no point in time when https://help.weblogin.cmu.edu/ can both inject script into https://help.weblogin.cmu.edu/blah and have script injected from https://attacker.cmu.edu/ in a system which allows document.domain to introduce security restrictions.

Comment 2

[Adam Barth]

Are you planning to propose this new security model for the web to the HTML working group?  If the working group decides to keep the current model, are you planning to continue to implement a non-standards compliant security model?  That seems very bad for the web.

Comment 3

[Andreas Gal :gal]

I can't speak for Mozilla's strategy, but we rarely intentionally diverge from standards, except if the standard is clearly wrong and there is consensus by implementers to ignore it. It might be worth discussing this change with the WG but for now I don't think anyone assumes that the current model is clearly wrong. Whether this is really that bad for the web I am a bit doubtful about. Extensive document.domain is not all that common, and it will likely get less over time. We are currently plotting out our approach to fix this. We will either lazily split compartments (brain transplant of document graphs), or use zones to keep each window in its own compartment.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, there was a bit of "laziness" on our part in making this decision. In particular, when we were implementing the original compartments stuff, we couldn't fix this bug as filed in an easy way (lazily splitting compartments wasn't possible until just before Firefox 4). We're definitely planning on fixing things to revoke access on document.domain setting.

Comment 5

[Brendan Eich [:brendan]]

Whose bug is this? Blake, Andreas?

/be

Comment 6

[Daniel Veditz [:dveditz]]

(In reply to comment #1)
> The effect is that document.domain changes can _drop_ security restrictions
> in Gecko 2.0 but not _introduce_ them, 

As broken as document.domain is in the first place it's not a good idea to change its historical behavior. That's just laying traps for unwary web app developers to fall into.

There's a reason the document.domain spec doesn't allow pages to re-lengthen document.domain back to the original domain once it's been truncated. Once a page has changed its document.domain it's tainted and shouldn't be allowed to spread that taint by rejoining its initial origin.

Comment 7

[Daniel Veditz [:dveditz]]

--> Andreas because it sounds like he needs to make a call on which approach to take.

We've diverged from the spec and this can put some sites at risk, we need to fix it. Seems unlikely to be fixed safely in Firefox 5 which is almost done, but we'd like to get this in Firefox 6.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

From a code standpoint, the safest milestone is either Firefox 7 or 8. We're targeting compartment-per-window for Firefox 7, which makes the fix for this bug a lot easier (brain transplanting proxies is always safer than brain transplanting wrapped natives).

Comment 9

[Asa Dotzler [:asa]]

We're up against the endgame in 6 and if we're not going to be doing anything here for 6, we probably don't need to keep tracking. Dveditz, what do you think?

Comment 10

[Johnny Stenback (:jst)]

Given Blake's comment above we won't be able to fix this for 6, tracking for 7, but I'm guessing the change will be invasive enough to port there too, so this might be pushed further...

Comment 11

[Johnny Stenback (:jst)]

Blake, can you take this? I don't think Andreas will have time to focus on this any time soon.

Comment 13

[Collin Jackson]

Could you please CC me on bug 729150? Thanks

Comment 14

[Bobby Holley (:bholley)]

I have patches for this. Coming right up. I'm going to flag bill for review on the nitty-gritty of the transplant code, since he's probably fresher on it than Blake and has a lower review burden.

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 1 - Hoist machinery for remapping a single wrapper into a separate function. v1

Aside from some renaming, no functionality has been changed.

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 2 - Move/Rename RemapWrappers. v1

Simple rename/move. No changes other than renaming some parameters.

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 3 - Codify |wrappedObject(value) == key| invariant. v1

Now that we're not doing that awful Location thing anymore, I think we should assert this. v1

Comment 18

[Bobby Holley (:bholley)]

Attachment: Part 4 - Introduce an API to recompute wrappers based on various filters. v1

Comment 19

[Bobby Holley (:bholley)]

Attachment: Part 5 - Recompute cross-compartment wrappers when setting document.domain. v1

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 6 - Use Subsumes Rather than Equals in XPConnect wrapper computation. v1

Now that we have nsExpandedPrincipal, the current way of doing things is wrong. For some reason, the old document.domain hackery was hiding the failures here.

Comment 21

[Bobby Holley (:bholley)]

Attachment: Part 7 - Stop doing dynamic security checks for document.domain. v1

\o/

Comment 22

[Bobby Holley (:bholley)]

Note that the tests for this are over in bug 601277.

Comment 23

[Bobby Holley (:bholley)]

FWIW I also pushed this to try earlier. The only bustage was in the nsExpandedPrincipal xpcshell tests, which prompted part 6 of the patch stack here.

https://tbpl.mozilla.org/?tree=Try&rev=3fc46f385017

Comment 24

[Bobby Holley (:bholley)]

CCing gabor so that he knows about patch 6.

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 637481 [details] [diff] [review]
Part 2 - Move/Rename RemapWrappers. v1

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

::: js/src/jswrapper.cpp
@@ +1128,5 @@
> +    AutoValueVector toTransplant(cx);
> +    if (!toTransplant.reserve(vector.length()))
> +        return false;
> +
> +    for (JSCompartment **p = vector.begin(), **end = vector.end(); p != end; ++p) {

While you're here, can you change this to a CompartmentsIter?

Comment 26

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 637482 [details] [diff] [review]
Part 3 - Codify |wrappedObject(value) == key| invariant. v1

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

::: js/src/jscompartment.cpp
@@ +260,5 @@
>          return false;
>  
> +    // We maintain the invariant that the key in the cross-compartment wrapper
> +    // map is always directly wrapped by the value.
> +    JS_ASSERT(Wrapper::wrappedObject(wrapper) == &key.reference().toObject());

Yay! Can you put a line break after the assertion? I think it deserves its own paragraph.

Also, can you rip out the junk in proxy_TraceObject that deals with the case where this assertion fails (the |if (!p) {...}| code and the comment)?

Comment 27

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 637483 [details] [diff] [review]
Part 4 - Introduce an API to recompute wrappers based on various filters. v1

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

::: js/src/jswrapper.cpp
@@ +1153,5 @@
> +{
> +    AutoValueVector toRecompute(cx);
> +    CompartmentVector &vector = cx->runtime->compartments;
> +
> +    for (JSCompartment **p = vector.begin(), **end = vector.end(); p != end; ++p) {

Please use CompartmentsIter.

@@ +1162,5 @@
> +        // Iterate over the wrappers, filtering appropriately.
> +        WrapperMap &pmap = (*p)->crossCompartmentWrappers;
> +        for (WrapperMap::Enum e(pmap); !e.empty(); e.popFront()) {
> +
> +            // Filter out non-objects.

No newline before this comment.

@@ +1169,5 @@
> +                continue;
> +
> +            // Filter by target compartment.
> +            Value wrapper = e.front().value.get();
> +            if (!targetFilter.Match(e.front().key.wrapped->compartment()))

You can just use k.wrapped->compartment().

@@ +1180,5 @@
> +    }
> +
> +    // Recompute all the wrappers in the list.
> +    for (Value *begin = toRecompute.begin(), *end = toRecompute.end();
> +         begin != end; ++begin)

Will this fit on a 99 character line?

::: js/src/jswrapper.h
@@ +253,5 @@
> +
> +// API to recompute all cross-compartment wrappers whose source and target
> +// match the given filters.
> +//
> +// These filters should live only on the stack.

This seems to imply that it's unsafe to put them on the heap, which I don't think is true.

@@ +255,5 @@
> +// match the given filters.
> +//
> +// These filters should live only on the stack.
> +struct CompartmentFilter {
> +    virtual bool Match(JSCompartment *c) const = 0;

This should be called match because methods use thisStyleOfNaming.

@@ +256,5 @@
> +//
> +// These filters should live only on the stack.
> +struct CompartmentFilter {
> +    virtual bool Match(JSCompartment *c) const = 0;
> +};

Please put an empty line after each struct.

Comment 28

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #27)
> > +// These filters should live only on the stack.
> 
> This seems to imply that it's unsafe to put them on the heap, which I don't
> think is true.

Well, CompartmentsWithPrincipals doesn't call hold on its principals, and SingleCompartment doesn't root it's compartment in any way. I'll add a comment and make that more explicit.

Comment 29

[Bobby Holley (:bholley)]

I was going to wait to land the whole stack until Blake does his set of reviews, but gabor wants to use the CompartmentFilter machinery in the earlier patches. So I addressed billm's comments and landed those to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/9a492f790821
http://hg.mozilla.org/integration/mozilla-inbound/rev/d2a65035b2ec
http://hg.mozilla.org/integration/mozilla-inbound/rev/684fc1db7deb
http://hg.mozilla.org/integration/mozilla-inbound/rev/54fb630b7077

These are just infrastructure. The bug itself isn't fixed until the rest land.

Comment 30

[Bobby Holley (:bholley)]

RyanVM merged these to m-c:

https://hg.mozilla.org/mozilla-central/rev/54fb630b7077
https://hg.mozilla.org/mozilla-central/rev/684fc1db7deb
https://hg.mozilla.org/mozilla-central/rev/d2a65035b2ec
https://hg.mozilla.org/mozilla-central/rev/9a492f790821

Removing the [Leave open], since I expect the next push to be the final one.

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 637484 [details] [diff] [review]
Part 5 - Recompute cross-compartment wrappers when setting document.domain. v1

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

::: caps/src/nsPrincipal.cpp
@@ +972,5 @@
> +  JSPrincipals *principals = nsJSPrincipals::get(static_cast<nsIPrincipal*>(this));
> +  bool success = js::RecomputeWrappers(cx, js::ContentCompartmentsOnly(),
> +                                       js::CompartmentsWithPrincipals(principals));
> +  NS_ENSURE_TRUE(success, NS_ERROR_FAILURE);
> +  success = js::RecomputeWrappers(cx, js::CompartmentsWithPrincipals(principals),

I feel like this belongs more in nsHTMLDocument rather than the principal itself (which is mostly clear of JS stuff)...

Looking at mxr, it seems like the only caller of nsIPrincipal::SetDomain is the document, so if you feel strongly about keeping this here, I guess that's OK.

Also, do we not need to enter request here or enter compartment?

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 637486 [details] [diff] [review]
Part 7 - Stop doing dynamic security checks for document.domain. v1

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

So glad to see documentDomainMakesSameOrigin die!

Comment 33

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #31)
> Comment on attachment 637484 [details] [diff] [review]
> Part 5 - Recompute cross-compartment wrappers when setting document.domain.
> v1
> 
> Review of attachment 637484 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: caps/src/nsPrincipal.cpp
> @@ +972,5 @@
> > +  JSPrincipals *principals = nsJSPrincipals::get(static_cast<nsIPrincipal*>(this));
> > +  bool success = js::RecomputeWrappers(cx, js::ContentCompartmentsOnly(),
> > +                                       js::CompartmentsWithPrincipals(principals));
> > +  NS_ENSURE_TRUE(success, NS_ERROR_FAILURE);
> > +  success = js::RecomputeWrappers(cx, js::CompartmentsWithPrincipals(principals),
> 
> I feel like this belongs more in nsHTMLDocument rather than the principal
> itself (which is mostly clear of JS stuff)...
> 
> Looking at mxr, it seems like the only caller of nsIPrincipal::SetDomain is
> the document, so if you feel strongly about keeping this here, I guess
> that's OK.

IMO it should live in SetDomain, since this is now our implementation of document.domain semantics. Doing SetDomain without doing the transplant results in incorrect behavior, so it seems like we should keep it there rather than adding a comment in the IDL saying "NB: this actually doesn't work unless you do this other thing as well".

> Also, do we not need to enter request here or enter compartment?

Aren't requests deprecated with single-threaded runtime? Or am I terribly misinformed? It's also not clear why we'd need to enter a compartment, here. Can you explain?

Comment 34

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 637484 [details] [diff] [review]
Part 5 - Recompute cross-compartment wrappers when setting document.domain. v1

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

No, I think I was confused. r=me

Comment 35

[Bobby Holley (:bholley)]

I checked this on try a while back, but I don't have the url on me. I remember it was green (it also passes local XPConnect tests). If not, I apologize.

Pushed to m-i:
http://hg.mozilla.org/integration/mozilla-inbound/rev/8f3340e10d5f
http://hg.mozilla.org/integration/mozilla-inbound/rev/e5579991e19e
http://hg.mozilla.org/integration/mozilla-inbound/rev/3f0ff9117f8b

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3f0ff9117f8b
https://hg.mozilla.org/mozilla-central/rev/e5579991e19e
https://hg.mozilla.org/mozilla-central/rev/8f3340e10d5f

Comment 37

[Lukas Blakk [:lsblakk] use ?needinfo]

This is too big a patch, depends on compartments-per-global, and is only a sec-moderate so we're going to wontfix this for ESR.

Comment 38

[Bobby Holley (:bholley)]

(In reply to Lukas Blakk [:lsblakk] from comment #37)
> This is too big a patch, depends on compartments-per-global, and is only a
> sec-moderate so we're going to wontfix this for ESR.

This should definitely not land anywhere but m-c.