495112 - Make sessionStorage be mapped by a domain again

Status: RESOLVED FIXED

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

Description

[Honza Bambas (:mayhemer)]

Attachment: v1

After bug 455070 has landed we got some regressions (bug 494543, bug 494810). 

Final decision made was to:
- leave the constructor name "Storage" for sessionStorage
- leave the correct interface (getItem returns string)
- revert mapping and security checking by an origin/principal and do it only by domain as before

Comment 1

[Adam Barth]

So this means sessionStorage is useless for HTTPS sites?  :(

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to comment #1)
> So this means sessionStorage is useless for HTTPS sites?  :(

Why should it be useless? Items stored on an https page will be seen by an http page, you mean this problem?

Comment 3

[Adam Barth]

Yes.  Active network attackers can compromise the confidentiality and integrity of sessionStorage for HTTPS sites.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Fixed security check for cloned sessionStorage. I verified that items stored by https page are NOT visible to http page.

Comment 5

[Mike Shaver (:shaver emeritus)]

I don't see any coverage of the https-vs-http thing in that patch?

Comment 6

[Anne (:annevk)]

To be clear, by "mapped by a domain" you mean "mapped by an origin"? Because otherwise this would be incompatible with other implementations of sessionStorage.

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to comment #5)
> I don't see any coverage of the https-vs-http thing in that patch?
It's already in the original implementation of globalStorage on which sessionStorage code is based. I just found a bug that is fixed in v1.1 allowing that code to work.

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to comment #6)
> To be clear, by "mapped by a domain" you mean "mapped by an origin"? Because
> otherwise this would be incompatible with other implementations of
> sessionStorage.

I mean by a domain, not by an origin. It causes regressions at the moment. We'll fix this in later release.

Comment 9

[Adam Barth]

Won't perpetuating this insecure behavior make it harder to fix in a later release?

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Fixed nit, same functionality.

Comment 11

[Adam Barth]

> Fixed security check for cloned sessionStorage. I verified that items stored by
> https page are NOT visible to http page.

Are you sure about that?  Why doesn't GetDomainURI(nsIPrincipal* aPrincipal, nsACString& aDomain) care about the scheme from nsIPrincipal?

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 379982 [details] [diff] [review]
v1.1

- In docshell/base/nsDocShell.cpp:

+GetDomainURI(nsIPrincipal* aPrincipal, nsACString& aDomain)

This method really just gets the domain from the principal, so I think we should name this GetPrincipalDomain().

r+sr=jst with that.

Comment 13

[Johnny Stenback (:jst)]

(In reply to comment #11)
> > Fixed security check for cloned sessionStorage. I verified that items stored by
> > https page are NOT visible to http page.
> 
> Are you sure about that?  Why doesn't GetDomainURI(nsIPrincipal* aPrincipal,
> nsACString& aDomain) care about the scheme from nsIPrincipal?

Once we have the time to make this use principals properly (which we don't for 3.5) this code will deal with http vs https at this level, most likely, but for now http vs https remains as a check further down the road when items are accessed, specifically code that calls IsCallerSecure(), just as it does in Firefox 3.0.

Comment 14

[Johnny Stenback (:jst)]

Attachment: Interdiff, fixes naming nit and adds test to make sure http page can't read values set by https site.

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 380012 [details] [diff] [review]
Interdiff, fixes naming nit and adds test to make sure http page can't read values set by https site.

>+    var gotValue = "no value";
>+    try {
>+      gotValue = sessionStorage.getItem("foo-https");
>+    } catch (e) {
>+    }
>+
>+    postMsg(gotValue);

Shouldn't that really be "threw" rather than "no value"?

>+<!--
>+  This test checks that sessionStorage values set in an https page
>+  is not readable from a non-https page from the same domain.
>+-->

are not readable

Comment 16

[Johnny Stenback (:jst)]

Marking this a blocker since this is where the partial backout of bug 455070 is being done. The fix here also fixes blocker bug 494502 and bug 494805.

Comment 17

[Johnny Stenback (:jst)]

Fixed on 1.9.1, both patches:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a444df427210
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/64c3ed84bda0

I'm leaving this bug open in case we want to take these changes on trunk momentarily as well, but I'll let Honza decide if he wants that. We should at least take the tests...

Comment 18

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

/me tries to figure out how we'd want to deal with a bug that's fixed1.9.1 but not FIXED on trunk ... I think we just want to mark this one FIXED and file a new bug for trunk?

Comment 19

[Mike Shaver (:shaver emeritus)]

We should fix trunk, not let bugs linger there.  When we have a better patch (in another bug) for trunk, we can land it.

Comment 20

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

OK, I think it's going to be way too confusing for us to not have this partial backout also landed on trunk. IMO, we should:

 - land this on trunk as soon as the tree is green there
 - mark it FIXED
 - file a follow-up bug to this and bug 455070 on whatever the remaining issues are and work those through on trunk

Please make it so.

Comment 21

[Johnny Stenback (:jst)]

Attachment: Trunk fix, untested.

This is the a combination of the two patches that landed for 1.9.1 here, but it was a manual merge so this needs some eyes, and comiling, and testing before it's ready to land.

Comment 22

[Johnny Stenback (:jst)]

Fixed on trunk by the fix for bug 495112. Bug 495337 filed as a followup.

http://hg.mozilla.org/mozilla-central/rev/363750f510ec