Bug 38151 – drop support for sessionStorage in sandbox iframes

• Component: Frames
• Status: RESOLVED FIXED

[eduardo]

From: https://bugs.webkit.org/show_bug.cgi?id=32369#c3

I think sessionStorage should be disabled in sandboxed iframes.

sandboxed.html: <script>sessionStorage.x='busted';</script>
sandboxer.html: <script>setInterval(function(){document.title=sessionStorage.x},500);</script><iframe sandbox="allow-scripts" src="sandboxed.html"></iframe>

[eduardo]

the previous code will set document.title to "busted", if the page is saving sensitive information in there, then it may introduce security problems.

[Adam Barth]

According to http://dev.w3.org/html5/webstorage/#the-sessionstorage-attribute, we should have one per origin.

[eduardo]

a sandboxed iframe is considered to have the same origin, or a new "unique" origin?
from http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html

"When the attribute is set, the content is treated as being from a unique origin"

[Adam Barth]

We're interested in the case where it has a unique origin.  The "correct" behavior is probably to create a new storage area for the unique origin.  I'm working up a patch that throws a security exception because that matches what we're doing for localStorage.

[eduardo]

is it useful to have sessionStorage on a window that has a unique origin every time is created?

[eduardo]

oh nvm, I just read your comment. a sec error is cool

[Adam Barth]

Here's the patch that turned this on: http://trac.webkit.org/changeset/56002

[Adam Barth]

Created attachment 55155 [details]
Patch

[WebKit Review Bot]

Attachment 55155 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2223001

[Adam Barth]

Created attachment 55170 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 55170 [details]
Patch

WebCore/storage/StorageEventDispatcher.cpp:60
 +              if (!ec)
Could/should this be an early return instead of further indent to the line-that-never-wrapped?

WebCore/storage/StorageEventDispatcher.cpp:61
 +                  frames[i]->document()->enqueueEvent(StorageEvent::create(eventNames().storageEvent, key, oldValue, newValue, sourceFrame->document()->url(), storage));
I'm rather anti-wrap, but I think even I might have used a local variable here.

WebKit/chromium/src/StorageAreaProxy.cpp:130
 +              Storage* storage = frames[i]->domWindow()->sessionStorage(ec);
Why not just check storage != NULL?

Seems OK, even if the EC handling is slightly inelegant.

[Eric Seidel (no email)]

Comment on attachment 55170 [details]
Patch

Adam should see my comments first.

[Adam Barth]

Comment on attachment 55170 [details]
Patch

I think its ok for now.  I copied the style from localStorage below.  If we want to change it, we should change both at once.

[WebKit Commit Bot]

Comment on attachment 55170 [details]
Patch

Clearing flags on attachment: 55170

Committed r58873: <http://trac.webkit.org/changeset/58873>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.