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

RESOLVED FIXED in Firefox 22

Status

()

defect
--
minor
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: jruderman, Assigned: bholley)

Tracking

(Blocks 1 bug, {regression, sec-high, testcase})

Trunk
mozilla23
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox21 unaffected, firefox22+ fixed, firefox23+ fixed, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [advisory embargo until the issue is fixed in chrome][adv-main22-])

Attachments

(11 attachments, 1 obsolete attachment)

113 bytes, text/html
Details
3.02 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.68 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.64 KB, text/html
Details
255 bytes, text/html
Details
921 bytes, text/html
Details
4.49 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.33 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.09 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.25 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.00 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Posted file 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.)
> 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.
Blocks: 850517
Component: Tabbed Browser → DOM
Flags: needinfo?(bobbyholley+bmo)
Keywords: regression
Product: Firefox → Core
> 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.
While I agree, apparently jetpack totally depends on getting at named frames; they explicitly added code to expose them on xrays.  :(
(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...
Flags: needinfo?(bobbyholley+bmo)
Assignee: nobody → bobbyholley+bmo
> 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.
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.
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
Can we get a sec-rating here?
Flags: needinfo?(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.
Flags: needinfo?(dveditz)
Keywords: sec-moderate
Posted patch Tests. v1 (obsolete) — Splinter Review
Attachment #738289 - Flags: review?(bzbarsky)
Comment on attachment 738288 [details] [diff] [review]
Move the named property check further down in XrayWrapper. v1

r=me
Attachment #738288 - Flags: review?(bzbarsky) → review+
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.
Attachment #738289 - Flags: review?(bzbarsky) → review+
Good catch.
Attachment #738289 - Attachment is obsolete: true
Attachment #738503 - Flags: review+
Flags: in-testsuite?
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
Attachment #738288 - Flags: approval-mozilla-aurora?
(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.
Posted file testcase 2 - XSS
This tries to get cookies for hacks.mozilla.org.
This got backed out for failing dom/tests/mochitest/bugs/test_bug440572.html
Attachment #738288 - Flags: approval-mozilla-aurora?
This should be placed on a different host from the testcase I'm about to put up.
In Chrome this gives a same-origin policy fail.
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...
(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.
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...
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
Whiteboard: [embargo until the issue is fixed in chrome]
Green on try. Uploading patches and flagging for review.
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.
Attachment #740347 - Flags: review?(bzbarsky)
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.
Attachment #740348 - Flags: review?(bzbarsky)
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.
Attachment #740349 - Flags: review?(bzbarsky)
(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.
Attachment #740352 - Flags: review?(bzbarsky)
Comment on attachment 740347 [details] [diff] [review]
Part 1 - Make resolveNativeProperty a virtual instance method in XrayTraits like resolveOwnProperty. v1

r=me
Attachment #740347 - Flags: review?(bzbarsky) → review+
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.
Attachment #740348 - Flags: review?(bzbarsky) → review+
(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 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.
Attachment #740349 - Flags: review?(bzbarsky) → review+
Comment on attachment 740350 [details] [diff] [review]
Part 4 - Move the named property check further down in XrayWrapper. v3

r=me
Attachment #740350 - Flags: review?(bzbarsky) → review+
Comment on attachment 740352 [details] [diff] [review]
Part 5 - Tests. v3

r=me
Attachment #740352 - Flags: review?(bzbarsky) → review+
> Now that we guarantee that resolveOwnProperty returns something if it finds it

Ah, I see.  OK, sounds goodl
(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?
> 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.
Whiteboard: [embargo until the issue is fixed in chrome] → [advisory embargo until the issue is fixed in chrome]
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
Attachment #740350 - Flags: approval-mozilla-aurora?
Attachment #740350 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Due to various rooting changes on m-c, this is going to need more love for Aurora than I'm comfortable giving.
Bobby, can you do the merging?
Flags: needinfo?(bobbyholley+bmo)
This is actually sec-high because any iframe can do this by setting its window.name.
Keywords: sec-moderatesec-high
Does that mean a third-party iframe can set window.name to mess with its parent?

Why does that make the bug sec-high?
(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.
Does that mean the security check happens on a different property than the one that is accessed??

What happens with the patch?
(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.
Whiteboard: [advisory embargo until the issue is fixed in chrome] → [advisory embargo until the issue is fixed in chrome][adv-main22-]
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.