Closed Bug 1251043 Opened 4 years ago Closed 4 years ago

Content Security Policy form-action directive is ignored

Categories

(Core :: DOM: Security, defect)

44 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: ilya.nesterov, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [domsecurity-active])

Attachments

(4 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.109 Safari/537.36

Steps to reproduce:

Load attached "form-action-src-blocked.html" in a browser and click "Submit" button.

You can also reproduce the issue if Content Security Policy delivered via content-security-policy http header. (You need to remove "<meta http-equiv="Content-Security-Policy" content="form-action 'none'"> from attached file, and adjust your http server settings to add "Content-Security-Policy: form-action 'none'" header to response)


Actual results:

Form successfully submitted


Expected results:

Form submit should be blocked. (Open the same file in Chrome)
Firefox 44.0.2 on OS X 10.11.3
Component: Untriaged → DOM: Security
Product: Firefox → Core
OS: Unspecified → All
Hardware: Unspecified → All
We need to investigate. I see a CSP error in the console but it seems the page still loads.
Blocks: csp-w3c-3
Status: UNCONFIRMED → NEW
Ever confirmed: true
Kate, any chance I can assign this one to you as well?
Flags: needinfo?(kmckinley)
Whiteboard: [domsecurity-backlog]
Assignee: nobody → kmckinley
Flags: needinfo?(kmckinley)
Whiteboard: [domsecurity-backlog] → [domsecurity-active]
I just stumbled across that piece of code which would explain the current behavior. Francois, are we missing a return statement here: http://hg.mozilla.org/mozilla-central/rev/5a9dc311e76f#l2.40

I suppose we should only perform
>  actionURL.forget(aActionURL);
if allowed by the CSP, right?

Most likely this will fix the failing wpt form-action tests as well, right? See: https://bugzilla.mozilla.org/show_bug.cgi?id=937025#c18
Flags: needinfo?(francois)
Ah I suppose upgrade-insecure-requests broke that:
http://hg.mozilla.org/mozilla-central/diff/6081d8a4054e/dom/html/HTMLFormElement.cpp#l1.49

That's why we should use early returns - rrgh!
Stealing that form you Kate!
Assignee: kmckinley → mozilla
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> I just stumbled across that piece of code which would explain the current
> behavior. Francois, are we missing a return statement here:
> http://hg.mozilla.org/mozilla-central/rev/5a9dc311e76f#l2.40
> 
> I suppose we should only perform
> >  actionURL.forget(aActionURL);
> if allowed by the CSP, right?

Yeah, it does look like we should have an early return in there.
Flags: needinfo?(francois)
So, after evaluating this bug I realized this is in fact a regression of Bug 1139297, we should fix and uplift.
Blocks: 1139297
Status: NEW → ASSIGNED
Keywords: regression
It's actually not that easy to write an automated test for this feature, because there is no error thrown if we block the form submission. If CSP blocks the load then we just log an error to the console. We test that we correctly log that error to the console within |test_form-action.html|.

Within this patch I am adding a test to make sure the form is actually not submitted.
Attachment #8734080 - Flags: review?(francois)
Attachment #8734078 - Flags: review?(francois) → review+
Comment on attachment 8734080 [details] [diff] [review]
bug_1251043_fix_csp_form_action_test_updates.patch

Review of attachment 8734080 [details] [diff] [review]:
-----------------------------------------------------------------

I wonder whether we should be throwing an error when the form submission fails for CSP reasons.

It would make the test simpler, but more importantly, it would give some feedback to webdevs that something went wrong in case they rely on that form submission to work.
Attachment #8734080 - Flags: review?(francois) → review+
(In reply to François Marier [:francois] from comment #11)
> It would make the test simpler, but more importantly, it would give some
> feedback to webdevs that something went wrong in case they rely on that form
> submission to work.

I agree, I'll consult people what they think about that. But let's not pollute this bug (since we need to uplift the patches within this bug).

Thanks!
https://hg.mozilla.org/mozilla-central/rev/fbc5d3de958a
https://hg.mozilla.org/mozilla-central/rev/d8705d309f1c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment on attachment 8734078 [details] [diff] [review]
bug_1251043_fix_csp_form_action.patch

Approval Request Comment
Regression introduced within FF42, but I suppose the form-action CSP directive is not widely used. I suggest we uplift to aurora and beta anyway. It's only a one line fix.

[Feature/regressing bug #]:
Bug 1139297 - Implement CSP upgrade-insecure-requests directive

[User impact if declined]:
forms can be submitted even though CSP might restrict submitting forms. Please note that a CSP error message would still be logged to the console, which might confuse people.

[Describe test coverage new/current, TreeHerder]:
Manual testing and automated mochitest attached to this bug.

[Risks and why]:
very low, it's only affecting sites that submit forms using the CSP directive 'form-action'.

[String/UUID change made/needed]:
no
Attachment #8734078 - Flags: approval-mozilla-beta?
Attachment #8734078 - Flags: approval-mozilla-aurora?
Comment on attachment 8734080 [details] [diff] [review]
bug_1251043_fix_csp_form_action_test_updates.patch

Approval Request Comment
See above comment.
Attachment #8734080 - Flags: approval-mozilla-beta?
Attachment #8734080 - Flags: approval-mozilla-aurora?
Comment on attachment 8734078 [details] [diff] [review]
bug_1251043_fix_csp_form_action.patch

Seems low risk, Aurora47+
Attachment #8734078 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8734080 [details] [diff] [review]
bug_1251043_fix_csp_form_action_test_updates.patch

Test patches don't need explicit approval, they are auto-approved.
Attachment #8734080 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
The test patch doesn't apply cleanly on aurora, can you rebase that patch?
Flags: needinfo?(mozilla)
Rebased tests for uplifting to aurora.
Flags: needinfo?(mozilla)
Attachment #8735696 - Flags: review+
If this were to be uplifted to beta, the rebased test patch for aurora does not apply cleanly to beta, so would need a newly rebased patch.
Christoph is out for a couple of weeks. Steve can you help us find someone to rebase the patch for beta?
Flags: needinfo?(sworkman)
Comment on attachment 8734078 [details] [diff] [review]
bug_1251043_fix_csp_form_action.patch

Too late for beta, not a recent regression. This will be fixed in 47.
Attachment #8734078 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8734080 [details] [diff] [review]
bug_1251043_fix_csp_form_action_test_updates.patch

Merge issues unresolved, too late for beta 46. This will ship with 47
Attachment #8734080 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Hi Liz - many apologies! I was partially out last week, and missed this. Christoph should be back this week, but it sounds like this has missed beta46 anyway. Sorry about that!
Flags: needinfo?(sworkman)
You need to log in before you can comment on or make changes to this bug.