Closed Bug 475894 Opened 16 years ago Closed 14 years ago

Consider sending X-FRAME-OPTIONS: SAMEORIGIN for ClickJacking prevention

Categories

(Bugzilla :: Bugzilla-General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: mkanat, Assigned: reed)

References

()

Details

(Whiteboard: [wanted-bmo][infrasec:crossdomain][ws:moderate])

Attachments

(1 file, 3 obsolete files)

IE 8 is going to have a new feature that helps prevent clickjacking, which prevents a site from being rendered in a frame with various restrictions. It's an HTTP header, X-Frame-Options. We could set it to "SAMEORIGIN" and then only Bugzilla could frame itself. I'm not sure if this is a good idea or if it will become any sort of web standard, but I figured I could file this bug and we could discuss it. Details at http://blogs.msdn.com/ie/archive/2009/01/27/ie8-security-part-vii-clickjacking-defenses.aspx
See bug 475530 for discussion on implementation (or not) of the header into Gecko.
I'd like to discuss this issue a bit more. FF is going to support x-frame-options and IE8 already does. It would be a security win for bugzilla to prevent click-jacking attacks via x-frame-options. In fact, a clickjacking attack was demo'ed against bugzilla in BlackHat europe to bypass CSRF controls. Thoughts?
Well, I'm for it. If somebody wants to provide a patch, I'll be happy to review it. I don't think framing Bugzilla is an important use case, and people who really have to could always edit the code. I would like some confirmation that this is indeed a helpful protection, as some comments from dveditz in bug 475530 indicate that it might not be as helpful as it seems.
I looked through bug 475530. The main point of concern I saw was comment 13 (copied below) <comment> Interesting, according to Eric's comments SAMEORIGIN is compared only against the origin of the top-level document; it ought to be checked all the way up the frame tree. I suppose that requires more work for a rare case, but it seems kind of half-baked if this is really a security measure and not just a feel-good item no one is expected to actually use. </comment> I think this limitation is not a big issue. The most common attack scenario I see for click jacking is an unrelated site framing bugzilla. In this case the top-level origin will be the attacker's site and thus not match the framed bugzilla page. In this scenario x-frame-options would work to protect us. The only scenario where this top-level checking would be an issue is if the attacker could inject their own frame into bugzilla and then execute the clickjacking attack within his injected frame (e.g. the top level would still be bugzilla and thus no protection from x-frame-options). However, arbitrary iframe injection into bugzilla would be a bigger deal anyway and I wouldn't expect protection from x-frame-options.
Per http://michael-coates.blogspot.com/2010/08/x-frame-option-support-in-firefox.html, IE8, Fx 3.6.9, Opera 10.50 and Safari 4 support it.
Flags: blocking4.0?
Summary: Consider sending X-FRAME-OPTIONS: SAMEORIGIN for ClickJacking prevention in IE 8 → Consider sending X-FRAME-OPTIONS: SAMEORIGIN for ClickJacking prevention
Attached patch patch - v1 (obsolete) (deleted) — Splinter Review
Assignee: general → reed
Status: NEW → ASSIGNED
(In reply to comment #4) My concerns about SAMEORIGIN in bug 475530 comment 13 shouldn't be taken as discouragement from using it -- but beware its limitations and don't count on it for too much. > I think this limitation is not a big issue. The most common attack scenario I > see for click jacking is an unrelated site framing bugzilla. [...] > In this scenario x-frame-options would work to protect us. It will certainly help in this most-common case. > The only scenario where this top-level checking would be an issue is if the > attacker could inject their own frame into bugzilla and then execute the > clickjacking attack within his injected frame (e.g. the top level would still > be bugzilla and thus no protection from x-frame-options). However, arbitrary > iframe injection into bugzilla would be a bigger deal anyway and I wouldn't > expect protection from x-frame-options. Generic bugzilla may not have this issue but BMO is _full_ of arbitrary frames--in testcases. In BMO the separate attachment hosts saves us, but that's a relatively recent feature. I think it's great bugzilla is planning on using this feature, my comments were simply that people need to carefully consider their entire site before relying on the SAMEORIGIN feature. Incidentally, if the separate attachment host feature is turned on then attachments must not use the x-frame-options feature. Attachment content is displayed in a non-same-origin frame on the attachment details page. In addition some testcases may also have one attachment frame another, possibly from another bug in rare cases. Perhaps, though, we don't care to cater to those edge cases.
I'll take this on 4.0 if we get it in before RC1, but I won't block 4.0 on it.
Flags: blocking4.0? → blocking4.0-
Target Milestone: --- → Bugzilla 4.0
Whiteboard: [wanted-bmo][sg:want]
Whiteboard: [wanted-bmo][sg:want] → [wanted-bmo][sg:want][ws:moderate]
Whiteboard: [wanted-bmo][sg:want][ws:moderate] → [wanted-bmo][infrasec:crossdomain][ws:moderate]
Attached patch patch - v2 (obsolete) (deleted) — Splinter Review
Ok, add the X-FRAME-OPTIONS header unless attachbase is on and the current page is attachment.cgi. Possible issue: This does NOT send the header if attachbase is on and if you go directly to the attachbase URL. However, if attachbase is off, the header will still be sent for attachment.cgi. Should the header be sent on the attachbase itself, or do we want to permit attachments to go outside the current origin?
Attachment #472998 - Attachment is obsolete: true
Attachment #498076 - Flags: review?(mkanat)
Attached patch patch - v3 (obsolete) (deleted) — Splinter Review
I think this is right, though I would like an answer to my last comment, as it's too late for me to be thinking correctly on how this should be...
Attachment #498076 - Attachment is obsolete: true
Attachment #498077 - Flags: review?(mkanat)
Attachment #498076 - Flags: review?(mkanat)
you must NOT send the header for attachbase.bugzilla.domain because then it can't be hosted in the attachment detail view page frame. without attachbase I think you can safely just send the header all the time, attachment content or not.
Attached patch patch - v4 (deleted) — Splinter Review
Send the header at all times UNLESS the current URL is part of the attachment base.
Attachment #498077 - Attachment is obsolete: true
Attachment #498464 - Flags: review?(mkanat)
Attachment #498077 - Flags: review?(mkanat)
Keywords: relnote
Comment on attachment 498464 [details] [diff] [review] patch - v4 Sure, this looks great. Thanks! :-)
Attachment #498464 - Flags: review?(mkanat) → review+
How far back does url_is_attachmentbase exist? If it exists on 3.6, we could backport to 3.6 too.
Flags: approval4.0+
Flags: approval+
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/ modified Bugzilla/CGI.pm Committed revision 7628. Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.0/ modified Bugzilla/CGI.pm Committed revision 7498.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
(In reply to comment #14) > How far back does url_is_attachmentbase exist? If it exists on 3.6, we could > backport to 3.6 too. It exists on 3.6, at least. If we were to backport this, there are a few other things (such as bug 453425 and possibly bug 562475) we should backport as well.
Added to relnotes in bug 713346.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: