Closed Bug 475530 Opened 16 years ago Closed 14 years ago

X-FRAME-OPTIONS header against "UI Redressing" AKA Clickjacking

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+
status2.0 --- wanted
status1.9.2 --- .9-fixed
status1.9.1 --- wontfix

People

(Reporter: ma1, Assigned: bsterne)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [parity-IE] [sg:want P2])

Attachments

(4 files, 2 obsolete files)

Microsoft has released IE8 RC1 yesterday, claiming to ship some "clickjacking protection". The few technical details available suggest that they implemented something like the "X-I-Do-Not-Want-To-Be-Framed-Across-Domains: yes" header suggested in http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2008-September/016284.html If this is really what they've done (documentation pending), doing the same (even UI-less) would be relatively trivial and probably desirable as a MSIE-parity enhancement.
Flags: wanted1.9.1?
I just received confirmation from a Microsoft source that my guesses were right. According to this source it's actually the "X-I-Do-Not-Want-To-Be-Framed-Across-Domains" header (funny name aside). Adding it to NoScript is trivial (as soon as we know the header name for sure) by hooking the already existent scriptless frame-breaking emulation. Can it be implemented in core as well in a reasonable time frame?
Blocks: clickjacking
Eric Lawrence's blog post explaining the technical details is out: http://blogs.msdn.com/ie/archive/2009/01/27/ie8-security-part-vii-clickjacking-defenses.aspx The only information obviously missing from my analysis was the name/value pair they choose for the triggering header, X-FRAME-OPTIONS: DENY I morphed the title to reflect this.
Summary: Cross-site subdocument restrictions against "UI Redressing" AKA Clickjacking → X-FRAME-OPTIONS: DENY against "UI Redressing" AKA Clickjacking
To be precise, they propose two values for the header: X-FRAME-OPTIONS: DENY prevents framing always. X-FRAME-OPTIONS: SAMEORIGIN prevents cross-site framing. Re-morphed the title accordingly.
Summary: X-FRAME-OPTIONS: DENY against "UI Redressing" AKA Clickjacking → X-FRAME-OPTIONS header against "UI Redressing" AKA Clickjacking
I wonder why X-FRAME-OPTIONS is plural. Even http headers like Accept-Language which can take several values are singular. Are there any additions planed?
Whiteboard: [parity-IE]
Whiteboard: [parity-IE] → [parity-IE] [sg:want?]
Instead of a very specific header, how about adding a "parent-src" option to Content Security Policy? http://people.mozilla.org/~bsterne/content-security-policy/details.html bsterne? (How much of CSP is in FF3.1 anyway? Any?) If we can persuade MS that CSP is worth doing (why, oh why don't they talk to us _before_ making these announcements?) then perhaps they could change to a compatible and extensible header scheme. Gerv
I can take a crack at this.
Assignee: nobody → dveditz
Whiteboard: [parity-IE] [sg:want?] → [parity-IE] [sg:want P2]
Note that DENY and SAMEORIGIN are effectively the same. Using DENY rather than SAMEORIGIN implies you don't trust your own site, but if there is untrusted code on your origin that code could extract the frame content (using XHR or something) and incorporate it anyway. DENY is more of a hint to your site designer, but obviously if we're going for parity we should do both.
I guess if the server was looking for the DENY page's URI in a referrer it might do some good.
<sigh> I commented on the IE post suggesting they consult, and do something more generic, and got this reply from EricLaw (kudos to him for actually replying): "@Gerv: Yes, making this a part of Content Security Policy and the like was considered, a point I alluded to in the conclusion. The reality is that adding CSP-like mechanisms is out of scope for IE8, and this is an issue that we wanted to address in the short-term. As noted in the post, we did have conversations with other browser venders in December about what we were doing and why. As for the suggestion that the "Don't frame me" header is somehow a unique Microsoft invention-- that's not the case, as demonstrated by links in the post." Does anyone know who the Mozilla contact was for these "conversations in December" he mentions? Gerv
Gerv: due to some black hole between redmond and mountain view we missed the phone meeting details, but we have been in contact (mail sent to the mozilla.org security address).
dveditz: Eric has explained the difference between DENY and OVERLAY: http://blogs.msdn.com/ie/archive/2009/01/27/ie8-security-part-vii-clickjacking-defenses.aspx#9382230 Gerv
(In reply to comment #5) > bsterne? (How much of CSP is in FF3.1 anyway? Any?) None, unfortunately :-(
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.
Dan, walking all the frames up to top is exactly what ClearClick does to decide if a framing setup is hostile or not. On the other hand, Eric's answer to this objection is that SAMEORIGIN is only for sites which are ensured not to include arbitrary subframes. In most cases, DENY is what you want to secure a page, e.g. http://www.google.com/accounts: http://www.google.com/imgres?imgurl=:&imgrefurl=http://www.google.com/accounts We could add ALLSAMEORIGIN or something like that for expressing this additional semantic, or break compatibility, or agree with them before stable releases.
BTW, I've just released the experimental X-FRAME-OPTIONS compatibility support I promised in NoScript 1.8.9.9, UI included: http://noscript.net/getit#devel Demo here: http://evil.hackademix.net/frameopts/
Here is the WebKit bug on this topic: https://bugs.webkit.org/show_bug.cgi?id=23907 I don't think we should extend the semantics (e.g., ALLSAMEORIGIN) without going through a standards process like WHATWG or HTML5 WG. The worst outcome is for each browser vendor to implement this feature in mutually incompatible ways.
as x-frame-options seems to be supported by not only by IE8, but also by safari4 and chrome3; any news on including this in FF 3.6 or 3.7?
I am a Web developer and Firefox user and would very much like to see this functionality added. ECMAScript/JavaScript is not a foolproof way for developers to address this problem.
Assignee: dveditz → bsterne
Attached patch fix v1 (obsolete) (deleted) — Splinter Review
First swag at a patch. I'll add some tests in the next day or two.
Attachment #432960 - Flags: review?(jst)
Comment on attachment 432960 [details] [diff] [review] fix v1 - In nsDocument::StartDocumentLoad(): + nsresult rv = CheckFrameOptions(); + NS_ENSURE_SUCCESS(rv, rv); CheckFrameOptions() never returns anything other than NS_OK. We should either make it a void method, or make it actually return an error in the case where it cancels the stream. The latter would probably be the safer option, we'd stop the load in its tracks right here rather than continuing to set up the document even though the channel has been canceled. - In nsDocument::CheckFrameOptions(): + if (thisWindow == topWindow) + return NS_OK; + + // If the value of the header is DENY, then the document + // should never be permitted to load as a subdocument. + else if (xfoHeaderValue.LowerCaseEqualsLiteral("deny")) Remove the else-after-return. + framingAllowed = false; + + // If the X-Frame-Options value is SAMEORIGIN, then the top frame in the + // parent chain must be from the same origin as this document. + else if (xfoHeaderValue.LowerCaseEqualsLiteral("sameorigin")) { Given that there's room to insert code in between these else statements I'd recommend you brace these if checks even if they're single line if checks, move the comments inside the following if, and that way there's much less risk of someone later on stumbling in here and adding code in between the else statements. + topWindow->GetDocument(getter_AddRefs(topDOMDoc)); + if (topDOMDoc) { + nsCOMPtr<nsIDocument> topDoc = do_QueryInterface(topDOMDoc); It's perfectly fine to pass null to do_QueryInterface(), so you can remove that if (topDOMDoc) check. Otherwise this looks great! I'd like to glance over an updated patch tho, so r- until then.
Attachment #432960 - Flags: review?(jst) → review-
Attached patch fix v2 (obsolete) (deleted) — Splinter Review
Addresses jst's review comments, adds tests, removes "try again" button from the error page.
Attachment #432960 - Attachment is obsolete: true
Attachment #436568 - Flags: ui-review?(johnath)
Attachment #436568 - Flags: review?(jst)
Comment on attachment 436568 [details] [diff] [review] fix v2 - In nsDocument::CheckFrameOptions(): + if (!framingAllowed) { + // cancel the load and show an error page + mChannel->Cancel(NS_ERROR_X_FRAME_OPTIONS_VIOLATION); + return NS_ERROR_CONTENT_BLOCKED; Seems we'd be better of if we return the same error that we pass to Cancel() here. Looks good with that changed. r=jst
Attachment #436568 - Flags: review?(jst) → review+
Comment on attachment 436568 [details] [diff] [review] fix v2 >diff --git a/browser/locales/en-US/chrome/overrides/appstrings.properties b/browser/locales/en-US/chrome/overrides/appstrings.properties >--- a/browser/locales/en-US/chrome/overrides/appstrings.properties >+++ b/browser/locales/en-US/chrome/overrides/appstrings.properties > cspFrameAncestorBlocked=This page has a content security policy that prevents it from being embedded in this way. >+xFrameOptionsBlocked=This page cannot be displayed in a frame due to the policy of the publisher. Given the similarity of the x frame options header and csp's frame ancestor rules, I'd expect the language to be much closer, here. e.g. This page has a frame policy that prevents it from being embedded in this way. > <!ENTITY cspFrameAncestorBlocked.title "Blocked by Content Security Policy"> > <!ENTITY cspFrameAncestorBlocked.longDesc "<p>&brandShortName; prevented this page from loading in this way because the page has a content security policy that disallows it.</p>"> > >+<!ENTITY xFrameOptionsBlocked.title "Blocked by Frame Options"> >+<!ENTITY xFrameOptionsBlocked.longDesc " >+<p>In order to view this content you must visit the page directly.</p> >+"> Ditto for these strings. (And implicitly ditto for the other copies of both sets of strings).
Attached patch fix v3 (deleted) — Splinter Review
Carried forward jst's r+. Addresses johnath's ui-review comments.
Attachment #436568 - Attachment is obsolete: true
Attachment #439318 - Flags: ui-review?(johnath)
Attachment #439318 - Flags: review+
Attachment #436568 - Flags: ui-review?(johnath)
Attached image Screenshot of error page (deleted) —
Depends on: 561916
The strings should be the same for CSP and x-frame-options. Users don't need to know which HTTP header was used (ux-implementation-level). See bug 561916 for more detailed UI feedback.
Would be nice to get this backported to 3.6.x--even without the UI it'll help.
blocking2.0: --- → ?
status2.0: --- → wanted
Attached patch redirect to about:blank (deleted) — Splinter Review
Same approach as v3 but after canceling channel, redirect to about:blank instead of localized neterror page.
Attachment #458456 - Flags: review?(dveditz)
Attachment #458456 - Flags: review?(dveditz) → review?(jst)
I don't think this should depend on bug 561916. The about:blank version should land now and we can decide what to do with the error page before Firefox 4 ships.
No longer depends on: 561916
Attachment #458456 - Flags: review?(jst) → review+
Comment on attachment 458456 [details] [diff] [review] redirect to about:blank I'll try to ping jst in person but requesting approval2.0 here.
Attachment #458456 - Flags: approval2.0?
blocking2.0: ? → final+
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment on attachment 458456 [details] [diff] [review] redirect to about:blank a=beltzner
Attachment #458456 - Flags: approval2.0? → approval2.0+
Attached patch 1.9.2 branch patch (deleted) — Splinter Review
Attachment #459629 - Flags: approval1.9.2.8?
Attachment #459629 - Flags: review?(jst)
Depends on: 581226
Attachment #459629 - Flags: review?(jst) → review+
Comment on attachment 459629 [details] [diff] [review] 1.9.2 branch patch Approved for 1.9.2.9, a=dveditz
Attachment #459629 - Flags: approval1.9.2.8? → approval1.9.2.8+
Out of intellectual curiosity....as I didn't see it here or on the documented page, @comment 38... How does one provide that response header .. . via an instruction to Apache?
You would configure Apache to deliver it, yes: Header always append X-Frame-Options <setting> I'll add that to the doc.
Thanks, Eric: My test proved up that it can not be placed into my root's .htaccess file, which I use because I do not have access to the server's configuration files. Have I correctly simply pasted the line: Header always append X-Frame-Options SAMEORIGIN to my .htaccess? Or, must the server administrator make changes somewhere?
The current implementation of X-Frame-Options is not complete! IE8+ also supports ALLOW-FROM origin: http://blogs.msdn.com/b/ieinternals/archive/2010/03/30/combating-clickjacking-with-x-frame-options.aspx Are there are plans to implement this in firefox?
IETF is working on standardizing Frame-Options: http://tools.ietf.org/html/draft-gondrom-frame-options-01 My preference is to implement the feature per an open standard, not per MS's latest blog post on the topic.
Attachment #439318 - Flags: ui-review?(johnath)
Flags: wanted1.9.1?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: