Closed Bug 836132 Opened 8 years ago Closed 8 years ago

'X-Frame-Options: ALLOW-FROM' isn't in accordance with IETF-draft

Categories

(Core :: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: yosuke.hasegawa, Assigned: philames)

References

Details

Attachments

(2 files, 4 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.0; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130123083802

Steps to reproduce:

http://tools.ietf.org/html/draft-ietf-websec-x-frame-options-01 shows that:
--
   The RFC 822 [RFC0822] EBNF of the X-Frame-Options header is:

         X-Frame-Options = "Frame-Options" ":" "DENY"/ "SAMEORIGIN" /
                                 ("ALLOW-FROM" ":" URI)
--

however, implementation of Firefox looks not correctly for "ALLOW-FROM" directive.
IETF-draft shows colon after the word "ALLOW-FROM", but Firefox accepts like as bellow, without colon.
--
X-Frame-Options: ALLOW-FROM http://example.com
--



Actual results:

Here's the result of X-Frame-Options with ALLOW-FROM parameter on Firefox and IE when opening a page on http://example.jp and reading iframe with X-Frame-Options.
IE isn't in accordance with IETF-draft but Firefox is looser than IE.

'x' is blocked, 'o' is displayed.

    value of X-Frame-Options        | IE8 IE9 IE10    Firefox
------------------------------------+--------------------------
    ALLOW-FROM:example.jp             x   x   x       o
    ALLOW-FROM: example.jp            x   x   x       o
    ALLOW-FROM example.jp             x   x   x       x
    ALLOW-FROM:http://example.jp      x   x   x       o
    ALLOW-FROM: http://example.jp     x   x   x       o
    ALLOW-FROM http://example.jp      o   o   o       o
    ALLOW-FROM:example.com            x   x   x       o
    ALLOW-FROM: example.com           x   x   x       o
    ALLOW-FROM example.com            x   x   x       x
    ALLOW-FROM:http://example.com     x   x   x       o
    ALLOW-FROM: http://example.com    x   x   x       o
    ALLOW-FROM http://example.com     x   x   x       x
OS: Windows Vista → All
Component: General → Document Navigation
Status: UNCONFIRMED → NEW
Component: Document Navigation → Security
Ever confirmed: true
Flags: needinfo?(sstamm)
Depends on: 690168
Based on the results table, it seems IE8-10 are violating the spec too, right?  The only time IE allows the load is for an X-Frame-Options value _without_ the colon.  Also, it doesn't look like Firefox is consistent in your tests (allowing some with colons and some without colons).  So I'm a bit confused and would like to see the test cases.

Do you have a test site or script I could use to reproduce this?  I could write one, but will take a while to get around to this.

Phil: the patch in bug 690168 explicitly avoids looking for a colon, should we update it to look for such syntax?
Blocks: 690168
No longer depends on: 690168
Flags: needinfo?(sstamm) → needinfo?(philames)
Yes, IE8-10 are also violating the spec. I've already reported it to Microsoft.
Here's a test page: http://utf-8.jp/cgu-bin/xfo.cgi
mistake. test page is here: http://utf-8.jp/cgi-bin/xfo.cgi
I wonder if it might be easier to fix spec than code 
http://www.ietf.org/mail-archive/web/websec/current/msg01510.html
This seems to be fixed in latest Firefox?
I see some false positives (and negatives if you don't require the colon); same as the original report.  Here's what I'm getting in Win7 Nightly:

(On site http://utf-8.jp/cgi-bin/xfo.cgi):

1  (allowed) X-Frame-Options: ALLOW-FROM:utf-8.jp
2  (allowed) X-Frame-Options: ALLOW-FROM: utf-8.jp
3  [blocked] X-Frame-Options: ALLOW-FROM utf-8.jp
4  (allowed) X-Frame-Options: ALLOW-FROM:http://utf-8.jp
5  (allowed) X-Frame-Options: ALLOW-FROM: http://utf-8.jp
6  (allowed) X-Frame-Options: ALLOW-FROM http://utf-8.jp
7  (allowed) X-Frame-Options: ALLOW-FROM:example.com
8  (allowed) X-Frame-Options: ALLOW-FROM: example.com
9  [blocked] X-Frame-Options: ALLOW-FROM example.com
10 (allowed) X-Frame-Options: ALLOW-FROM:http://example.com
11 (allowed) X-Frame-Options: ALLOW-FROM: http://example.com
12 [blocked] X-Frame-Options: ALLOW-FROM http://example.com

Looks to me like items 6,7-8,10-11 are all false negatives (and should be blocked).  The mochitests for bug 690168 test number 6 and 12.  We should probably have tests for the rest too.

Also, probably a new and different bug, but we should make the "blocked" content frame clearer.  A white frame doesn't say anything about why it didn't load.  (Spec suggests this.)
Attached patch some tests (obsolete) — Splinter Review
here's a first whack at implementing the provided tests as a mochitest.
Please see http://lists.w3.org/Archives/Public/public-webappsec/2013Feb/0037.html

"Where X-Frame-Options is currently implemented, and as specified at the IETF, the ALLOW-FROM directive allows only a single value.

As part of moving future, normative work on frame-options in to the UI Security draft, we have preserved this syntax restriction, (with the addition of allowing “self”) but want to revisit it."
Sorry for the delay in chiming in here. The implementation was driven by skimming this blog post: http://blogs.msdn.com/b/ieinternals/archive/2010/03/30/combating-clickjacking-with-x-frame-options.aspx (which lacks the colon).

I'd be more keen to suggest removing the colon as a requirement, but that's probably laziness speaking and would reduce consistency between FF/IE so it's probably simpler just to rework the patch to also accept a colon before the origin is specified.
Flags: needinfo?(philames)
If I read things correctly, IE never accepts a colon. In which case the spec and FF should be aligned, no?
Firefox still allows loads for some ALLOW-FROM policies that should prohibit the load; either way there are bugs to fix in our implementation.

I agree with Phil that we might just want to optionally accept the colon (or not), but either way we need to fix some of the errors in the tests I mentioned in comment 6.
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #11)
> Firefox still allows loads for some ALLOW-FROM policies that should prohibit
> the load; either way there are bugs to fix in our implementation.

Please no optional syntax elements unless they are required for compatibility (== optional in IE as well).
So, right now, the check looks for a leading substring of:

static const char allowFrom[] = "allow-from "

to determine whether it's an allow-from directive or not.  Serving:

X-Frame-Options: allow-from: [...]

fails this check, and therefore this fails open (i.e. it's not deny, sameorigin, or "allow-from ").

I guess the question is whether or not a colon is really required? IE doesn't seem to support a colon, and as Julian rightly points out fails closed on encountering ALLOW-FROM<anything other than a space followed by a URI>.  For the reason above, Firefox fails open.  If we change the declaration to:

static const char allowFrom[] = "allow-from";

then the Substring()-extracted URI including a colon would later fail to parse in NS_NewURI(), and Firefox would fail closed/stop the load - but directives of the form ALLOW-FROMhttp://example.com would be accepted

If we adjust the Substring() call to account for a separator, then you end up in the state where you accept "ALLOW-FROM:http://example.com" but not "ALLOW-FROM : example.com" which seems dumb.  Or, it would also be possible to add a check that "allow-from" is immediately followed by whitespace.

I have no strong opinion on any of these approaches which would bring behavior closer to IE, they're all very trivial - but it would probably be most helpful to know if IE is going to optionally support colons or if the spec is "wrong."

Addressing the parsing issue I think will resolve most/all of Sid's concerns around some of the conflicting test cases (note that many of the false negatives bump into the colon-instead-of-space-in-header scenario and I think 6 is actually correct - those are same-origin).
The spec IMHO is wrong (see related mailing list discussion). As far as I understand both IE's behavior and Microsoft's "documentation", there never was a colon.
I think this bug brings out a more important problem: the current XFO implementation fails open instead of fails close (as Phil pointed out). IMHO, regardless of what we decide on supporting the colon, the code should be changed to fail close. 

Instead of changing the parser, I suggest changing the code so that the default, on seeing a XFO header, is blocking any framing. After that, if we see a correct value for it, we unblock appropriately.

----
I also vote for not adding support for the :. The whole point of the `spec', if I understand it right, was to document what browsers were already doing. By definition, we change the implementation because of the spec. IIRC, when Phil implemented support, there was no such spec.
I guess there's the existential question of, "if you specify allow-from without a URI, have you really specified an x-frame-options header?" but I agree with your statement.

Pretty sure this does the trick, but not sure who to r+ on it.
Note that http://tools.ietf.org/html/draft-ietf-websec-x-frame-options-02 has removed the ":" from the syntax.
Hardware: x86 → All
...it would also be good to verify that the spec is correct in that the keywords are matched case-insensitively.
From http://tools.ietf.org/html/draft-ietf-websec-x-frame-options-02#section-2.2 :

"The values are specified as ABNF strings, and therefore are case-insensitive"

and the relevant methods in the code use "[header-value].LowerCaseEqualsLiteral(...)" so they match case-insensitively.

One note, I think the spec is incorrect in stating that FF/Chrome support colons in 2.2.2, Chrome has no support at all for Allow-From (just my pending patch which has the same behavior as the one that led to this bug), and obviously colons are not supported here either (and the intent seems to be to not permit them).
Comment on attachment 713576 [details] [diff] [review]
Stricter handling of X-F-O allow-from

Flagging jst from bug 475530 who seemed to review similar functionality.
Attachment #713576 - Flags: review?(jst)
Comment on attachment 713576 [details] [diff] [review]
Stricter handling of X-F-O allow-from

Moving r? to bzbarsky since he reviewed the original patch, and has been tagged as reviewer in https://bugzilla.mozilla.org/show_bug.cgi?id=725490
Attachment #713576 - Flags: review?(jst) → review?(bzbarsky)
Comment on attachment 713576 [details] [diff] [review]
Stricter handling of X-F-O allow-from

>+        if (Substring(policy, allowFromLen, 1).FindChar(' ') == -1 &&
>+            Substring(policy, allowFromLen, 1).FindChar('\t') == -1) {

Is this trying to be:

  if (policy.Length() == allowFromLen ||
      (policy[allowFromLen] != ' ' &&
       policy[allowFromLen] != '\t')) {

?  Or is it trying to do something else?  If so, it should probably document what the goal is....

>+	  return false;
>+	}

These lines are both mis-indented.
Attachment #713576 - Flags: review?(bzbarsky) → review-
Addresses bzbarsky's comments
Attachment #713576 - Attachment is obsolete: true
Phil, Is that updated patch ready for review?
Flags: needinfo?(philames)
Comment on attachment 727673 [details] [diff] [review]
Stricter handling of X-F-O allow-from v2

Yes, sorry for not flagging it.
Attachment #727673 - Flags: review?(bzbarsky)
Flags: needinfo?(philames)
Comment on attachment 727673 [details] [diff] [review]
Stricter handling of X-F-O allow-from v2

r=me
Attachment #727673 - Flags: review?(bzbarsky) → review+
Comment on attachment 727673 [details] [diff] [review]
Stricter handling of X-F-O allow-from v2

Sid, could you give this a once-over too?  Should we be passing your tests attached to this bug with this patch, or are they not related?  I can't quite tell from the discussion anymore.  :(
Attachment #727673 - Flags: review?(sstamm)
Nope, my tests as posted here shouldn't all pass... we're supposed to reject the syntax with the colon according to Julian (comment 17).  

So the following tests that were "allowed" before should be "denied" instead:

+    "afa1": "ALLOW-FROM:mochi.test:8888",
+    "afa2": "ALLOW-FROM: mochi.test:8888",
+    "afa4": "ALLOW-FROM:http://mochi.test:8888",
+    "afa5": "ALLOW-FROM: http://mochi.test:8888",

-Sid
OK.  Are you going to adapt the tests as needed, or should someone else be doing it?
Flags: needinfo?(sstamm)
Comment on attachment 727673 [details] [diff] [review]
Stricter handling of X-F-O allow-from v2

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

::: docshell/base/nsDSURIContentListener.cpp
@@ +384,2 @@
>          rv = NS_NewURI(getter_AddRefs(uri),
>                         Substring(policy, allowFromLen));

Will this URI creation chop off all leading whitespace?  (I'm imagining a situation where someone puts in 3 spaces between allow-from and the host for some strange reason).
Flags: needinfo?(sstamm)
> Will this URI creation chop off all leading whitespace?

Yes.
Attached patch updated tests (obsolete) — Splinter Review
Here are tests updated per the discussion.

A fix should be passing these tests, and I verfied the current patch on this bug does.  bz: can you just double-check these tests are sane?

One thing to note... these tests assume the following is invalid:
  x-frame-options: ALLOW-FROM mochi.test:8888

The URI spec, RFC 3986, requires a scheme.  The Frame-Options draft cites it, so I'm assuming we want to reject this shorthand.
Attachment #712731 - Attachment is obsolete: true
Attachment #734797 - Flags: review?(bzbarsky)
Attachment #727673 - Flags: review?(sstamm) → review+
Comment on attachment 734797 [details] [diff] [review]
updated tests

r=me
Attachment #734797 - Flags: review?(bzbarsky) → review+
Sid, can you drive this in, please?
Flags: needinfo?(sstamm)
Yeah, starting with this:
https://tbpl.mozilla.org/?tree=Try&rev=f16e328956c8
Flags: needinfo?(sstamm)
Hm, lets try that again.  I apparently suck at try chooser syntax.
https://tbpl.mozilla.org/?tree=Try&rev=d81eb0e9d8ea
One small suggestion, sorry for not thinking of this before you started the try run - should there also be tests along the lines:

"afd12": "ALLOW-FROM"
"afd13": "ALLOW-FROM "
"afd14": "ALLOW-FROM:"

to ensure that this continues to fail closed?
Good idea, Phil. Can you add them to the tests patch and try the tests locally to see what happens?  I don't think we need to re-run on try just to add these tests.
Attached patch Additional test cases. (obsolete) — Splinter Review
As expected, they pass:

 0:05.81 TEST-PASS | unknown test url | test allow-from-deny-12
 0:05.81 TEST-PASS | unknown test url | test allow-from-deny-13
 0:05.81 TEST-PASS | unknown test url | test allow-from-deny-14

I attached a manually hacked up version of your patch (s/11/14/ in the for loops, add the 3 extra cases to testHeaders).
Attachment #735788 - Flags: review?(sstamm)
Comment on attachment 735788 [details] [diff] [review]
Additional test cases.

Note: This is a replacement patch for the other tests (not a diff to apply on top of it).  Carrying over r=bz since the update was trivial.
Attachment #735788 - Attachment is patch: true
Attachment #735788 - Flags: review?(sstamm) → review+
Attachment #734797 - Attachment is obsolete: true
Previous tests patch was malformed and didn't apply.  Fixed.
Attachment #735788 - Attachment is obsolete: true
Attachment #735833 - Flags: review+
Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6a95781f51c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2af04ff7494

Thanks for bearing with us, Phil.
Assignee: nobody → philames
Flags: in-testsuite+
Target Milestone: --- → mozilla23
No problem.  Sorry the original feature patch wasn't more robust.
FWIW Veracode recently did a survey of "security header" use and their data sets are available
http://www.veracode.com/blog/2013/03/security-headers-on-the-top-1000000-websites-march-2013-report/

In the March 2013 survey they encountered X-Frame-Options 19500 times each on Firefox and Chrome (22 more when using Chrome on the same sites), of which the vast majority (17600) were sameorigin. Only 15 per browser were allow-from (compared to ~200 containing the invalid "GOFORIT") so we're not going to be breaking real sites no matter what we do. An additional eight simply said "ALLOW" and seven more had "Allow From <site>" (no hyphen)

The valid ones:

http://tune-forex.com/
   ALLOW-FROM *
https://www.tune-forex.com/
   ALLOW-FROM *
http://slevomol.cz/
   ALLOW-FROM http://clickheat.slevomol.cz
https://slevomol.cz/
   ALLOW-FROM http://clickheat.slevomol.cz
http://rally-base.com/
   allow-from http://www.fiaerc.com/
https://rally-base.com/
   allow-from http://www.fiaerc.com/
http://www.ucb.co.uk/
   Allow-FROM http://www.ucb.co.uk
http://bka.de/
   ALLOW-FROM https://www.facebook.com/bka.wiesbaden
http://www.bka.de/DE/Home/homepage__node.html?__nnn=true
   ALLOW-FROM https://www.facebook.com/bka.wiesbaden
http://www.polizei.de/Polizei/DE/Home/homepage__node.html?__nnn=true
   ALLOW-FROM https://www.facebook.com/bka.wiesbaden
https://polizei.de/
   ALLOW-FROM https://www.facebook.com/bka.wiesbaden
http://multipluscard.hr/
   ALLOW-FROM https://www.multipluscard.hr
http://www.kupibilet.ru/
   ALLOW-FROM metrika.yandex.ru
http://dotz.com.br/preHome.aspx?ReturnUrl=%2F
   ALLOW-FROM SAMEDOMAIN, www.facebook.com/programadotz
https://dotz.com.br/preHome.aspx?ReturnUrl=%2F
   ALLOW-FROM SAMEDOMAIN, www.facebook.com/programadotz
You need to log in before you can comment on or make changes to this bug.