Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user
Status: Fixed
Owner:
OOO through Oct 22
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug-Security



Sign in to add a comment
CSP form-action seems to be ignored if target="_blank"
Reported by scott.he...@gmail.com, Jul 21 2016 Back to list
UserAgent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.82 Safari/537.36

Steps to reproduce the problem:
1. Issue a CSP that includes form-action 'self'
2. Add a form to a page that POSTs elsewhere with target="_blank"
3. Submit the form

What is the expected behavior?
The form should be blocked.

What went wrong?
I would have expected the form to be blocked but it is not.

Did this work before? N/A 

Chrome version: 52.0.2743.82  Channel: stable
OS Version: 10.0
Flash Version: Shockwave Flash 22.0 r0

There is a test page here: https://scotthelme.co.uk/form-action-test-bug/

Firefox blocks both forms, Chrome only blocks the one without a target attribute.
 
Comment 1 by est...@chromium.org, Jul 21 2016
Cc: mkwst@chromium.org est...@chromium.org
Components: Blink>SecurityFeature
Labels: -OS-Windows OS-All
Taking a look. Confirmed it does appear to be blocked in FF but not in Chrome. Hmmmm....
Labels: Security_Impact-Stable
Status: Available
What severity should this be tagged as?
Comment 3 by est...@chromium.org, Jul 21 2016
Labels: Security_Severity-Low
Tentatively tagging as Low severity, though I think Medium could be justifiable as well.

It looks like the form action is being checked in FrameLoader::shouldContinueForNavigationPolicy(). For the target=_blank case, I think we go through createWindowForRequest() in CreateWindow.cpp, which ends up creating a *new* frame and calling shouldContinueForNavigationPolicy() on that frame's loader. That new frame doesn't have a CSP associated with it, so we end up checking the form action against a blank CSP rather than the original document's CSP. That's my current theory at least.
Comment 4 by est...@chromium.org, Jul 21 2016
Owner: est...@chromium.org
Comment 5 by mkwst@chromium.org, Jul 21 2016
Yeah, I can believe that we're doing the wrong thing in this case. We should look at targeted openings as well (e.g. target="window-i-opened-earlier"), though I think those should probably already be taken care of.

I'd call it low as well, since it's not possible to popup a new window, form-submitted or otherwise, without a user gesture, but it's certainly a clever bypass.
Comment 6 by est...@chromium.org, Jul 21 2016
For target="window-i-opened-earlier", it looks to me like we might be applying the CSP of the target window? We do targetFrame->navigate(request) which looks to me like it'll end up calling shouldContinueForNavigationPolicy() on targetFrame's FrameLoader. I'll verify...
Comment 7 by mkwst@chromium.org, Jul 21 2016
Hrm. That would also be wrong. :(

A quick workaround would be to check the form action before we go into the navigation code. In ` HTMLFormElement::scheduleFormSubmission` perhaps? It wouldn't catch redirects, but I suspect that doing the navigation bits "right" is going to take some time and thought.
Project Member Comment 8 by sheriffbot@chromium.org, Jul 22 2016
Status: Assigned
Comment 9 by est...@chromium.org, Jul 25 2016
AHHHH this bug is super hard.

As Mike pointed out in comment 7, it would be pretty simple to catch the initial URL of the form submission in scheduleFormSubmission(), but that wouldn't handle redirects.

It would be more work but reasonably straightforward to catch redirects for target=_blank submissions. I have a prototype of how to do this by attaching a CSP to apply during document load to FrameLoader, passed in via FrameLoadRequest. I think this would make sense, but...

It's a giant pain to make this all work for form submissions targeting remote frames. That would require plumbing the CSP from the origin document to the browser process, and possibly plumbing it back down to the remote frame for non-PlzNavigate AND being able to apply the CSP in the browser process for PlzNavigate. (I think. Not sure.)

Ordinarily I'd do the scheduleFormSubmission() fix just to get it partially fixed, but I'm a little sad about essentially making the bug public yet still quite exploitable.

Mostly writing this up as a brain dump for myself -- will think more about this later!
Cc: lukasza@chromium.org
+lukasza: are you currently working on moving CSP to the browser process? I think we would need that in order to fully fix this CSP bypass. If a.com includes a form that submits to b.com in a remote frame, then the navigation of the b.com frame would have to take into account the form-action directive of the a.com frame's CSP.

Do you have any thoughts about this?
Since r394200 we replicate CSP directives to remote frame proxies (so - the problem you described in #c9 is hopefully already taken care of).  We rely on the replicated directives in enforcement of frame-src and soon will start depending on them for plugin-types (see https://crrev.com/2213593002).  I hope it is possible to rely on replicated directives for form-action, but I don't really understand how we figure out which frame's CSP we should use when checking for CSP violations in the form's target frame.


RE: lukasza: are you currently working on moving CSP to the browser process

Sadly, no.  I am just trying to make CSP enforcement OOPIF-compatible within constraints of the current design.
Cc: alex...@chromium.org dcheng@chromium.org
(I'm adding a couple more site isolation people in case they have thoughts as well.)

Thanks for the info, lukasza! It's useful to know that we're already replicating CSP directives for remote frames, though unfortunately I don't think it's quite enough to take care of this case. When we are navigating a remote frame due to a form submission, we want the navigating frame to be able to see the CSP directives of the frame that initiated the form submission -- but at the point of doing the navigation, I don't think the navigating frame knows which frame initiated the navigation, so it can't peek at its CSP directives.

(In other words, exactly what you said: in the form's target frame, we don't know which frame's CSP we should use when checking for CSP violations.)

So here is what I am thinking for this bug. What I would like to do is attach an (optional) CSP to a ResourceRequest, and serialize it into WebURLRequest so that it can be passed from the form's frame to the form target frame. In FrameFetchContext::canRequestInternal(), we currently check the request against the document's CSP. Instead, I'd like to check it against the request's CSP if the request has a CSP set, and fall back to the document's CSP if the request does not have a CSP set.

Most ResourceRequests would not have CSPs attached to them, but HTMLFormElement::scheduleFormSubmission() would set the origin document's CSP on the ResourceRequest that it creates, so that form submission requests would always get checked against the origin document's CSP.

I think this would take care of all cases (target=_blank, submitting to a remote frame, submitting to the current frame) but wouldn't yet take care of PlzNavigate, for which we'd have to actually apply CSP in the browser process.

I'd like to get Mike's take on this before I start implementing, so I'll wait until next week when he's back in the office.
1). Missing information about the origin that initiated navigation is also at the bottom of  issue 635400 

2). Maybe instead of attaching *CSP* (to ResourceRequest and elsewhere) we can instead attach identifier of the frame that initiated the navigation.

I am raising (2) as an option because A) I think sending *whole* CSP over IPC is not well defined currently (we only have IPCs for sending individual CSP headers) and B) I hope that doing this might help with (1).
My worry about (2) is whether it could be racy. e.g. what if the following situation is possible:

1. Frame A submits a form to Frame B. We attach the identifier of Frame A to the request.
2. Frame A navigates, before the form submission request reaches Frame B.
3. Frame B receives the navigation request and reads Frame A's CSP, but by this point Frame A's CSP has changed.

Maybe that's not even possible because Frame B would have received the form navigation request before it could have heard about Frame A's CSP changing...?
Please can you add troyhunt@hotmail.com to this issue? He originally tipped me off to the problem. 
Cc: troyh...@hotmail.com
What's happening with this one folks, looks pretty dormant? Anyone have any fundamental objections about me sharing publicly? This is a pretty important part of the messaging around CSP implementations, i.e. that there is still idiosyncratic browser behaviour.
This got pushed off my plate; lukasza@, any chance you'd be able to take this (at least for enforcement, if not reporting, which is probably trickier)?
Thinking a little bit more about the ways to abuse this, we could probably do with a fix sooner rather than later. This effectively allows an attacker to inject a login form that will post cross-origin, allowing the theft of credentials. 

The form-action directive is currently worthless in Chrome. 
Would it be fair to say that form-action blocking of form->final-redirect is lower priority than form-action blocking of form->immediate-url?  Or that at least maybe it is worth to plug the immediate-form-action enforcement hole (by doing CSP checks in scheduleFormSubmission), while we spend longer time deciding whether or how to also enforce form-action-followed-by-redirect?


Attack via redirect need additional vulnerability in the server
===============================================================

Perhaps I am misunderstanding what threats form-action CSP is supposed to protect against.  One threat is exposing form data to an unintended origin.  Are there other threats that form-action CSP tries to protect against (threats that wouldn't better be addressed via child-src or frame-ancestors CSP)?

If we enforce form-action CSP in scheduleFormSubmission (so - for URL of form.action, but not for URLs of subsequent redirects), then to forward form data to an attacker-controller origin, the attacker has to 1) either control the server doing the redirect (at which point the security boundary has already been crossed - the attacker has already got hold of the form data) or 2) has to trick the server into doing the redirect.  I'd argue that #2 is a higher bar, given that the server doing the redirect is trusted by being whitelisted via form-action CSP.

Additionally, if the attacker can trick the server into doing a redirect, then maybe the attacker can trick the server into forwarding/disclosing the data in some other way [i.e. without doing a redirect].  And please note that the form data will not be forwarded as part of the redirect as long as the redirect is a 301 or 302.  The server would have to do (or be tricked into doing) a 307 or 308 to forward the POST data along.


Enforcing form-action on redirects may lead to brittleness in some scenarios
============================================================================

In some scenarios example.com might post form data to another, trusted origin (e.g. forward some secrets/credentials to secure.provider.com).  In such scenarios the trusted origin controls the form data right from the first redirect hop.  Redirects should be seen as implementation details of the other origin - secure.provider.com should be free to introduce a redirect to backend.secure.provider.com without breaking example.com (regardless of what form-action csp example.com uses).

I am not sure how this interacts with Embedding-CSP.

And I am not saying that we shouldn't enforce CSP for directives other than form-action (because things like frame-src protect against threats other than data disclosure [like being able to script the CSP-protected-frame via window.parent]).
One more thoughts about redirects here - if we do care about the redirects for form-action enforcement, then we should not only verify the final redirect target, but also all the intermediate hops, right?
Looking at the CSP spec I think that it also says to only look at origin of the original requests for form-action enforcement (and to ignore redirects and/or final response origins):

- Most CSP directives ask for "Pre-request check" and "Post-request check" (e.g. img-src, child-src, connect-src, script-src, worker-src").  In particular, I note that frame-src check asks to execute "§6.6.1.4 Does response to request match source list?" (the *response* being the key word here IMO).

- form-action is the only directive that asks for "Pre-Navigation Check".  I note that the spec asks to execute "§6.6.1.3 Does request match source list?" (the *request* being the key word here IMO).
Re: #20: I could be convinced either way, curious what mkwst thinks. I found https://github.com/w3c/webappsec-csp/issues/8 which suggests that mkwst's intention so far has been to enforce form-action on redirects, but possibly considering relaxing it in CSP3. I do think that there's something to be said for consistency (all other directives applied on redirects so form-action should too).

Re #21: I would think it should apply for intermediate hops as well as the final destination (if it applies for the final destination at all).

Re #22: I agree with you that the spec as written doesn't enforce form-action on redirects, but that might just mean there's a spec bug that needs fixing. In fact I'm kind of confused as to how CSP3+Fetch handles redirects on intermediate hops in general. "Should internalResponse to request be blocked by Content Security Policy?" is invoked in step 16 of Main Fetch (https://fetch.spec.whatwg.org/#main-fetch), but if I'm reading correctly, at that point |response| is after already having followed redirects (in HTTP Fetch). And "Should request be blocked by Content Security Policy?" is invoked in step 6 of Main Fetch, which is called recursively on redirects, but "Should request be blocked by Content Security Policy?" only takes the request's URL (which IIUC is the initial url, not the current URL) into account. So I don't see anywhere that intermediate hops get checked against CSP, for any directive much less for form-action.

(Also, oof, I just realized there's a second bug in our implementation here, which is that we should stripping the paths when checking form actions on redirect, see  issue 452821 . I'll file a separate bug for that.)
re: #20, this probably isn't the right venue to decide what behavior we'd like the spec to mandate. Would you be interested in moving this conversation to the bug that Emily noted (and probably pinging some implementers from other browsers).

re: #21, We do care about intermediate hops.

re: #22, I think that CSP intends to enforce on redirects, but it's tough to parse out why that's the case because it hops back and forth between a few specifications (and it does look like there's a bug in HTML: 

1. HTML creates a navigational request in https://html.spec.whatwg.org/#process-a-navigate-fetch (step #2). Note that the redirect mode is "manual".

2. Step 8 of that algorithm asks CSP whether the navigation is allowed. If not, explode.

3. If allowed, Fetch does its work (step 8.1), grabs a response, and processes it. Note that because the redirect mode is "manual", Fetch hasn't followed any redirect at this point, so the response gathered could be a 302 or 307 or whatever.

4. Steps 9-12 read the response, and manually handle the redirect logic. There's a bug here, as step 9 says "run this step again", but should say something like "run step 8 again". Of course, saying that would be nuts, so we need to refactor things again.
(Filed https://github.com/w3c/webappsec-csp/issues/134 based on what I mentioned in comment 23, that I can't figure out how CSP gets applied to intermediate hops in a redirect chain for normal non-form-action directives)
Thank you for opening the webappsec-csp issue - I've tried to summarize my comments above into the other bug tracker.

I agree that it is difficult to decipher the desired treatment of redirects from the specs (and admit that I stopped at the CSP spec, without considering the Fetch spec :-/).  How feasible would it be to encode the desired behavior as a set of tests that can be run by various browser vendors (in theory this is what testharness.js is supposed to facilitate, right?)?  I am not sure how one would setup test environment for such tests (i.e. layout tests can instruct the http test server to do redirects, but I am not sure how this kind of test setup can be done in a way that works across all major browser vendors).
lukasza: I think web-platform-tests is exactly what you're looking for :) https://github.com/w3c/web-platform-tests
Project Member Comment 28 by bugdroid1@chromium.org, Nov 4 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4ac4aff49c4c539bce6d8a0d8800c01324bb6bc0

commit 4ac4aff49c4c539bce6d8a0d8800c01324bb6bc0
Author: lukasza <lukasza@chromium.org>
Date: Fri Nov 04 17:09:52 2016

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

BUG= 630332 

Review-Url: https://codereview.chromium.org/2464123004
Cr-Commit-Position: refs/heads/master@{#429922}

[add] https://crrev.com/4ac4aff49c4c539bce6d8a0d8800c01324bb6bc0/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-blocked-when-target-blank-expected.txt
[add] https://crrev.com/4ac4aff49c4c539bce6d8a0d8800c01324bb6bc0/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-blocked-when-target-blank.html
[add] https://crrev.com/4ac4aff49c4c539bce6d8a0d8800c01324bb6bc0/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-blocked-when-target-cross-site-window-expected.txt
[add] https://crrev.com/4ac4aff49c4c539bce6d8a0d8800c01324bb6bc0/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-blocked-when-target-cross-site-window.html
[modify] https://crrev.com/4ac4aff49c4c539bce6d8a0d8800c01324bb6bc0/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-blocked-expected.txt
[modify] https://crrev.com/4ac4aff49c4c539bce6d8a0d8800c01324bb6bc0/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-blocked.html
[modify] https://crrev.com/4ac4aff49c4c539bce6d8a0d8800c01324bb6bc0/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-blocked-expected.txt
[modify] https://crrev.com/4ac4aff49c4c539bce6d8a0d8800c01324bb6bc0/third_party/WebKit/Source/core/html/HTMLFormElement.cpp

estark@ has kindly pointed out that the discussion about form-action VS redirects is happening at https://github.com/w3c/webappsec-csp/issues/8
Status: Fixed
I think this issue can be marked as fixed.

I've opened issue 663512 to remember to follow-up once we have a decision in https://github.com/w3c/webappsec-csp/issues/8 wrt handling of redirects.  
estark@ - we probably should request a merge to M55 for r429922, right?  There is a risk that the fix starts blocking something important, but this should be fine given that there are still 3+ weeks until M55 is promoted to Stable.  WDYT?
Re #31: thanks for fixing this! I think it's worthwhile to request a merge to 55, but if the TPMs don't want to take it, that seems fine to me. (Low severity bug that's been around for a long time.)
Project Member Comment 33 by sheriffbot@chromium.org, Nov 9 2016
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: Merge-Request-55
r429922 was initially in 56.0.2910.0, so I think it had enough bake time on Canary.

Also - I think the risk of blocking something important is lower than I originally thought - if an Important Web Site uses form-action in a way that would be blocked/broken by r429922, then this web site should already be broken in Firefox (which already blocks in this scenario as pointed out in "comment 0" above).

So - I think this should be relatively safe to merge.
Comment 35 by dimu@chromium.org, Nov 9 2016
Labels: -Merge-Request-55 Merge-Approved-55 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M55 (branch: 2883)
Project Member Comment 36 by bugdroid1@chromium.org, Nov 9 2016
Labels: -merge-approved-55 merge-merged-2883
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/41e21987fa2803688abdc4700cc577a148ab4e9a

commit 41e21987fa2803688abdc4700cc577a148ab4e9a
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Wed Nov 09 23:39:44 2016

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

BUG= 630332 

Review-Url: https://codereview.chromium.org/2464123004
Cr-Commit-Position: refs/heads/master@{#429922}
(cherry picked from commit 4ac4aff49c4c539bce6d8a0d8800c01324bb6bc0)

Review URL: https://codereview.chromium.org/2487973004 .

Cr-Commit-Position: refs/branch-heads/2883@{#515}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[add] https://crrev.com/41e21987fa2803688abdc4700cc577a148ab4e9a/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-blocked-when-target-blank-expected.txt
[add] https://crrev.com/41e21987fa2803688abdc4700cc577a148ab4e9a/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-blocked-when-target-blank.html
[add] https://crrev.com/41e21987fa2803688abdc4700cc577a148ab4e9a/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-blocked-when-target-cross-site-window-expected.txt
[add] https://crrev.com/41e21987fa2803688abdc4700cc577a148ab4e9a/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-blocked-when-target-cross-site-window.html
[modify] https://crrev.com/41e21987fa2803688abdc4700cc577a148ab4e9a/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-blocked-expected.txt
[modify] https://crrev.com/41e21987fa2803688abdc4700cc577a148ab4e9a/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-blocked.html
[modify] https://crrev.com/41e21987fa2803688abdc4700cc577a148ab4e9a/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-blocked-expected.txt
[modify] https://crrev.com/41e21987fa2803688abdc4700cc577a148ab4e9a/third_party/WebKit/Source/core/html/HTMLFormElement.cpp

Labels: M-55
Labels: -Hotlist-Merge-Approved
Labels: Release-0-M55
Labels: CVE-2016-5225
Project Member Comment 41 by sheriffbot@chromium.org, Feb 15 2017
Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Sign in to add a comment