Bug 61581 – Check access policy on all storage operations

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Check access policy on all storage operations

Created attachment 95072 [details]
Patch

Adam, no idea whether you're the right person to review. Feel free to punt to somebody else

[Adam Barth]

Comment on attachment 95072 [details]
Patch

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

> Source/WebCore/storage/StorageArea.h:58
> +        virtual bool canAccessStorage(Frame* sourceFrame) const = 0;

Typically we don't like to use Frames for security checks because it's slightly loose as to which Document is in the Frame.  Instead, we prefer to use Document or (ideally) SecurityOrigin.  In this case, it seems like StorageArea is mainly using Frame as its context object, so that might be something to fix in the future rather than in this patch.

More generally, it seems like this work should be done at the WebKit layer (not the WebCore layer) since you're just asking the embedder whether this is allowed.

(In reply to comment #3)
> (From update of attachment 95072 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95072&action=review
> 
> > Source/WebCore/storage/StorageArea.h:58
> > +        virtual bool canAccessStorage(Frame* sourceFrame) const = 0;
> 
> Typically we don't like to use Frames for security checks because it's slightly loose as to which Document is in the Frame.  Instead, we prefer to use Document or (ideally) SecurityOrigin.  In this case, it seems like StorageArea is mainly using Frame as its context object, so that might be something to fix in the future rather than in this patch.
> 
> More generally, it seems like this work should be done at the WebKit layer (not the WebCore layer) since you're just asking the embedder whether this is allowed.

I tried that before, but that meant that I need to add the Frame to each method in StorageArea, and it seemed worse that moving the check to Storage

[Adam Barth]

> I tried that before, but that meant that I need to add the Frame to each method in StorageArea, and it seemed worse that moving the check to Storage

How does the storage area know which storage to interact with?  That's what you should use to do the security check.

[Adam Barth]

Looks like most of them take a Frame anyway to check quota.

Created attachment 95087 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 95072 [details]
Patch

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

[Adam Barth]

Comment on attachment 95087 [details]
Patch

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

Generally looks good.  I'll look in more detail when I get to a real computer.

> Source/WebCore/ChangeLog:7
> +

I would add more explanation to the changelog explaining why we're implement this, especially why this calls out to the embedder.

[Adam Barth]

What's the testing plan for this code?

Created attachment 95127 [details]
Patch

[Adam Barth]

Comment on attachment 95127 [details]
Patch

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

This looks great.

> LayoutTests/platform/chromium/permissionclient/storage-permission.html:22
> +    if (window.layoutTestController && layoutTestController.setStorageAllowed)
> +        layoutTestController.setStorageAllowed(true);

You might want to log a message that this test requires layoutTestController.setStorageAllowed so can't be run in a browser (most LayoutTests can be run in a browser)

> Source/WebCore/storage/StorageAreaImpl.h:49
> +        virtual String key(unsigned index, Frame* sourceFrame) const;

Usually we pass the context object as the first argument, but I see that you're just following established convention in this interface.  When we change these to take a Document instead of a Frame, we should probably move the parameter to the first position.

> Source/WebKit/chromium/public/WebView.h:101
> +    virtual WebPermissionClient* permissionClient() = 0;

Changes to the public WebKit API require fishd approval.

> Source/WebKit/chromium/src/WebViewImpl.cpp:299
> +

extra blank line?

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1838
> +void LayoutTestController::setStorageAllowed(const CppArgumentList& arguments, CppVariant* result)

I can't believe we're still using this ancient CPPObject interface!

Created attachment 95133 [details]
Patch

(In reply to comment #12)
> (From update of attachment 95127 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95127&action=review
> 
> This looks great.
> 
> > LayoutTests/platform/chromium/permissionclient/storage-permission.html:22
> > +    if (window.layoutTestController && layoutTestController.setStorageAllowed)
> > +        layoutTestController.setStorageAllowed(true);
> 
> You might want to log a message that this test requires layoutTestController.setStorageAllowed so can't be run in a browser (most LayoutTests can be run in a browser)

Done.

> 
> > Source/WebCore/storage/StorageAreaImpl.h:49
> > +        virtual String key(unsigned index, Frame* sourceFrame) const;
> 
> Usually we pass the context object as the first argument, but I see that you're just following established convention in this interface.  When we change these to take a Document instead of a Frame, we should probably move the parameter to the first position.

I added a FIXME comment for that

> 
> > Source/WebKit/chromium/public/WebView.h:101
> > +    virtual WebPermissionClient* permissionClient() = 0;
> 
> Changes to the public WebKit API require fishd approval.

I dropped that change. Instead, I'm now maintaining a global instance as part of TestShell

> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:299
> > +
> 
> extra blank line?
> 
> > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1838
> > +void LayoutTestController::setStorageAllowed(const CppArgumentList& arguments, CppVariant* result)
> 
> I can't believe we're still using this ancient CPPObject interface!

Comment on attachment 95133 [details]
Patch

i'll first have to rebase after 61583 has landed

[Adam Barth]

Comment on attachment 95133 [details]
Patch

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

> Tools/DumpRenderTree/chromium/WebPermissions.h:43
> +    // Sets the policy whether to allow storage or not.
> +    void setStorageAllowed(bool storageAllowed) { m_storageAllowed = storageAllowed; }

Does this state leak from test to test now that this client is global?  Maybe we need to call a reset() method on it when we transition between tests?

Created attachment 95191 [details]
Patch

(In reply to comment #16)
> (From update of attachment 95133 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95133&action=review
> 
> > Tools/DumpRenderTree/chromium/WebPermissions.h:43
> > +    // Sets the policy whether to allow storage or not.
> > +    void setStorageAllowed(bool storageAllowed) { m_storageAllowed = storageAllowed; }
> 
> Does this state leak from test to test now that this client is global?  Maybe we need to call a reset() method on it when we transition between tests?

done

[Adam Barth]

Comment on attachment 95191 [details]
Patch

Looks great.  Thanks!

Created attachment 95207 [details]
Patch

Comment on attachment 95207 [details]
Patch

rebased

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 95207 [details]:

java/lc3/ConvertJSObject/ToLong-002.html bug 61676 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 95207 [details]
Patch

Rejecting attachment 95207 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'land-a..." exit_code: 1

Last 500 characters of output:
runk)
	M	Source/JavaScriptCore/ChangeLog
	M	Source/JavaScriptCore/Configurations/Base.xcconfig
	M	Source/WebKit/mac/ChangeLog
	M	Source/WebKit/mac/Configurations/Base.xcconfig
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/Configurations/Base.xcconfig
	M	Source/WebKit2/ChangeLog
	M	Source/WebKit2/Configurations/Base.xcconfig
r87580 = 99d842dc5ed5aa8decb50e950db3463f077121db (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.

Full output: http://queues.webkit.org/results/8735841

Created attachment 95230 [details]
Patch

Comment on attachment 95230 [details]
Patch

and filled in the reviewer...

[WebKit Commit Bot]

Comment on attachment 95230 [details]
Patch

Clearing flags on attachment: 95230

Committed r87597: <http://trac.webkit.org/changeset/87597>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.