Implement the form-action Content Security Policy directive.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike West]

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

[Mike West]

Created attachment 157907 [details]
First pass.

[Mike West]

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 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.

[Mike West]

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!

[Mike West]

Created attachment 158010 [details]
Patch

[Adam Barth]

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 ?

[Adam Barth]

This approach looks reasonable to me.  Let's get Bug 93850 resolved and then we can finish this one up.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Mike West]

Created attachment 158087 [details]
Rebase, fix tests.

[Mike West]

(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. :)

[Mike West]

Created attachment 158337 [details]
Rebase on top of plugin-types.

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

[Mike West]

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.

[Adam Barth]

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.

[Mike West]

Created attachment 158375 [details]
Jochen's feedback.

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.

[Mike West]

Created attachment 158385 [details]
No ResourceError.

[Mike West]

(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. :)

[WebKit Review Bot]

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

[Peter Beverloo]

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.

[Mike West]

Created attachment 158764 [details]
*sigh*

[Mike West]

Created attachment 158765 [details]
Beverloo + PHP = ♥

[Mike West]

(In reply to comment #23)
> Created an attachment (id=158765) [details]
> Beverloo + PHP = ♥

Thanks, Peter! :)

[WebKit Review Bot]

Comment on attachment 158765 [details]
Beverloo + PHP = ♥

Clearing flags on attachment: 158765

Committed r125772: <http://trac.webkit.org/changeset/125772>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.