Closed Bug 495112 Opened 15 years ago Closed 15 years ago

Make sessionStorage be mapped by a domain again

Categories

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

1.9.1 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: mayhemer, Assigned: mayhemer)

References

()

Details

(Keywords: fixed1.9.1, Whiteboard: [partial backout of bug 455070])

Attachments

(3 files, 2 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
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
Attachment #379954 - Flags: review?(jst)
So this means sessionStorage is useless for HTTPS sites?  :(
(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?
Yes.  Active network attackers can compromise the confidentiality and integrity of sessionStorage for HTTPS sites.
Attached patch v1.1 (obsolete) — Splinter Review
Fixed security check for cloned sessionStorage. I verified that items stored by https page are NOT visible to http page.
Attachment #379954 - Attachment is obsolete: true
Attachment #379954 - Flags: review?(jst)
I don't see any coverage of the https-vs-http thing in that patch?
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.
(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.
(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.
Won't perpetuating this insecure behavior make it harder to fix in a later release?
Attached patch v1.1Splinter Review
Fixed nit, same functionality.
Attachment #379975 - Attachment is obsolete: true
Attachment #379982 - Flags: review?(jst)
> 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 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.
Attachment #379982 - Flags: superreview+
Attachment #379982 - Flags: review?(jst)
Attachment #379982 - Flags: review+
(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 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
Attachment #380012 - Flags: review?(jwalden+bmo) → review+
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.
Flags: blocking1.9.1+
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1
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...
Keywords: fixed1.9.1
/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?
We should fix trunk, not let bugs linger there.  When we have a better patch (in another bug) for trunk, we can land it.
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.
Whiteboard: [partial backout of bug 455070]
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.
Blocks: 495337
Fixed on trunk by the fix for bug 495112. Bug 495337 filed as a followup.

http://hg.mozilla.org/mozilla-central/rev/363750f510ec
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: