Last Comment Bug 761667 - (CVE-2013-1696) Firefox ignores the X-Frame-Options header when using server push
(CVE-2013-1696)
: Firefox ignores the X-Frame-Options header when using server push
Status: VERIFIED FIXED
[adv-main22+]
: sec-moderate
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 13 Branch
: All All
: -- major (vote)
: mozilla23
Assigned To: Sid Stamm [:geekboy or :sstamm]
:
Mentors:
: 763346 (view as bug list)
Depends on:
Blocks: 761043
  Show dependency treegraph
 
Reported: 2012-06-05 09:34 PDT by Frédéric Buclin
Modified: 2014-01-21 13:26 PST (History)
14 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
fixed
wontfix
wontfix


Attachments
testcase (470 bytes, text/html)
2013-02-08 10:07 PST, Frédéric Buclin
no flags Details
fix (for aurora) (10.00 KB, patch)
2013-04-19 17:39 PDT, Sid Stamm [:geekboy or :sstamm]
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
fix (10.37 KB, patch)
2013-04-22 16:03 PDT, Sid Stamm [:geekboy or :sstamm]
mozbugs: review+
Details | Diff | Splinter Review

Description Frédéric Buclin 2012-06-05 09:34:45 PDT
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.
Comment 1 Boris Zbarsky [:bz] (TPAC) 2012-06-05 09:42:28 PDT
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?
Comment 2 Frédéric Buclin 2012-06-05 10:00:35 PDT
(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.
Comment 3 Boris Zbarsky [:bz] (TPAC) 2012-06-05 10:05:42 PDT
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?
Comment 4 Daniel Veditz [:dveditz] 2012-06-05 17:44:11 PDT
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.
Comment 5 Sid Stamm [:geekboy or :sstamm] 2012-06-05 18:06:20 PDT
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?
Comment 6 Frédéric Buclin 2012-06-10 13:49:02 PDT
*** Bug 763346 has been marked as a duplicate of this bug. ***
Comment 7 Reed Loden [:reed] (use needinfo?) 2012-06-10 13:54:21 PDT
Bug 763346 also points out that Gecko ignores X-FRAME-OPTIONS when Content-disposition is 'inline'.
Comment 8 Frédéric Buclin 2012-06-10 13:58:06 PDT
(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. :)
Comment 9 Gervase Markham [:gerv] 2012-06-11 03:05:41 PDT
CCing Mario, who independently discovered this in bug 763346.

Gerv
Comment 10 Frédéric Buclin 2012-09-17 07:14:24 PDT
Is there anything we can do server-side till this bug is fixed in Firefox, besides disabling server push?
Comment 11 Boris Zbarsky [:bz] (TPAC) 2012-09-18 21:12:42 PDT
Not that I know of, no.
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2013-02-08 03:16:05 PST
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.
Comment 13 Sid Stamm [:geekboy or :sstamm] 2013-02-08 09:50:37 PST
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.
Comment 14 Frédéric Buclin 2013-02-08 10:07:30 PST
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.
Comment 15 Sid Stamm [:geekboy or :sstamm] 2013-04-19 17:39:33 PDT
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.
Comment 16 Boris Zbarsky [:bz] (TPAC) 2013-04-19 20:21:28 PDT
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?
Comment 17 Sid Stamm [:geekboy or :sstamm] 2013-04-22 15:27:20 PDT
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.
Comment 18 Daniel Veditz [:dveditz] 2013-04-22 15:42:41 PDT
It's a small enough patch that it'd be nice to get this working correctly on Aurora.
Comment 19 Sid Stamm [:geekboy or :sstamm] 2013-04-22 16:03:49 PDT
Created attachment 740539 [details] [diff] [review]
fix

Was bitrotted by bug 836132 (trivial merge).  This is an updated patch.
Comment 20 Sid Stamm [:geekboy or :sstamm] 2013-04-23 10:32:41 PDT
inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/e5f8c65894da
Comment 21 Ryan VanderMeulen [:RyanVM] 2013-04-23 18:50:24 PDT
https://hg.mozilla.org/mozilla-central/rev/e5f8c65894da
Comment 22 Sid Stamm [:geekboy or :sstamm] 2013-04-24 10:00:55 PDT
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
Comment 23 Ryan VanderMeulen [:RyanVM] 2013-04-24 14:28:49 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/27d47826a8e8
Comment 24 Frédéric Buclin 2013-06-20 16:27:46 PDT
The testcase from bug 761046 no longer works, meaning that Fx 22 now correctly takes the header into account.

Note You need to log in before you can comment on or make changes to this bug.