Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
9 years ago
2 years ago

People

(Reporter: mao, Assigned: bsterne)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+, status2.0 wanted, status1.9.2 .9-fixed, status1.9.1 wontfix)

Details

(Whiteboard: [parity-IE] [sg:want P2], URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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?
(Reporter)

Comment 1

9 years ago
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?

Updated

9 years ago
Blocks: 457011
(Reporter)

Comment 2

9 years ago
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
(Reporter)

Comment 3

9 years ago
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

Comment 4

9 years ago
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?

Updated

9 years ago
Whiteboard: [parity-IE]

Updated

9 years ago
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
(Assignee)

Comment 12

9 years ago
(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.
(Reporter)

Comment 14

9 years ago
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.
(Reporter)

Comment 15

9 years ago
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/
(Reporter)

Comment 16

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

Comment 17

9 years ago
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

8 years ago
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

8 years ago
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)

Updated

8 years ago
Assignee: dveditz → bsterne
(Assignee)

Comment 20

8 years ago
Created attachment 432960 [details] [diff] [review]
fix v1

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-
(Assignee)

Comment 22

7 years ago
Created attachment 436568 [details] [diff] [review]
fix v2

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).
(Assignee)

Comment 25

7 years ago
Created attachment 439318 [details] [diff] [review]
fix v3

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)
(Assignee)

Comment 26

7 years ago
Created attachment 440778 [details]
Screenshot of error page

Updated

7 years ago
Depends on: 561916

Comment 27

7 years ago
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: --- → ?
status1.9.2: --- → wanted
status2.0: --- → wanted
(Assignee)

Comment 29

7 years ago
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.
Attachment #458456 - Flags: review?(dveditz)
(Assignee)

Updated

7 years ago
Attachment #458456 - Flags: review?(dveditz) → review?(jst)
(Assignee)

Comment 30

7 years ago
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

Updated

7 years ago
Attachment #458456 - Flags: review?(jst) → review+
(Assignee)

Comment 31

7 years ago
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+
(Assignee)

Comment 32

7 years ago
Checked in about:blank version:
http://hg.mozilla.org/mozilla-central/rev/2e8648d72189
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Comment on attachment 458456 [details] [diff] [review]
redirect to about:blank

a=beltzner
Attachment #458456 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 34

7 years ago
Created attachment 459629 [details] [diff] [review]
1.9.2 branch patch
Attachment #459629 - Flags: approval1.9.2.8?
(Assignee)

Updated

7 years ago
Attachment #459629 - Flags: review?(jst)
(Assignee)

Updated

7 years ago
Depends on: 581226

Updated

7 years ago
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+
(Assignee)

Comment 36

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a2e1894b6ecf
1.9.2.9-fixed per comment 36
status1.9.2: wanted → .9-fixed
Keywords: dev-doc-needed
Now documented:

https://developer.mozilla.org/en/The_X-FRAME-OPTIONS_response_header
Keywords: dev-doc-needed → dev-doc-complete

Comment 39

7 years ago
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.

Comment 41

7 years ago
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?
status1.9.1: --- → wontfix

Comment 42

6 years ago
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?
(Assignee)

Comment 43

6 years ago
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)
(Reporter)

Updated

2 years ago
Flags: wanted1.9.1?
You need to log in before you can comment on or make changes to this bug.