Enforce form-action CSP even when form.target is present.

Enforce form-action CSP even when form.target is present.

BUG=630332
Committed: https://crrev.com/4ac4aff49c4c539bce6d8a0d8800c01324bb6bc0
Cr-Commit-Position: refs/heads/master@{#429922}

[Łukasz Anforowicz, 2016-11-01 23:35:15]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 23:35:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 01:38:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 01:38:50]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-11-02 04:32:40]

lukasza@chromium.org changed reviewers:
+ estark@chromium.org, mkwst@chromium.org

[Łukasz Anforowicz, 2016-11-02 04:32:41]

mkwst@ / estark@, could you please take a look?

I think it makes sense to start enforcing form-action for forms via
HTMLFormElement::scheduleFormSubmission.  Even if it doesn't cover some cases
(e.g. redirects), it does seem to be a strict improvement (fixing the behavior
and adding correct enforcement for forms with a target attribute).

FWIW, in https://crbug.com/630332#c20 I am trying to argue that redirects are
low-priority (or maybe irrelevant altogether) in case of form-action directive. 
Let me know if what I wrote there makes sense (or not :-).

https://codereview.chromium.org/2464123004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-blocked-expected.txt
(right):

https://codereview.chromium.org/2464123004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-blocked-expected.txt:1:
CONSOLE ERROR: Refused to send form data to
'http://127.0.0.1:8000/navigation/resources/form-target.pl' because it violates
the following Content Security Policy directive: "form-action 'none'".
Checking form-action CSP inside HTMLFormElement::scheduleFormSubmission means
that we get to see line numbers (the location where
document.getElementById('submit').click() is called) in the test output.

https://codereview.chromium.org/2464123004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2464123004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1588: 
I am not sure if the addition of the comment above is useful.  Maybe I should
just remove it... :-/

At any rate, I see the need to handle redirects for enforcement of child-src,
ancestor-src, and other CSP directives where the checked (and potentially
malicious) resource ends up next to the protected resource.  But... I am not
sure if handling of redirects is useful (because the protected resource [the
form data] gets out into the wild as soon as the first http request is made
(i.e. the protected resource is entrusted to the form-action - we have to trust
that the server receiving the data won't misuse the data and won't be tricked
into disclosing the data to another party [via redirect or otherwise]).  Please
let me know if this makes sense (e.g. by replying to
https://crbug.com/630332#c20)

[estark, 2016-11-02 23:26:47]

Thank you so much for picking this up! LGTM, I think enforcing only for initial
submission is worthwhile at the moment. I'm mulling over your arguments about
redirects in general and will respond on the bug.

https://codereview.chromium.org/2464123004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-blocked-when-target-blank.html
(right):

https://codereview.chromium.org/2464123004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-blocked-when-target-blank.html:24:

Could you also add a test case for a submission to a different window? (i.e.
window.open and then target="<opened window's name>")

https://codereview.chromium.org/2464123004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2464123004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1588: 
On 2016/11/02 04:32:41, Łukasz Anforowicz wrote:
> I am not sure if the addition of the comment above is useful.  Maybe I should
> just remove it... :-/

Hmm. This is a weird situation; I'd vote for removing the comment. My fear is
that it draws attention to a CSP bypass. If we end up deciding that we do indeed
want to enforce for redirects, it's probably better not to have a comment
drawing attention to it.

> 
> At any rate, I see the need to handle redirects for enforcement of child-src,
> ancestor-src, and other CSP directives where the checked (and potentially
> malicious) resource ends up next to the protected resource.  But... I am not
> sure if handling of redirects is useful (because the protected resource [the
> form data] gets out into the wild as soon as the first http request is made
> (i.e. the protected resource is entrusted to the form-action - we have to
trust
> that the server receiving the data won't misuse the data and won't be tricked
> into disclosing the data to another party [via redirect or otherwise]). 
Please
> let me know if this makes sense (e.g. by replying to
> https://crbug.com/630332#c20)

[Łukasz Anforowicz, 2016-11-03 00:05:25]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 00:05:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2016-11-03 00:15:30]

Thanks for the review.  mkwst@ - your turn please? :-)

https://codereview.chromium.org/2464123004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-blocked-when-target-blank.html
(right):

https://codereview.chromium.org/2464123004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-blocked-when-target-blank.html:24:

On 2016/11/02 23:26:47, estark wrote:
> Could you also add a test case for a submission to a different window? (i.e.
> window.open and then target="<opened window's name>")

Done.  (I've also made the other window cross-site, in case it matters for test
coverage + tested with and without --site-per-process).

https://codereview.chromium.org/2464123004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2464123004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1588: 
On 2016/11/02 23:26:47, estark wrote:
> On 2016/11/02 04:32:41, Łukasz Anforowicz wrote:
> > I am not sure if the addition of the comment above is useful.  Maybe I
should
> > just remove it... :-/
> 
> Hmm. This is a weird situation; I'd vote for removing the comment. My fear is
> that it draws attention to a CSP bypass. If we end up deciding that we do
indeed
> want to enforce for redirects, it's probably better not to have a comment
> drawing attention to it.
> 
> > 
> > At any rate, I see the need to handle redirects for enforcement of
child-src,
> > ancestor-src, and other CSP directives where the checked (and potentially
> > malicious) resource ends up next to the protected resource.  But... I am not
> > sure if handling of redirects is useful (because the protected resource [the
> > form data] gets out into the wild as soon as the first http request is made
> > (i.e. the protected resource is entrusted to the form-action - we have to
> trust
> > that the server receiving the data won't misuse the data and won't be
tricked
> > into disclosing the data to another party [via redirect or otherwise]). 
> Please
> > let me know if this makes sense (e.g. by replying to
> > https://crbug.com/630332#c20)

Done - I've removed the comment.

[Łukasz Anforowicz, 2016-11-03 00:15:35]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 00:16:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 02:07:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 02:07:29]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-11-03 12:48:26]

This LGTM for the moment, but as discussed on the bug, I think there's more work
to do (unless we reject redirects entirely as a class of things we care about).

Do we fire the `error` event? Do we still fire the `submit` event?

[Łukasz Anforowicz, 2016-11-03 18:41:40]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[Łukasz Anforowicz, 2016-11-03 18:41:50]

On 2016/11/03 12:48:26, Mike West wrote:
> This LGTM for the moment, but as discussed on the bug, I think there's more
work
> to do (unless we reject redirects entirely as a class of things we care
about).

Thanks for the review.  I'll hold off with pushing to CQ until Friday - in case
I misunderstood what you are referring to below when saying: "`error` event".

> Do we fire the `error` event? Do we still fire the `submit` event?

Thank you for mentioning that.  I assume that by 'error' you mean
'securitypolicyviolation' event.  FWIW, I've added test verification for both of
these events (and it is also much nicer to terminate the test after
'securitypolicyviolation' event, rather than after an arbitrary delay of 1
second as before).

Oh, and the answer is: yes, we fire both 'securitypolicyviolation' and 'submit'
events in case of a form submission blocked by form-action CSP directive.

[commit-bot: I haz the power, 2016-11-03 18:42:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 19:44:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 19:44:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Łukasz Anforowicz, 2016-11-04 15:10:17]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 15:10:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 15:15:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 15:15:20]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-11-04 17:02:57]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-11-04 17:02:57]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2464123004/#ps60001
(title: "Added verification of 'securitypolicyviolation' and 'submit' events.")

[commit-bot: I haz the power, 2016-11-04 17:03:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 17:10:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-04 17:16:12]

Description was changed from

==========
Enforce form-action CSP even when form.target is present.

BUG=630332
==========

to

==========
Enforce form-action CSP even when form.target is present.

BUG=630332

Committed: https://crrev.com/4ac4aff49c4c539bce6d8a0d8800c01324bb6bc0
Cr-Commit-Position: refs/heads/master@{#429922}
==========

[commit-bot: I haz the power, 2016-11-04 17:16:14]

Patchset 4 (id:??) landed as
https://crrev.com/4ac4aff49c4c539bce6d8a0d8800c01324bb6bc0
Cr-Commit-Position: refs/heads/master@{#429922}