Bug 655649 (CVE-2012-3985)

Script access checks should use effective script origin, not origin

RESOLVED FIXED in Firefox 16

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Collin Jackson, Assigned: bholley)

Tracking

({regression, sec-moderate})

Trunk
mozilla16
regression, sec-moderate
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox5- wontfix, firefox6- wontfix, firefox7- wontfix, firefox8+ wontfix, firefox9 wontfix, firefox10 wontfix, firefox11 affected, firefox12 affected, firefox13 affected, firefox16 fixed, firefox-esr10 wontfix, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:moderate][advisory-tracking+] (sg:high against sites using document.domain)[cpg])

Attachments

(7 attachments)

(Reporter)

Description

6 years ago
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.
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.
Component: Security: CAPS → XPConnect
QA Contact: caps → xpconnect
(Reporter)

Updated

6 years ago
Depends on: 601277

Comment 2

6 years ago
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

6 years ago
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.
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.
Whose bug is this? Blake, Andreas?

/be
(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.
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
status-firefox5: --- → affected
tracking-firefox5: --- → ?
tracking-firefox6: --- → ?
Keywords: regression
Whiteboard: [sg:moderate] (sg:high against sites using document.domain)
--> 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.
Assignee: nobody → gal
tracking-firefox5: ? → -
tracking-firefox6: ? → +
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).

Updated

6 years ago
Whiteboard: [sg:moderate] (sg:high against sites using document.domain) → [sg:moderate] (sg:high against sites using document.domain) [probably not 'till 7, see #8]

Comment 9

6 years ago
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?
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...
status-firefox5: affected → wontfix
status-firefox6: --- → wontfix
status-firefox7: --- → affected
tracking-firefox6: + → -
tracking-firefox7: --- → +
Blake, can you take this? I don't think Andreas will have time to focus on this any time soon.
Assignee: gal → mrbkap
status-firefox7: affected → wontfix
status-firefox8: --- → affected
status-firefox9: --- → affected
tracking-firefox7: + → -
tracking-firefox8: --- → +

Updated

6 years ago
status-firefox8: affected → wontfix

Updated

5 years ago
Depends on: 650353
Duplicate of this bug: 729150
status-firefox-esr10: --- → affected
status-firefox10: --- → wontfix
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
status-firefox9: affected → wontfix

Updated

5 years ago
Whiteboard: [sg:moderate] (sg:high against sites using document.domain) [probably not 'till 7, see #8] → [sg:moderate] (sg:high against sites using document.domain)[cpg]
(Reporter)

Comment 13

5 years ago
Could you please CC me on bug 729150? Thanks
Keywords: sec-moderate
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.
Created attachment 637480 [details] [diff] [review]
Part 1 - Hoist machinery for remapping a single wrapper into a separate function. v1

Aside from some renaming, no functionality has been changed.
Attachment #637480 - Flags: review?(wmccloskey)
Created attachment 637481 [details] [diff] [review]
Part 2 - Move/Rename RemapWrappers. v1

Simple rename/move. No changes other than renaming some parameters.
Attachment #637481 - Flags: review?(wmccloskey)
Created attachment 637482 [details] [diff] [review]
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
Attachment #637482 - Flags: review?(wmccloskey)
Created attachment 637483 [details] [diff] [review]
Part 4 - Introduce an API to recompute wrappers based on various filters. v1
Attachment #637483 - Flags: review?(wmccloskey)
Created attachment 637484 [details] [diff] [review]
Part 5 - Recompute cross-compartment wrappers when setting document.domain. v1
Attachment #637484 - Flags: review?(mrbkap)
Created attachment 637485 [details] [diff] [review]
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.
Attachment #637485 - Flags: review?(mrbkap)
Created attachment 637486 [details] [diff] [review]
Part 7 - Stop doing dynamic security checks for document.domain. v1

\o/
Attachment #637486 - Flags: review?(mrbkap)
Note that the tests for this are over in bug 601277.
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
CCing gabor so that he knows about patch 6.
Attachment #637480 - Flags: review?(wmccloskey) → review+
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?
Attachment #637481 - Flags: review?(wmccloskey) → review+
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)?
Attachment #637482 - Flags: review?(wmccloskey) → review+
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.
Attachment #637483 - Flags: review?(wmccloskey) → review+

Updated

5 years ago
Assignee: mrbkap → bobbyholley+bmo
(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.
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.
Whiteboard: [sg:moderate] (sg:high against sites using document.domain)[cpg] → [Leave open after merge] [sg:moderate] (sg:high against sites using document.domain)[cpg]
Whiteboard: [Leave open after merge] [sg:moderate] (sg:high against sites using document.domain)[cpg] → [leave open] [sg:moderate] (sg:high against sites using document.domain)[cpg]
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.
Whiteboard: [leave open] [sg:moderate] (sg:high against sites using document.domain)[cpg] → [sg:moderate] (sg:high against sites using document.domain)[cpg]
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?
Attachment #637484 - Flags: review?(mrbkap)

Updated

5 years ago
Attachment #637485 - Flags: review?(mrbkap) → review+
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!
Attachment #637486 - Flags: review?(mrbkap) → review+
(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 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
Attachment #637484 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/3f0ff9117f8b
https://hg.mozilla.org/mozilla-central/rev/e5579991e19e
https://hg.mozilla.org/mozilla-central/rev/8f3340e10d5f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox16: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
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.
status-firefox-esr10: affected → wontfix
(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.

Updated

5 years ago
Depends on: 775879
Whiteboard: [sg:moderate] (sg:high against sites using document.domain)[cpg] → [sg:moderate][advisory-tracking+] (sg:high against sites using document.domain)[cpg]
Alias: CVE-2012-3985
Group: core-security
Duplicate of this bug: 667861
Duplicate of this bug: 742728
You need to log in before you can comment on or make changes to this bug.