Last Comment Bug 690168 - Implement Allow-From syntax for X-Frame-Options
: Implement Allow-From syntax for X-Frame-Options
Status: RESOLVED FIXED
[sg:want]
: dev-doc-complete, sec-want
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla18
Assigned To: Phil Ames
:
Mentors:
Depends on: 836132
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-28 17:05 PDT by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2014-12-14 11:20 PST (History)
16 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch. (12.03 KB, patch)
2012-08-23 12:55 PDT, Phil Ames
bzbarsky: review+
Details | Diff | Splinter Review
X-Frame-Options Allow-From revision 2 (15.86 KB, patch)
2012-08-24 18:23 PDT, Phil Ames
bzbarsky: review+
Details | Diff | Splinter Review

Description Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-09-28 17:05:32 PDT
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] 2011-10-18 18:51:08 PDT
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 2012-08-23 12:55:59 PDT
Created attachment 654751 [details] [diff] [review]
Patch.
Comment 3 Phil Ames 2012-08-23 12:56:20 PDT
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] 2012-08-24 02:18:45 PDT
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 2012-08-24 10:44:58 PDT
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 2012-08-24 10:46:25 PDT
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 [:bz] 2012-08-24 14:28:34 PDT
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 2012-08-24 18:23:02 PDT
Created attachment 655238 [details] [diff] [review]
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 [:bz] 2012-08-24 18:30:10 PDT
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 [:bz] 2012-08-27 12:46:55 PDT
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] 2012-08-27 19:17:11 PDT
https://hg.mozilla.org/mozilla-central/rev/72755799451c
Comment 12 Matthew N. [:MattN] 2012-12-03 18:39:34 PST
I did a quick update of https://developer.mozilla.org/en-US/docs/The_X-FRAME-OPTIONS_response_header
Comment 13 O. Atsushi (Torisugari) 2014-12-12 04:52:26 PST
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?) 2014-12-14 11:20:28 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.