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?
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.
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.
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.
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 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?
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.
It's a small enough patch that it'd be nice to get this working correctly on Aurora.
Created attachment 740539 [details] [diff] [review] fix Was bitrotted by bug 836132 (trivial merge). This is an updated patch.
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
The testcase from bug 761046 no longer works, meaning that Fx 22 now correctly takes the header into account.