Set Origin header to "null" for cross origin redirects.

In order to avoid invaliding CSRF protections on the server, it is
necessary to set the Origin header to "null" on cross origin redirect.
This avoids problems where a malicious server redirects a POST request
to a sender to cause a confused deputy problem where the server believes
the request originated with the Origin header origin.

This adds a check to url_request.cc where if the redirect is cross
origin and an Origin header is present, this sets the Origin header to
"null." If there is not Origin header, it leaves it as-is.

R=rsleevi@chromium.org
BUG=465517

Review URL: https://codereview.chromium.org/1017583002

Cr-Commit-Position: refs/heads/master@{#322685}

12 files changed, 122 insertions, 3 deletions

diff --git a/net/data/url_request_unittest/redirect301-to-https b/net/data/url_request_unittest/redirect301-to-https
new file mode 100644
index 0000000..7898192
--- /dev/null
+++ b/net/data/url_request_unittest/redirect301-to-https
@@ -0,0 +1 @@
+a
diff --git a/net/data/url_request_unittest/redirect301-to-https.mock-http-headers b/net/data/url_request_unittest/redirect301-to-https.mock-http-headers
new file mode 100644
index 0000000..9a3429d
--- /dev/null
+++ b/net/data/url_request_unittest/redirect301-to-https.mock-http-headers
@@ -0,0 +1,2 @@
+HTTP/1.1 301 Yo
+Location: https://test.test/test
diff --git a/net/data/url_request_unittest/redirect302-to-https b/net/data/url_request_unittest/redirect302-to-https
new file mode 100644
index 0000000..7898192
--- /dev/null
+++ b/net/data/url_request_unittest/redirect302-to-https
@@ -0,0 +1 @@
+a
diff --git a/net/data/url_request_unittest/redirect302-to-https.mock-http-headers b/net/data/url_request_unittest/redirect302-to-https.mock-http-headers
new file mode 100644
index 0000000..15f785f
--- /dev/null
+++ b/net/data/url_request_unittest/redirect302-to-https.mock-http-headers
@@ -0,0 +1,2 @@
+HTTP/1.1 302 Yo
+Location: https://test.test/test
diff --git a/net/data/url_request_unittest/redirect303-to-https b/net/data/url_request_unittest/redirect303-to-https
new file mode 100644
index 0000000..7898192
--- /dev/null
+++ b/net/data/url_request_unittest/redirect303-to-https
@@ -0,0 +1 @@
+a
diff --git a/net/data/url_request_unittest/redirect303-to-https.mock-http-headers b/net/data/url_request_unittest/redirect303-to-https.mock-http-headers
new file mode 100644
index 0000000..eafebf8
--- /dev/null
+++ b/net/data/url_request_unittest/redirect303-to-https.mock-http-headers
@@ -0,0 +1,2 @@
+HTTP/1.1 303 Yo
+Location: https://test.test/test
diff --git a/net/data/url_request_unittest/redirect307-to-https b/net/data/url_request_unittest/redirect307-to-https
new file mode 100644
index 0000000..7898192
--- /dev/null
+++ b/net/data/url_request_unittest/redirect307-to-https
@@ -0,0 +1 @@
+a
diff --git a/net/data/url_request_unittest/redirect307-to-https.mock-http-headers b/net/data/url_request_unittest/redirect307-to-https.mock-http-headers
new file mode 100644
index 0000000..6bb3531
--- /dev/null
+++ b/net/data/url_request_unittest/redirect307-to-https.mock-http-headers
@@ -0,0 +1,2 @@
+HTTP/1.1 307 Yo
+Location: https://test.test/test
diff --git a/net/data/url_request_unittest/redirect308-to-https b/net/data/url_request_unittest/redirect308-to-https
new file mode 100644
index 0000000..7898192
--- /dev/null
+++ b/net/data/url_request_unittest/redirect308-to-https
@@ -0,0 +1 @@
+a
diff --git a/net/data/url_request_unittest/redirect308-to-https.mock-http-headers b/net/data/url_request_unittest/redirect308-to-https.mock-http-headers
new file mode 100644
index 0000000..28ece09
--- /dev/null
+++ b/net/data/url_request_unittest/redirect308-to-https.mock-http-headers
@@ -0,0 +1,2 @@
+HTTP/1.1 308 Yo
+Location: https://test.test/test
diff --git a/net/url_request/url_request.cc b/net/url_request/url_request.cc
index 455bdf2..c62854f 100644
--- a/net/url_request/url_request.cc
+++ b/net/url_request/url_request.cc
@@ -37,6 +37,8 @@
 #include "net/url_request/url_request_job_manager.h"
 #include "net/url_request/url_request_netlog_params.h"
 #include "net/url_request/url_request_redirect_job.h"
+#include "url/gurl.h"
+#include "url/origin.h"
 
 using base::Time;
 using std::string;
@@ -54,6 +56,8 @@ void StripPostSpecificHeaders(HttpRequestHeaders* headers) {
   // These are headers that may be attached to a POST.
   headers->RemoveHeader(HttpRequestHeaders::kContentLength);
   headers->RemoveHeader(HttpRequestHeaders::kContentType);
+  // TODO(jww): This is Origin header removal is probably layering violation and
+  // should be refactored into //content. See https://crbug.com/471397.
   headers->RemoveHeader(HttpRequestHeaders::kOrigin);
 }
 
@@ -940,6 +944,29 @@ int URLRequest::Redirect(const RedirectInfo& redirect_info) {
     method_ = redirect_info.new_method;
   }
 
+  // Cross-origin redirects should not result in an Origin header value that is
+  // equal to the original request's Origin header. This is necessary to prevent
+  // a reflection of POST requests to bypass CSRF protections. If the header was
+  // not set to "null", a POST request from origin A to a malicious origin M
+  // could be redirected by M back to A.
+  //
+  // In the Section 4.2, Step 4.10 of the Fetch spec
+  // (https://fetch.spec.whatwg.org/#concept-http-fetch), it states that on
+  // cross-origin 301, 302, 303, 307, and 308 redirects, the user agent should
+  // set the request's origin to an "opaque identifier," which serializes to
+  // "null." This matches Firefox and IE behavior, although it supercedes the
+  // suggested behavior in RFC 6454, "The Web Origin Concept."
+  //
+  // See also https://crbug.com/465517.
+  //
+  // TODO(jww): This is probably layering violation and should be refactored
+  // into //content. See https://crbug.com/471397.
+  if (redirect_info.new_url.GetOrigin() != url().GetOrigin() &&
+      extra_request_headers_.HasHeader(HttpRequestHeaders::kOrigin)) {
+    extra_request_headers_.SetHeader(HttpRequestHeaders::kOrigin,
+                                     url::Origin().string());
+  }
+
   referrer_ = redirect_info.new_referrer;
   first_party_for_cookies_ = redirect_info.new_first_party_for_cookies;
 
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index f19cc01..ebf033d 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -2790,6 +2790,44 @@ class URLRequestTestHTTP : public URLRequestTest {
       LOG(WARNING) << "Request method was: " << request_method;
   }
 
+  // Requests |redirect_url|, which must return a HTTP 3xx redirect.
+  // |request_method| is the method to use for the initial request.
+  // |redirect_method| is the method that is expected to be used for the second
+  // request, after redirection.
+  // |origin_value| is the expected value for the Origin header after
+  // redirection. If empty, expects that there will be no Origin header.
+  void HTTPRedirectOriginHeaderTest(const GURL& redirect_url,
+                                    const std::string& request_method,
+                                    const std::string& redirect_method,
+                                    const std::string& origin_value) {
+    TestDelegate d;
+    scoped_ptr<URLRequest> req(
+        default_context_.CreateRequest(redirect_url, DEFAULT_PRIORITY, &d));
+    req->set_method(request_method);
+    req->SetExtraRequestHeaderByName(HttpRequestHeaders::kOrigin,
+                                     redirect_url.GetOrigin().spec(), false);
+    req->Start();
+
+    base::RunLoop().Run();
+
+    EXPECT_EQ(redirect_method, req->method());
+    // Note that there is no check for request success here because, for
+    // purposes of testing, the request very well may fail. For example, if the
+    // test redirects to an HTTPS server from an HTTP origin, thus it is cross
+    // origin, there is not an HTTPS server in this unit test framework, so the
+    // request would fail. However, that's fine, as long as the request headers
+    // are in order and pass the checks below.
+    if (origin_value.empty()) {
+      EXPECT_FALSE(
+          req->extra_request_headers().HasHeader(HttpRequestHeaders::kOrigin));
+    } else {
+      std::string origin_header;
+      EXPECT_TRUE(req->extra_request_headers().GetHeader(
+          HttpRequestHeaders::kOrigin, &origin_header));
+      EXPECT_EQ(origin_value, origin_header);
+    }
+  }
+
   void HTTPUploadDataOperationTest(const std::string& method) {
     const int kMsgSize = 20000;  // multiple of 10
     const int kIterations = 50;
@@ -6156,58 +6194,97 @@ TEST_F(URLRequestTestHTTP, Post302RedirectGet) {
   EXPECT_TRUE(ContainsString(data, "Accept-Charset:"));
 }
 
-// The following tests check that we handle mutating the request method for
-// HTTP redirects as expected.
-// See http://crbug.com/56373 and http://crbug.com/102130.
+// The following tests check that we handle mutating the request for HTTP
+// redirects as expected.
+// See https://crbug.com/56373, https://crbug.com/102130, and
+// https://crbug.com/465517.
 
 TEST_F(URLRequestTestHTTP, Redirect301Tests) {
   ASSERT_TRUE(test_server_.Start());
 
   const GURL url = test_server_.GetURL("files/redirect301-to-echo");
+  const GURL https_redirect_url =
+      test_server_.GetURL("files/redirect301-to-https");
 
   HTTPRedirectMethodTest(url, "POST", "GET", true);
   HTTPRedirectMethodTest(url, "PUT", "PUT", true);
   HTTPRedirectMethodTest(url, "HEAD", "HEAD", false);
+
+  HTTPRedirectOriginHeaderTest(url, "GET", "GET", url.GetOrigin().spec());
+  HTTPRedirectOriginHeaderTest(https_redirect_url, "GET", "GET", "null");
+  HTTPRedirectOriginHeaderTest(url, "POST", "GET", std::string());
+  HTTPRedirectOriginHeaderTest(https_redirect_url, "POST", "GET",
+                               std::string());
 }
 
 TEST_F(URLRequestTestHTTP, Redirect302Tests) {
   ASSERT_TRUE(test_server_.Start());
 
   const GURL url = test_server_.GetURL("files/redirect302-to-echo");
+  const GURL https_redirect_url =
+      test_server_.GetURL("files/redirect302-to-https");
 
   HTTPRedirectMethodTest(url, "POST", "GET", true);
   HTTPRedirectMethodTest(url, "PUT", "PUT", true);
   HTTPRedirectMethodTest(url, "HEAD", "HEAD", false);
+
+  HTTPRedirectOriginHeaderTest(url, "GET", "GET", url.GetOrigin().spec());
+  HTTPRedirectOriginHeaderTest(https_redirect_url, "GET", "GET", "null");
+  HTTPRedirectOriginHeaderTest(url, "POST", "GET", std::string());
+  HTTPRedirectOriginHeaderTest(https_redirect_url, "POST", "GET",
+                               std::string());
 }
 
 TEST_F(URLRequestTestHTTP, Redirect303Tests) {
   ASSERT_TRUE(test_server_.Start());
 
   const GURL url = test_server_.GetURL("files/redirect303-to-echo");
+  const GURL https_redirect_url =
+      test_server_.GetURL("files/redirect303-to-https");
 
   HTTPRedirectMethodTest(url, "POST", "GET", true);
   HTTPRedirectMethodTest(url, "PUT", "GET", true);
   HTTPRedirectMethodTest(url, "HEAD", "HEAD", false);
+
+  HTTPRedirectOriginHeaderTest(url, "GET", "GET", url.GetOrigin().spec());
+  HTTPRedirectOriginHeaderTest(https_redirect_url, "GET", "GET", "null");
+  HTTPRedirectOriginHeaderTest(url, "POST", "GET", std::string());
+  HTTPRedirectOriginHeaderTest(https_redirect_url, "POST", "GET",
+                               std::string());
 }
 
 TEST_F(URLRequestTestHTTP, Redirect307Tests) {
   ASSERT_TRUE(test_server_.Start());
 
   const GURL url = test_server_.GetURL("files/redirect307-to-echo");
+  const GURL https_redirect_url =
+      test_server_.GetURL("files/redirect307-to-https");
 
   HTTPRedirectMethodTest(url, "POST", "POST", true);
   HTTPRedirectMethodTest(url, "PUT", "PUT", true);
   HTTPRedirectMethodTest(url, "HEAD", "HEAD", false);
+
+  HTTPRedirectOriginHeaderTest(url, "GET", "GET", url.GetOrigin().spec());
+  HTTPRedirectOriginHeaderTest(https_redirect_url, "GET", "GET", "null");
+  HTTPRedirectOriginHeaderTest(url, "POST", "POST", url.GetOrigin().spec());
+  HTTPRedirectOriginHeaderTest(https_redirect_url, "POST", "POST", "null");
 }
 
 TEST_F(URLRequestTestHTTP, Redirect308Tests) {
   ASSERT_TRUE(test_server_.Start());
 
   const GURL url = test_server_.GetURL("files/redirect308-to-echo");
+  const GURL https_redirect_url =
+      test_server_.GetURL("files/redirect308-to-https");
 
   HTTPRedirectMethodTest(url, "POST", "POST", true);
   HTTPRedirectMethodTest(url, "PUT", "PUT", true);
   HTTPRedirectMethodTest(url, "HEAD", "HEAD", false);
+
+  HTTPRedirectOriginHeaderTest(url, "GET", "GET", url.GetOrigin().spec());
+  HTTPRedirectOriginHeaderTest(https_redirect_url, "GET", "GET", "null");
+  HTTPRedirectOriginHeaderTest(url, "POST", "POST", url.GetOrigin().spec());
+  HTTPRedirectOriginHeaderTest(https_redirect_url, "POST", "POST", "null");
 }
 
 // Make sure that 308 responses without bodies are not treated as redirects.