Closed Bug 1024557 Opened 5 years ago Closed 2 years ago

[CSP] ignore (x-)frame-options if CSP with frame-ancestors directive exists

Categories

(Core :: DOM: Security, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: geekboy, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 files, 5 obsolete files)

When x-frame-options or the frame-options header exists in parallel with a frame-ancestors directive in CSP, the CSP policy should be used and the frame-options policy should be disregarded (probably with a warning in the developer console).
Whiteboard: [domsecurity-backlog]
Priority: -- → P2
Priority: P2 → P3
Whiteboard: [domsecurity-backlog] → [domsecurity-backlog2]
Details to show that the standard is to use CSP as first priority: https://www.w3.org/TR/CSP11/#frame-ancestors-and-frame-options
Severity: enhancement → normal
Duplicate of this bug: 1343934
If this bug is fixed then there is no need to set (x-)frame-options header.So try to fix this
I didn't realize this was actually the case in Firefox, meaning a lot of my advice that I've been giving people (set DENY + frame-ancestors https://example.com) is wrong.  :\
This was a shock to discover. Thanks, Reed. It would be stellar if we could prioritize this.
Clearing "backlog" tag to trigger re-triage. I just assumed we had implemented this per-spec correctly. Since we're messing with frame-ancestors in bug 1364859 and bug 1367531 this may be an opportune time to re-work this.

If take another shot at addressing bug 725490 (as Chrome is now trying) maybe the best bet is to just treat an XFO header as a single-purpose CSP header, tagged so we can drop it if we find a "real" frame-ancestors policy.
Blocks: csp-w3c-2
No longer depends on: CSP
Whiteboard: [domsecurity-backlog2]
Assignee: nobody → ckerschb
Priority: P3 → P2
Whiteboard: [domsecurity-active]
Status: NEW → ASSIGNED
Hey Smaug, currently x-frame-options is evaluated before CSP, hence we can't simply ignore it if frame-ancestors is present in our current architecture. Is there any particular reason for that? I think what we should do is the following:

a) Move the xfo implementation out of nsDSURIContentListener.cpp and into dom/security/xfo.cpp. Because I think XFO belongs into the responsibilities of dom/security anyway. Or is there any reason not to move it out of nsDSURIContentListener.cpp?

b) Apply CSP within nsDocument and right after check XFO (and ignore it if frame-ancestors is present). [see attached dummy but working patch]

c) Does that all sounds good, then I will go ahead and get that ready?
Attachment #8873528 - Flags: feedback?(bugs)
Attached patch bug_1024557_xfo_csp_test.patch (obsolete) — Splinter Review
I already have an automated testcase as well. Needs some fine tuning, but almost there.
(Hi from websec concerns!) That test case looks good, and I asked :April to look it over and she says it's good as well.
I'll need to read quite some code here. I haven't reviewed frame-options stuff, I think. You may want ask bz.
Comment on attachment 8873528 [details] [diff] [review]
bug_1024557_ignore_x-frame-options.patch

But if he doesn't have time, I could take a look. I'd just need to read the relevant specs and our implementation etc.
Attachment #8873528 - Flags: feedback?(bugs)
Smaug, it seems Boris is not accepting review requests at the moment. Any chance you could take a look? Please note that I am not actually changing any of the XFO semantics itself.

Here are notes about the patch:

At the moment x-frame-options is enforced within nsDSURIContentListener.cpp and happens before CSP gets parsed and applied. In fact however, x-frame-options should be ignored if the CSP directive frame-ancestors is present. I am not entirely sure what other responsibilities nsDSURIContentListener.cpp has, but to me it makes sense to take the XFO code out of nsDSURIContentListener.cpp and move it over to dom/security. Ultimately, I think xfo and frame-ancestors should use the same implemementation, but that requires a different bug. If you prefer that the XFO code remains in nsDSURIContentListener.cpp then I am happy to move it back.

A few notes:
* Currently CheckFrameOptions is consulted within DoContent. Within this patch I am moving the entry point over to nsDocument, I suppose there is not much downside to that besides that more code gets executed before we actually cancel the channel, but we already facing that same problem for frame-ancestors anyway.
* I am only moving the three functions (CheckFrameOptions, CheckOneFrameOptionsPolicy, ReportXFOViolation) over to FramingChecker.cpp without any modifications to them besides that we are now passing an explicit 'aDocShell' argument. The only new code is ShouldIgnoreFrameOptions() which causes CheckFrameOptions to bail early if the CSP directive frame-ancestors is present.
* The XFO code itself could use a little love, e.g. we should not use localication instead of hardcoded messages when reporting to the console. I'll file a follow up bug to polish that up a little.
* In order to use the same code I had to make FramingChecker a friend class within nsDocShell, otherwise we can not access GetHttpChannel.
* CheckFrameOptions in nsDocument.h as well as x-frame-options within const headers[] seem to relicts from the old days when x-frame-options still lived in nsDocument.h, removing those.
* Can we reuse NS_ERROR_CSP_FRAME_ANCESTOR_VIOLATION as the error when cancelling the channel for xfo errors itself or do we have to define a new one? I think reuse is fine because then we can also reuse error components like [1]. But I am also not entirely sure hence I wanted to check.

[1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#4979
Attachment #8873528 - Attachment is obsolete: true
Attachment #8873770 - Flags: review?(bugs)
Attachment #8873529 - Attachment is obsolete: true
Attachment #8873771 - Flags: review?(bugs)
If you still prefer Boris to look at it, that's fine too. It's not that critical to land that change, it could wait a little before it gets reviewed.
Comment on attachment 8873771 [details] [diff] [review]
bug_1024557_csp_ignore_xfo_test.patch

I think we need a test for Content-Security-Policy-Report-Only too.
(that x-frame-options isn't ignored if Content-Security-Policy-Report-Only is used)
Attachment #8873771 - Flags: review?(bugs) → review-
FWIW, in general moving tons of code and also changing behavior makes reviewing a lot harder.
Code moves should happen separately.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)
> * Currently CheckFrameOptions is consulted within DoContent. Within this
> patch I am moving the entry point over to nsDocument, I suppose there is not
> much downside to that besides that more code gets executed before we
> actually cancel the channel, but we already facing that same problem for
> frame-ancestors anyway.


Why you're moving the check?
Comment on attachment 8873770 [details] [diff] [review]
bug_1024557_ignore_x-frame-options.patch

I don't understand what this all code move has to do with this bug.
Can we please keep changes simple and do clean ups / code moves separately.

Or do I misunderstand something here?
Attachment #8873770 - Flags: review?(bugs) → review-
Blocks: 1370788
Olli, sorry for making changes as well as moving code around in the same patch. I totally agree that we should have two separate bugs:

a) Within this bug I only make code changes now. The only thing I do within this patch is to expose the functionality of CheckFrameOptions() so we can call within nsDocument. We need to move the check there because CSP initialization needs to happen before performing the XFO check, so we can ignore it if frame-ancestors is present in the CSP.

b) I filed Bug 1370788 to move XFO out of nsDSURIContentListener.cpp and into dom/security. That's where XFO belongs in my opinion. Further we should use the same implementation for XFO and frame-ancestors in the end, but that's yet a different bug.
Attachment #8873770 - Attachment is obsolete: true
Attachment #8875196 - Flags: review?(bugs)
Olli, as requested I added a test for CSP-report-only to make sure XFO is not ignored if the CSP is shipped in report only mode.
Attachment #8873771 - Attachment is obsolete: true
Attachment #8875197 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #20)
> Why you're moving the check?

The reason we have to move the check is because CSP initialization needs to happen before we are evaluating XFO. At the moment XFO is evaluated within DoContent which happens before InitCSP(), hence we have to move the check.
Attachment #8875197 - Flags: review?(bugs) → review+
Comment on attachment 8875196 [details] [diff] [review]
bug_1024557_ignore_x-frame-options.patch

>@@ -8391,17 +8401,16 @@ nsDocument::RetrieveRelevantHeaders(nsIC
> 
>     static const char *const headers[] = {
>       "default-style",
>       "content-style-type",
>       "content-language",
>       "content-disposition",
>       "refresh",
>       "x-dns-prefetch-control",
>-      "x-frame-options",
>       "referrer-policy",
>       // add more http headers if you need
>       // XXXbz don't add content-location support without reading bug
>       // 238654 and its dependencies/dups first.
>       0
>     };
> 
This change seems to be unrelated. Perhaps drop it

> # %1$S is the value of the deprecated Referrer Directive.
> deprecatedReferrerDirective = Referrer Directive ‘%1$S’ has been deprecated. Please use the Referrer-Policy header instead.
>+# LOCALIZATION NOTE (IgnoringSrcBecauseOfDirective):
>+# %1$S is the name of the src that is ignored.
>+# %2$S is the name of the directive that causes the src to be ignored.
>+IgnoringSrcBecauseOfDirective=Ignoring “%1$S” because of “%2$S” directive.
There are some odd characters here, or at least bugzilla shows them such
Please ensure this is right


> NS_IMETHODIMP
>+nsCSPContext::GetEnforcesFrameAncestors(bool *outEnforcesFrameAncestors)
yikes, this file has odd coding style. No need to change.

>+{
>+  *outEnforcesFrameAncestors = false;
>+  for (uint32_t i = 0; i < mPolicies.Length(); i++) {
>+     if (!mPolicies[i]->getReportOnlyFlag() &&
extra space before if
Attachment #8875196 - Flags: review?(bugs) → review+
Thanks smaug, addressed your nits. Carrying over r+
Attachment #8875196 - Attachment is obsolete: true
Attachment #8875402 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68517278b72e
Ignore x-frame-options if CSP with frame-ancestors exists. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/38e06c672764
Test XFO is ignored when frame-ancestors is present. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/68517278b72e
https://hg.mozilla.org/mozilla-central/rev/38e06c672764
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.