Last Comment Bug 475530 - X-FRAME-OPTIONS header against "UI Redressing" AKA Clickjacking
: X-FRAME-OPTIONS header against "UI Redressing" AKA Clickjacking
Status: RESOLVED FIXED
[parity-IE] [sg:want P2]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: All All
: -- normal with 11 votes (vote)
: ---
Assigned To: Brandon Sterne (:bsterne)
:
:
Mentors:
http://hackademix.net/2009/01/27/ehy-...
Depends on: 581226
Blocks: clickjacking
  Show dependency treegraph
 
Reported: 2009-01-27 07:25 PST by Giorgio Maone [:mao]
Modified: 2016-02-08 06:41 PST (History)
50 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
wanted
.9-fixed
wontfix


Attachments
fix v1 (14.17 KB, patch)
2010-03-16 17:18 PDT, Brandon Sterne (:bsterne)
jst: review-
Details | Diff | Splinter Review
fix v2 (20.00 KB, patch)
2010-04-01 14:24 PDT, Brandon Sterne (:bsterne)
jst: review+
Details | Diff | Splinter Review
fix v3 (20.02 KB, patch)
2010-04-15 13:12 PDT, Brandon Sterne (:bsterne)
brandon: review+
Details | Diff | Splinter Review
Screenshot of error page (74.18 KB, image/png)
2010-04-22 09:21 PDT, Brandon Sterne (:bsterne)
no flags Details
redirect to about:blank (9.84 KB, patch)
2010-07-19 15:43 PDT, Brandon Sterne (:bsterne)
jst: review+
mbeltzner: approval2.0+
Details | Diff | Splinter Review
1.9.2 branch patch (8.55 KB, patch)
2010-07-22 16:22 PDT, Brandon Sterne (:bsterne)
jst: review+
dveditz: approval1.9.2.9+
Details | Diff | Splinter Review

Description Giorgio Maone [:mao] 2009-01-27 07:25:38 PST
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.
Comment 1 Giorgio Maone [:mao] 2009-01-27 14:39:27 PST
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?
Comment 2 Giorgio Maone [:mao] 2009-01-28 02:09:34 PST
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.
Comment 3 Giorgio Maone [:mao] 2009-01-28 08:14:56 PST
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.
Comment 4 Arthur 2009-01-28 08:26:41 PST
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?
Comment 5 Gervase Markham [:gerv] 2009-01-28 16:33:52 PST
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
Comment 6 Daniel Veditz [:dveditz] 2009-01-28 17:20:37 PST
I can take a crack at this.
Comment 7 Daniel Veditz [:dveditz] 2009-01-28 17:35:24 PST
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.
Comment 8 Daniel Veditz [:dveditz] 2009-01-28 17:36:20 PST
I guess if the server was looking for the DENY page's URI in a referrer it might do some good.
Comment 9 Gervase Markham [:gerv] 2009-01-28 17:39:18 PST
<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
Comment 10 Daniel Veditz [:dveditz] 2009-01-28 18:11:01 PST
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).
Comment 11 Gervase Markham [:gerv] 2009-01-28 20:22:48 PST
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
Comment 12 Brandon Sterne (:bsterne) 2009-01-29 09:24:01 PST
(In reply to comment #5)
> bsterne? (How much of CSP is in FF3.1 anyway? Any?)

None, unfortunately :-(
Comment 13 Daniel Veditz [:dveditz] 2009-01-29 11:17:18 PST
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 14 Giorgio Maone [:mao] 2009-01-29 11:41:13 PST
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.
Comment 15 Giorgio Maone [:mao] 2009-01-29 11:48:44 PST
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/
Comment 16 Giorgio Maone [:mao] 2009-01-29 16:15:05 PST
Screenshots (both IE8 and Fx+NS) here: http://hackademix.net/2009/01/29/x-frame-options-in-firefox/
Comment 17 Adam Barth 2009-02-11 17:32:32 PST
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.
Comment 18 frank goossens 2009-12-07 02:14:52 PST
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?
Comment 19 Denny Crane 2010-02-21 20:55:09 PST
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.
Comment 20 Brandon Sterne (:bsterne) 2010-03-16 17:18:12 PDT
Created attachment 432960 [details] [diff] [review]
fix v1

First swag at a patch.  I'll add some tests in the next day or two.
Comment 21 Johnny Stenback (:jst, jst@mozilla.com) 2010-03-16 17:43:44 PDT
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.
Comment 22 Brandon Sterne (:bsterne) 2010-04-01 14:24:31 PDT
Created attachment 436568 [details] [diff] [review]
fix v2

Addresses jst's review comments, adds tests, removes "try again" button from the error page.
Comment 23 Johnny Stenback (:jst, jst@mozilla.com) 2010-04-01 14:53:42 PDT
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
Comment 24 Johnathan Nightingale [:johnath] 2010-04-15 10:30:23 PDT
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).
Comment 25 Brandon Sterne (:bsterne) 2010-04-15 13:12:47 PDT
Created attachment 439318 [details] [diff] [review]
fix v3

Carried forward jst's r+.  Addresses johnath's ui-review comments.
Comment 26 Brandon Sterne (:bsterne) 2010-04-22 09:21:01 PDT
Created attachment 440778 [details]
Screenshot of error page
Comment 27 Jesse Ruderman 2010-04-26 17:16:33 PDT
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.
Comment 28 Daniel Veditz [:dveditz] 2010-07-16 08:22:45 PDT
Would be nice to get this backported to 3.6.x--even without the UI it'll help.
Comment 29 Brandon Sterne (:bsterne) 2010-07-19 15:43:27 PDT
Created attachment 458456 [details] [diff] [review]
redirect to about:blank

Same approach as v3 but after canceling channel, redirect to about:blank instead of localized neterror page.
Comment 30 Brandon Sterne (:bsterne) 2010-07-19 15:56:30 PDT
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.
Comment 31 Brandon Sterne (:bsterne) 2010-07-21 08:16:00 PDT
Comment on attachment 458456 [details] [diff] [review]
redirect to about:blank

I'll try to ping jst in person but requesting approval2.0 here.
Comment 32 Brandon Sterne (:bsterne) 2010-07-21 08:57:38 PDT
Checked in about:blank version:
http://hg.mozilla.org/mozilla-central/rev/2e8648d72189
Comment 33 Mike Beltzner [:beltzner, not reading bugmail] 2010-07-21 14:19:11 PDT
Comment on attachment 458456 [details] [diff] [review]
redirect to about:blank

a=beltzner
Comment 34 Brandon Sterne (:bsterne) 2010-07-22 16:22:30 PDT
Created attachment 459629 [details] [diff] [review]
1.9.2 branch patch
Comment 35 Daniel Veditz [:dveditz] 2010-07-23 11:16:36 PDT
Comment on attachment 459629 [details] [diff] [review]
1.9.2 branch patch

Approved for 1.9.2.9, a=dveditz
Comment 36 Brandon Sterne (:bsterne) 2010-07-23 12:16:58 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a2e1894b6ecf
Comment 37 Daniel Veditz [:dveditz] 2010-07-25 10:29:46 PDT
1.9.2.9-fixed per comment 36
Comment 38 Eric Shepherd [:sheppy] 2010-08-12 11:10:50 PDT
Now documented:

https://developer.mozilla.org/en/The_X-FRAME-OPTIONS_response_header
Comment 39 john ruskin 2010-09-08 18:32:31 PDT
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?
Comment 40 Eric Shepherd [:sheppy] 2010-09-10 10:59:50 PDT
You would configure Apache to deliver it, yes:

Header always append X-Frame-Options <setting>

I'll add that to the doc.
Comment 41 john ruskin 2010-09-10 18:23:56 PDT
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?
Comment 42 spamfagos 2011-08-02 01:06:25 PDT
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?
Comment 43 Brandon Sterne (:bsterne) 2011-08-02 08:31:02 PDT
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.

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