Michal Zalewski Reporter
	Description • 12 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.

	David Chan [:dchan]
	Comment 1 • 12 years ago

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

	Daniel Veditz [:dveditz]
	Comment 2 • 12 years ago

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?

	Michal Zalewski Reporter
	Comment 3 • 12 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.

	Adam Barth
	Comment 4 • 12 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.

	Daniel Veditz [:dveditz]
	Comment 5 • 12 years ago

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.

	Michal Zalewski Reporter
	Comment 6 • 12 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).

	Daniel Veditz [:dveditz]
	Comment 7 • 12 years ago

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?

	Adam Barth
	Comment 8 • 12 years ago

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

Not from me.

	Sam Weinig
	Comment 9 • 12 years ago

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

None from me either.

	David Chan [:dchan]
	Comment 10 • 12 years ago

What are the steps to move forward with changing XFO SAMEORIGIN behavior?

	Michal Zalewski Reporter
	Comment 11 • 12 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.

	Michal Zalewski Reporter
	Comment 12 • 12 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.
---

	David Chan [:dchan]
	Comment 13 • 12 years ago

I haven't had time to look at this in a while, unassigning so that someone else can take it

	Devdatta Akhawe [:devd]
	Comment 14 • 11 years ago

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?

	Ian Melven :imelven
	Comment 15 • 11 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.

	Devdatta Akhawe [:devd]
	Comment 16 • 11 years ago

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.

	Ian Melven :imelven
	Comment 17 • 11 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)

	Devdatta Akhawe [:devd]
	Comment 18 • 11 years ago

I actually can't think of cases where things would break. Does anyone have examples  where things break?

	Michal Zalewski Reporter
	Comment 19 • 11 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.

	Ian Melven :imelven
	Comment 20 • 11 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.

	Devdatta Akhawe [:devd]
	Comment 21 • 11 years ago

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.

	Jeff Walden [:Waldo]
	Comment 22 • 11 years ago

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.

	Ian Melven :imelven
	Comment 23 • 11 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 :)

	Devdatta Akhawe [:devd]
	Comment 25 • 11 years ago

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.

	Devdatta Akhawe [:devd]
	Comment 26 • 11 years ago

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

	Phil Ames
	Comment 27 • 11 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?

	Devdatta Akhawe [:devd]
	Comment 28 • 11 years ago

Comment on attachment 720345 [details] [diff] [review]
Make XFO check allancestors by default

asking bz instead.

	Boris Zbarsky [:bzbarsky]
	Comment 29 • 11 years ago

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

	Devdatta Akhawe [:devd]
	Comment 30 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 31 • 11 years ago

> I think the idea is that "this shouldn't happen. Lets fail close"

OK, I can buy that.

	Ian Melven :imelven
	Comment 34 • 11 years ago

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

	Ian Melven :imelven
	Comment 35 • 11 years ago

Also, it looks like you can carry over bz's r+ on the latest patch :)

	Daniel Veditz [:dveditz]
	Comment 36 • 11 years ago

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/

	Daniel Veditz [:dveditz]
	Comment 38 • 11 years ago

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)

	Devdatta Akhawe [:devd]
	Comment 40 • 11 years ago

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.

	Devdatta Akhawe [:devd]
	Comment 43 • 11 years ago

Comment on attachment 760227 [details] [diff] [review]
Make XFO check allancestors by default

carrying over r+ from bz

	Devdatta Akhawe [:devd]
	Comment 44 • 11 years ago

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?

	Devdatta Akhawe [:devd]
	Comment 45 • 11 years ago

try looks good to me.
https://tbpl.mozilla.org/?tree=Try&rev=674c7249e71f

	Ian Melven :imelven
	Comment 46 • 11 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.

	David Chan [:dchan]
	Comment 47 • 11 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

	Devdatta Akhawe [:devd]
	Comment 48 • 11 years ago

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.

	Garrett Robinson [:grobinson]
	Comment 49 • 11 years ago

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.

	Sid Stamm [:geekboy or :sstamm]
	Comment 50 • 11 years ago

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

	Ian Melven :imelven
	Comment 51 • 11 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..

	Devdatta Akhawe [:devd]
	Comment 52 • 11 years ago

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.

	Devdatta Akhawe [:devd]
	Comment 53 • 10 years ago

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.

	j.j.
	Comment 54 • 7 years ago

Blink is trying to fix this (again):
https://groups.google.com/a/chromium.org/forum/m/#!topic/blink-dev/fsDaKFqvU20

	Mike West
	Comment 55 • 7 years ago

We're planning on changing SAMEORIGIN to walk the ancestor chain in Chrome 60, which is branching today. Given the numbers discussed in the blink-dev@ thread linked in comment 54 above, I hope we're more successful than we were last time. It would be marvelous if y'all were interested in implementing the same behavior.

	Mike West
	Comment 56 • 7 years ago

FYI: The change to SAMEORIGIN is shipping in Chrome 60, which hit stable last Wednesday. So far, the level of displeasure seems low enough that I'm pretty sure it's going to stick.

	Mike West
	Comment 57 • 7 years ago

Ping. Chrome's shipping this. It would be lovely if y'all would do the same! :)

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 58 • 7 years ago

Hey Mike,
I see you added web platform tests, is this all Chrome is using to test this change?
Also do you have any breakage data/info you could share at all?
Thanks

	Mike West
	Comment 61 • 7 years ago

The web platform tests under https://github.com/w3c/web-platform-tests/tree/master/x-frame-options are indeed how we're testing this code. We have some unit tests higher up in the stack, but those seem like they'd be difficult to share, and WPT covers the web-visible bits.

With regard to breakage, I haven't seen any bug reports since we shipped it ~4 months ago. I can't say that there's _no_ breakage, but any breakage we might have caused seems well within our tolerances. Chrome's not planning on reverting the change.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 62 • 7 years ago

Thanks Mike, my only concern is there aren't tests for allow-from which we support. I managed to find your tests in the end, I can't see anything we don't really have.

We *could* implement that allow-from as a follow up perhaps.

This would mean example.com implemented as allow-from bank.com would fail in this config:

bank.com frames example.com framing example.com

We could bend the specification further and allow sameorigin in this case to reduce breakage.

:dveditz - Should we implement a hack of "allow-from", just implement it the same or implement it as a secondary bug?

	Mike West
	Comment 63 • 7 years ago

I think it's totally reasonable to add `ALLOW-FROM` tests to WPT, though I don't have any plans to implement it in Chrome. We'll just fail those tests.

That said, I think y'all's current `ALLOW-FROM` implementation doesn't check the ancestor tree. I think it would be ideal if you took the same path we're moving towards with `SAMEORIGIN`, and applied the `ALLOW-FROM` list to each ancestor. That said, I don't have any data to share with you about how much that would end up breaking things (which is one of the reasons I'm reluctant to implement it in Chrome).

Personally, I'd prefer we ask folks to use `frame-ancestors` instead, which has had clear semantics from the start, and has been deployed interoperably with pretty wide coverage these days.

	Daniel Veditz [:dveditz]
	Comment 65 • 7 years ago

(In reply to Mike West from comment #63)
> I think y'all's current `ALLOW-FROM` implementation doesn't check the ancestor
> tree. I think it would be ideal if you took the same path we're moving towards
> with `SAMEORIGIN`, and applied the `ALLOW-FROM` list to each ancestor.

rfc7034 doesn't support an ALLOW-FROM "list"--you get one.

If Chrome supported ALLOW-FROM I'd happily enforce it against each ancestor, but since Chrome doesn't making it stricter only risks breaking Firefox on sites that were happily insecure anyway. It would be ideal if sites migrated to CSP frame-ancestors.

> :dveditz - Should we implement a hack of "allow-from", just implement it the
> same or implement it as a secondary bug?

Leave allow-from alone in this bug so we can land the SAMEORIGIN improvements without worrying about site compat. If you want to file a follow-up bug about allow-from then maybe add some telemetry here to measure how much it's used and how much we might break with a stricter setting. But honestly I wouldn't bother. Every major browser supports the better/clearer CSP frame-ancestors and sites would be foolish to rely on an X-Frame-Options setting that Chrome doesn't support.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 66 • 7 years ago

I think this can go in as we have a test for it via the web platform, :smaug should we write out own as well?

I think we should file a bug to investigate why there isn't a SecurityError raised for accessing the frame's contentDocument which can happen outside of this bug:
  testing/web-platform/meta/x-frame-options/sameorigin.sub.html.ini
I spent a while looking into this but it's unrelated to what we are changing here anyway.

I will file the follow up for allow-from also and keep that out of this based on comments from :dveditz.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 67 • 7 years ago

If there is a wpt test for this, that should be enough, assuming that test actually cover all the needed cases.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 68 • 7 years ago
mozreview-review

Comment on attachment 8927602 [details]
Bug 725490 - Change XFO sameorigin to check all ancestors for same origin.

https://reviewboard.mozilla.org/r/198896/#review208456

::: dom/security/FramingChecker.cpp:72
(Diff revision 1)
>    // content-type docshell doesn't work because some chrome documents are
>    // loaded in content docshells (see bug 593387).
>    nsCOMPtr<nsIDocShellTreeItem> thisDocShellItem(
>      do_QueryInterface(static_cast<nsIDocShell*>(aDocShell)));
> -  nsCOMPtr<nsIDocShellTreeItem> parentDocShellItem;
> +  nsCOMPtr<nsIDocShellTreeItem> parentDocShellItem,
> +                                topDocShellItem;

nsCOMPtr<nsIDocShellTreeItem> topDocShellItem;

::: dom/security/FramingChecker.cpp:85
(Diff revision 1)
>    }
>  
>    // Traverse up the parent chain and stop when we see a docshell whose
>    // parent has a system principal, or a docshell corresponding to
>    // <iframe mozbrowser>.
>    while (NS_SUCCEEDED(

So we do iterate to the top here already. Could you try to merge these two loops?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 71 • 7 years ago
mozreview-review

Comment on attachment 8927602 [details]
Bug 725490 - Change XFO sameorigin to check all ancestors for same origin.

https://reviewboard.mozilla.org/r/198896/#review208502

	Pulsebot
	Comment 72 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b3d1e9847a7a
Change XFO sameorigin to check all ancestors for same origin. r=smaug

	Pulsebot
	Comment 73 • 7 years ago

Backout by aiakab@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6c79f94201d6
Backed out changeset b3d1e9847a7a for mochitest failures in  dom/base/test/test_x-frame-options.html r=backout on a CLOSED TREE

	Arthur Iakab [arthur_iakab]
	Comment 74 • 7 years ago

Backed out changeset b3d1e9847a7a (bug 725490) for mochitest failures in dom/base/test/test_x-frame-options.html r=backout on a CLOSED TREE

Here is the log: https://treeherder.mozilla.org/logviewer.html#?job_id=148359772&repo=autoland&lineNumber=2185

Here is the back out revision: https://hg.mozilla.org/integration/autoland/rev/6c79f94201d65f6b5343c289161c2e38deb8cebe

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 77 • 6 years ago

This is now fixed, https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a8b3a208f11539cdff9395872766c1c3cc8071a&selectedJob=153708729 the only test I can see failing is a 2018 NY treat for us :D

:smaug can I get another r+ as I moved the code snippet, basically in non e10s mode I was trying to check the system URI which it doesn't have access to, I should have been failing the loop early.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 78 • 6 years ago

r+ for what part? Just add to the review queue.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 79 • 6 years ago

Since your last r+, it doesn't look like reviewboard will let me remove you as an r+.
The difference is here: https://reviewboard.mozilla.org/r/198896/diff/3-5/
A confirmation this minor change is ok should be enough I think, as mentioned reviewboard thinks you still have it r+'d.

Thanks

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 80 • 6 years ago

On can always ask review using bugzilla ;) But ok, looking.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 81 • 6 years ago

s/On/One/

r+

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 82 • 6 years ago

(In reply to Olli Pettay [:smaug] from comment #81)
> s/On/One/
That was for my own comment.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 83 • 6 years ago

Yeah I should have done that hah. Thanks!

	Pulsebot
	Comment 84 • 6 years ago

Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/351c75ab74c9
Change XFO sameorigin to check all ancestors for same origin. r=smaug

	Raul Gurzau (:RaulG)
	Comment 85 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/351c75ab74c9

	Chris Mills [:cmills]
	Comment 86 • 6 years ago

I've documented this by:

Added a line into the XFO reference article to explain it:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Frame-Options#Syntax

Adding a note to the Fx59 rel notes:
https://developer.mozilla.org/en-US/Firefox/Releases/59#Security

Updating the browser compat data repo with a note:
https://github.com/mdn/browser-compat-data/pull/867


Let me know if this reads OK. Thanks!

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 87 • 6 years ago

One note for https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Frame-Options#Syntax we currently implement ALLOW the same which has the same issues here.

I'm not sure if there is a plan to do this at the moment either.

Everything else is great though thanks!

	Chris Mills [:cmills]
	Comment 88 • 6 years ago

(In reply to Jonathan Kingston [:jkt] from comment #87)
> One note for
> https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Frame-
> Options#Syntax we currently implement ALLOW the same which has the same
> issues here.
> 
> I'm not sure if there is a plan to do this at the moment either.
> 
> Everything else is great though thanks!

Is this a mozilla proprietary value? I can only see ALLOW-FROM in the RFC definition. Where can I find out more details about this?

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 89 • 6 years ago

Sorry was tired, I meant ALLOW-FROM. It suffers from the same issue as SAMEORIGIN in that it doesn't traverse the frames checking each one.

	Chris Mills [:cmills]
	Comment 90 • 6 years ago

(In reply to Jonathan Kingston [:jkt] from comment #89)
> Sorry was tired, I meant ALLOW-FROM. It suffers from the same issue as
> SAMEORIGIN in that it doesn't traverse the frames checking each one.

Ah, OK. I've updated the SAME-ORIGIN description to mention this. I think this'll do for now; let me know if you discover or file a bug related to this, and we can reference that from there too.

Thanks :jkt!

	Ankit Butola
	Comment 91 • 6 years ago

Can anyone help me on the Iframe issue,
We have configured X frame option value as same origin and when we are trying to login through www.genre.com/clientlogin we are not getting the expected page that is www.genre.com/clientlogin/mygcr. 

the login box is uding Iframe.

please suggest any alternative for iframe if there is any.