Disallow redirects in SDCH dictionary fetches.

net/url_request/sdch_dictionary_fetcher.cc

Index: net/url_request/sdch_dictionary_fetcher.cc
diff --git a/net/url_request/sdch_dictionary_fetcher.cc b/net/url_request/sdch_dictionary_fetcher.cc
index 1141aab8024485d62faa3d78da951e2d48f6206f..0fbae6f6b4a88b4aad35caaf9924a69a209e2c37 100644
--- a/net/url_request/sdch_dictionary_fetcher.cc
+++ b/net/url_request/sdch_dictionary_fetcher.cc
@@ -18,6 +18,7 @@
 #include "net/base/sdch_net_log_params.h"
 #include "net/http/http_response_headers.h"
 #include "net/log/net_log.h"
+#include "net/url_request/redirect_info.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_status.h"
 #include "net/url_request/url_request_throttler_manager.h"
@@ -143,10 +144,22 @@ void SdchDictionaryFetcher::Cancel() {
   fetch_queue_->Clear();
 }
 
+void SdchDictionaryFetcher::OnReceivedRedirect(
+    URLRequest* request,
+    const RedirectInfo& redirect_info,
+    bool* defer_redirect) {
+  DCHECK_EQ(next_state_, STATE_SEND_REQUEST_PENDING);
+
+  redirect_url_ = redirect_info.new_url;
+  next_state_ = STATE_RECEIVED_REDIRECT;
+
+  DoLoop(OK);
+}
+
 void SdchDictionaryFetcher::OnResponseStarted(URLRequest* request) {
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(request, current_request_.get());
-  DCHECK_EQ(next_state_, STATE_SEND_REQUEST_COMPLETE);
+  DCHECK_EQ(next_state_, STATE_SEND_REQUEST_PENDING);
   DCHECK(!in_loop_);
 
   // Confirm that the response isn't a stale read from the cache (as
@@ -228,8 +241,11 @@ int SdchDictionaryFetcher::DoLoop(int rv) {
       case STATE_SEND_REQUEST:
         rv = DoSendRequest(rv);
         break;
-      case STATE_SEND_REQUEST_COMPLETE:
-        rv = DoSendRequestComplete(rv);
+      case STATE_RECEIVED_REDIRECT:
+        rv = DoReceivedRedirect(rv);
+        break;
+      case STATE_SEND_REQUEST_PENDING:
+        rv = DoSendRequestPending(rv);
         break;
       case STATE_READ_BODY:
         rv = DoReadBody(rv);
@@ -259,7 +275,7 @@ int SdchDictionaryFetcher::DoSendRequest(int rv) {
     return OK;
   }
 
-  next_state_ = STATE_SEND_REQUEST_COMPLETE;
+  next_state_ = STATE_SEND_REQUEST_PENDING;
 
   FetchInfo info;
   bool success = fetch_queue_->Pop(&info);
@@ -279,7 +295,20 @@ int SdchDictionaryFetcher::DoSendRequest(int rv) {
   return ERR_IO_PENDING;
 }
 
-int SdchDictionaryFetcher::DoSendRequestComplete(int rv) {
+int SdchDictionaryFetcher::DoReceivedRedirect(int rv) {
+  // Don't allow redirect to go cross-origin or cross-port.  This test also
+  // forbids going cross-scheme, which is probably not necessary, but will
+  // be necessary when/if SDCH is restricted to HTTPS.

[Ryan Sleevi, 2016/04/13 00:30:44]

The way this comment is worded, I think you're confused about cross-origin
(origin = scheme, host, port). That is "cross-port" and "cross-scheme" are
redundant with "cross-origin", and the latter comment doesn't really explain why
it would be necessary, which is perhaps the most useful thing.

[Mike West, 2016/04/13 15:49:31]

How are we not already restricting SDCH to HTTPS? I thought that was a
requirement to begin with?

Also +1 to Ryan's comments. Cross-origin includes cross-port (and cross-scheme).

+  if (redirect_url_.GetOrigin() != current_request_->url().GetOrigin()) {

[eroman, 2016/04/13 00:12:38]

+rsleevi for security perspective, although mkwst may be a better person to ask
for this as he has been cleaning up our origin checks.

I would be *very* cautious about sprinkling more GetOrigin() calls into //net.
The specific meaning of these origin comparisons is subtle and depends on the
context -- i.e. see SecurityContext::canDisplay() vs
SecurityContext::canAccess() vs SecurityContext::canRequest().

I don't have access to the bug so not sure how to evaluate this. Can you add
more details on what exactly the goals are ?

For instance some considerations:
  * This will disallow redirects between subdomains
  * This will disallow redirects to data:URLs (I believe). Kind of weird, but
not really a same origin violation.
  * Can't redirect from http --> https
  * Can't redirect to dictionary hosted on third party site (i.e. CDN).

[Ryan Sleevi, 2016/04/13 00:30:44]

Right, Mike is definitely the better reviewer here, because the whole policy
regarding redirects is mediated in Blink land and is much more closely tied to
access policies and the effective security origin than the literal security
origin (and CORS).

I'd like to get Mike's feedback, but my initial reaction is that this probably
isn't quite right either. This is where it'd really be helpful if SDCH had a
spec (and I'd really like to push on that; we shouldn't keep working on SDCH or
keeping it enabled if we don't have a clear spec, preferably with one that
explains how things like this work - is this a Fetch()-enabled request, or
something lower? As implemented, it doesn't fit into any of the other web
platform feature explainers, including link-rel bits)

[Mike West, 2016/04/13 10:54:07]

1. I agree with Ryan that a spec which explained how fetching worked for SDCH
would be useful. I'd be happy to help put something together if you're
interested in doing so (with the obvious caveats that I don't actually know
anything about SDCH).

2. Is there actually value in thinking about any of this? That is, how much pain
would it cause to simply reject redirect responses when fetching SDCH
dictionaries?

3. Based on the discussion in the bug, it sounds like the dictionary we fetch
from `https://victim.test` after redirection from `https://malicious.test` will
only be used on `https://victim.test`, and cannot provide
`https://malicious.test` with information about the dictionary's contents. Is
that accurate? If so, then there's not much to worry about in terms of
information leakage.

[Mike West, 2016/04/13 15:49:31]

On the narrow question of this code: I'd suggest switching to
`url::Origin(redirect_url_).IsSameOriginWith(url::Origin(current_request_->url()))`.
(Yes, it is terribly confusing that `GURL::GetOrigin()` returns a `GURL` and not
a `url::Origin`. I'm working on it.). This check seems fine, since what we care
about here is the origin of the URL that you tried to load, and the origin of
the URL you've been directed to. As Eric noted, this will exclude a few things
you might care about: `https://www.example.com` couldn't redirect to
`https://sdch.example.com`, nor to `http://[anything]`, nor to `data:` (though I
think we might block that anyway?).

On the broader question of what behavior we'd like to create: I think it would
be simpler for us to simply block redirects, as opposed to only blocking
cross-origin redirects. I don't know enough about your user base, but I suspect
that would be doable; there doesn't seem to be a ton of benefit to allowing
`example.com` to point to a different resource on its own origin (whereas I
could imagine benifits for them pointing to their CDN or something).

+    current_request_->Cancel();

[eroman, 2016/04/13 00:12:38]

I think this is redundant -- calling ResetRequest() below cancels too right?

[Randy Smith (Not in Mondays), 2016/04/13 18:18:46]

Yeah, I think you're right.  Good point.

+    ResetRequest();
+    next_state_ = STATE_SEND_REQUEST;

[eroman, 2016/04/13 00:12:38]

Shouldn't there be a return statement here, and error out?

[Randy Smith (Not in Mondays), 2016/04/13 18:18:46]

Ooops.  Yes, there should.  That suggests my tests were incomplete, too.  I
think the issue's moot now, but thanks for catching it.

+  }
+  next_state_ = STATE_SEND_REQUEST_PENDING;
+  return ERR_IO_PENDING;
+}
+
+int SdchDictionaryFetcher::DoSendRequestPending(int rv) {
   DCHECK(CalledOnValidThread());
 
   // If there's been an error, abort the current request.