Bug 761667 (CVE-2013-1696)

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

VERIFIED FIXED in Firefox 22

Status

()

Core
DOM
--
major
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: Frédéric Buclin, Assigned: geekboy)

Tracking

(Blocks: 1 bug, {sec-moderate})

13 Branch
mozilla23
sec-moderate
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox21 wontfix, firefox22+ fixed, firefox23+ fixed, firefox-esr17 wontfix, b2g18 wontfix)

Details

(Whiteboard: [adv-main22+])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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?
(Reporter)

Comment 2

5 years ago
(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.
(Assignee)

Comment 5

5 years ago
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?
Keywords: sec-moderate
(Reporter)

Updated

5 years ago
Duplicate of this bug: 763346
Bug 763346 also points out that Gecko ignores X-FRAME-OPTIONS when Content-disposition is 'inline'.
(Reporter)

Comment 8

5 years ago
(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
Blocks: 761043
(Reporter)

Comment 10

5 years ago
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
(Assignee)

Comment 13

4 years ago
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.
(Reporter)

Comment 14

4 years ago
Created attachment 711885 [details]
testcase

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.
(Assignee)

Comment 15

4 years ago
Created attachment 739899 [details] [diff] [review]
fix (for aurora)

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)
(Assignee)

Comment 17

4 years ago
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.
(Assignee)

Updated

4 years ago
Attachment #739899 - Flags: review?(jst)
It's a small enough patch that it'd be nice to get this working correctly on Aurora.
status-b2g18: --- → wontfix
status-firefox21: --- → wontfix
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox-esr17: --- → wontfix
tracking-firefox22: --- → ?
tracking-firefox23: --- → +
(Assignee)

Comment 19

4 years ago
Created attachment 740539 [details] [diff] [review]
fix

Was bitrotted by bug 836132 (trivial merge).  This is an updated patch.
Attachment #739899 - Attachment is obsolete: true
Attachment #740539 - Flags: review+
(Assignee)

Comment 20

4 years ago
inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/e5f8c65894da
Target Milestone: --- → mozilla23
https://hg.mozilla.org/mozilla-central/rev/e5f8c65894da
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox23: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Comment 22

4 years ago
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?

Updated

4 years ago
tracking-firefox22: ? → +

Updated

4 years ago
Attachment #739899 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/27d47826a8e8
status-firefox22: affected → fixed
Whiteboard: [adv-main22+]
(Reporter)

Comment 24

4 years ago
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
You need to log in before you can comment on or make changes to this bug.