X-Frame-Options: SAMEORIGIN largely useless as implemented

NEW
Unassigned

Status

()

Core
DOM: Security
P3
normal
5 years ago
9 months ago

People

(Reporter: Michal Zalewski, Unassigned, NeedInfo)

Tracking

({dev-doc-needed, sec-want})

10 Branch
dev-doc-needed, sec-want
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: also applies to ALLOW-FROM, [domsecurity-backlog1], URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
Currently, X-Frame-Options: SAMEORIGIN performs a check only against the top-level window. Therefore, it is possible to do have a framing chain such as good.com -> evil.com -> good.com [X-Frame-Options: SAMEORIGIN]

Demo:
http://lcamtuf.coredump.cx/xfo/

Unfortunately, this makes the mechanism largely pointless for a good number of sites. Any site that allows a rogue ad to be displayed in an IFRAME; or that frames third-party content for other reasons (e.g., iGoogle, Image Search results, Facebook gadgets), is effectively not protected, because the framed content from evil.com can load and arbitrarily decorate any page in the same origin as the top-level window, and entice the user to interact with it.

A simple way to fix it would be to make sure that the SAMEORIGIN check traverses all ancestors, rather than just looking at 'top'. I would expect this to be compatible with current uses of XFO, and actually make the feature useful.
Component: Security → Security
Product: Firefox → Core
QA Contact: firefox → toolkit

Comment 1

5 years ago
Created attachment 595625 [details] [diff] [review]
check ancestors for XFO - wip

It seems weird that XFO checks for the top level document by design. I'm not sure if the assertions made by the original Microsoft blog post are still true. [1] The top level check seems to cause problems with iframe sandboxes. A game site which may uses iframe sandbox to protect itself, would become vulnerable if their XFO is set to same origin. This is one of the more plausible attacks I can think of.

I've attached a patch which checks each of the ancestors of the document to see if any violate XFO: sameorigin . However there are still some quirks that need to be worked out, and tests written. It blocks the specific PoC, though the current patch will log the following error

"Security Error: Content at http://lcamtuf.coredump.cx/xfo/nested.cgi may not load data from http://beaver-peak.us/xfo2.html."

The error message is clearly wrong since it isn't nested.cgi causing the error.

[1] - http://blogs.msdn.com/b/ie/archive/2009/01/27/ie8-security-part-vii-clickjacking-defenses.aspx#9382230
Assignee: nobody → dchan+bugzilla
We agree that the current XFO de facto specification of checking against the top frame is pretty useless, which is why we explicitly did it differently in the CSP frame-ancestors directive. That, now, is being deprecated in CSP in favor of a standardized [X-]Frame-Options where I certainly hope the header assertion is checked all the way to the top.

abarth: what is Chromium/webkit doing with XFO these days?
(Reporter)

Comment 3

5 years ago
Also checks top-level. I chatted with Adam and he wasn't excited about fixing XFO (essentially used the argument that it's worth the effort only if I can convince Mozilla or Microsoft to do that).

My take on this is that XFO is one of the relatively few new security mechanisms that are actually depended on heavily, and people have abandoned frame-busting JS in favor of it. So keeping it broken for the foreseeable future seems suboptimal, and the longer it stays in, the harder it will be to get rid of it.

Comment 4

5 years ago
> abarth: what is Chromium/webkit doing with XFO these days?

The same as what IE did originally.

Who's the best person to talk with at IE these days about XFO?  Ideally everyone would agree that checking all the ancestors is better and we'd all change our implementations.  I'm wary of changing WebKit alone and would like at least one or two other vendors to change on a similar schedule.
I'd be happy to change if we can convince Microsoft or if it's specified in the IETF Frame-Options spec and that gets reasonably close to being accepted. Otherwise we break sites without much to stand on.
(Reporter)

Comment 6

5 years ago
I won't fight too vigorously, but keep in mind that as implemented right now, it offers a false sense of security to many sites that switched from framebusting JS to "X-F-O: SAMEORIGIN" - and that this property is fairly unintuitive.

From that perspective, it almost seems better to drop support for it if no changes are planned.

As for breakage, I suspect it's a lot less likely to cause trouble than most other security changes made in recent years (say, MIME type checking on CSS, frame descendant policy, better HTML parsing algorithms).
This issue is already understood amongst the people writing implementations (and attacking sites), we should un-hide this to give site authors a chance to defend themselves.

Any objections from the Chrome or webkit folks on doing that?
Whiteboard: [sg:want]

Comment 8

5 years ago
> Any objections from the Chrome or webkit folks on doing that?

Not from me.

Comment 9

5 years ago
> Any objections from the Chrome or webkit folks on doing that?

None from me either.

Comment 10

5 years ago
What are the steps to move forward with changing XFO SAMEORIGIN behavior?
Keywords: sec-want
(Reporter)

Comment 11

5 years ago
Hey guys,

Just FYI, we're getting an increasing number of vulnerability reports with exploits that abuse this behavior to effectively bypass XFO.
(Reporter)

Comment 12

5 years ago
BTW, the draft spec is now updated with a possible solution:

http://tools.ietf.org/id/draft-gondrom-frame-options-02.txt

---
   In the case of SAMEORIGIN and ALLOW-FROM, there is also an optional
   flag "AllAncestors".  If this flag is set, it means that browsers
   MUST validate the URL of each hosting frame up to the top level and
   only allow the framing if all ancestor frames' origins are either the
   same as in SAMEORIGIN or included in the ALLOW-FROM list.
---

Comment 13

4 years ago
I haven't had time to look at this in a while, unassigning so that someone else can take it
Assignee: dchan+bugzilla → nobody
Group: core-security
Couple of data points:

https://github.com/twitter/secureheaders/blob/master/lib/secure_headers/headers/x_frame_options.rb

https://github.com/evilpacket/helmet/blob/master/lib/middleware/xframe.js

Both the *libraries* are getting it wrong (if I am not wrong :). I argue that this should be changed to default to checking all ancestors. I think this is a case where Firefox should care less about the standard and more about doing the right thing for everyone. We can also have a setting where the x-frame-options is ignored if a CSP frame-options is present. Those people who really really want to check only the parent can switch to CSP. I can't imagine who they will be.

If I am not wrong, the effort to switch to checking all ancestors by default should be trivial. I am happy to put in the effort to make it to default check all ancestors. 

What do others think?

Comment 15

4 years ago
(In reply to Devdatta Akhawe [:devd] from comment #14)
> Couple of data points:
> 
> https://github.com/twitter/secureheaders/blob/master/lib/secure_headers/
> headers/x_frame_options.rb
> 
> https://github.com/evilpacket/helmet/blob/master/lib/middleware/xframe.js
> 
> Both the *libraries* are getting it wrong (if I am not wrong :). I argue
> that this should be changed to default to checking all ancestors. I think
> this is a case where Firefox should care less about the standard and more
> about doing the right thing for everyone. We can also have a setting where
> the x-frame-options is ignored if a CSP frame-options is present. Those
> people who really really want to check only the parent can switch to CSP. I
> can't imagine who they will be.
> 
> If I am not wrong, the effort to switch to checking all ancestors by default
> should be trivial. I am happy to put in the effort to make it to default
> check all ancestors. 
> 
> What do others think?

the UI Safety frame options spec says the ancestors should be checked, see
http://www.w3.org/TR/UISafety/#frame-options

"If the directive-value contains the keyword-source 'self' alone to indicate that the resource may be embedded only if all ancestors are in the same origin as the protected resource."

so really i think it should be the other way around - if people want to check all ancestors, they should use CSP (especially since this is where frame-options is actively being worked on currently). That's just my personal opinion though, and there's some open issues in that section of the spec. Also see comment 5, I think breakage is a real issue here for sites currently using XFO.
My whole claim is that people don't know that they want to check all ancestors. Based on advice given online or for some other reason I don't know, everyone sets "XFO sameorigin" thinking it protects them against clickjacking. In many cases, it might not---as a result, things are broken. 

See the results on XFO at http://www.veracode.com/blog/2012/11/security-headers-report/

XFO is by far the most widely implemented security header and XFO:SameOrigin is the most common setting. I don't think it is the case that people choosing sameorigin decided "I don't really want to check allancestors". More likely, they didn't know about this threat.

Comment 17

4 years ago
(In reply to Devdatta Akhawe [:devd] from comment #16)
> My whole claim is that people don't know that they want to check all
> ancestors. Based on advice given online or for some other reason I don't
> know, everyone sets "XFO sameorigin" thinking it protects them against
> clickjacking. In many cases, it might not---as a result, things are broken. 
> 
> See the results on XFO at
> http://www.veracode.com/blog/2012/11/security-headers-report/
> 
> XFO is by far the most widely implemented security header and XFO:SameOrigin
> is the most common setting. I don't think it is the case that people
> choosing sameorigin decided "I don't really want to check allancestors".
> More likely, they didn't know about this threat.

ok, maybe we should implement some telemetry to see if this would change the behavior of any sites ? (which is basically implementing this and ignoring the result of checking all ancestors at first)
I actually can't think of cases where things would break. Does anyone have examples  where things break?
(Reporter)

Comment 19

4 years ago
I don't think we'd see any breakage on our properties (and most of the XFO headers seen by Veracode are probably our various products / domains, as we set the header by default).

This would only affect a case where somebody has an interim non-same-origin frame in between two same-origin documents - and wants the frame to be shown in this case, but not when the outer document is missing... which seems unlikely.

Comment 20

4 years ago
i agree it's unlikely, but it would be a lot easier to be sure about that if we had telemetry data to support that theory - then if it's true, we turn on the check for real - then maybe other browsers will start enforcing the check as well (is there a Webkit bug for this?)

I still think that we should implement the CSP frame-options directive which has no backwards compatibility concerns, there is code in CSP to do some of it already due to Gecko's non-spec frame-ancestors directive support, but i suppose that is a separate bug.

Updated

4 years ago
Blocks: 846978

Updated

4 years ago
No longer blocks: 846978
The problem with telemetry is that it has to go through all the channels and reach release, then we have to wait to get data, and then we can switch the defaults. we are talking at the minimum a year's latency. I think this change is far far less likely to cause breakage than changes made in the past (see comment 6) or changes being made currently (3p cookies).

Thanks for filing the bug! given the lack of adoption of CSP currently (see veracode link) and the time it will take for CSP1.1 to be available and adopted, it is also at least 1 year for frame-options to widely effective; likely far more.
Is there a reason not to try implementing the change for Nightly and maybe Aurora users, and seeing if anyone reports bustage for a release or two?  No need to inflict it on beta/release without being reasonably certain it'll fly, but at least if people break from this there, we know more than when we started.

Comment 23

4 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #22)
> Is there a reason not to try implementing the change for Nightly and maybe
> Aurora users, and seeing if anyone reports bustage for a release or two?  No
> need to inflict it on beta/release without being reasonably certain it'll
> fly, but at least if people break from this there, we know more than when we
> started.

No - I think someone writing a patch for this and at least trying out some major sites with it would be a great start. Isaac Dawson's data as used in his post on the Veracode blog post is public (http://beacua.se/secheaders/secheaders_11022012.zip), so there's a good list of sites using XFO there to begin with there as well. 

I also agree this is much less likely to cause breakage than other recent changes, and your points about lack of CSP uptake are also well taken. 

(In reply to Devdatta Akhawe [:devd] from comment #21)
> The problem with telemetry is that it has to go through all the channels and
> reach release, then we have to wait to get data, and then we can switch the
> defaults. we are talking at the minimum a year's latency. 

I think that's an overexaggeration - telemetry can be uplifted (hopefully fairly quickly) and if we see something like no release users at all being broken by this change in the first month or so, that would be awfully compelling (to me at least.)

> I think this change is far far less likely to cause breakage than changes made > in the past (see comment 6) or changes being made currently (3p cookies).

yes, i agree with that.

> Thanks for filing the bug! given the lack of adoption of CSP currently (see
> veracode link) and the time it will take for CSP1.1 to be available and
> adopted, it is also at least 1 year for frame-options to widely effective;
> likely far more.

yes, that's true. I don't oppose changing the behavior if we have actual data to support the assertions of lack of breakage since Gecko would essentially be 'going first', but as has been said, let's start with a patch :)
Created attachment 720345 [details] [diff] [review]
Make XFO check allancestors by default
Comment on attachment 720345 [details] [diff] [review]
Make XFO check allancestors by default

Here's a patch. I will need to create tests. Asking for a review from Phil who wrote the original patch.

@jeff: no reason. I don't know how to do that.
Attachment #720345 - Flags: review?(philames)
Also, another datapoint: the MDN XFO docs also make the same mistake and give a possibly false sense of security 

https://developer.mozilla.org/en-US/docs/HTTP/X-Frame-Options

Comment 27

4 years ago
I think you might have the wrong person -- I wrote a patch to support X-F-O Allow-From, not X-F-O in general.  While the patch looks reasonable, I'm not familiar enough with the Firefox code to say definitively that it will have the desired effect.

Perhaps someone from https://bugzilla.mozilla.org/show_bug.cgi?id=475530 might be more appropriate?  bsterne, or jst?

Updated

4 years ago
Keywords: dev-doc-needed
Comment on attachment 720345 [details] [diff] [review]
Make XFO check allancestors by default

asking bz instead.
Attachment #720345 - Flags: review?(philames) → review?(bzbarsky)
Comment on attachment 720345 [details] [diff] [review]
Make XFO check allancestors by default

>+    // XXXdevd We diverge from the spec for practical reason

How about we file a bug on the spec and reference it here?

>+    // We need to check the location of this window and the location of the each
>     // window, if we're not the top. 

s/the each window/each of our ancestors/

>+    // stop when we see a docshell whose  parent has a system principal, or a

Fix the spacing.

>         else {
>+          return false;

Why?  That doesn't obviously seem right to me...  At the very least needs documentation, though I suspect you just want to treat it as hitting the root.

>+        // If the X-Frame-Options value is SAMEORIGIN, then the parnet frame 

"parent".

r=me with those nits
Attachment #720345 - Flags: review?(bzbarsky) → review+
Thanks! I will take care of all the issues save for the return false:

> Why?  That doesn't obviously seem right to me...  At the very least needs
> documentation, though I suspect you just want to treat it as hitting the
> root.

hmm..the "+return false" seems to be a quirk on the diff's part. I didn't add it, it is already there: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDSURIContentListener.cpp#343 

I think jst suggested adding it in : https://bugzilla.mozilla.org/show_bug.cgi?id=581226#c15
 
I am not sure also why it is return false. I think the idea is that "this shouldn't happen. Lets fail close" Is that convincing enough or do you think this warrants more investigation?  I can ask jst/bsterne, but I doubt either will remember.
> I think the idea is that "this shouldn't happen. Lets fail close"

OK, I can buy that.
Created attachment 721326 [details] [diff] [review]
Make XFO check allancestors by default
Attachment #720345 - Attachment is obsolete: true
Created attachment 721327 [details] [diff] [review]
Make XFO check allancestors by default
Attachment #721326 - Attachment is obsolete: true

Comment 34

4 years ago
I'd like to see some mochitests for this change, please.

Comment 35

4 years ago
Also, it looks like you can carry over bz's r+ on the latest patch :)
update on the Veracode report linked from comment 16, they're re-run the survey:

http://www.veracode.com/blog/2013/03/security-headers-on-the-top-1000000-websites-march-2013-report/
Duplicate of this bug: 877724
And while we're cautiously worrying over here about breaking web compatibility, it looks like IE has simply done this since IE 9 according to http://webstersprodigy.net/2012/09/13/clickjacking-google/ (thanks for the link, Johnathan). I tested IE 10 and it does seem to enforce SAMEORIGIN all the way to the top, as if the proposed "AllAncestors" flag were set.

We should just do it (and do the same for ALLOW-FROM)
Whiteboard: [sg:want] → also applies to ALLOW-FROM
Created attachment 760181 [details] [diff] [review]
Make XFO check allancestors by default
Attachment #721327 - Attachment is obsolete: true
Comment on attachment 760181 [details] [diff] [review]
Make XFO check allancestors by default

Given the nature of the fix, do I need to replicate each and every XFO test but this time for all-ancestors? Or would just 1 test for all ancestors be sufficient and we can rely on existing tests for everything else? It seems quite unnecessary to create 2 copies of the same tests---both would exercise the same code base.
Attachment #760181 - Flags: review+
Created attachment 760225 [details] [diff] [review]
Tests to check allancestors by default
Created attachment 760227 [details] [diff] [review]
Make XFO check allancestors by default
Attachment #760181 - Attachment is obsolete: true
Comment on attachment 760227 [details] [diff] [review]
Make XFO check allancestors by default

carrying over r+ from bz
Attachment #760227 - Flags: review+
Comment on attachment 760225 [details] [diff] [review]
Tests to check allancestors by default

Ian: Can you take a look at the test I added? Also, can you answer comment 40?
Attachment #760225 - Flags: review?(imelven)
try looks good to me.
https://tbpl.mozilla.org/?tree=Try&rev=674c7249e71f

Comment 46

4 years ago
For the record, I strongly support doing this given comment 38, I'm going to take a look at Dev's patches as soon as I can and have asked a few other people if they have time to take a look too.

Comment 47

4 years ago
I originally thought there might be some issues with mozbrowser iframes, but the code looks like it works for it. 

Would it be possible to add some tests for mozbrowser? I can help you with setting up the needed page permissions. I think a single test with mozbrowser at the top level would be sufficient. The main thing I want to make sure is covered is the break clause for the IsBrowserOrApp(). Granted the code flow should check the parent before setting curDocShell

mozbrowser frame origin = bar
 iframe origin = foo
   iframe origin = foo XFO = SAMORIGIN
I am not sure that should be part of this bug---the mozbrowser issue existed before this too, right? I am surprised the previous implementation didn't have to write a mozbrowser test.
Attachment #760225 - Flags: review?(ian.melven) → review?(grobinson)
Comment on attachment 760225 [details] [diff] [review]
Tests to check allancestors by default

Punting to Sid, as I do not feel qualified to review this.
Attachment #760225 - Flags: review?(grobinson) → review?(sstamm)
Comment on attachment 760225 [details] [diff] [review]
Tests to check allancestors by default

Review of attachment 760225 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/test/file_x-frame-options_main.html
@@ +40,5 @@
>  </script>
>  
>  
> +<iframe id="grandchild-same-origin-deny" src="http://example.com/tests/content/base/test/file_x-frame-options_page.sjs?testid=grandchild-same-origin&xfo=gcs"></iframe><br>
> +

Change this to use addFrame (see code just above it) to make this more readable.

Please also put in a comment that explains how this test is going to differ from the rest; for example:
// this creates a gcs iframe that has a subframe for test sameorigin1 
// to test that a grandchild frame checks all ancestors.

Please additionally rename gcs to something that is more meaningful.  What this test is really doing is creating a subframe so maybe something like parent_for_sameorigin1 (means that this frame won't have an XFO, but its subframe will, and the xfo value will be keyed as sameorigin1).

::: content/base/test/file_x-frame-options_page.sjs
@@ +51,5 @@
>    }
> +  else if (query['xfo'] === 'gcs') {
> +    response.write("<iframe src='http://mochi.test:8888/tests/content/base/test/file_x-frame-options_page.sjs?testid=sameorigin1&xfo=sameorigin'></iframe>");
> +    return;
> +  }

else is a little misleading and not necessary here.  This block of code is doing something completely different than the if block above it, so please put in a comment to explain what this code is doing.

I think testid=sameorigin1 is already taken by another test in file_x-frame-options_main.html.  Please use a new for this test (allancestors or something).  You can reuse the xfo value, but don't reuse the testid value or it might confuse the test harness.

::: content/base/test/test_x-frame-options.html
@@ +143,5 @@
> +  frame = harness.contentDocument.getElementById("sameorigin1");
> +  var test4 = frame.contentDocument.getElementById("test").textContent;
> +  is(test4, "sameorigin1", "test sameorigin1");
> +  is(test12, null, "test allow-from-deny");
> +

This is confusing to me.  Please make a clear comment to explain how you're "checking allancestors" and use new variable names (test13, test4 are already defined and you're redefining them here).
Attachment #760225 - Flags: review?(sstamm) → review-

Comment 51

4 years ago
Just as a heads up, Blink has made this change and has experienced some breakage already : see https://code.google.com/p/chromium/issues/detail?id=305244 

so it seems like our concerns about breakage weren't completely unfounded... 

dveditz has also proposed that frame-options be included as part of the CSP 1.1 spec, which I strongly support - we can spec things right for frame-options the first time and avoid the problems with XFO..
The breakage is in Google and the comment 11 says that they need this behavior to stop exploits. So, I am not sure whether to count this as actual breakage. Maybe Michal can elaborate?
---
The status: I am currently swamped and haven't worked on Firefox for a while. I am not sure when I will get to it, so anyone else who has time should feel free to fix the test.
While the fix to the tests are not too hard, I am going to wait for Michal to elaborate on the breakage due to XFO allancestors and whether this patch should be blocked or not. While I think this patch is the right thing to do, we can't really afford to break google.
Flags: needinfo?(lcamtuf)
Component: Security → DOM: Security
Priority: -- → P3
Whiteboard: also applies to ALLOW-FROM → also applies to ALLOW-FROM, [domsecurity-backlog1]
You need to log in before you can comment on or make changes to this bug.