Closed Bug 761667 (CVE-2013-1696) Opened 8 years ago Closed 7 years ago

Firefox ignores the X-Frame-Options header when using server push

Categories

(Core :: DOM: Core & HTML, defect, major)

13 Branch
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla23
Tracking Status
firefox21 --- wontfix
firefox22 + fixed
firefox23 + fixed
firefox-esr17 --- wontfix
b2g18 --- wontfix

People

(Reporter: LpSolit, Assigned: geekboy)

References

(Blocks 1 open bug)

Details

(Keywords: sec-moderate, Whiteboard: [adv-main22+])

Attachments

(2 files, 1 obsolete file)

Bugzilla uses the "X-Frame-Options: SAMEORIGIN" header to prevent clickjacking, but for some reason, Firefox ignores this header for buglists. A testcase is attached to bug 761046. This seems related to the fact that buglists use the server push technology, see e.g. http://search.cpan.org/~markstos/CGI.pm-3.59/lib/CGI.pm#Server_Push. Opera, Chrome, Safari and IE8+ correctly prevent buglists from being loaded in frames, but Firefox doesn't.

Tested with Fx 13, and dveditz reproduced with a nightly build, from what he told me on IRC.
Are you using the server push thing with all browsers?

Are you putting that header in the HTTP headers for the multipart response, or in the headers for the individual parts?
(In reply to Boris Zbarsky (:bz) from comment #1)
> Are you using the server push thing with all browsers?

Gecko-based browsers + Opera only (Opera since Bugzilla 4.3.1+ only). Server push is disabled for WebKit/Safari + IE, so I agree that I should have been clearer about that. :)


> Are you putting that header in the HTTP headers for the multipart response,
> or in the headers for the individual parts?

From what I can see when using the HttpFox extension, the header is set in both cases.
OK.  So we only look for the header on HTTP responses, but the document is being rendered from a part, not an HTTP response.  So we never actually see the header.

Sid, is the right behavior to look for the header on the part, on the multipart response, or in both places?
There isn't exactly a specification for x-frame-options, other than "do what IE does". But we're sending different content to IE in this case so we'll have to test.
There's a working draft of a spec here: http://tools.ietf.org/html/draft-gondrom-frame-options

It's not specified what do do in the case of multipart responses (I think grabbing it from a part should be legit), nor is it specified what to do in the case of multiple Frame-Options headers.

I think maybe if we encounter multiple headers (say if it occurs in both places) we should fail closed: DENY.  We should probably add code to grab the header from the part.

Dan: did you notice what IE does?

Brandon: do you have any thoughts?
Duplicate of this bug: 763346
Bug 763346 also points out that Gecko ignores X-FRAME-OPTIONS when Content-disposition is 'inline'.
(In reply to Sid Stamm [:geekboy] from comment #5)
> Dan: did you notice what IE does?

IE doesn't support server push, AFAIK, so I guess it does nothing in this specific case. :)
CCing Mario, who independently discovered this in bug 763346.

Gerv
Is there anything we can do server-side till this bug is fixed in Firefox, besides disabling server push?
Not that I know of, no.
Sid, can you own this, or find someone to own this? Seems the hard part here is deciding what we want to do, writing the patch should be fairly simple.
Assignee: nobody → sstamm
Too bad this isn't specified in the frame-options draft.  :(
http://tools.ietf.org/html/draft-ietf-websec-frame-options

I think it's a good idea for gecko to process the header for each part as they come down the pipe.  You're probably right, jst, that this will be easy -- as long as we do this already for other HTTP headers in the multipart/x-mixed-replace streams.

I could probably tackle this one.  Can someone attach an http stream transcript that illustrates this issue ... or a test case?  That'll make things go much more quickly.
Attached file testcase (obsolete) —
This testcase loads a buglist in an iframe. As Bugzilla passed the "X-Frame-Options: SAMEORIGIN" header, the buglist should not appear in the iframe. But it does with Firefox.
Attached patch fix (for aurora)Splinter Review
So I learned about multipart/x-mixed-replace and how we have part channels and made this fix, complete with two tests.  Lucky for me there was already something in docshell to do the hard parts.

jst: can you review since you reviewed my last x-f-o patch?

bz: you seem to be all over this bug too, so I'd like to know what you think too.
Attachment #711885 - Attachment is obsolete: true
Attachment #739899 - Flags: review?(jst)
Flags: needinfo?(bzbarsky)
Comment on attachment 739899 [details] [diff] [review]
fix (for aurora)

r=me, fwiw.  I'm fine with not having jst also review if you are.  I assume the test does fail without the code change?
Attachment #739899 - Flags: review+
Flags: needinfo?(bzbarsky)
Thanks for the review, bz!  Yeah, I verified that the test fails without the code change. Since this is sec-moderate, I'll land this soon.  I don't think it's necessary to uplift this fix, but flag me if we want it and I'll make the patches.
Attachment #739899 - Flags: review?(jst)
It's a small enough patch that it'd be nice to get this working correctly on Aurora.
Attached patch fixSplinter Review
Was bitrotted by bug 836132 (trivial merge).  This is an updated patch.
Attachment #739899 - Attachment is obsolete: true
Attachment #740539 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e5f8c65894da
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment on attachment 739899 [details] [diff] [review]
fix (for aurora)

While this was bitrotted by changes on mozilla-central, it still applies cleanly to the aurora branch.  This is the patch we should take into aurora if approved.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): problem in initial implementation of feature
User impact if declined: Users will not be protected from clickjacking on some sites (including bugzilla.mozilla.org)
Testing completed (on m-c, etc.): it's in m-c
Risk to taking this patch (and alternatives if risky): not very risky - minor change
String or IDL/UUID changes made by this patch: None
Attachment #739899 - Attachment description: fix → fix (for aurora)
Attachment #739899 - Attachment is obsolete: false
Attachment #739899 - Flags: approval-mozilla-aurora?
Attachment #739899 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [adv-main22+]
The testcase from bug 761046 no longer works, meaning that Fx 22 now correctly takes the header into account.
Status: RESOLVED → VERIFIED
Alias: CVE-2013-1696
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.