337311 - Incorrect security checks in Storage impl

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

The domain security checks in the Storage impl are incorrect in several ways:

1)  They are inconsistent in what they check against.  For example,
    GetSessionStorage uses the documentURI of mDocument, while OpenInternal uses
    the currentURI of the window's webnavigation object.  In my opinion, we
    should always be using the URI of our principal for "our" URI.  That will
    incidentally make storage be usable from javascript:- or data:-generated
    subframes.
2)  They don't take into account changes in the "current URI" properly.  For
    example, there are preferences in Firefox that will make OpenInternal do the
    load in the current window.  This will force things like abou:blank content
    viewer creation in some cases.  Since the URI is gotten from the docshell at
    the very end of the OpenInternal call, it may not be the same URI as the
    docshell had at the beginning of the call.  This issue remains even if we
    use the principal, since the principal can also change.
3)  Not all callsites deal correctly with URIs without a host.  For example, in
    OpenInternal we compare thisDomain to newDomain; if both of the URIs
    involved throw from GetAsciiHost (which lots of URIs do), then this check
    will always succeed, which is not good.  Please check the rv in all places
    where GetAsciiHost is called.

I haven't done a complete audit of this stuff, fwiw, the above just jumped out at me....  We might want to do such an audit.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Another thought.  Given that <a ping> wants very similar security checks, maybe we should have a security manager method that takes a principal and a URI (or two principals?) and returns whether they have the "same host"... Not sure it's worth it yet, but something to think on.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

retargetted at beta1

Comment 3

[Mike Schroepfer]

Retargeting to a3

Comment 4

[Johnny Stenback (:jst)]

I believe this was addressed by attachment 221904 [details] [diff] [review] in bug 335540.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

I'm going to retarget this one at beta1, since both jst and Enn seem to think it's fixed, and bz's on vacation. He should really be the one to close the bug, though.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So I looked at the code we landed.  Now GetSessionStorage() does use the principal, but of the outer window, not the inner one.  If we get storage off an inner window that's no longer the current inner, do we want to use the outer's current principal, or the inner's principal?

Bailing out if the principal has no URI means that pages with the system principal can't use storage.  I guess that's OK, but should GetSessionStorage maybe throw in that case instead of silently returning NS_OK?

OpenInternal also uses the outer's principal; that ok?  I guess that's consistent with what the docshell does...

Note that I still haven't read all the code, nor am I likely to have time to.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

mrbkap: jst's gone for a bit, and boris was wondering if maybe you could take a look at this, specifically the question "If we get storage off
an inner window that's no longer the current inner, do we want to use the
outer's current principal, or the inner's principal?"

We'd really like to be able to ship DOM storage with Fx2, so we need to know if bz's concerns need to be addressed before beta1. 

Comment 8

[Johnny Stenback (:jst)]

Attachment: Make window.open and sessionStorage use the inner's principal

This fixes 2 things:

1) It makes window.open() use the principals of the window on which open() was called and not that of that window's outer window, unless of course open() was called on an outer window which is often the case. Even in that case I believe this is acceptable since those cases shouldn't occur in normal webpages, and the only side-effect of that is that the opened window won't have a cloned session storage etc from the opening document but rather a clone of the session storage etc of the current page in the opening window.

2) It makes nsGlobalWindow::GetSessionStorage() not forward the call to the outer window and thus ends up using the principals in the inner window, as long as the property is accessed on an inner window. The makes the code use the correct principals since this getter is only callable from code in the same domain as the window it's called on.

Comment 9

[Johnny Stenback (:jst)]

IMO we don't *have to* hold beta for this, but I'd like to see this fixed in the release. It's IMO a fairly low-risk change and I'd be ok landing after beta1, but I'm also happy to land this before assuming the patch gets reviews etc.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 228348 [details] [diff] [review]
Make window.open and sessionStorage use the inner's principal

jst's going to make nsGlobalWindow::GetSessionStorage forward to the inner all the time for additional consistency and smaller brainprint. The end result should be the same.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 228348 [details] [diff] [review]
Make window.open and sessionStorage use the inner's principal

Looks good, sr=sicking

Comment 12

[Johnny Stenback (:jst)]

Fix checked in with mrbkap's suggested change.

Comment 13

[Johnny Stenback (:jst)]

Attachment: 1.8 branch version of the above.

Comment 14

[Johnny Stenback (:jst)]

Fix landed on the 1.8 branch.

Comment 15

[Boris Zbarsky [:bzbarsky]]

jst, could you document the new arg to OpenInternal in nsGlobalWindow.h?

Comment 16

[Johnny Stenback (:jst)]

bz, how about:

+   * @param aCalleePrincipal The principal of the window on which open* was
+   *                         called. This can different than the current
+   *                         principals if open* was called on the outer
+   *                         window in a scope other than the current scope.

?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Looks good, if s/can different/can be different/.  Thanks!

Comment 18

[Hixie (not reading bugmail)]

FYI: The spec was just changed to only use the page's origin. No more domain stuff.