Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED

Status

()

Core
DOM
P3
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: annevk, Assigned: Surya Ismail)

Tracking

unspecified
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

10 years ago
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).

Updated

10 years ago
Flags: blocking1.9?

Updated

10 years ago
Blocks: 389508
Surya, could you have a look at this?
Assignee: nobody → suryaismail
Flags: blocking1.9? → blocking1.9+
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Priority: -- → P3
(Assignee)

Comment 2

10 years ago
Created attachment 292954 [details] [diff] [review]
Adds internationalized label and default port support, adds referer-root
Attachment #292954 - Flags: review?(jonas)

Comment 3

10 years ago
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

10 years ago
Oh, the ACE stuff could use review from some intl or networking folks.
(Assignee)

Updated

10 years ago
Attachment #292954 - Flags: review?(jonas)
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.
(Assignee)

Comment 6

10 years ago
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

10 years ago
> 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.


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?
(Assignee)

Comment 9

10 years ago
Created attachment 293622 [details] [diff] [review]
Adds internationalized label, default port and referer-root support
Attachment #292954 - Attachment is obsolete: true
(Assignee)

Comment 10

10 years ago
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

10 years ago
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?
(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.
(Assignee)

Comment 13

10 years ago
Created attachment 293710 [details] [diff] [review]
Referer-root, IDN labels, default port

Moved DefaultPort to NetUtils, changed HTTP headers in tests to be in ToAscii
Attachment #293622 - Attachment is obsolete: true
(Assignee)

Comment 14

10 years ago
Comment 12, you're right. I'm going to go memorize the spec now.
(Assignee)

Updated

10 years ago
Attachment #293710 - Flags: review?(jonas)
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
Attachment #293710 - Flags: review?(jonas) → review-

Comment 16

10 years ago
> 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...

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

That sounds great.
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.
(Assignee)

Comment 19

10 years ago
Created attachment 297314 [details] [diff] [review]
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?
Attachment #293710 - Attachment is obsolete: true
You could put it in nsContentUtils I think

Comment 21

10 years ago
Wouldn't nsNetUtil make more sense?
Actually yeah, given that that function is so small that seems like the right place.
(Assignee)

Comment 23

10 years ago
Created attachment 298477 [details] [diff] [review]
More corrections
Attachment #297314 - Attachment is obsolete: true
Attachment #298477 - Flags: review?(bent.mozilla)
(Reporter)

Comment 24

10 years ago
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 public-appformats@w3.org? Thanks!
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(':');
Attachment #298477 - Flags: review?(bent.mozilla) → review-
(Assignee)

Comment 26

10 years ago
Created attachment 298726 [details] [diff] [review]
More corrections
Attachment #298477 - Attachment is obsolete: true
(Assignee)

Comment 27

10 years ago
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

10 years ago
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 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!
Attachment #298726 - Flags: review+

Comment 30

10 years ago
The nsNetUtil function needs to be inline, yes.
(Assignee)

Comment 31

10 years ago
Created attachment 299074 [details] [diff] [review]
More corrections
Attachment #298726 - Attachment is obsolete: true
Attachment #299074 - Flags: review?(bent.mozilla)
Comment on attachment 299074 [details] [diff] [review]
More corrections

Awesome, thanks!
Attachment #299074 - Flags: superreview?(jonas)
Attachment #299074 - Flags: review?(bent.mozilla)
Attachment #299074 - Flags: review+
(Assignee)

Comment 33

10 years ago
Ben, thanks a lot for the review and encouragement. Do I need super-review for this?
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.
Yes you do. I'll try to do it monday
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.
Attachment #299074 - Flags: superreview?(jonas) → superreview+
Created attachment 300324 [details] [diff] [review]
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.
Attachment #299074 - Attachment is obsolete: true
Created attachment 300329 [details] [diff] [review]
Fix review comments v1.5

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

Also merged to tip.
Attachment #300324 - Attachment is obsolete: true
Attachment #300329 - Flags: approval1.9b3?
Comment on attachment 300329 [details] [diff] [review]
Fix review comments v1.5

With that r=bent sr=sicking
Attachment #300329 - Flags: superreview+
Attachment #300329 - Flags: review+

Comment 40

10 years ago
> 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!
(Reporter)

Comment 41

10 years ago
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

10 years ago
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.
Flags: in-testsuite?
(Reporter)

Comment 43

10 years ago
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.
The patch contains lots of tests. Unfortunately some things are hard to test as the mochitest server only accepts GET requests.

Comment 45

10 years ago
It clearly doesn't have a test for comment 16...
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

10 years ago
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 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
Attachment #300329 - Flags: approval1.9b3? → approval1.9b3-
Depends on: 366371
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.
Attachment #300329 - Flags: approval1.9b3- → approval1.9b3?
Comment on attachment 300329 [details] [diff] [review]
Fix review comments v1.5

a=beltzner
Attachment #300329 - Flags: approval1.9b3? → approval1.9b3+

Updated

10 years ago
Depends on: 415096
Is there any way this bug could have caused bug 415096?
I have network.http.sendRefererHeader=0 in my prefs.  It doesn't seem
to block Referer-Root from being sent?
(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".
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.
(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.
Ah, it looks like permissions.default.xmlhttprequest=3 does what I want.
http://kb.mozillazine.org/Permissions.default.image#Related_preferences
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

10 years ago
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 ?

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.
Anyhow, this bug was fixed by Suryas patch. Marking it so.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.