690168 - Implement Allow-From syntax for X-Frame-Options

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Our X-Frame-Options support is incomplete. We should honor the Allow-From syntax or, if not then document (blog about) why we think it is not worth doing.

Comment 1

[Daniel Veditz [:dveditz]]

This was an unintentional oversight. There was no spec and we were working from an earlier blog than the one where Eric Lawrence described the allow-from option.
http://blogs.msdn.com/b/ieinternals/archive/2010/03/30/combating-clickjacking-with-x-frame-options.aspx

The above blog says allow-from supports only a single origin. A more recent draft proposal to the IETF websec group (now expired) says allow-from takes a list (but also, the header is Frame-Options w/out the X- prefix). Since the author of the spec is from Microsoft we should test IE 9 and 10 and see whether they support a list or just a single item. Might have to support both headers differently.

Comment 2

[Phil Ames]

Attachment: Patch.

Comment 3

[Phil Ames]

I just attached a patch (with mochikit tests) which enables support for “Allow-From [uri]” notation in the X-Frame-Options header.  I compared the behavior of patched Firefox with IE9 with the below values (syntax:IE permitted/patched Firefox permitted/header-value) which was sent by a cross-origin site (the top origin was http://www.intra.net/)

Probably the most substantial difference is that IE doesn’t seem to regard the port number.  A similar patch for WebKit/Chrome which I will be submitting soon produces the same behavior as this patch to Firefox, though.

N/N/X-Frame-Options: Allow-From
N/N/X-Frame-Options: Allow-From xyz
N/N/X-Frame-Options: Allow-From www.intra.net
Y/Y/X-Frame-Options: Allow-From http://www.intra.net
Y/Y/X-Frame-Options: Allow-From http://www.intra.net/
Y/Y/X-Frame-Options: Allow-From http://www.intra.net:80/
Y/N/X-Frame-Options: Allow-From http://www.intra.net:81/
Y/N/X-Frame-Options: Allow-From http://www.intra.net:443/
Y/Y/X-Frame-Options: Allow-From http://www.intra.net/foo/bar/

Comment 4

[Ludovic Hirlimann [:Usul]]

Comment on attachment 654751 [details] [diff] [review]
Patch.

I think boris could review this, ain't sure. If you want your code to get into the product you need to flag reviewers.

Comment 5

[Ian Melven :imelven]

Thanks for working on this, Phil ! It makes sense to me to take the port number into account, like CSP does. 

I looked through the patch and nothing jumped out, but I'm not super familiar with this area of the codebase and haven't seen this style of mochitest using screenshots before, let's see what Boris says :)

Comment 6

[Phil Ames]

Thanks for looking!  I modeled the tests after the existing XFrameOptions tests, hence the screenshots.  I look forward to Boris' feedback.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 654751 [details] [diff] [review]
Patch.

>+++ b/docshell/base/nsDSURIContentListener.cpp
>+    NS_NAMED_LITERAL_STRING(allowFrom, "allow-from");
>+    const PRUint32 allowFromLen = allowFrom.Length();

This seems like it would allow a policy like "allow-fromhttp://foo" (without a space there).  Should this code be enforcing a space?  It's not clear to me whether there's a syntax spec for this somewhere...

>+    if (policy.Length() > allowFromLen) {
>+        nsReadingIterator<PRUnichar> begin, end;
>+        policy.BeginReading(begin);
>+        policy.BeginReading(end);
>+        end.advance(allowFromLen);
>+        isAllowFrom = CaseInsensitiveFindInReadable(allowFrom, begin, end);
>+    }

How about:

  static const char allowFrom[] = "allow-from";
  const PRUint32 allowFromLen = ArrayLength(allowFrom) - 1;
  isAllowFrom = StringHead(policy, allowFromLen).LowerCaseEqualsLiteral(allowFrom);

?  Again, modulo the question about space above.

>+            nsCOMPtr<nsIIOService> ios = do_GetIOService(&rv);
>+            if (!ios || NS_FAILED(rv))
>+              return false;
>+
>+            nsAutoString urlStr;
>+            urlStr = Substring(policy, allowFromLen);
>+            rv = ios->NewURI(NS_LossyConvertUTF16toASCII(urlStr),
>+                             nullptr, nullptr, getter_AddRefs(uri));

This should be:

  rv = NS_NewURI(getter_AddRefs(uri), Substring(policy, allowFromLen));
  if (NS_FAILED(rv))
    return false;

(in particular, avoiding the lossy conversion above is pretty important, because it's lossy).

The tests look fine, but seem to be browser-element only, so only run on FirefoxOS.  You probably want to also add tests for this in content/base/test/test_x-frame-options.html

r=me with those nits, and with the whitespace issue resolved, though I'd like to see the resulting diff.

Thank you for the patch!

Comment 8

[Phil Ames]

Attachment: X-Frame-Options Allow-From revision 2

Thanks for the feedback!  You were correct about the spacing; I believe this new patch addresses that.

The syntax is described here, and does indicate a space being required: http://tools.ietf.org/html/draft-ietf-websec-x-frame-options-00#section-2.1

Thanks also for pointing out the other test location.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 655238 [details] [diff] [review]
X-Frame-Options Allow-From revision 2

r=me

I pushed this to try as https://tbpl.mozilla.org/?tree=Try&rev=175b80d41203 to make sure all the tests pass and whatnot.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Tests pass.  I pushed this as http://hg.mozilla.org/integration/mozilla-inbound/rev/72755799451c

Thank you again for the patch and the excellent tests!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/72755799451c

Comment 12

[Matthew N. [:MattN]]

I did a quick update of https://developer.mozilla.org/en-US/docs/The_X-FRAME-OPTIONS_response_header

Comment 13

[O. Atsushi (Torisugari)]

I think this bug needs a bit more brush-ups:

* Catch html <meta> tag when HTTP header is absent ([parity-chrome]);
  e.g.
 <meta http-equiv="X-FRAME-OPTIONS" content="SAMEORIGIN">

* According to the dev-doc, this error needs a new error page along with a new net (or security) error code so as to do l18n besides hard-coded log messages added by bug 789776.

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to O. Atsushi (Torisugari) from comment #13)
> I think this bug needs a bit more brush-ups:
> 
> * Catch html <meta> tag when HTTP header is absent ([parity-chrome]);
>   e.g.
>  <meta http-equiv="X-FRAME-OPTIONS" content="SAMEORIGIN">
> 
> * According to the dev-doc, this error needs a new error page along with a
> new net (or security) error code so as to do l18n besides hard-coded log
> messages added by bug 789776.

Please file two separate bugs for those issues. Otherwise, they'll likely not get done.