X-Frame-Options: SAMEORIGIN largely useless as implemented

RESOLVED FIXED in Firefox 59

Status

()

P3
normal
RESOLVED FIXED
7 years ago
10 months ago

People

(Reporter: lcamtuf, Assigned: jkt)

Tracking

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

10 Branch
mozilla59
dev-doc-complete, sec-want
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

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

Attachments

(2 attachments, 8 obsolete attachments)

(Reporter)

Description

7 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
Posted patch check ancestors for XFO - wip (obsolete) — Splinter Review
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

7 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

7 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

7 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

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

Not from me.

Comment 9

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

None from me either.
What are the steps to move forward with changing XFO SAMEORIGIN behavior?
(Reporter)

Comment 11

7 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

7 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.
---
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?
(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.
(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

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

6 years ago
Blocks: 846978

Updated

6 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.
(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 :)
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

6 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

6 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.
Attachment #720345 - Attachment is obsolete: true
Attachment #721326 - Attachment is obsolete: true
I'd like to see some mochitests for this change, please.
Also, it looks like you can carry over bz's r+ on the latest patch :)
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
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+
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)
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.
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-
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]
See Also: → bug 1364859

Comment 55

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

Comment 56

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

Comment 57

a year ago
Ping. Chrome's shipping this. It would be lovely if y'all would do the same! :)
Flags: needinfo?(lcamtuf)
(Assignee)

Comment 58

a year 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
Assignee: nobody → jkt
Flags: needinfo?(mkwst)
(Assignee)

Comment 59

a year ago
Posted patch bug725490.patch (obsolete) — Splinter Review
(Assignee)

Comment 60

a year ago
Posted patch bug725490.patch (obsolete) — Splinter Review
Attachment #8925084 - Attachment is obsolete: true

Comment 61

a year 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.
Flags: needinfo?(mkwst)
(Assignee)

Comment 62

a year 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?
Flags: needinfo?(dveditz)

Comment 63

a year 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.
Attachment #595625 - Attachment is obsolete: true
Flags: needinfo?(dveditz)
Attachment #760227 - Attachment is obsolete: true
Attachment #8925485 - Attachment is obsolete: true
(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.
(Assignee)

Updated

a year ago
Attachment #8927602 - Flags: review?(bugs)
(Assignee)

Comment 66

a year 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.
Flags: needinfo?(bugs)
If there is a wpt test for this, that should be enough, assuming that test actually cover all the needed cases.
Flags: needinfo?(bugs)

Comment 68

a year 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?
Attachment #8927602 - Flags: review?(bugs) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 71

a year 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
Attachment #8927602 - Flags: review?(bugs) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 72

a year 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
Keywords: checkin-needed

Comment 73

a year 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
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 77

a year 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.
Flags: needinfo?(bugs)
r+ for what part? Just add to the review queue.
Flags: needinfo?(bugs)
(Assignee)

Comment 79

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

Updated

a year ago
Flags: needinfo?(bugs)
On can always ask review using bugzilla ;) But ok, looking.
Flags: needinfo?(bugs)
s/On/One/

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

Comment 83

a year ago
Yeah I should have done that hah. Thanks!

Comment 84

a year 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

Comment 85

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/351c75ab74c9
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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!
Flags: needinfo?(jkt)
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 87

a year 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!
Flags: needinfo?(jkt)
(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?
(Assignee)

Comment 89

a year 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.
(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!
Whiteboard: also applies to ALLOW-FROM, [domsecurity-backlog1] → also applies to ALLOW-FROM, [domsecurity-backlog1][adv-main59-]

Comment 91

11 months 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.
You need to log in before you can comment on or make changes to this bug.