Bug 63257 – When blocking localStorage, Firefox throws a security exception on access, and maybe so should we

• Component: WebKit Misc.
• Status: RESOLVED FIXED

We just don't return data, and throw a quota exceeded exception on setItem. For the sake of making it easier for web authors, we maybe should throw a security exception on all access instead

[Alexey Proskuryakov]

<rdar://problem/7703121>

What kinds of blocking do you intend to track with this bug? The above Radar is about Safari private browsing mode, for instance.

I was really talking about the WebKit/chromium implementation of this. In http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/StorageAreaProxy.cpp?rev=87597#L92 we check whether access to localStorage is granted according to the embedder, and if not, throw an exception.

[Alexey Proskuryakov]

OK. Both Chromium and cross-platform code will need to be fixed though.

[Dan Carney]

Created attachment 163879 [details]
Initial patch with only chromium port complete

Comment on attachment 163879 [details]
Initial patch with only chromium port complete

View in context: https://bugs.webkit.org/attachment.cgi?id=163879&action=review

> Source/WebCore/bindings/v8/custom/V8StorageCustom.cpp:46
> +        goto fail;

why don't you write something like

if (UNLIKELY(ec)) {
  setDOMException(ec, info.GetIsolate();
  return;
}

instead of the goto here and in the other places?

[kov's GTK+ EWS bot]

Comment on attachment 163879 [details]
Initial patch with only chromium port complete

Attachment 163879 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13848174

[Build Bot]

Comment on attachment 163879 [details]
Initial patch with only chromium port complete

Attachment 163879 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13845225

[Build Bot]

Comment on attachment 163879 [details]
Initial patch with only chromium port complete

Attachment 163879 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13832808

[Early Warning System Bot]

Comment on attachment 163879 [details]
Initial patch with only chromium port complete

Attachment 163879 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13855037

[Gyuyoung Kim]

Comment on attachment 163879 [details]
Initial patch with only chromium port complete

Attachment 163879 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13851145

[Early Warning System Bot]

Comment on attachment 163879 [details]
Initial patch with only chromium port complete

Attachment 163879 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13839555

[WebKit Review Bot]

Comment on attachment 163879 [details]
Initial patch with only chromium port complete

Attachment 163879 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13844312

New failing tests:
fast/storage/storage-detached-iframe.html

[Michael Nordman]

Comment on attachment 163879 [details]
Initial patch with only chromium port complete

View in context: https://bugs.webkit.org/attachment.cgi?id=163879&action=review

> Source/WebCore/storage/StorageAreaImpl.cpp:107
> +bool StorageAreaImpl::canAccessStorage(Frame* frame) const

Not sure about this method body? If i'm reading it right, it looks like it changes existing behavior about when storage is accessible. Are those changes intentional?

> Source/WebCore/storage/StorageAreaImpl.cpp:111
> +    if (frame->page()->settings()->privateBrowsingEnabled())

It looks like local and session storage are no longer accessible when 'private browsing' under any circumstances.

> Source/WebCore/storage/StorageAreaImpl.cpp:113
>      if (m_storageType != LocalStorage)

And SessionStorage is no longer accessible ever.

[Michael Nordman]

Instead of handling this in the bindings layer, have you considered doing these checks in the StorageArea impls?

Notice how ::setItem() is done with the ec out param being plumbed down to area. Maybe consistently do the same for all of these methods. A benefit would be not  checking 'canAccess' twice in the chromium port on each query. When measuring perf times in the past, the cost of the canAccess call was actually a pretty significant taker of time.

[Brady Eidson]

> > Source/WebCore/storage/StorageAreaImpl.cpp:111
> > +    if (frame->page()->settings()->privateBrowsingEnabled())
> 
> It looks like local and session storage are no longer accessible when 'private browsing' under any circumstances.

Actually, they already aren't available under "private browsing" in any circumstance.

I'm okay with this patch with regards to 3rd-party storage blocking resulting in these exceptions to match Firefox.

But I think it would be unwise to start throwing the exceptions when in private browsing...  as that announces to the website that the user is in private browsing!!!

[Dan Carney]

(In reply to comment #15)
> > > Source/WebCore/storage/StorageAreaImpl.cpp:111
> > > +    if (frame->page()->settings()->privateBrowsingEnabled())
> > 
> > It looks like local and session storage are no longer accessible when 'private browsing' under any circumstances.
> 
> Actually, they already aren't available under "private browsing" in any circumstance.
> 
> I'm okay with this patch with regards to 3rd-party storage blocking resulting in these exceptions to match Firefox.
> 
> But I think it would be unwise to start throwing the exceptions when in private browsing...  as that announces to the website that the user is in private browsing!!!

Yes, that makes sense.  I'll leave the private browsing check exactly as is, and add an additional check called canAccessStorage which will allow implementation specific security exceptions to be thrown.

[Dan Carney]

Created attachment 164084 [details]
Another round - still chromium only complete

Okay, I've taken the above comments into consideration, and done the following:

* moved all the checks into the StorageAreaImpls and inlined the calls in StorageArea. This seemed like the most elegant way to handle the conflicting requirements of private browsing and the chromium access check.

* introduced a canAccessStorage method which in the default implementation just checks for a detached frame. I'm not sure if this actually correct as I need to check what Firefox does here.

* for chromium, introduced an access check cache and optimized the code paths for the cached case as apparently this really slows down storage access.  This obsoletes https://bugs.webkit.org/show_bug.cgi?id=88412.  Note that this means all accesses after the user has the storage object are likely to succeed even if the permissions are changed.

If everyone is okay with this approach, I'll clean up the code and fix the JSC bindings.

[WebKit Review Bot]

Attachment 164084 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/storage/storage-detached-..." exit_code: 1
Source/WebCore/bindings/v8/custom/V8StorageCustom.cpp:44:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebCore/bindings/v8/custom/V8StorageCustom.cpp:49:  Omit int when using unsigned  [runtime/unsigned] [1]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 164084 [details]
Another round - still chromium only complete

Attachment 164084 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13841701

[Build Bot]

Comment on attachment 164084 [details]
Another round - still chromium only complete

Attachment 164084 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13847531

[Early Warning System Bot]

Comment on attachment 164084 [details]
Another round - still chromium only complete

Attachment 164084 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13849510

[Build Bot]

Comment on attachment 164084 [details]
Another round - still chromium only complete

Attachment 164084 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13844590

[Gyuyoung Kim]

Comment on attachment 164084 [details]
Another round - still chromium only complete

Attachment 164084 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13841711

[Brady Eidson]

(In reply to comment #17)
> Created an attachment (id=164084) [details]
> Another round - still chromium only complete
> 
> Okay, I've taken the above comments into consideration, and done the following:
>...
> * introduced a canAccessStorage method which in the default implementation just checks for a detached frame. I'm not sure if this actually correct as I need to check what Firefox does here.

Jeffrey Pfau (cc'ed) is adding a "3rd party storage blocking" feature that allows Webkit to block any 3rd party script from accessing any storage technologies.

I think this canAccessStorage method should also check whether the access is disqualified based on 3rd party storage blocking.

[Dan Carney]

(In reply to comment #24)
> (In reply to comment #17)
> > Created an attachment (id=164084) [details] [details]
> > Another round - still chromium only complete
> > 
> > Okay, I've taken the above comments into consideration, and done the following:
> >...
> > * introduced a canAccessStorage method which in the default implementation just checks for a detached frame. I'm not sure if this actually correct as I need to check what Firefox does here.
> 
> Jeffrey Pfau (cc'ed) is adding a "3rd party storage blocking" feature that allows Webkit to block any 3rd party script from accessing any storage technologies.
> 
> I think this canAccessStorage method should also check whether the access is disqualified based on 3rd party storage blocking.

There is already the following check in DOMWindow::localStorage and ::sessionStorage accessors:

document->securityOrigin()->canAccessLocalStorage()

I believe that checks for third party accesses and throws the exception.  I can try to add it to the canAccessStorage, but that would be potentially expensive...

[Brady Eidson]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #17)
> > > Created an attachment (id=164084) [details] [details] [details]
> > > Another round - still chromium only complete
> > > 
> > > Okay, I've taken the above comments into consideration, and done the following:
> > >...
> > > * introduced a canAccessStorage method which in the default implementation just checks for a detached frame. I'm not sure if this actually correct as I need to check what Firefox does here.
> > 
> > Jeffrey Pfau (cc'ed) is adding a "3rd party storage blocking" feature that allows Webkit to block any 3rd party script from accessing any storage technologies.
> > 
> > I think this canAccessStorage method should also check whether the access is disqualified based on 3rd party storage blocking.
> 
> There is already the following check in DOMWindow::localStorage and ::sessionStorage accessors:
> 
> document->securityOrigin()->canAccessLocalStorage()
> 
> I believe that checks for third party accesses and throws the exception.  I can try to add it to the canAccessStorage, but that would be potentially expensive...

I didn't mean to recommend a specific implementation detail so much as to make sure the feature worked reasonably with this change.  If it already works, then that's great.

[Dan Carney]

(In reply to comment #26)
> (In reply to comment #25)
> I didn't mean to recommend a specific implementation detail so much as to make sure the feature worked reasonably with this change.  If it already works, then that's great.

I'm not 100% certain it does, but I'll check it.

[Vicki Pfau]

I've just landed http://trac.webkit.org/changeset/128653 which will likely affect this patch at least in some shape.

[Dan Carney]

Created attachment 169176 [details]
Patch

[Michael Nordman]

Comment on attachment 169176 [details]
Patch

Thnx for moving the logic out of the bindings layer and into less voodoo'ish webcore classes.

View in context: https://bugs.webkit.org/attachment.cgi?id=169176&action=review

> Source/WebCore/page/DOMWindow.cpp:749
> +        if (!m_sessionStorage->area()->canAccessStorage(m_frame)) {

Are these canAccessStorage() tests really needed for the window.sessionStorage and the window.localStorage accessors? Each method within the storage itself has been modified to test interally, this extra layer of checking on the window attribute value means we'll generally be checking twice per operation, and the first check will incur the larger cost since its bypassing the cached value.

And if the check on the window attribute value really is needed... as coded it's bypassing the area's cached value and will incur the full cost of looking up the policy value. Could this check utilize the cached value?

Answering that first question first would be good. If we could avoid the test for the window attribute access all the better.

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:137
> +    if (UNLIKELY(!frame || !frame->page()))

Is there any case where 'frame' would be NULL? Not saying there isn't, just asking.

Comment on attachment 169176 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169176&action=review

>> Source/WebCore/page/DOMWindow.cpp:749
>> +        if (!m_sessionStorage->area()->canAccessStorage(m_frame)) {
> 
> Are these canAccessStorage() tests really needed for the window.sessionStorage and the window.localStorage accessors? Each method within the storage itself has been modified to test interally, this extra layer of checking on the window attribute value means we'll generally be checking twice per operation, and the first check will incur the larger cost since its bypassing the cached value.
> 
> And if the check on the window attribute value really is needed... as coded it's bypassing the area's cached value and will incur the full cost of looking up the policy value. Could this check utilize the cached value?
> 
> Answering that first question first would be good. If we could avoid the test for the window attribute access all the better.

In firefox, foo = window.sessionStorage will already throw an exception, and since it's about matching that behavior, I'd say the test is required

>> Source/WebKit/chromium/src/StorageAreaProxy.cpp:137
>> +    if (UNLIKELY(!frame || !frame->page()))
> 
> Is there any case where 'frame' would be NULL? Not saying there isn't, just asking.

I think there's none, just page can be null (see the detached iframe layout test)

[Adam Barth]

Comment on attachment 169176 [details]
Patch

I mentioned this on another patch, but I might as well mention it here as well.  The UNLIKELY doesn't really do anything.  We should use it only when we have direct evidence that it actually improves performance.  Otherwise it just becomes copy pasta.

[Dan Carney]

Comment on attachment 169176 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169176&action=review

>> Source/WebCore/page/DOMWindow.cpp:749
>> +        if (!m_sessionStorage->area()->canAccessStorage(m_frame)) {
> 
> Are these canAccessStorage() tests really needed for the window.sessionStorage and the window.localStorage accessors? Each method within the storage itself has been modified to test interally, this extra layer of checking on the window attribute value means we'll generally be checking twice per operation, and the first check will incur the larger cost since its bypassing the cached value.
> 
> And if the check on the window attribute value really is needed... as coded it's bypassing the area's cached value and will incur the full cost of looking up the policy value. Could this check utilize the cached value?
> 
> Answering that first question first would be good. If we could avoid the test for the window attribute access all the better.

Presently there in no way to ensure the cache is cleared, so I had to leave an uncached version on calls to window.localStorage and window.sessionStorage since the point of the bug was to replicate firefox's behaviour an throw an exception on bad access.  In general, once the storage object is retrieved, the cache will take over, which is much faster than the present implementation, which is constantly checking with the permissionclient (i think across processes) at great expense.

>> Source/WebKit/chromium/src/StorageAreaProxy.cpp:137
>> +    if (UNLIKELY(!frame || !frame->page()))
> 
> Is there any case where 'frame' would be NULL? Not saying there isn't, just asking.

I don't know, but the null check here has been taken over from the refactor of Storage, which had it all over the place.

[Dan Carney]

(In reply to comment #32)
> (From update of attachment 169176 [details])
> I mentioned this on another patch, but I might as well mention it here as well.  The UNLIKELY doesn't really do anything.  We should use it only when we have direct evidence that it actually improves performance.  Otherwise it just becomes copy pasta.

Yeah I've been trying to avoid it in generally, it's just that this patch started out trying to additionally fix a performance problem from another bug, so they were there a long time ago.  I'll get rid of them, as I'm assuming the access cache benefit is the big win and actually performing db operations will take way longer than checking a few fields.

[Michael Nordman]

Comment on attachment 169176 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169176&action=review

>>>> Source/WebCore/page/DOMWindow.cpp:749
>>>> +        if (!m_sessionStorage->area()->canAccessStorage(m_frame)) {
>>> 
>>> Are these canAccessStorage() tests really needed for the window.sessionStorage and the window.localStorage accessors? Each method within the storage itself has been modified to test interally, this extra layer of checking on the window attribute value means we'll generally be checking twice per operation, and the first check will incur the larger cost since its bypassing the cached value.
>>> 
>>> And if the check on the window attribute value really is needed... as coded it's bypassing the area's cached value and will incur the full cost of looking up the policy value. Could this check utilize the cached value?
>>> 
>>> Answering that first question first would be good. If we could avoid the test for the window attribute access all the better.
>> 
>> In firefox, foo = window.sessionStorage will already throw an exception, and since it's about matching that behavior, I'd say the test is required
> 
> Presently there in no way to ensure the cache is cleared, so I had to leave an uncached version on calls to window.localStorage and window.sessionStorage since the point of the bug was to replicate firefox's behaviour an throw an exception on bad access.  In general, once the storage object is retrieved, the cache will take over, which is much faster than the present implementation, which is constantly checking with the permissionclient (i think across processes) at great expense.

The trouble is that i don't think consumers of this api generally retrieve the 'storage' attribute value and keep it around. Usage looks more like get it when you need it...

for (i = 0; i < localStorage.length(); i++) {
  localStorage.getItem(localStorage.key(i));
}

... so the test on the window attribute value access kind of defeats the additional cache in WebCore::StorageArea.

The check today generally does not go across process boundaries, but it does call out through the webkit api and retrieve a cached value from a larger collection maintained on the client side. It's not a great expense, but the data type transformations when calling back and forth across the webkit api do add up when iterating of some number of values.

The logic that goes around the really fast cache in WebCore::StorageArea does nothing to clear out that larger cached collection in the renderer. If that larger cache is not cleared on settings changes (i don't think it is but jochen might know better), i think clearing the WebCore::StorageArea cache doesn't accomplish anything except to make it a bit slower.

But honestly, regardless of what happens to the larger cache... I'd prefer to see the fast cache utilized here too. We're talking about possible changes in behavior between chrome/ff when the content setting value is changed some time after a page is up and running and having already accessed localStorage. If there is a difference in that narrow case... i'd assert thats just fine.

[Dan Carney]

> But honestly, regardless of what happens to the larger cache... I'd prefer to see the fast cache utilized here too. We're talking about possible changes in behavior between chrome/ff when the content setting value is changed some time after a page is up and running and having already accessed localStorage. If there is a difference in that narrow case... i'd assert thats just fine.

Okay, I'm indifferent, and you're the only one who's spoken up, so I'll use the fast cache everywhere but keep the check on the window.localStorage accessor to mimic firefox. I'll repost those changes shortly.

[Dan Carney]

Created attachment 169884 [details]
Patch

[Michael Nordman]

thnx and lgtm!

[WebKit Review Bot]

Comment on attachment 169884 [details]
Patch

Clearing flags on attachment: 169884

Committed r132183: <http://trac.webkit.org/changeset/132183>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.