Bug 20792 – Add origin header to POST requests

• Component: Forms
• Status: RESOLVED FIXED

[Adam Barth]

To prevent CSRF attacks, we should send an 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.

[Adam Barth]

Created attachment 23364 [details]
patch

Here is a simple implementation.

[Adam Barth]

Comment on attachment 23364 [details]
patch

Removing review flag because this patch doesn't properly handle HTTPS -> HTTP requests.

[Adam Barth]

For reference, here is the Mozilla bug on this topic:

https://bugzilla.mozilla.org/show_bug.cgi?id=446344

Here are Brandon Sterne's thoughts on the topic:

http://people.mozilla.org/~bsterne/content-security-policy/origin-header-proposal.html

[Sam Weinig]

What is the desired behavior for HTTPS -> HTTP requests?

[Adam Barth]

we'd like the header to be like

Origin: https://www.example.com

The current patch shows "null".  Keep in mind that this case only occurs if the user clicks through the "insecure form submission" dialog.

[Collin Jackson]

Created attachment 23646 [details]
Updated patch

We updated the patch to attach an Origin header to POST requests even if the referrer is suppressed. We also added some more tests. The origin-header-for-xmlhttprequest.html test shows the Origin header getting attached to XMLHttpRequest GET requests -- I need to investigate whether this is a bug or a feature.

[Adam Barth]

(In reply to comment #6)
> The origin-header-for-xmlhttprequest.html test shows the Origin header getting
> attached to XMLHttpRequest GET requests -- I need to investigate whether this
> is a bug or a feature.

This occurs in the latest nightly without this patch, so it's a feature.  :)

[Collin Jackson]

(In reply to comment #7)
> This occurs in the latest nightly without this patch, so it's a feature.  :)

This patch is currently attaching an Origin header to same-origin GET XMLHttpRequests that's not already present in the nightly. Cross-origin GET requests are unaffected by this patch.

[Sam Weinig]

Comment on attachment 23646 [details]
Updated patch

+        // Don't send an Origin header for GET or HEAD to avoid privacy issues.

This comment could use a little more information.  What privacy issues future me may wonder?

-    request.setHTTPHeaderField("Origin", accessControlOrigin());
+    request.setHTTPHeaderField("Origin", m_doc->securityOrigin()->toHTTPOrigin());

This can use request .setHTTPOrigin().

-    request.setHTTPHeaderField("Origin", accessControlOrigin());
+    request.setHTTPHeaderField("Origin", m_doc->securityOrigin()->toHTTPOrigin());

As can this.

This doesn't break sending the origin for XHR does it?  

Otherwise, awesome!  I think one other person should review these changes as well, as I am far from an authority on FrameLoader.  Brady, Anders, Darin, or Maciej?

[Adam Barth]

Created attachment 23743 [details]
baseline for XHR origin behavior

[Adam Barth]

Created attachment 23744 [details]
updated patch

Patch updated with Sam's comments.  Also, I added a bunch of tests for the Origin header behavior for XHR.  I split this into two patches so you can see exactly how this patch changes the behavior.  (The previous patch messed up the XHR behavior a bit, so I tweaked a few things.)

[Maciej Stachowiak]

Has this been proposed to the HTTP Working Group or another appropriate standards body? I like the idea but I'd like to make sure we put it on track to being officially placed in the appropriate standard.

[Adam Barth]

We proposed this informally to the webapps working group a number of months ago, which (I think) is how Mozilla got interested in implementing this.  It wasn't clear which spec would be most appropriate.  The computation of the header value is speced in HTML 5 for postMessage and for use by cross-site XHR.  I doubt the HTTP Working Group want an dependency on HTML 5.  :)

[Anne van Kesteren]

It is part of Access Control for Cross-Site Requests and I will make sure it is registered as an official HTTP header in due course. (It won't be part of the HTTP specification of course, but other specifications can register header names.)

[Darin Adler]

Comment on attachment 23744 [details]
updated patch

The biggest problem is that this sprinkles calls to a new function all over the loader classes. I'm concerned that we have to add the addHTTPOriginIfNeeded function call in so many places. It seems to me that will be hard to maintain -- it will be very easy to introduce a security problem by forgetting to add a call to that function. Is there any way to streamline this? I suppose we have the same issue with the referrer, but it's not even true that these are all the sites that set the referrer!

The loader is already not so great in this respect, but it seems this patch makes it even worse.

+    RefPtr<SecurityOrigin> origin = SecurityOrigin::createEmpty();
+    return origin->toHTTPOrigin();

The local variable isn't needed and the code would read more cleanly without it.

+    // Make sure we send the Origin header properly.
+    addHTTPOriginIfNeeded(request, String());

I don't understand the meaning of the word "properly" in this comment.

+    // will leak the internal host name.  Similar privacy concerns have lead

+        // header.  This is similar to toString(), except that the empty

We normally don't use two spaces between sentences in comments.

+        RefPtr<SecurityOrigin> emptyOrigin = SecurityOrigin::createEmpty();
+        origin = emptyOrigin->toHTTPOrigin();

The local variable isn't needed and the code would read more cleanly without it. Maybe also a helper function named SecurityOrigin::emptyHTTPOrigin() would be good?

+        // SecurityOrigin is represetned as the string "null".

Typo: "represetned".

I'm going to say review+ despite my comments and minor concerns.

[Adam Barth]

Fixed in r37317.  I definitely agree that FrameLoader needs to be reorganized, but it's a bit too big a project for me to take right now.