Bug 61581 - Check access policy on all storage operations
Summary: Check access policy on all storage operations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-26 16:36 PDT by jochen
Modified: 2011-05-27 20:27 PDT (History)
2 users (show)

See Also:


Attachments
Patch (8.11 KB, patch)
2011-05-26 16:37 PDT, jochen
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (13.23 KB, patch)
2011-05-26 17:33 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (26.20 KB, patch)
2011-05-27 00:07 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (25.62 KB, patch)
2011-05-27 00:44 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (26.05 KB, patch)
2011-05-27 10:59 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (25.85 KB, patch)
2011-05-27 12:54 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (25.84 KB, patch)
2011-05-27 16:53 PDT, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2011-05-26 16:36:06 PDT
Check access policy on all storage operations
Comment 1 jochen 2011-05-26 16:37:09 PDT
Created attachment 95072 [details]
Patch
Comment 2 jochen 2011-05-26 16:47:59 PDT
Adam, no idea whether you're the right person to review. Feel free to punt to somebody else
Comment 3 Adam Barth 2011-05-26 17:01:14 PDT
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.
Comment 4 jochen 2011-05-26 17:04:50 PDT
(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
Comment 5 Adam Barth 2011-05-26 17:06:28 PDT
> 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.
Comment 6 Adam Barth 2011-05-26 17:08:06 PDT
Looks like most of them take a Frame anyway to check quota.
Comment 7 jochen 2011-05-26 17:33:47 PDT
Created attachment 95087 [details]
Patch
Comment 8 WebKit Commit Bot 2011-05-26 17:39:10 PDT
Comment on attachment 95072 [details]
Patch

Attachment 95072 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8740237
Comment 9 Adam Barth 2011-05-26 19:34:42 PDT
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.
Comment 10 Adam Barth 2011-05-26 20:27:33 PDT
What's the testing plan for this code?
Comment 11 jochen 2011-05-27 00:07:05 PDT
Created attachment 95127 [details]
Patch
Comment 12 Adam Barth 2011-05-27 00:17:22 PDT
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!
Comment 13 jochen 2011-05-27 00:44:34 PDT
Created attachment 95133 [details]
Patch
Comment 14 jochen 2011-05-27 00:46:13 PDT
(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 15 jochen 2011-05-27 10:32:07 PDT
Comment on attachment 95133 [details]
Patch

i'll first have to rebase after 61583 has landed
Comment 16 Adam Barth 2011-05-27 10:36:56 PDT
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?
Comment 17 jochen 2011-05-27 10:59:10 PDT
Created attachment 95191 [details]
Patch
Comment 18 jochen 2011-05-27 10:59:44 PDT
(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
Comment 19 Adam Barth 2011-05-27 11:00:30 PDT
Comment on attachment 95191 [details]
Patch

Looks great.  Thanks!
Comment 20 jochen 2011-05-27 12:54:36 PDT
Created attachment 95207 [details]
Patch
Comment 21 jochen 2011-05-27 12:56:18 PDT
Comment on attachment 95207 [details]
Patch

rebased
Comment 22 WebKit Commit Bot 2011-05-27 16:16:16 PDT
The commit-queue encountered the following flaky tests while processing attachment 95207 [details]:

java/lc3/ConvertJSObject/ToLong-002.html bug 61676 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 23 WebKit Commit Bot 2011-05-27 16:17:54 PDT
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
Comment 24 jochen 2011-05-27 16:53:51 PDT
Created attachment 95230 [details]
Patch
Comment 25 jochen 2011-05-27 16:54:24 PDT
Comment on attachment 95230 [details]
Patch

and filled in the reviewer...
Comment 26 WebKit Commit Bot 2011-05-27 20:27:29 PDT
Comment on attachment 95230 [details]
Patch

Clearing flags on attachment: 95230

Committed r87597: <http://trac.webkit.org/changeset/87597>
Comment 27 WebKit Commit Bot 2011-05-27 20:27:36 PDT
All reviewed patches have been landed.  Closing bug.