Implement Allow-From syntax for X-Frame-Options

RESOLVED FIXED in mozilla18

Status

()

Core
Security
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: briansmith, Assigned: Phil Ames)

Tracking

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

Trunk
mozilla18
dev-doc-complete, sec-want
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:want])

Attachments

(1 attachment, 1 obsolete attachment)

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.
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.
Whiteboard: [sg:want]
Keywords: sec-want
(Assignee)

Comment 2

5 years ago
Created attachment 654751 [details] [diff] [review]
Patch.
(Assignee)

Comment 3

5 years ago
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 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.
Attachment #654751 - Flags: review?(bzbarsky)

Comment 5

5 years ago
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 :)
Assignee: nobody → philames
Status: NEW → ASSIGNED
(Assignee)

Comment 6

5 years ago
Thanks for looking!  I modeled the tests after the existing XFrameOptions tests, hence the screenshots.  I look forward to Boris' feedback.
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!
Attachment #654751 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 8

5 years ago
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.
Attachment #654751 - Attachment is obsolete: true
Attachment #655238 - Flags: review?(bzbarsky)
Attachment #655238 - Attachment is patch: true
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.
Attachment #655238 - Flags: review?(bzbarsky) → review+
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!
Flags: in-testsuite+
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/72755799451c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
I did a quick update of https://developer.mozilla.org/en-US/docs/The_X-FRAME-OPTIONS_response_header
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 836132
No longer blocks: 836132
Depends on: 836132
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.
(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.
You need to log in before you can comment on or make changes to this bug.