1287806 - X-Frame-Options header incorrect for attachments

Status: RESOLVED FIXED

(developer.mozilla.org :: Security, defect)

Description

[John Whitlock [:jwhitlock]]

What did you do?
================
See headers for an attached file, such as:

curl -I http://mdn.mozillademos.org/files/8445/internet-schema-3.png

What happened?
==============
The X-Frame-Options header value is:

X-Frame-Options: ALLOW-FROM: developer.mozilla.org


What should have happened?
==========================
It should not include the colon after ALLOW-FROM

X-Frame-Options: ALLOW-FROM developer.mozilla.org


Is there anything else we should know?
======================================
The header is added in the Kuma code, and will be a quick fix:

https://github.com/mozilla/kuma/blob/master/kuma/attachments/views.py#L52

The most likely outcome of the broken header is that browsers ignore it. Investigation is needed in staging to verify that the site does not require this broken functionality to work.

Comment 1

[April King [:April]]

I should note that this way of using the header is completely deprecated and only supported by Firefox.

If you want the equivalent functionality, you should instead use:

Content-Security-Policy: frame-ancestors https://developer.mozilla.org

This is supported by Chrome, Firefox, and Safari at least.  You can then either just leave out XFO, or you can set it to DENY or SAMEORIGIN, which would block mdn.mozillademos.org from being framed by developer.mozilla.org in IE/Edge, and would work properly in the others.  I would probably set CSP and then leave off XFO.

Comment 2

[John Whitlock [:jwhitlock]]

There is a PR submitted to fix this:

https://github.com/mozilla/kuma/pull/3922

I have not gotten started with CSP, so I have a learning hill to climb. Are there CSP validators I should lean on? Do we need to setup a reporting interface first? Can this even be checked locally, or do we need to get into a production environment? Any book or tutorial recomendations?

Comment 3

[April King [:April]]

What I will usually do is take a test system and set:

Content-Security-Policy: default-src 'none'

And with the dev tools open (Chrome is better than Firefox for this), refresh the page.  It will tell you every resource that was blocked by CSP.  Add those sources (img-src, script-src, etc.) until that no longer happens.

If you'd like, I'd be happy to build one for you.  Just point me at a domain and I'll get you at least most of the way there.  :)

Comment 4

[April King [:April]]

Note that you can also do what I do and that is use the production site and run your HTTP requests through a middlebox proxy that can add the headers.  That's a good deal more complicated for most folks though.  But it is an option!

Comment 5

[John Whitlock [:jwhitlock]]

I'm removing my mentorship because that puts it on a list of "good first bugs", and there's nothing about this that makes it a good first bug:

1) Our development environment isn't a close copy of the production environment, so it's not adequate to test this kind of change
2) Our staging environment also doesn't have a completely working demos / attachment system, so it's possible that a change won't be able to be fully tested there, meaning
3) The first real test of this change will be in production, and we're not 100% sure what would work or break with the change.
4) We don't have any CSP reporting endpoint or service setup, so we'd be relying on staff to notice or users to report a problem.

I haven't been able to verify if site currently relies on a broken X-Frame-Options value. One place where attachments are used is this page:

https://developer.mozilla.org/en-US/docs/Web/API/Web_Animations_API/Using_the_Web_Animations_API#The_CSS_version

The tumbling Alice demo is in an iframe with a source of:

https://mdn.mozillademos.org/en-US/docs/Web/API/Web_Animations_API/Using_the_Web_Animations_API$samples/Playable_code?revision=1102971

The iframe page includes CSS that loads background images:

https://mdn.mozillademos.org/files/13683/bg-tunnel-border-left.svg

Does this iframe header apply to the image in this case? Should the allowed host be developer.mozilla.org, or mdn.mozillademos.org? Or would it only count if the image was used directly, and not in a CSS property? I'm not sure what a good test of the header would be, to see if it still works before and after.

Additionally, we have "open in codepen" and "open in jsfiddle" links, to load the sample in those tools. If we do that, the demos should still work, and not be missing images.  The JSFiddle one works.  Does this work because the header is broken and ignored?  Or because it isn't being displayed by itself?  What are we trying to accomplish with the header in the first place?

These are all the things I'm not sure of, and why I don't want to involve a new contributor.

Comment 6

[Anthony Maton [:anthony]]

I did a new PR following the demand made in the previous PR.
This new PR doesn't add the CSP but fix the header syntax.

I'm wondering if it's worth opening a bug about the CSP in itself

Comment 7

[Anthony Maton [:anthony]]

Ok for information such bug already exist (sorry I didn't notice it)

https://bugzilla.mozilla.org/show_bug.cgi?id=948151

Comment 8

[April King [:April]]

:jwhitlock, basically anything an iframe loads from needs to have the proper XFO or CSP for it.  I believe that also includes subresources, such as when CSS includes an image.

Comment 9

[April King [:April]]

I might suggest that instead of using XFO, you instead just use CSP for the framing stuff.  The directive is frame-ancestors, so like this:

frame-ancestors https://developer.mozilla.org https://whatever.mozilla.org https://etc.mozilla.org

Browsers that understand frame-ancestors (most of them) will implement it, and you can have plenty of sources.  Browsers that don't simply leave things open like they are now with the broken header.

Comment 10

[MDN Team (:mdn-dev)]

Commits pushed to master at https://github.com/mozilla/kuma

https://github.com/mozilla/kuma/commit/e63d7bd843fe65fb0a5c6349dfe96643a649fe15
fix bug 1287806 : Syntax correction on X-Frame-Options header for attachments

https://github.com/mozilla/kuma/commit/eb14ae1342a05bb1b78a40e6dcd01462e52b7ac4
Merge pull request #3936 from MatonAnthony/fix-1287806-xframe-options

fix bug 1287806 : Syntax correction on X-Frame-Options header

Comment 11

[John Whitlock [:jwhitlock]]

The test page works with the fixed X-Frame-Options header:

https://mdn.mozillademos.org/en-US/docs/Web/API/Web_Animations_API/Using_the_Web_Animations_API$samples/Playable_code?revision=1102971

I'm leaving the bug open for the future CSP work, adding that as a blocker

Comment 12

[Peter Bengtsson [:peterbe]]

I'm going to boldly resolve this. https://github.com/mozilla/kuma/commit/eb14ae1342a05bb1b78a40e6dcd01462e52b7ac4
solved the "immediate" problem. 3 years ago.
It was during investigating this we discovered (Hi April!) that we should pivot and instead invest in CSP and if/when doing so we should do it all properly. That's what https://bugzilla.mozilla.org/show_bug.cgi?id=948151 is all about.

The day we tackle https://bugzilla.mozilla.org/show_bug.cgi?id=948151 we must make a mental note to check that iframes work correctly but it feels quite fundamental anyway so it just "should get done".

Also, to say that this blocks #948151 isn't right. Same can be said about https://bugzilla.mozilla.org/show_bug.cgi?id=1287621.
(at the moment, not sure what the difference is between these two bugs)
This bug (about attachments) would most likely automatically resolve itself once CSP is working properly. We'll get there.