860494 - Error in tabbrowser's DOMTitleChanged with <iframe name="top">

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

JavaScript error: chrome://browser/content/tabbrowser.xml, line 2808: NS_ERROR_FAILURE: Failure

> if (contentWin != contentWin.top)

http://hg.mozilla.org/mozilla-central/annotate/1a426b650b1d/browser/base/content/tabbrowser.xml#l2833 hasn't changed lately.

Does this mean global scope pollution affects the view of content from chrome?  I'd expect wrappers to prevent that.  (Note recent changes to global scope pollution: bug 850517, bug 823227.)

Comment 1

[Boris Zbarsky [:bzbarsky]]

> Does this mean global scope pollution affects the view of content from chrome?

The particular global scope pollution that exposes frame names does, sadly.

On the other hand, I didn't think it was supposed to override stuff on the proto chain!  Why do we not have test coverage for these things?  :(  Presumably a regression from bug 850517.  The old code used to do:

-  if (!ObjectIsNativeWrapper(cx, obj) ||
-      xpc::WrapperFactory::XrayWrapperNotShadowing(obj, id)) {

but the new code in XrayWrapper<Base, Traits>::getPropertyDescriptor doesn't check for shadowing.  What it should really be doing is that we should never look for named frames if resolveNativeProperty found something.

Comment 2

[Jesse Ruderman]

> What it should really be doing is that we should never look for named frames if 
> resolveNativeProperty found something.

Even that doesn't sound like the most secure default for chrome->content interaction.

Comment 3

[Boris Zbarsky [:bzbarsky]]

While I agree, apparently jetpack totally depends on getting at named frames; they explicitly added code to expose them on xrays.  :(

Comment 4

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #3)
> While I agree, apparently jetpack totally depends on getting at named
> frames; they explicitly added code to expose them on xrays.  :(

Well, they're also available cross-origin in both gecko and in the spec, right? So we need to Xray them anyway...

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Well, they're also available cross-origin in both gecko and in the spec, right?

Hmm.  Not sure what the spec status of them is.

Comment 6

[Bobby Holley (:bholley)]

Oh, actually I misread the spec. The "dynamic nested browsing context properties" are available cross-orgin, and when you jump to the definition it talks about named browsing contexts. But they're actually different sections.

So yeah, per spec only indexed properties are allowed cross-origin. I'll investigate aligning with the spec here.

Comment 7

[Bobby Holley (:bholley)]

Ok, it looks like the spec doesn't really match reality here. I filed a bug:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=21674

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Can we get a sec-rating here?

Comment 9

[Daniel Veditz [:dveditz]]

> Can we get a sec-rating here?

I'm at a loss. The specific chrome error reported by the testcase (failure to update the title) could maybe be used for minor spoofing. But if we're shadowing or exposing properties we shouldn't (comment 2) it could lead to all kinds of mischief (sec-high or -critical) if you find some browser chrome or add-on performing some convenient action. I don't know which it is.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Move the named property check further down in XrayWrapper. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 738288 [details] [diff] [review]
Move the named property check further down in XrayWrapper. v1

r=me

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 738289 [details] [diff] [review]
Tests. v1

>+    is(iwin.parent, window, "iframe names shouldn't shadow |parent| via Xray");

Did you mean to have an <iframe name="parent"> in the kid?

r=me with that fixed.

Comment 14

[Bobby Holley (:bholley)]

Attachment: Tests. v2 r=bz

Good catch.

Comment 15

[Bobby Holley (:bholley)]

Actually, this testcase should be fine to check in, I think.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ab16b6003e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b9f7fd0996f0

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 738288 [details] [diff] [review]
Move the named property check further down in XrayWrapper. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 850517
User impact if declined: security
Testing completed (on m-c, etc.): just pushed to m-i 
Risk to taking this patch (and alternatives if risky): not risky
String or IDL/UUID changes made by this patch: none

Comment 17

[moz_bug_r_a4]

(In reply to Daniel Veditz [:dveditz] from comment #9)
> > Can we get a sec-rating here?
> 
> I'm at a loss. The specific chrome error reported by the testcase (failure
> to update the title) could maybe be used for minor spoofing. But if we're
> shadowing or exposing properties we shouldn't (comment 2) it could lead to
> all kinds of mischief (sec-high or -critical) if you find some browser
> chrome or add-on performing some convenient action. I don't know which it is.

By combining this bug with bug 858101, it's possible to perform an XSS attack.  I'll attach a testcase.

Comment 18

[moz_bug_r_a4]

Attachment: testcase 2 - XSS

This tries to get cookies for hacks.mozilla.org.

Comment 19

[Bobby Holley (:bholley)]

This got backed out for failing dom/tests/mochitest/bugs/test_bug440572.html

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Subframe for testcase

This should be placed on a different host from the testcase I'm about to put up.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase to use with that subframe

In Chrome this gives a same-origin policy fail.

Comment 22

[Boris Zbarsky [:bzbarsky]]

So at first glance, what Safari (which has the sanest behavior) here does is that if a property is a "native" property that's not supposed to be exposed cross-origin then it returns undefined even if a frame with that name exists.

But it exposes random unrelated frame names.

One thing I didn't try in Safari is what happens if an expando is set somewhere on the proto chain and then a frame name shadows it...

Comment 23

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #22)
> So at first glance, what Safari (which has the sanest behavior) here does is
> that if a property is a "native" property that's not supposed to be exposed
> cross-origin then it returns undefined even if a frame with that name exists.

I think this is what we should do here too.
 
> One thing I didn't try in Safari is what happens if an expando is set
> somewhere on the proto chain and then a frame name shadows it...

This should be a non-issue for us, since everything will be over Xrays.

Comment 24

[Boris Zbarsky [:bzbarsky]]

So I just tried that, and Safari treats that the same as if the expando were not set on the proto chain.

The Safari behavior seems like the sanest one if we're going to expose cross-origin names at all...

Comment 25

[Bobby Holley (:bholley)]

So as a summary, the basic security issue at play here is that UAs allow cross-origin access to named-subframes [1], but can run into problems if, once their cross-origin filtering policy allows the access through, their anti-shadowing mechanism overrides the subframe name and sticks in the corresponding native property.

For Gecko, this only affects 23 an 22, because previously we didn't have good shadowing protection for Xrays and thus always returned the subframe. This isn't the behavior we're going to use going forward here, but it's fine from a security-perspective. And even on Nightly/Aurora, our security wrappers generally save us here for the most part.

Chromium is a different story. It appears that they leak things like navigator cross-origin in this case, and because they don't have security wrappers, this is likely a very serious problem. I'm going to file a bug with them shortly.

[1] - https://www.w3.org/Bugs/Public/show_bug.cgi?id=21674

Comment 26

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=237aacce9603

Comment 27

[Bobby Holley (:bholley)]

Green on try. Uploading patches and flagging for review.

Comment 28

[Bobby Holley (:bholley)]

Attachment: Part 1 - Make resolveNativeProperty a virtual instance method in XrayTraits like resolveOwnProperty. v1

The current setup is just an artifact of how it used to be before I refactored
Xrays. Have it as a virtual trap is more flexible since it allows us to invoke
the right trap by just calling GetXrayTraits(wrapper) from non-templatized code.

Comment 29

[Bobby Holley (:bholley)]

Attachment: Part 2 - Clarify the semantics of XrayTraits::resolveOwnProperty. v1

Right now, it sometimes fills out |desc|, and sometimes just defines the property
on the holder. This can get confusing, so let's refine the semantics here and
describe them in a big comment.

Comment 30

[Bobby Holley (:bholley)]

Attachment: Part 3 - Check for native properties before checking named children on XOWs. v1

This is more or less the patch you already reviewed, but there's enough change in
the context that's it's probably worth having a second look at it.

Comment 31

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #30)
> Created attachment 740349 [details] [diff] [review]
> Part 3 - Check for native properties before checking named children on XOWs.
> v1
> 
> This is more or less the patch you already reviewed, but there's enough
> change in
> the context that's it's probably worth having a second look at it.

Er, wait. That applies to the next patch, not this one. Sorry.

Comment 32

[Bobby Holley (:bholley)]

Attachment: Part 4 - Move the named property check further down in XrayWrapper. v3

Comment 33

[Bobby Holley (:bholley)]

Attachment: Part 5 - Tests. v3

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 740347 [details] [diff] [review]
Part 1 - Make resolveNativeProperty a virtual instance method in XrayTraits like resolveOwnProperty. v1

r=me

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 740348 [details] [diff] [review]
Part 2 - Clarify the semantics of XrayTraits::resolveOwnProperty. v1

So the change in getOwnPropertyDescriptor to not check the holder just deoptimizes some to improve correctness, right?

r=me if so.

Comment 36

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #35)
> Comment on attachment 740348 [details] [diff] [review]
> Part 2 - Clarify the semantics of XrayTraits::resolveOwnProperty. v1
> 
> So the change in getOwnPropertyDescriptor to not check the holder just
> deoptimizes some to improve correctness, right?

Well, not really. The point is that it _used_ to have to check it even after calling resolveOwnProperty because resolveOwnProperty might have resolved something onto the holder (with NewResolve) without actually returning anything. Now that we guarantee that resolveOwnProperty returns something if it finds it, doing a holder lookup here no longer makes sense.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 740349 [details] [diff] [review]
Part 3 - Check for native properties before checking named children on XOWs. v1

I'd really rather callers passed handles for wrapper and id instead of rerooting here.  Can you do the rooting in a separate bug, please?

>+    if (traits == &XPCWrappedNativeXrayTraits::singleton)
>+        resolvingId.construct(wrapper, id);

Why is that conditional on the traits class?

r=me, I guess assuming we really do want to be calling traits->resolveOwnProperty here....  Might be worth documenting why we want to do that.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 740350 [details] [diff] [review]
Part 4 - Move the named property check further down in XrayWrapper. v3

r=me

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 740352 [details] [diff] [review]
Part 5 - Tests. v3

r=me

Comment 40

[Boris Zbarsky [:bzbarsky]]

> Now that we guarantee that resolveOwnProperty returns something if it finds it

Ah, I see.  OK, sounds goodl

Comment 41

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #37)
> I'd really rather callers passed handles for wrapper and id instead of
> rerooting here.  Can you do the rooting in a separate bug, please?

Yeah, XPConnect is currently in flux because of all of the exact rooting patches. I don't want to make any big changes becasue they might conflict with the work evilpies and jonco are doing. I'll sweep over after they're done and clean stuff like this up if it persists.

> >+    if (traits == &XPCWrappedNativeXrayTraits::singleton)
> >+        resolvingId.construct(wrapper, id);
> 
> Why is that conditional on the traits class?

because resolvingId is an XPCWN construction. We actually have a Dummmy resolvingId mechanism for DOM Xrays, so that we can do XrayTraits::ResolvingId, but that requires having they type (i.e. being in a templated function) rather than the instance like we have here.
 
> r=me, I guess assuming we really do want to be calling
> traits->resolveOwnProperty here....  Might be worth documenting why we want
> to do that.

Can you clarify what's unclear? resolveOwnProperty gets us all the things that NewResolve resolves, which presumably we want here, no?

Comment 42

[Boris Zbarsky [:bzbarsky]]

> Yeah, XPConnect is currently in flux because of all of the exact rooting patches.

Sure, but the point is you don't really need to add a root here in this patch, right?

> resolveOwnProperty gets us all the things that NewResolve resolves, which presumably we
> want here, no?

If it only gets us the things that should be exposed on xrays, then yes.

Comment 43

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/89701e65d892
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/936188849dfa
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/986c25861d1f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7722c041120e

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/89701e65d892
https://hg.mozilla.org/mozilla-central/rev/936188849dfa
https://hg.mozilla.org/mozilla-central/rev/986c25861d1f
https://hg.mozilla.org/mozilla-central/rev/7722c041120e

Comment 45

[Bobby Holley (:bholley)]

Comment on attachment 740350 [details] [diff] [review]
Part 4 - Move the named property check further down in XrayWrapper. v3

This approval request applies to the whole set of patches here (but not the tests), as well as the followup fix in bug 865260.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 850517
User impact if declined: Security bugs.
Testing completed (on m-c, etc.): Baked on m-c.
Risk to taking this patch (and alternatives if risky): Moderately risky. No alternatives other than just leaving it as-is. 
String or IDL/UUID changes made by this patch: None

Comment 46

[Ryan VanderMeulen [:RyanVM]]

Due to various rooting changes on m-c, this is going to need more love for Aurora than I'm comfortable giving.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Bobby, can you do the merging?

Comment 48

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/326abb047cd3
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/1a3c64946d11
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/4cfb9a059a6b
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/89478f8d7ec8

Comment 49

[Bobby Holley (:bholley)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f7cbd446013e

Comment 50

[Bobby Holley (:bholley)]

I filed this on chrome: 
https://code.google.com/p/chromium/issues/detail?id=237022&thanks=237022&ts=1367363612

Comment 51

[Bobby Holley (:bholley)]

This is actually sec-high because any iframe can do this by setting its window.name.

Comment 52

[Jesse Ruderman]

Does that mean a third-party iframe can set window.name to mess with its parent?

Why does that make the bug sec-high?

Comment 53

[Bobby Holley (:bholley)]

(In reply to Jesse Ruderman from comment #52)
> Does that mean a third-party iframe can set window.name to mess with its
> parent?
> 
> Why does that make the bug sec-high?

Suppose a.com loads evil.com in an iframe. evil.com can then set |window.name = "localStorage"|, and do |parent.localStorage| to access the localStorage object of the cross-origin parent.

Comment 54

[Jesse Ruderman]

Does that mean the security check happens on a different property than the one that is accessed??

What happens with the patch?

Comment 55

[Bobby Holley (:bholley)]

(In reply to Jesse Ruderman from comment #54)
> Does that mean the security check happens on a different property than the
> one that is accessed??

Yes. The access is name-based, and we have a name collision here. See comment 25.

> What happens with the patch?

We deny cross-origin access to any property that would resolve to a non-cross-origin-accessible native property, even if there's a subframe with the same name.

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4453da5ca811