397878 - Use a Referer-Root header for all cross-site XMLHttpRequest requests

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Anne (:annevk)]

Use a Rererer-Root (sic) header for all cross-site XMLHttpRequest requests as per http://dev.w3.org/2006/waf/access-control/Overview.html to indicate where the request is coming from. This header should always persist and is designed to not leak any information hidden in the URI (apart from the scheme, ihost, port components).

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Surya, could you have a look at this?

Comment 2

[Surya Ismail]

Attachment: Adds internationalized label and default port support, adds referer-root

Comment 3

[Boris Zbarsky [:bzbarsky]]

A few comments based on skimming the patch.  This is not a real review (in particular, I didn't look at any of the ACE details).

1)  Why is ISO-88591-1 preferred?  Is there a spec reason?  If so, please cite
    it in the comments?  If not, does the spec say anything about how non-ASCII
    encodings are to be indicated in this header?  If it's the standard MIME
    way, we have utilities for that which this code should use.
2)  Please don't hardcode default ports.  Ask necko for them instead
    (getProtocolHandler from the IO service, and ask it for the default port).
    Perhaps that should be factored out into an nsNetUtil method, since we'll
    have at least 3 callers (here, NS_GetRealPort, and the script security
    manager).
3)  mPrincipal might be null, or might have a null URI, no?  I thought Jonas
    had a bug on that already....
4)  The patch adds Windows newlines to a bunch of files.  Don't do that, please.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, the ACE stuff could use review from some intl or networking folks.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hey Surya, thanks for doing this! Are you writing up a new patch? There is already code that will convert a string ToASCII algorithm, see
http://lxr.mozilla.org/mozilla/source/netwerk/base/src/nsStandardURL.cpp#1002
for one place where we call it.

Comment 6

[Surya Ismail]

Reply to Comment #4
1)  The spec doesn't say. The value comes from a test ^header^ file.
    The file is in UTF-8 but by the time it gets to the code, it's an
    octet stream.  Is this a quirk of the httpserver?
    This document (http://www.w3.org/International/O-HTTP-charset)
    says "HTTP 1.1 says that the default charset is ISO-8859-1. But there
    are too many unlabeled documents in other encodings, so browsers use
    the reader's preferred encoding when there is no explicit charset
    parameter."
    Since the reader's choice failed, it seemed to make sense to try the
    default.
2)  To use the protocol handlers, you need a full URI, correct?  In this
    case, a URI is not guaranteed, just a scheme. Is there a better way
    to get a port from a scheme?
3)  With bug386823 fixed, mPrincipal should work. Added some asserts.
4)  Very sorry about the newlines. Will be more careful in future.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> The spec doesn't say.

Sounds like a bug in the spec.  Have you raised it with the W3C?

> Is this a quirk of the httpserver?

Sounds like it might be, yes.  Bug filed?

> 2)  To use the protocol handlers, you need a full URI, correct?

No.  All you need is a scheme.  See the code I mentioned in comment 3 that does exactly what you want here.

Comment 8

[Jeff Walden [:Waldo]]

The spec's definition of "referrer root URI" says that it must include a port number even if it's the default, which your tests don't hit because they only use port 8000.

I'd prefer if you used a new subdomain of example.org/com/net like ält.example.org or something, so as not to expand into the usable domain namespace.  If you need IDN at the second level, the new example.test domains set up by ICANN would also do the task, given that "they will be removed unconditionally and permanently from the Domain Name System at the end of evaluation" per <http://idn.icann.org/>.

(In reply to comment #6)
> Reply to Comment #4
> 1)  The spec doesn't say. The value comes from a test ^header^ file.
>     The file is in UTF-8 but by the time it gets to the code, it's an
>     octet stream.  Is this a quirk of the httpserver?

Comment 7 says that bz seems to understand the stated problem here, but frankly I can't make head nor tail of it, even looking at the tests in the patch.  Can you explain exactly what the problem is (preferably without my having to understand the cross-site XHR access mechanism) so I can understand the potential problem that exists here?

Comment 9

[Surya Ismail]

Attachment: Adds internationalized label, default port and referer-root support

Comment 10

[Surya Ismail]

Comment 7/8 is my mistake in part. After more reading, spec says they are MIME headers (have changed the tests). What happens is I create a ^headers^ file in utf-8 encoding. Then later, I read the header and it is in ISO8859-1 encoding. So ält.example.org becomes ält.example.org. Existing code seems to expect utf-16 or utf-8. If this isn't correct behaviour, I'll write a test case and submit a bug.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Please don't copy/paste the default port function like that.  Put it in nsNetUtil.h, and use it in those other callers I mentioned?

Comment 12

[Jeff Walden [:Waldo]]

(In reply to comment #10)
> After more reading, spec says they are MIME headers (have changed the tests).

Erm, doesn't the second note in 4.2 say that only Punycode versions that don't need MIME encoding are valid (also 4.1 first green line of text)?  I think the Unicode concern only applies to Referer-Root as it doesn't yet specify the ToASCII restriction.

Comment 13

[Surya Ismail]

Attachment: Referer-root, IDN labels, default port

Moved DefaultPort to NetUtils, changed HTTP headers in tests to be in ToAscii

Comment 14

[Surya Ismail]

Comment 12, you're right. I'm going to go memorize the spec now.

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 293710 [details] [diff] [review]
Referer-root, IDN labels, default port

Jonas asked me to offload this review, hope you don't mind. Looks pretty good, some small nits mostly.

>Index: content/base/src/nsCrossSiteListenerProxy.cpp
> ...
>+static void
>+LabelToACE(const nsACString &idn, nsACString &result)

Failure is not impossible here so assertions shouldn't be used - how about warnings? Make sure to null check idnSrv before you deref it. You could always make this function return a PRBool to indicate success.

>+static PRBool
>+ACEEquals(const nsACString &aPattern, const nsCString &domain)

Make domain an nsACString and use the Data() method rather than get().

>+    // Convert subdomain patern to ACE

This block is indented incorrectly.

>+    while (EatSubdomainChar(iter, end)) {}

Since this is the only use of the EatSubdomainChar function I'd put the loop inside of the function to get rid of the awkward empty while here, maybe rename to EatSubdomainChars?

>Index: content/base/src/nsXMLHttpRequest.cpp
> ...
>+  // Waiting for bug 386823 but, mPrincipal should be set during OpenRequest
>+  NS_ASSERTION(mPrincipal, "No principal set during OpenRequest");
>+  mPrincipal->GetURI(getter_AddRefs(requestingURI));
>+  NS_ASSERTION(requestingURI, "Principal has null URI");

Since that bug hasn't been fixed yet I think you should null check for the time being. Add a comment to change them to assertions once that bug lands?

>+  mPrincipal->GetURI(getter_AddRefs(requestingURI));
>+  NS_ASSERTION(requestingURI, "Principal has null URI");

I wonder if this code could simply use nsIURI's prePath attribute if no username/password is set? Then you could avoid all this manual concatenation.

>+  //nsCString uri;

Remove that.

>+  PRInt32 port;
>+  requestingURI->GetPort(&port);
>+  if (port == -1)
>+    port = NS_GetDefaultPort(scheme.get());

Want to bail if port is still -1?

>+  uri.Append(NS_LITERAL_CSTRING("://"));

Use 'AppendLiteral("://")', same below.

>Index: content/base/test/Makefile.in
> ...
>+		file_CrossSiteXHR2_pass3_redirect.xml \
>+        test_CrossSiteXHR3.html \

You're mixing tabs and spaces here... Spaces are more correct but tabs are used above, so I guess I'd keep using tabs.

>Index: netwerk/base/public/nsNetUtil.h
> ...
> }
> 
>+
>+/**
>+ * This function is a helper function to get a scheme's default port.

You added an extra line above this comment.

>+  nsCOMPtr<nsIIOService> grip;
>+  rv = net_EnsureIOService(&ioService, grip);

No point setting rv here if you're not going to check it.

Also, please remove the code from the ScriptSecurityManager and make it use your new function as bz suggested, http://mxr.mozilla.org/seamonkey/source/caps/src/nsScriptSecurityManager.cpp#342

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Make domain an nsACString and use the Data() method rather than get().

I'm not sure that's necessarily a good idea, fwiw.  If it's not an API method, or expected to be called directly by API methods, using nsCString is probably better in new code.

> I wonder if this code could simply use nsIURI's prePath attribute if no
> username/password is set? Then you could avoid all this manual concatenation.

This definitely needs to happen.  For one thing, for many schemes the "://" part is wrong.  The right course of action here, perhaps, is to clone the URI, setUserPass() to the empty string, and then get the prepath.

That will keep you from sending things like "data://:-1" or whatnot...

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

> clone the URI, setUserPass() to the empty string, and then get the prepath.

That sounds great.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Usernames and passwords should never be set on uris loading using access-control. We are already checking that in nsXMLHttpRequest iirc. The reason for this is that it would allow distributed brute-force attacks using every browser that uses your site.

Comment 19

[Surya Ismail]

Attachment: Corrections

Ben, thanks for the review. I have a question about nsIURI's prePath (comment 16 and 17). It has to be converted to ascii. The code is already in nsCrossSiteListenerProxy.cpp, LabelToACE. Should I put it in a utility function somewhere?

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You could put it in nsContentUtils I think

Comment 21

[Boris Zbarsky [:bzbarsky]]

Wouldn't nsNetUtil make more sense?

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually yeah, given that that function is so small that seems like the right place.

Comment 23

[Surya Ismail]

Attachment: More corrections

Comment 24

[Anne (:annevk)]

There are some other things that need to be resolved somehow

1. Using OPTIONS for the preflight request

2. Caching the result of the preflight request

3. The syntax of the Access-Control HTTP header

For 3 it has been suggested to have this instead:

Access-Control: allow="*" deny="example.org" method="POST PUT"

As the comma-separated syntax suggested for the method part now is problematic. Care to follow up on that on [e-mail]? Thanks!

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 298477 [details] [diff] [review]
More corrections

Surya, it looks like this patch is incomplete (missing nsNetUtil.h changes including the implementation of NS_StringToACE) so I'm going to mark it r-. Here are some small comments on all the rest:

>+EatSubdomainChars(nsACString::const_iterator& aIter,

Maybe assert that aIter <= aEnd?

>+  while (aIter != aEnd) {
>+    if (aIter != aEnd && *aIter != '.' && *aIter != ':' &&

The 'aIter != aEnd' test in the if statement is now unnecessary.

>+ACEEquals(const nsACString &aPattern, const nsCString &domain)
>+{
>+  if (aPattern.LowerCaseEqualsASCII(domain.Data(), domain.Length()))

If you keep nsCString for domain's type then I'd revert that to domain.get() there and below.

>Index: content/base/src/nsXMLHttpRequest.cpp
> ...
>+  // Change NS_ENSURE to NS_ASSERION when bug is fixed'

Nit: typo, NS_ASSERTION

>+  principalURI->Clone(getter_AddRefs(requestingURI));

NS_ENSURE_STATE(requestingURI) there

>+    uri.Append(":");

Use AppendChar(':');

Comment 26

[Surya Ismail]

Attachment: More corrections

Comment 27

[Surya Ismail]

Smaller than comparison doesn't work for type nsACString::const_iterator. Should I change the type to const char *?
> Maybe assert that aIter <= aEnd?

I can't find AppendChar in the string util libraries. Sorry, where is it? 
> Use AppendChar(':');

Comment 28

[Boris Zbarsky [:bzbarsky]]

There's Append(':'), which is what you probably want here, right?

As for the assertion, there is a Distance() function, iirc, that you can use to assert the relative positions...

Comment 29

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 298726 [details] [diff] [review]
More corrections

Sorry, bz is correct, I meant Append(':'). And for the assertion the simplest test might be to do this:

  aIter.get() <= aEnd.get()

Finally:

> +static PRBool
> +NS_StringToACE(const nsACString &idn, nsACString &result)

Any reason why you didn't inline that like the rest of the functions in that file?

> +  if (!NS_SUCCEEDED(rv))

That should be 'if (NS_FAILED(rv))'.

r=me with those changes, thanks for doing this great work!

Comment 30

[Boris Zbarsky [:bzbarsky]]

The nsNetUtil function needs to be inline, yes.

Comment 31

[Surya Ismail]

Attachment: More corrections

Comment 32

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 299074 [details] [diff] [review]
More corrections

Awesome, thanks!

Comment 33

[Surya Ismail]

Ben, thanks a lot for the review and encouragement. Do I need super-review for this?

Comment 34

[Jeff Walden [:Waldo]]

Yes, hence the sr request on the patch.  In general, things need sr unless there's an explicit exemption or the author of the patch is a peer in the code the patch touches.  The areas I know that have exceptions are currently js, browser, toolkit, and xpcom.

Comment 35

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes you do. I'll try to do it monday

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 299074 [details] [diff] [review]
More corrections

>+EatSubdomainChars(nsACString::const_iterator& aIter,
>+                  nsACString::const_iterator& aEnd)
>+{
>+  NS_ASSERTION(aIter.get() <= aEnd.get(), "EatSubdomainChars failed");
>+
>+  while (aIter != aEnd) {
>+    if (*aIter != '.' && *aIter != ':' &&
>+        *aIter != ' ' && *aIter != '\t' && *aIter != '\r' &&
>+        *aIter != '\n' && *aIter != '\f' && *aIter != '\v') {
>+      ++aIter;
>+    }
>+    else
>+      return;
>+  }
>+}

This doesn't match what the spec says. You should follow rfc3490 (page 10)

>Index: content/base/src/nsXMLHttpRequest.cpp
>+  // Work out the requesting URI
>+  // Waiting for bug 386823 but, mPrincipal should be set during OpenRequest
>+  // Change NS_ENSURE to NS_ASSERTION when bug is fixed
>+  NS_ENSURE_STATE(mPrincipal);
>+  nsCOMPtr<nsIURI> principalURI;
>+  mPrincipal->GetURI(getter_AddRefs(principalURI));
>+  NS_ENSURE_STATE(principalURI);
>+  nsCOMPtr<nsIURI> requestingURI;
>+  principalURI->Clone(getter_AddRefs(requestingURI));
>+  NS_ENSURE_STATE(requestingURI);
>+  
>+  // Remove userPass, just in case
>+  requestingURI->SetUserPass(EmptyCString());
>+  // Convert to ascii
>+  nsCString prepath, uri;
>+  requestingURI->GetPrePath(prepath);
>+  if (!NS_StringToACE(prepath, uri))
>+    return NS_ERROR_DOM_BAD_URI;
>+  // If needed, append the default port
>+  PRInt32 port;
>+  requestingURI->GetPort(&port);
>+  if (port == -1) {
>+    nsCString scheme;
>+    requestingURI->GetScheme(scheme);
>+    port = NS_GetDefaultPort(scheme.get());
>+    if (port == -1)
>+      return NS_ERROR_DOM_BAD_URI;
>+    uri.Append(":");
>+    uri.AppendInt(port);
>+  }

Seems like more work to clone and then modify the uri, than to just pick out the parts you need and concatenate them. You also need to add this header to the request that happens before a POST request.

Looks awesome otherwise!! Great tests too!

I'll attach a patch that addresses this just because I want to land tonight. Hope that's ok.

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Fix review comments

This patch also fixes the way wildcards work to match the spec, sorry I wasn't clear on that that needed to be done initially. Just the last couple of lines in VerifyAndMatchDomainPattern needed changing.

Oh, final review comment I fixed; please always terminate files with a newline. Partially some tools freak out otherwise, partially patches just look so ugly without it.

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Fix review comments v1.5

And with that final comment I do the same thing at the end of nsCrossSiteListenerProxy :)

Also merged to tip.

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 300329 [details] [diff] [review]
Fix review comments v1.5

With that r=bent sr=sicking

Comment 40

[Boris Zbarsky [:bzbarsky]]

> Seems like more work to clone and then modify the uri, than to just pick out
> the parts you need and concatenate them.

That's what the patch _used_ to do.  It's wrong.  See comment 16.  Please put the clone-and-replace back!

Comment 41

[Anne (:annevk)]

Comment 16 is also wrong in suggesting that referer root URIs with no host-based authority get a value other than the literal string null.

Comment 42

[Boris Zbarsky [:bzbarsky]]

If the spec explicitly says that (and I trust there will be a test for this in the test suite?), fine.  That's not what the code sicking attached does.  I was just commenting on the patch from the point of view of URI object manipulation in Gecko, not from the point of view of XMLHttpRequest spec compliance.

Speaking of which... Shouldn't there be tests in this bug?  It seems like we're adding a good bit of failure-prone code here.

Comment 43

[Anne (:annevk)]

The specification has no testsuite yet as small details are still changing. The specification has been there for a long time but people are slow with reviewing and I'm not perfect:

  http://dev.w3.org/2006/waf/access-control/

I do consider it pretty stable now.

Comment 44

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The patch contains lots of tests. Unfortunately some things are hard to test as the mochitest server only accepts GET requests.

Comment 45

[Boris Zbarsky [:bzbarsky]]

It clearly doesn't have a test for comment 16...

Comment 46

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

True. Though how do we create a document with a data-uri principal? Or any other type of uri that would be an issue? The only way I can think of would be from file which we can't really add to mochitest afaik?

Boris, would it work to get the host and if that is empty set the string to "null"?

Comment 47

[Boris Zbarsky [:bzbarsky]]

From chrome, loading a data: URI will just give you a data-uri principal.  Doing it from content should be impossible.  So it'd have to be a browser mochitest.

You could GetHost(), and if that either throws or returns empty string set the string to null.  But the "://" thing probably needs to go anyway; it's not necessarily the right thing for all extension URI schemes, even ones with a host.  It's safer to SetUserPass.

Comment 48

[Mike Connor [:mconnor]]

Comment on attachment 300329 [details] [diff] [review]
Fix review comments v1.5

I don't see a clear case for taking this before b3 rather than after, please renom if there's a clear risk/reward case to be had

Comment 49

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 300329 [details] [diff] [review]
Fix review comments v1.5

So I think we really should take this. It'll give people a better opportunity to test the new security features of cross-site XHR. This'll get us wider testing of this new API giving us better opportunity to fix any security issues before final release.

The risk is really small since this only affects cross-site XHR which really isn't used yet.

Comment 50

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 300329 [details] [diff] [review]
Fix review comments v1.5

a=beltzner

Comment 51

[Steve England [:stevee]]

Is there any way this bug could have caused bug 415096?

Comment 52

[Mats Palmgren (inactive)]

I have network.http.sendRefererHeader=0 in my prefs.  It doesn't seem
to block Referer-Root from being sent?

Comment 53

[Uri Bernstein]

(In reply to comment #52)
> I have network.http.sendRefererHeader=0 in my prefs.  It doesn't seem
> to block Referer-Root from being sent?
> 

See the discussion here: http://www.nabble.com/-xhr--cross-site-proposal-headers-td11739790.html (especially the last few points). As I understand it, the very idea of using a special header (instead of "referer"), was to ensure it will be sent even for users who choose not to send "referer".

Comment 54

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I guess we could add a separate pref for referer-root, though I don't really see the point of doing that. Why would you want to block this header? Tracking is still very possible using just uris if the page so wishes.

Comment 55

[Mats Palmgren (inactive)]

(In reply to comment #54)
> Why would you want to block this header?

For the same reason I want to opt-out from 'referer' and 'ping'.

> Tracking is still very possible using just uris if the page so wishes.

Sure, but it looks to me that Referer-Root makes tracking *easier*.
Thinking about it, what I really want is a pref to turn off cross-site
XHR completely.  Sorry for being suspicious about this fancy new feature
but it just doesn't fit my preference for security and privacy.

Comment 56

[Mats Palmgren (inactive)]

Ah, it looks like permissions.default.xmlhttprequest=3 does what I want.
http://kb.mozillazine.org/Permissions.default.image#Related_preferences

Comment 57

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't agree that XHR makes it easier. Compare

<img src="tracker.com/?from=mysite.com">

vs.

<script>
xhr = new XMLHttpRequest();
xhr.open("GET", "tracker.com");
xhr.send();
</script>

But sure, adding a pref to turn off cross-site XHR completely sounds like a good idea. We've done this in the past for new features in case there are catastrophic security bugs in them.

The referer header is different in that tracking will happen automatically using normal link traversal. I.e. a site can track where you are coming from, without the originating site having to do anything. In fact the originating site has to opt out from the tracking using complicated meta redirects.

Comment 58

[tabmix.onemen]

since 2008-01-31
Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService).newURI(url, null, null);
throw NS_ERROR_DOM_BAD_URI if url is of the form chrome://ietab/content/reloaded.html?url=SOME SITE ADDRESS
IEtab extension use chrome://ietab/content/reloaded.html?url= to load the page in IE 

maybe this but cause that throw ?

Comment 59

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not sure I'm following what you're saying. But it definitely has nothing to do with this bug. If you think there's a problem in how we parse chrome urls, please file a separate bug.

Comment 60

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Anyhow, this bug was fixed by Suryas patch. Marking it so.

Comment 61

[Jeff Walden [:Waldo]]

I'm really not up to date on XXX spec stuff, but if this bug is relevant to the current implementation, it could be tested now.