Bug 93777 - Implement the form-action Content Security Policy directive.
Summary: Implement the form-action Content Security Policy directive.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords: WebExposed
Depends on: 93850
Blocks: 85558
  Show dependency treegraph
 
Reported: 2012-08-12 04:23 PDT by Mike West
Modified: 2012-08-16 05:42 PDT (History)
5 users (show)

See Also:


Attachments
First pass. (25.90 KB, patch)
2012-08-12 13:07 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch (31.21 KB, patch)
2012-08-13 08:53 PDT, Mike West
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (543.15 KB, application/zip)
2012-08-13 11:19 PDT, WebKit Review Bot
no flags Details
Rebase, fix tests. (35.39 KB, patch)
2012-08-13 13:19 PDT, Mike West
no flags Details | Formatted Diff | Diff
Rebase on top of plugin-types. (35.52 KB, patch)
2012-08-14 08:16 PDT, Mike West
no flags Details | Formatted Diff | Diff
Jochen's feedback. (35.44 KB, patch)
2012-08-14 11:22 PDT, Mike West
no flags Details | Formatted Diff | Diff
No ResourceError. (35.38 KB, patch)
2012-08-14 11:48 PDT, Mike West
no flags Details | Formatted Diff | Diff
*sigh* (35.23 KB, patch)
2012-08-16 03:23 PDT, Mike West
no flags Details | Formatted Diff | Diff
Beverloo + PHP = ♥ (35.24 KB, patch)
2012-08-16 03:26 PDT, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 2012-08-12 04:23:09 PDT
The CSP 1.1 editor's draft defines the `form-action` directive, which limits the acceptable actions for form submission by whitelisting specific targets[1]. As this directive is still experimental, we should implement it behind the CSP_NEXT flag.

[1]: https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#form-action--experimental
Comment 1 Mike West 2012-08-12 13:07:52 PDT
Created attachment 157907 [details]
First pass.
Comment 2 Mike West 2012-08-12 13:14:24 PDT
This isn't a patch for landing, but I think it's worth putting up for review. Two issues I see:

1. After a lot of back and forth with GDB, 'MainResourceLoader::willSendRequest' is the only place I see that gets called both on form submission, and on a redirect after form submission. That's where this patch places the CSP checks. I'm not sure that's good enough, as I don't think it gives the ability to check a chain of redirects (and I'm also not sure that checking `httpBody` is enough to determine if we're in a form submission?).

2. It doesn't currently implement the response the spec mandates: an empty 400 response. I'm at a bit of a loss as to how I can generate such a thing.

WDYT, Adam?

CCing Jochen as well, as he knows strange things about loaders.

Thanks!
Comment 3 jochen 2012-08-13 04:00:21 PDT
Comment on attachment 157907 [details]
First pass.

I don't think the MainResourceLoader is the right place to do the check:

- submissions to javascript URls never hit the MainResourceLoader
- what about submissions using GET?
- what about XHRs using POST?

Can you add tests for all three cases?

I would either tell the MainResourceLoader that it's processing a form submission, or have the main frame loader call back to the frame loader in willSendRequest so it can do the check

ResourceLoader has a cancel method that takes a ResourceError. You can use this to create a custom error code.
Comment 4 Mike West 2012-08-13 08:40:31 PDT
GET submissions were a good suggestion, thanks Jochen. As it turns out, cancelling them is buggy: covered in issue 93850.

I'll upload a patch in a moment that addresses your concerns (with the exception of XHR, as you noted in chat that they don't go through MainResourceLoader). It includes a fix for the above-noted issue, which I'll take out once it's landed on trunk.

I've added a call to ::cancel with a ResourceError, as you suggested. It might or might not work. It's certainly the case that neither a 'load' nor 'error' event is generated on the form, which I believe Adam wanted in order to drop the `setTimeout` call to end the test. I'll dig into that a bit more, but ideas would be welcome. :)

Thanks!
Comment 5 Mike West 2012-08-13 08:53:59 PDT
Created attachment 158010 [details]
Patch
Comment 6 Adam Barth 2012-08-13 10:48:56 PDT
Comment on attachment 158010 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.h:224
> +    // Content Security Policy related functions.

I'd skip this comment.  It doesn't really add anything.

> Source/WebCore/page/ContentSecurityPolicy.cpp:716
> +    if (type == "formAction")

formAction -> form-action ?
Comment 7 Adam Barth 2012-08-13 10:49:35 PDT
This approach looks reasonable to me.  Let's get Bug 93850 resolved and then we can finish this one up.
Comment 8 WebKit Review Bot 2012-08-13 11:19:54 PDT
Comment on attachment 158010 [details]
Patch

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

New failing tests:
http/tests/security/contentSecurityPolicy/worker-connect-src-blocked.html
http/tests/security/contentSecurityPolicy/1.1/form-action-src-blocked.html
http/tests/security/contentSecurityPolicy/1.1/form-action-src-redirect-blocked.html
http/tests/security/contentSecurityPolicy/connect-src-eventsource-blocked.html
http/tests/security/contentSecurityPolicy/source-list-parsing-malformed-meta.html
http/tests/security/contentSecurityPolicy/1.1/form-action-src-javascript-blocked.html
http/tests/security/contentSecurityPolicy/connect-src-websocket-blocked.html
http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-blocked.html
http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-blocked.html
Comment 9 WebKit Review Bot 2012-08-13 11:19:58 PDT
Created attachment 158053 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 10 Mike West 2012-08-13 13:19:00 PDT
Created attachment 158087 [details]
Rebase, fix tests.
Comment 11 Mike West 2012-08-13 13:24:53 PDT
(In reply to comment #7)
> This approach looks reasonable to me.  Let's get Bug 93850 resolved and then we can finish this one up.

Rebased on top of that fix, and I'm happy to fix any issues you see in the patch. :)
Comment 12 Mike West 2012-08-14 08:16:18 PDT
Created attachment 158337 [details]
Rebase on top of plugin-types.
Comment 13 jochen 2012-08-14 08:40:07 PDT
Comment on attachment 158337 [details]
Rebase on top of plugin-types.

Overall looks good. Let's wait what Adam thinks


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

> Source/WebCore/loader/FrameLoader.cpp:308
> +        // hit it.

i'd argue that this is obvious from the code below :)

> Source/WebCore/loader/FrameLoader.cpp:919
> +bool FrameLoader::checkIfFormActionAllowed(const KURL& url) const

maybe we should call it "...ByCSP" as the calling side will generate an error event that claims it stems from CSPs?

> Source/WebCore/loader/MainResourceLoader.cpp:208
> +        cancel(ResourceError(newRequest.url().host(), 400, newRequest.url().string(), "CSP"));

I think the error domain is not a hostname. I think the empty string would be the correct domain here

> Source/WebCore/page/ContentSecurityPolicy.cpp:895
> +        checkSource(m_formAction.get(), url);

nit. why not put it all in one line?

> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-allowed.html:20
> +  <form action='/navigation/resources/form-target.pl' id='theform' method='post'>

nit. 4 spaces. not that it would matter
Comment 14 Mike West 2012-08-14 10:10:48 PDT
Comment on attachment 158337 [details]
Rebase on top of plugin-types.

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

>> Source/WebCore/loader/FrameLoader.cpp:919
>> +bool FrameLoader::checkIfFormActionAllowed(const KURL& url) const
> 
> maybe we should call it "...ByCSP" as the calling side will generate an error event that claims it stems from CSPs?

Sure. Easy enough.

>> Source/WebCore/loader/MainResourceLoader.cpp:208
>> +        cancel(ResourceError(newRequest.url().host(), 400, newRequest.url().string(), "CSP"));
> 
> I think the error domain is not a hostname. I think the empty string would be the correct domain here

The ResourceError/ResourceErrorBase classes weren't exactly overflowing with documentation. I was guessing at most of these values. ;)

Can you shed some light on what they're actually used for?

>> Source/WebCore/page/ContentSecurityPolicy.cpp:895
>> +        checkSource(m_formAction.get(), url);
> 
> nit. why not put it all in one line?

Because that's unreadable. :)

>> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-allowed.html:20
>> +  <form action='/navigation/resources/form-target.pl' id='theform' method='post'>
> 
> nit. 4 spaces. not that it would matter

I'm not sure how I ended up with two spaces here and four above. Thanks, I'll fix it.
Comment 15 Adam Barth 2012-08-14 10:37:17 PDT
Comment on attachment 158337 [details]
Rebase on top of plugin-types.

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

LGTM.  I'll let jochen do the final review once you've posted a patch that addresses his comments.

>>> Source/WebCore/page/ContentSecurityPolicy.cpp:895
>>> +        checkSource(m_formAction.get(), url);
>> 
>> nit. why not put it all in one line?
> 
> Because that's unreadable. :)

I think it's fine to have this code span multiple lines.
Comment 16 Mike West 2012-08-14 11:22:54 PDT
Created attachment 158375 [details]
Jochen's feedback.
Comment 17 jochen 2012-08-14 11:36:08 PDT
Comment on attachment 158375 [details]
Jochen's feedback.

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

> Source/WebCore/loader/MainResourceLoader.cpp:208
> +        cancel(ResourceError("", 400, newRequest.url().string(), "CSP"));

I read through the source a bit, and I think it's wrong to create a ResourceError here. Usually, when we want a specific kind of error, we'd call the FrameLoaderClient to give us such an error.

I also read the spec, and it doesn't say that we should create a 400, it says we should react as if we'd received a 400. I believe just calling cancel() would suffice in that case.
Comment 18 Mike West 2012-08-14 11:48:58 PDT
Created attachment 158385 [details]
No ResourceError.
Comment 19 Mike West 2012-08-14 11:50:47 PDT
(In reply to comment #17)
> (From update of attachment 158375 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158375&action=review
> 
> > Source/WebCore/loader/MainResourceLoader.cpp:208
> > +        cancel(ResourceError("", 400, newRequest.url().string(), "CSP"));
> 
> I read through the source a bit, and I think it's wrong to create a ResourceError here. Usually, when we want a specific kind of error, we'd call the FrameLoaderClient to give us such an error.

Alright. I'll ask you about this tomorrow, since the loader code is still a bit of a black box for me. Or something more complicated than a box. :)
Comment 20 WebKit Review Bot 2012-08-16 02:48:14 PDT
Comment on attachment 158385 [details]
No ResourceError.

Rejecting attachment 158385 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ebkit-commit-queue/Source/WebKit/chromium/third_party/ots --revision 94 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
42>At revision 94.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/13518104
Comment 21 Peter Beverloo 2012-08-16 03:22:06 PDT
Comment on attachment 158385 [details]
No ResourceError.

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

> LayoutTests/http/tests/navigation/resources/redirection-response.php:7
> +if ($_GET['host']) {

You should use "if (isset($_GET['host']))" here. Per WebKit style, you could also skip the curly brackets.
Comment 22 Mike West 2012-08-16 03:23:23 PDT
Created attachment 158764 [details]
*sigh*
Comment 23 Mike West 2012-08-16 03:26:47 PDT
Created attachment 158765 [details]
Beverloo + PHP = ♥
Comment 24 Mike West 2012-08-16 03:27:20 PDT
(In reply to comment #23)
> Created an attachment (id=158765) [details]
> Beverloo + PHP = ♥

Thanks, Peter! :)
Comment 25 WebKit Review Bot 2012-08-16 05:42:25 PDT
Comment on attachment 158765 [details]
Beverloo + PHP = ♥

Clearing flags on attachment: 158765

Committed r125772: <http://trac.webkit.org/changeset/125772>
Comment 26 WebKit Review Bot 2012-08-16 05:42:31 PDT
All reviewed patches have been landed.  Closing bug.