761667 - (CVE-2013-1696) Firefox ignores the X-Frame-Options header when using server push

Status: VERIFIED FIXED

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

Description

[Frédéric Buclin]

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 [:bzbarsky]]

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]

(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 [:bzbarsky]]

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]]

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]]

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 7

[Reed Loden [:reed]]

Bug 763346 also points out that Gecko ignores X-FRAME-OPTIONS when Content-disposition is 'inline'.

Comment 8

[Frédéric Buclin]

(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]]

CCing Mario, who independently discovered this in bug 763346.

Gerv

Comment 10

[Frédéric Buclin]

Is there anything we can do server-side till this bug is fixed in Firefox, besides disabling server push?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Not that I know of, no.

Comment 12

[Johnny Stenback (:jst)]

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]]

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]

Attachment: 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]]

Attachment: 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 [: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?

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

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]]

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]]

Attachment: fix

Was bitrotted by bug 836132 (trivial merge).  This is an updated patch.

Comment 20

[Sid Stamm [:geekboy or :sstamm]]

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/e5f8c65894da

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e5f8c65894da

Comment 22

[Sid Stamm [:geekboy or :sstamm]]

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]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/27d47826a8e8

Comment 24

[Frédéric Buclin]

The testcase from bug 761046 no longer works, meaning that Fx 22 now correctly takes the header into account.