Last Comment Bug 446344 - Implement Origin header CSRF mitigation
: Implement Origin header CSRF mitigation
Status: NEW
[sg:want][necko-would-take]
: sec-want
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: unspecified
: All All
: P1 enhancement with 25 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
http://tools.ietf.org/html/rfc6454
: 1031630 (view as bug list)
Depends on:
Blocks: csrf WBGP
  Show dependency treegraph
 
Reported: 2008-07-20 17:52 PDT by Collin Jackson
Modified: 2016-02-09 03:19 PST (History)
41 users (show)
jonas: blocking1.9.1-
jonas: wanted1.9.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (6.83 KB, patch)
2008-08-29 01:29 PDT, Adam Barth
bzbarsky: superreview-
Details | Diff | Splinter Review
work in progress (7.50 KB, patch)
2008-09-04 03:57 PDT, Adam Barth
no flags Details | Diff | Splinter Review
updated work in progress (10.16 KB, patch)
2008-09-05 23:10 PDT, Adam Barth
no flags Details | Diff | Splinter Review
patch with tests (13.87 KB, patch)
2008-09-12 01:52 PDT, Adam Barth
no flags Details | Diff | Splinter Review
patch updated with bz's comments (13.87 KB, patch)
2008-09-12 09:24 PDT, Adam Barth
no flags Details | Diff | Splinter Review
Updated patch with NS_GetOriginFromURI in nsHttpChannel.cpp (4.91 KB, patch)
2008-09-21 20:49 PDT, Collin Jackson
bzbarsky: superreview-
Details | Diff | Splinter Review

Description Collin Jackson 2008-07-20 17:52:07 PDT
On Wed, Jul 9, 2008 at 6:59 PM, Jonas Sicking wrote on the WHATWG mailing list:
> Hi All,
>
> The Access-Control spec [1] adds an 'Origin' header that is submitted with
> all requests. I propose that we specify that <form> POSTs should do the
> same. This would be a very powerful mechanism to prevent CSRF attacks as it
> would allow CSRF prevention to happen in the server, rather than in the
> application layer.
>
> This way servers could be configured to reject all POST requests that have
> an Origin header from a different site.
>
> This wouldn't replace the normal CSRF protections sites need to do for now,
> but eventually enough UAs implement this that servers can just reject POSTs
> that doesn't have 'Origin' set. This would be especially true if we can get
> this feature backported into old browsers (we'll likely backport it to FF3).
>
> / Jonas
>
> [1] http://dev.w3.org/2006/waf/access-control/

I would like to see this proposal implemented in Firefox. Adam Barth, John Mitchell, and I have posted our paper in support of the Origin header as a CSRF defense at <http://crypto.stanford.edu/websec/csrf/>. Here is an excerpt:

To prevent CSRF attacks, we propose modifying browsers to send a Origin header with POST requests that identifies the origin that initiated the request. If the browser cannot determine the origin, the browser sends the value "null".

== Privacy ==

The Origin header improves on the Referer header by respecting
the user’s privacy:

1. The Origin header includes only the information required to identify the
   principal that initiated the request (typically the scheme, host, and port
   of the active document’s URL). In particular, the Origin header does not
   contain the path or query portions of the URL included in the Referer
   header that invade privacy without providing additional security.

2. The Origin header is sent only for POST requests, whereas the Referer
   header is sent for all requests. Simply following a hyperlink (e.g., from
   a list of search results or from a corporate intranet) does not send the
   Origin header, preventing the majority of accidental leakage of sensitive
   information.

By responding to privacy concerns, the Origin header will likely not be widely
suppressed.

== Server Behavior == 

To use the Origin header as a CSRF defense, servers should behave as follows:

1. All state-modifying requests, including login requests, must be sent using
   the POST method. In particular, state-modifying GET requests must
   be blocked in order to address the forum poster theat model.

2. If the Origin header is present, the server must reject any requests whose
   Origin header contains an undesired value (including null). For example,
   a site could reject all requests whose Origin indicated the request was
   initiated from another site.

== Security Analysis == 

Although the Origin header has a simple design, the use of the header as a CSRF defense has a number of subtleties.

* Rollback and Suppression. Because a supporting browser will always
  include the Origin header when making POST requests, sites can detect
  that a request was initiated by a supporting browser by observing the
  presence of the header. This design prevents an attacker from making a
  supporting browser appear to be a non-supporting browser. Unlike the
  Referer header, which is absent when suppressed by the browser, the
  Origin header takes on the value null when suppressed by the browser.

* DNS Rebinding. In existing browsers, the Origin header can be
  spoofed for same-site XMLHttpRequests. Sites that rely only on network
  connectivity for authentication should use one of the existing DNS
  rebinding defenses, such as validating the Host header. This 
  requirement is complementary to CSRF protection and also applies to all
  the other existing CSRF defenses.

* Plug-ins. If a site opts into cross-site HTTP requests via crossdomain.xml,
  an attacker can use Flash Player to set the Origin header in cross-site 
  requests. Opting into cross-site HTTP requests also defeats secret token
  validation CSRF defenses because the tokens leak during cross-site HTTP
  requests. To prevent these (and other) attacks, sites should not opt into
  cross-site HTTP requests from untrusted origins.
Comment 1 Gervase Markham [:gerv] 2008-08-27 04:04:53 PDT
Jesse says:

"The proposal sounds good to me. I was never a fan of referrer-hiding, so you might want to get an opinion from someone who understood that."

Gerv
Comment 2 Adam Barth 2008-08-29 01:29:14 PDT
Created attachment 336013 [details] [diff] [review]
Patch

Here is a patch that attaches the Origin header to POST requests.
Comment 3 Boris Zbarsky [:bz] 2008-08-29 05:57:15 PDT
Comment on attachment 336013 [details] [diff] [review]
Patch

>+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp	Fri Aug 29 01:23:09 2008 -0700
>@@ -324,24 +324,35 @@

Please add -p to your diff flags?

>+    nsCAutoString method;
>+    GetRequestMethod(method);
>+    if (method.Equals("POST")) {

  if (mRequestHead.Method() == nsHttp::Post) {

is probably better

>+        nsCAutoString origin;
>+        rv = mRequestHead.GetHeader(nsHttp::Origin, origin);
>+        if (NS_FAILED(rv) || origin.IsEmpty()) {

It seems like:

  if (!mRequestHead.PeekHeader(nsHttp::Origin)) {

is enough, no?

> nsHttpChannel::SetReferrer(nsIURI *referrer)

So... this won't get called in a lot of cases.  In particular, there's some docshell code that skips calling this function under certain circumstances.  However, docshell will always set the "docshell.internalReferrer" property.  I don't recall the details of how this stuff works, so you might want to check with dveditz or whoever touched it last.  But it's worth looking into whether that should be used if present.

>+    nsCAutoString method;
>+    GetRequestMethod(method);
>+    if (method.Equals("POST")) {

See above for the faster way to check this.

>+        nsCAutoString origin;
>+        rv = clone->GetPrePath(origin);

This doesn't look right to me, for several reasons:

1)  This should probably use the innermost URI, not the referrer URI itself to handle jar:, view-source:, etc.

2)  GetPrePath doesn't ASCII-fy, which as I recall is a problem when putting stuff in http headers.  As far as I know the Origin spec talks about ascii-fied hostnames.

3)  I don't recall that GetPrePath canonicalizes default ports, which we need to do for Origin, right?

4)  GetPrePath will return things like "data:" or "about:" for non-hierarchical URIs, whereas I suspect we want to send "null" in those cases, though maybe we want to send the entire URI instead?  I'm not really happy with the latter, to be honest.

Ideally, we'd have an nsNetUtil function to compute the origin of a URI, to be used here, in nsPrincipal::GetOrigin, and in GetPrincipalDomainOrigin.  It would more or less follow whatever the Origin spec says.

As far as that goes, do we set data: URIs as referrers?  What about wyciwyg?  Or do we get the URI from the principal in those cases?  This might just need a followup bug, since I bet we do and that seems wrong to me.  The function I mention above might need, as a first cut, a way of indicating to the caller that it used the full spec as the origin so this particular caller can use "null" instead.

This probably also needs tests and review from biesi.
Comment 4 Adam Barth 2008-08-29 09:25:34 PDT
Thanks for the comments.  Unusual schemes aren't much of an issue because we only attach the Origin header for http, https, ftp, and gopher schemes.  Thanks for the pointer to internalReferrer.  I'll investigate an post a new patch.
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2008-08-29 10:49:26 PDT
The point is that you can have something like:

jar:http://foo.com/bar.jar!/bin.html

I don't really know how that works with POSTs, but I'm pretty sure that you really do want to get the innermost URI. You can use NS_GetInnermostURI to do it.
Comment 6 Adam Barth 2008-08-29 16:40:39 PDT
Comment on attachment 336013 [details] [diff] [review]
Patch

Clearing the review flag while I take another go at this.
Comment 7 Adam Barth 2008-09-04 03:57:26 PDT
Created attachment 336833 [details] [diff] [review]
work in progress

Addresses most of the comments above.  Not sure how to deal with schemes like "about" and "data".  Next stop: tests.
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2008-09-04 20:34:03 PDT
Hmm.. ideally I think we want to get the origin out of an nsIPrincipal, rather than an nsIURI. But we can't really change nsIHttpChannel at this point (though we will in the future).

So what I think we should do is to go through all callers of SetReferrer to make sure that they get their referrer URI out of a principal, rather than a GetDocumentURI for example. It would help here with an NS_SetHTTPReferrer(nsIHTTPChannel*, nsIPrincipal*)

This should elliminate most cases when the URI is a data uri or a javascript uri. After that I think we can make the Origin header have the value "null" for those cases, including when the principal is chrome.


Another thing, should we set this on all non-GET/HEAD channels, not just on POST? I.e. should DELETE have an origin as well? I would think so.
Comment 9 Adam Barth 2008-09-05 23:10:45 PDT
Created attachment 337183 [details] [diff] [review]
updated work in progress

Thanks for the comments.

> So what I think we should do is to go through all callers of SetReferrer to
> make sure that they get their referrer URI out of a principal, 

Maybe we should do this in a separate patch?  That looks like it will involve touching a lot of code.

> This should elliminate most cases when the URI is a data uri or a javascript
> uri. After that I think we can make the Origin header have the value "null" for
> those cases, including when the principal is chrome.

Turns out we already have a white list of allowed referrer schemes.  I factored out that code and used it here too.

> Another thing, should we set this on all non-GET/HEAD channels, not just on
> POST? I.e. should DELETE have an origin as well? I would think so.

Yeah, that's a good idea.  Fixed.
Comment 10 Jonas Sicking (:sicking) PTO Until July 5th 2008-09-06 10:25:16 PDT
separate patch seems good for the referred thing. Is the patch ready for review then? If so, please request reviews
Comment 11 Adam Barth 2008-09-06 11:51:26 PDT
> separate patch seems good for the referred thing. Is the patch ready for review
> then? If so, please request reviews

Almost there.  I'm going to add some tests first.
Comment 12 Adam Barth 2008-09-12 01:52:12 PDT
Created attachment 338278 [details] [diff] [review]
patch with tests

Here is an updated patch with tests.
Comment 13 Boris Zbarsky [:bz] 2008-09-12 05:58:44 PDT
Comment on attachment 338278 [details] [diff] [review]
patch with tests

I assume the test can't be a necko unit test because we rely on the docshell referrer-setting behavior?  Might be good to also have a unit test where we set the referrer manually, too.  Or file a followup bug to create that, as well as to create a test for a jar: referrer.

> +++ b/content/base/test/test_bug446344.xhtml	

> +  is(window.frames[0].document.getElementsByTagName("pre")[0].innerHTML ...

How about window.frames[0].document.textContent ?

And perhaps some line-breaking within 80 chars?  Same for the window.frames[1] case.

>+addLoadEvent(submitPOSTForm, ok);

What's the 'ok' part doing there?

>+NS_GetOriginFromURI(nsIURI     *uri,

So for a non-nsIURL nsIURI this function will return an error, but more or less by accident (the SetUserPass will return error).  Is this the behavior we want?  If so, we should document it, and perhaps make it less accidental.

>+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp	Fri Sep 12 01:46:02 2008 -0700
>+    // Make sure we send an Origin header for POST requests to tell the server

s/POST requests/requests which are not GET or HEAD/

>+            origin = NS_LITERAL_CSTRING("null");

  origin.AssignLiteral("null");

>+        nsCAutoString originASCII;
>+        rv = NS_StringToACE(origin, originASCII);

Hmm...  Should we be using GetAsciiHost instead of GetPrePath while building up the origin string?
Comment 14 Boris Zbarsky [:bz] 2008-09-12 05:59:25 PDT
Honza, I think you also have a patch with a GetOrigin function in it.  You and Adam should coordinate on this.
Comment 15 Honza Bambas (:mayhemer) 2008-09-12 06:37:13 PDT
As I remember the GetOrigin (or NS_GetSchemaHostPort) function has been removed because it was no longer necessary. Anyway, it seems its need for it raises. So, putting function for extraction of an origin with respect to NS_SecurityCompareURIs function to nsNetUtils.h would be appropriate.

Adam, I can create a patch on top of your patch that will introduce this new function. Would you agree with that? Other option is to submit a new bug and make it blocking this one. I know about some patches that would need this "GetOrigin" function too.
Comment 16 Adam Barth 2008-09-12 09:23:50 PDT
(In reply to comment #13)
> I assume the test can't be a necko unit test because we rely on the docshell
> referrer-setting behavior?

This is my understanding.

> Might be good to also have a unit test where we set
> the referrer manually, too.  Or file a followup bug to create that, as well as
> to create a test for a jar: referrer.

Ok.  I'll create a follow-up bug.

> How about window.frames[0].document.textContent ?

Fixed.

> And perhaps some line-breaking within 80 chars?  Same for the window.frames[1]
> case.

Fixed.

> >+addLoadEvent(submitPOSTForm, ok);
> 
> What's the 'ok' part doing there?

No idea.  Copy/paste from another test.  Removed.

> >+NS_GetOriginFromURI(nsIURI     *uri,
> 
> So for a non-nsIURL nsIURI this function will return an error, but more or less
> by accident (the SetUserPass will return error).  Is this the behavior we want?
>  If so, we should document it, and perhaps make it less accidental.

What is a non-nsIURL nsIURI (that sounds paradoxical)?  I just moved this code from the postMessage implementation so I could reuse it.  It looks like setReferrer calls the same function...  Is there a way to achieve the same effect without generating the error?

> s/POST requests/requests which are not GET or HEAD/

Fixed.

> >+            origin = NS_LITERAL_CSTRING("null");
> 
>   origin.AssignLiteral("null");

Fixed.

> >+        nsCAutoString originASCII;
> >+        rv = NS_StringToACE(origin, originASCII);
> 
> Hmm...  Should we be using GetAsciiHost instead of GetPrePath while building up
> the origin string?

postMessage was using GetPrePath (which I think was introduced for this purpose).

(In reply to comment #15)
> Adam, I can create a patch on top of your patch that will introduce this new
> function. Would you agree with that? Other option is to submit a new bug and
> make it blocking this one. I know about some patches that would need this
> "GetOrigin" function too.

This patch already contains a NS_GetOriginFromURI in nsNetUtils.h.  Is there some reason to move it to a separate patch (maybe to easy integration)?
Comment 17 Adam Barth 2008-09-12 09:24:44 PDT
Created attachment 338331 [details] [diff] [review]
patch updated with bz's comments
Comment 18 Honza Bambas (:mayhemer) 2008-09-12 10:12:10 PDT
Comment on attachment 338331 [details] [diff] [review]
patch updated with bz's comments

>diff -r 018b8ac99a4c netwerk/base/public/nsNetUtil.h
>--- a/netwerk/base/public/nsNetUtil.h	Thu Aug 28 17:17:55 2008 -0700
>+++ b/netwerk/base/public/nsNetUtil.h	Fri Sep 12 09:08:47 2008 -0700
>@@ -1411,24 +1411,62 @@ NS_GetInnermostURI(nsIURI *uri)
>         NS_ADDREF(uri);
>         return uri;
>     }
> 
>     nsresult rv = nestedURI->GetInnermostURI(&uri);
>     if (NS_FAILED(rv)) {
>         return nsnull;
>     }
> 
>     return uri;
> }
> 
>+inline nsresult
>+NS_GetOriginFromURI(nsIURI     *uri,
>+                    nsACString &origin)
>+{
>+    NS_PRECONDITION(uri, "Must have URI");
>+
>+    nsCOMPtr<nsIURI> innermostURI = NS_GetInnermostURI(uri);
>+    if (!innermostURI)
>+        return NS_ERROR_FAILURE;
>+
>+    // We need to clone innermostURI here because NS_GetInnermostURI could
>+    // return an alias of uri.
>+    nsCOMPtr<nsIURI> originURI;
>+    nsresult rv = innermostURI->Clone(getter_AddRefs(originURI));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // Normalize default ports, if necessary.
>+    PRInt32 port;
>+    if (NS_SUCCEEDED(originURI->GetPort(&port))) {
>+        nsCAutoString scheme;
>+        originURI->GetScheme(scheme);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+        PRInt32 defaultPort = NS_GetDefaultPort(scheme.get());
>+        if (port == defaultPort) {
>+            rv = originURI->SetPort(-1);
>+            NS_ENSURE_SUCCESS(rv, rv);
>+        }
>+    }
>+
>+    // Extract the PrePath portion of the URI (sans UserPass).
>+    rv = originURI->SetUserPass(EmptyCString());
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = originURI->GetPrePath(origin);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    return NS_OK;
>+}
>+
> /**
>  * Get the "final" URI for a channel.  This is either the same as GetURI or
>  * GetOriginalURI, depending on whether this channel has
>  * nsIChanel::LOAD_REPLACE set.  For channels without that flag set, the final
>  * URI is the original URI, while for ones with the flag the final URI is the
>  * channel URI.
>  */
> inline nsresult
> NS_GetFinalChannelURI(nsIChannel* channel, nsIURI** uri)
> {
>     *uri = nsnull;
>     nsLoadFlags loadFlags = 0;

This function does not reflect NS_SecurityCompareURIs. Origin is not just the schema/host/port triple but more in case of file:// and some other schemes. Please see also bug 451081. It would probably be best if NS_GetOriginFromURI would return the same parts that NS_SecurityCompareURIs works with. Other problem is that GetPrePath doesn't work for some schemes at all from their nature.
Comment 19 Adam Barth 2008-09-12 10:19:09 PDT
(In reply to comment #18)
> This function does not reflect NS_SecurityCompareURIs. Origin is not just the
> schema/host/port triple but more in case of file:// and some other schemes.

I see.  We might not be able to use the same function then.  This function has to produce the string "file://" for file URLs to work properly with postMessage.
Comment 20 Honza Bambas (:mayhemer) 2008-09-12 10:36:04 PDT
So you want to have to functions: one dedicated to what is needed for postMessage and another just for what is needed to have an Origin header? The former, what I propose, could easily be used to fix bug 357323.

Actually, I start to believe that "GetOrigin" should be method of nsIURI implementations via new interface as a string. I don't much like the way how "imap", "mailbox" and "news" URIs have to be handled a different way using a flag. Also there must be a special code for "file" URIs. The new interface could also provide method to compare two origins, but it is disputable because we are dependent on implementers to obey the origin rules.
Comment 21 Jonas Sicking (:sicking) PTO Until July 5th 2008-09-12 14:02:58 PDT
I do think we should have a GetOrigin function, but it should live on nsIPrincipal rather than nsIURI. You really should never use nsIURIs as bases for security checks, but rather nsIPrincipals. (yes, lots of code currently using nsIURIs, and we've had bugs because of it)
Comment 22 Honza Bambas (:mayhemer) 2008-09-12 14:15:09 PDT
You are right, we have both methods on nsIPrincipal:
http://hg.mozilla.org/mozilla-central/annotate/9b2a99adc05e/caps/idl/nsIPrincipal.idl#l152

and a way to compare here:
http://hg.mozilla.org/mozilla-central/annotate/9b2a99adc05e/caps/idl/nsIPrincipal.idl#l88

I believe we can find a way to use nsIPrincipal for all issues mentioned here including this one bug. But, I don't know how to get the principal in nsHttpChannel.
Comment 23 Boris Zbarsky [:bz] 2008-09-12 18:53:25 PDT
Jonas, the problem is that for a lot of these cases the HTML5 spec defines same-origin checks between an entity we have a principal for (e.g. a document) and a URI-to-be-loaded (hence no principal yet).  We've been using CheckMayLoad for this, basically....
Comment 24 Honza Bambas (:mayhemer) 2008-09-13 13:41:16 PDT
So, when I need to get origin string for purposes like database storage or hash table mapping (or purposes of this bug) I should do following?

  principal = nsScriptSecurityManager.getCodebasePrincipal(URI);
  origin = principal.origin;

And by the way, principal.checkMayLoad calls NS_SecurityCompareURIs, when it fails then it tries to still return success in case both URIs (the principal's and the tested one) are file URIs and passes some others, less strict conditions (see the code).
Comment 25 Adam Barth 2008-09-15 00:53:53 PDT
I'm a bit unclear how to modify the patch to take this feedback into account.  Would you prefer that I:

1) Remove NS_GetOriginFromURI, leave postMessage as is, and implement the URI -> Origin header conversion in nsHttpChannel.cpp.

2) Rename NS_GetOriginFromURI to something that makes it clear the function is for converting URIs to the string needed for the Origin header.

3) Leave the patch as is.
Comment 26 Jonas Sicking (:sicking) PTO Until July 5th 2008-09-15 01:58:59 PDT
I'm fine either way personally.
Comment 27 Boris Zbarsky [:bz] 2008-09-15 12:29:51 PDT
>  principal = nsScriptSecurityManager.getCodebasePrincipal(URI);
>  origin = principal.origin;

That depends on whether you want an origin string that takes document.domain into account or not.  If not, then this would work fine.  If we need to take document.domain into account, then it's no good.

Note that ideally (very ideally) we would pass along all the information we have (the nsIPrincipal or nsIURI or whatever we happen to have) and then at the last possible moment actually do anything like extracting information from them.  So if the hashtable can be keyed on nsIPrincipal, say, then it should be.

Adam, if the Origin header needs something different than other places that need origins, then it needs a separate function.  Otherwise I would like us to have no more than one place where we extract origins from URIs in any one given way.
Comment 28 Adam Barth 2008-09-15 12:47:38 PDT
Sounds good.  We don't need to take document.domain into account for this purpose.  I'll test the getCodebasePrincipal approach and post a new patch.
Comment 29 Jonas Sicking (:sicking) PTO Until July 5th 2008-09-15 15:15:08 PDT
Yeah, we definitely should not take document.domain into account. In fact, we basically never ever should except in the very few cases where it's needed for web compat.

(In fact, now with postMessage implemented we ideally should kill support for setting document.domain. Unfortunately that would currently break sites)
Comment 30 Collin Jackson 2008-09-21 20:49:44 PDT
Created attachment 339728 [details] [diff] [review]
Updated patch with NS_GetOriginFromURI in nsHttpChannel.cpp

This patch leaves postMessage as is, and implements the URI -> Origin header conversion in nsHttpChannel.cpp.

(We tried the GetCodebasePrincipal approach, but our attempt to include nsIScriptSecurityManager.h failed. According to Gavin the netwerk module isn't allowed to depends on caps.)
Comment 31 Johnny Stenback (:jst, jst@mozilla.com) 2008-09-24 16:42:32 PDT
This should probably get beta exposure, marking P2.
Comment 32 Boris Zbarsky [:bz] 2008-09-26 07:55:14 PDT
You can't use the GetCodebasePrincipal thing, right.  But I'm still pretty unhappy about having multiple places in our codebase doing this.  It doesn't help that this caller actually wants different output than the others in some cases(!).  Could we please raise that issue with the spec author?  Then again, I don't see this "null" stuff in the spec....  It sounds to me like we should get that straightened out (and perhaps straighten out the fact that the ascii serialization in html5 is empty).

I just realized that I totally missed comment 16.  My apologies.  Responses follow.

> What is a non-nsIURL nsIURI

Any object that implements nsIURI but not nsIURL, pretty much.  In particular nsSimpleURI.  This is commonly used for protocols that have no concept of "host".

The real question here is what you want behavior to be for such URIs, and making it very explicit (with comments or something, if nothing else, but better yet not relying on a side-effect of SetUserPass()) so that people don't "fix" away the desired behavior.

> postMessage was using GetPrePath (which I think was introduced for this
> purpose).

prePath has been on nsIURI since 2002-Mar-05.  But more to the point, GetPrePath will just not give the same output as the results of GetAsciiHost.  The spec pretty clearly indicates we want the latter here, as far as I can tell.  See http://dev.w3.org/2006/waf/access-control/#source-origin and follow the links.

Note that by contrast the spec for postMessage explicitly talks about the unicode serialization of the origin.

Now that I think about this more, by the way, and actually read the Origin spec, it seems to me that we should really be serializing the URI of the principal of the form node or some such here (guaranteed to have one in docshell, since we're limiting this to POST; the only other place we do POST is in XHR, and we can use its principal there), and not the referrer URI, which is a totally different beastie in some cases.

That is, maybe the way to go is to create the origin in the callers and set it as another property in the HTTP channel property bag, rather than trying to get to security information that HTTP simply doesn't have from inside the HTTP channel.
Comment 33 Jonas Sicking (:sicking) PTO Until July 5th 2008-10-14 20:52:42 PDT
So I think we should use the implementation of uri->origin-string mapping that is in this patch. That is IMHO the one that makes the most sense out of all the ones i've heard. We should then lobby to get it adopted in the various specs, I think that should be doable.

We can migrate the other origin implementations over to this new method as the specs are updated.

Ideally I would like to change the signature of nsIHttpChannel to either make SetReferrer take an nsIPrincipal, or add a SetOrigin method that takes an nsIPrincipal. This seems hard right now for a number of reasons (interfaces frozen, don't want necko to depend on caps).

For now I think we should add a method somewhere reachable (nsContentUtils.h? caps somewhere?) with the signature:
NS_SetOriginOnChannel(nsIHttpChannel* aChannel, nsIPrincipal* aOrigin)

And call that in the places that need it and have the information.
Comment 34 Boris Zbarsky [:bz] 2008-10-15 13:33:09 PDT
This is the implementation that will put non-ASCII chars into the HTTP header?  I don't think that's desirable, since it violates the HTTP spec.
Comment 35 Jonas Sicking (:sicking) PTO Until July 5th 2008-10-15 13:42:13 PDT
Oh, crap, i missed that. You need to call AsciiHost or AsciiSpec.

It is actually a good question of what origin should be when exposed to javascript, such as in postMessage. In any even we should use the punycode when sending as a HTTP header.
Comment 36 Gervase Markham [:gerv] 2008-10-16 02:01:16 PDT
The JavaScript question is a tricky one. As I understand it, the current attitude from the IETF is that IDN is a display technology, and underlying plumbing uses the xn-- form throughout. So that would suggest exposing it as xn--.

However, I think Hixie wants to make it possible in HTML5 to use the proper forms directly in HTML. So perhaps it would be best to consult with the HTML5WG.

Gerv
Comment 37 Jonas Sicking (:sicking) PTO Until July 5th 2008-12-03 15:20:34 PST
Not blocking on this. We'd definitely want this done as soon as possible, but there needs to be a spec first. And we can always land a fix in a dot-release.
Comment 38 Samuel Sidler (old account; do not CC) 2008-12-03 16:47:41 PST
(In reply to comment #37)
> And we can always land a fix in a dot-release.

Please don't use this to determine blocking/non-blocking status. We're much more cautious on what we take in "dot-releases" and tend to now only take security, stability, and regression-from-previous-dot-release fixes. If this is something you want fixed at all in 3.1.x, it should land in 3.1 final (3.1.0?).
Comment 39 Jonas Sicking (:sicking) PTO Until July 5th 2008-12-03 17:08:28 PST
This would be a security fix. We can't block yet since there is no spec, and possibly won't be until it's too late to get into 3.1 (i.e. might be after we ship 3.1)
Comment 40 Samuel Sidler (old account; do not CC) 2008-12-03 17:23:46 PST
(In reply to comment #39)
> This would be a security fix.

Then it should have an sg marking in the status whiteboard. By "security fix", I mean a fix to a security vulnerability found in the codebase, not an enhancement in function to existing features (unless that's the safest or only way to fix a reported vulnerability).
Comment 41 Johnathan Nightingale [:johnath] 2009-01-29 10:33:35 PST
Absent the conversation about whether it blocks or not, is this something that's still being actively worked on?
Comment 42 Adam Barth 2009-01-29 10:46:08 PST
Based on Comment #37, I'm working with the IETF's HTTPbis working group on an Internet-Draft for this.  You can see the latest draft here:

http://webblaze.cs.berkeley.edu/2009/origin/origin.txt

I'm happy to resume work on the patch.
Comment 43 Boris Zbarsky [:bz] 2009-01-29 11:18:30 PST
For what it's worth, we now have origin-string-getting functions in nsContentUtils.  If we need to move them to nsNetUtil, we can.
Comment 44 Adam Barth 2009-09-24 17:14:02 PDT
I think we've converged on how this should work, and we've harmonized with CORS.  It's probably time to try writing a new patch.
Comment 45 Brandon Sterne (:bsterne) 2011-01-04 16:31:20 PST
Adam, is the policy that the WebKit implementation of Origin follows documented anywhere?  I noticed that [1] states that "The user agent MAY include an Origin header in any HTTP request" and it appears that WebKit is only sending it with POST.  Does WebKit have any long-term plans to send it in other contexts?

[1] http://tools.ietf.org/html/draft-abarth-origin-09
Comment 46 Adam Barth 2011-01-05 01:20:07 PST
I haven't updated the WebKit implementation since the original prototype, but the plan is to match the spec:

https://datatracker.ietf.org/doc/draft-ietf-websec-origin/?include_text=1

If you'd like to have input on what the spec says, feel free to share your thoughts on websec@ietf.org or to email me directly.
Comment 47 thunberg 2013-04-27 20:22:28 PDT
Any plans to implement this? Would be very helpful since Chrome supports it already and CSRF mitigation via Origin header over tokens is a much cleaner solution.
Comment 48 Boris Zbarsky [:bz] 2013-04-29 09:15:25 PDT
Mounir?
Comment 49 Anne (:annevk) 2013-04-30 01:51:50 PDT
So from what I heard from Ian, and it would be great if Adam could confirm, is that this was only done for POST because including it everywhere broke some services.

https://tools.ietf.org/html/rfc6454 is unfortunately still vague on when exactly it should be included, but is clear on the serialization questions. My plan is to make it crystal clear in http://fetch.spec.whatwg.org/ for which requests it is to be included and what its value should be.
Comment 50 thunberg 2013-05-11 16:23:35 PDT
Anything yet from Adam?
Comment 51 Adam Barth 2013-05-24 19:15:40 PDT
We add it for non-GET requests.
Comment 52 Mounir Lamouri (:mounir) 2013-06-14 08:37:25 PDT
It seems that this neendinfo is a mistake given that I have not been involved with that.
Boris, if your intent was to have me look deeply into this and give my feedback, let me know.
Comment 53 Michał Gołębiowski [:m_gol] 2014-04-09 09:10:48 PDT
In our app (non-public yet) I create dynamically a form to the logout address and then submit it. Chrome adds the Origin header in such a case and Firefox doesn't.

The lack of this header causes Django 1.6 to check for CSRFToken so it breaks the site when run on localhost (e.g. during automated tests). I was wondering what makes the test break for some time...
Comment 54 Boris Zbarsky [:bz] 2014-07-09 20:59:25 PDT
*** Bug 1031630 has been marked as a duplicate of this bug. ***
Comment 55 Boris Zbarsky [:bz] 2014-07-09 21:02:20 PDT
This probably needs a different owner...

It's not clear whether necko should do this for all POSTs or whether docshell should do it.  That's more or less a necko API decision.
Comment 56 Andrew Sutherland 2014-07-12 19:53:36 PDT
Hi

These two pages seem to suggest that Origin is set on POST requests in Firefox. However, in my testing (testing tool: http://homakov.github.io/) this appears not to be the case. 


https://wiki.mozilla.org/Security/Origin
http://www.petefreitag.com/item/702.cfm

Implementing this seems like an important security improvement that would make it vastly easier to protect your website against CSRF attacks. Is this on Firefox's roadmap?
Comment 57 Alexander 2014-08-04 03:16:43 PDT
http://www.browserscope.org/security/test

This is another site testing origin header and other security related browser features. There are another 2-3 of them missing in firefox 31
Comment 58 Ben Bucksch (:BenB) 2014-08-25 17:18:44 PDT
I just had a long conversation with wgrant, lead developer of launchpad, one of the few websites that I (try to) use that require referers. He's saying he can't reasonably fix this https://bugs.launchpad.net/launchpad/+bug/560246 .

But if Firefox were to send Origin: for all same-origin (!) HTTP POST requests, Launchpad could check for that header. If it exists, and matches, the POST can be allowed even in the absense of Referer. Otherwise, Launchpad could still do its normal dirty tricks for other browser. But at least it would be fixed for Firefox and Chrome.

Chromium already does this.

Please note that Origin: should be sent only (!) for same-origin requests. If they were to be sent for cross-origin requests, we'd have (almost) the same privacy problem as with Referer. (To a lesser extend, because it's only the hostname, not the URL, but still the site can see which site I was on.) The warnings of RFC 2616 Section 15.1.3 apply here as well.
Comment 59 Ben Bucksch (:BenB) 2014-11-19 16:17:33 PST
Henri Sivonen wrote in bug 704320 comment 7:
> > network.http.sendRefererHeader set to zero
> I did so, too, for years with success, but had to stop when I started using Launchpad
> for reporting Ubuntu bugs.

Exactly the same for me. So, the lack of this feature has a direct effect on people's privacy.
Comment 60 Anne (:annevk) 2014-11-20 02:07:44 PST
Origin is included for cross-origin HTTP POST as well (when issued through XMLHttpRequest, not <form>), as part of the CORS protocol. Chrome implements that as well. Including when done through <img crossorigin> or some such. However, in such cases the two parties have already decided to cooperate.
Comment 61 Tanvi Vyas - behind on reviews [:tanvi] 2015-04-14 15:04:18 PDT
What is the status of this bug?  Do we still want to use the Origin header for CSRF Mitigation?  What happens if a user turns off referrers completely in the browser for privacy reasons?  The Origin header should also then be stripped, and hence using it for CSRF mitigation would be as error prone as using the referrer for CSRF mitigation.

Given we have added ways to reduce referrer information (ex: meta referrer https://bugzilla.mozilla.org/show_bug.cgi?id=704320) and are considering changing the default referrer policy, does this bug still make sense?
Comment 62 Xidorn Quan [:xidorn] (UTC+10) 2015-04-14 16:27:40 PDT
I think always sending Origin header in all HTTP POST request should be fine even regards the privacy consideration, because I don't see any reason why a normal website, out of CSRF attack, would want to send cross-origin post request, but not want the target to know it.
Comment 63 Anne (:annevk) 2015-04-14 21:54:49 PDT
If the user turns off referrers completely, are you asserting we'd break every site that uses CORS? I doubt that.

Note also that 1) Origin includes less information than Referer by design. No path or query is revealed. 2) As Xidorn indicates this only affects <form method=POST>.
Comment 64 Boris Zbarsky [:bz] 2015-04-14 22:12:31 PDT
> are you asserting we'd break every site that uses CORS? 

Arguably, we should...  The "disable referrers completely" preference is a bit of a footgun if you take it seriously.  More precisely, break sites that don't just want to expose their stuff to the whole world.

> Origin includes less information than Referer by design.

Not necessarily true given the fine-grained control possible over Referer now...

In any case, we should really figure out what our "disable referer" stuff should mean in terms of the Origin header...  we can have an argument about whether things are opt-in or not, but it really would help most to just understand the set of problems the Referer preferences are trying to solve.
Comment 65 Anne (:annevk) 2015-04-15 01:30:53 PDT
So the only preferences I find for Referer are in about:config which makes them essentially always be the default values for our users. It's not entirely clear to me why a single header warrants four preferences, but I guess that's a separate bug.

The case Tanvi came up with was about changing the default referrer policy. Even if we change the default of that, CORS will have to remain working. Given that, adding modest XSRF protection to <form method=POST> seems like a good idea.
Comment 66 Brad Hill 2015-08-12 14:00:05 PDT
Agreed with Anne.  There is a difference between passive, generally accidental or unintended, leaking of information via Referrer, vs cross-origin requests with direct information access via CORS or taking actions via POST, and the origin is a pretty limited set of information. While token based approaches are superior for XSRF in many cases, there are some cases where tokens are more error-prone and Origin can be very useful.  e.g. if an attacker is trying to force a user to login to an account they control via CSRF, the attacker likely already knows the value of the CSRF token.

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