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

Status: RESOLVED FIXED

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

Description

[Giorgio Maone [:ma1]]

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

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

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

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]

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

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

I can take a crack at this.

Comment 7

[Daniel Veditz [:dveditz]]

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

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

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

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

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

(In reply to comment #5)
> bsterne? (How much of CSP is in FF3.1 anyway? Any?)

None, unfortunately :-(

Comment 13

[Daniel Veditz [:dveditz]]

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

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

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

Screenshots (both IE8 and Fx+NS) here: http://hackademix.net/2009/01/29/x-frame-options-in-firefox/

Comment 17

[Adam Barth]

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]

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]

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

Attachment: fix v1

First swag at a patch.  I'll add some tests in the next day or two.

Comment 21

[Johnny Stenback (: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.

Comment 22

[Brandon Sterne (:bsterne)]

Attachment: fix v2

Addresses jst's review comments, adds tests, removes "try again" button from the error page.

Comment 23

[Johnny Stenback (: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

Comment 24

[Johnathan Nightingale [:johnath]]

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

Attachment: fix v3

Carried forward jst's r+.  Addresses johnath's ui-review comments.

Comment 26

[Brandon Sterne (:bsterne)]

Attachment: Screenshot of error page

Comment 27

[Jesse Ruderman]

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

Would be nice to get this backported to 3.6.x--even without the UI it'll help.

Comment 29

[Brandon Sterne (:bsterne)]

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

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

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

Checked in about:blank version:
http://hg.mozilla.org/mozilla-central/rev/2e8648d72189

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 458456 [details] [diff] [review]
redirect to about:blank

a=beltzner

Comment 34

[Brandon Sterne (:bsterne)]

Attachment: 1.9.2 branch patch

Comment 35

[Daniel Veditz [:dveditz]]

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a2e1894b6ecf

Comment 37

[Daniel Veditz [:dveditz]]

1.9.2.9-fixed per comment 36

Comment 38

[Eric Shepherd [:sheppy]]

Now documented:

https://developer.mozilla.org/en/The_X-FRAME-OPTIONS_response_header

Comment 39

[john]

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

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]

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]

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

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.