X-Chrome-Connected is stripped when it should not be in headers.

chrome/browser/signin/chrome_signin_helper.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/signin/chrome_signin_helper.h"
 
 #include "base/strings/string_util.h"
 #include "build/build_config.h"
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
 #include "chrome/browser/profiles/profile_io_data.h"
@@ skipping 100 matching lines @@
     ManageAccountsParams empty_params;
     empty_params.service_type = GAIA_SERVICE_TYPE_NONE;
     return empty_params;
   }
 
   return BuildManageAccountsParamsIfExists(request, io_data->IsOffTheRecord());
 }
 
 }  // namespace
 
-bool AppendMirrorRequestHeaderHelper(net::URLRequest* request,
+bool FixMirrorRequestHeaderHelper(net::URLRequest* request,

[eroman, 2016/08/23 20:02:36]

style -- fix indentation

[Ramin Halavati, 2016/08/26 17:04:31]

Done.

                                      const GURL& redirect_url,
                                      ProfileIOData* io_data,
                                      int child_id,
                                      int route_id) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
   if (io_data->IsOffTheRecord())
     return false;
 
 #if !defined(OS_ANDROID)
   extensions::WebViewRendererState::WebViewInfo webview_info;
   bool is_guest = extensions::WebViewRendererState::GetInstance()->GetInfo(
       child_id, route_id, &webview_info);
   // Do not set the x-chrome-connected header on requests from a native signin
   // webview, as identified by an empty extension id which means the webview is
   // embedded in a webui page, otherwise user may end up with a blank page as
   // gaia uses the header to decide whether it returns 204 for certain end
   // points.
   if (is_guest && webview_info.owner_host.empty())
     return false;
 #endif  // !defined(OS_ANDROID)
 
   int profile_mode_mask = PROFILE_MODE_DEFAULT;
   if (io_data->incognito_availibility()->GetValue() ==
           IncognitoModePrefs::DISABLED ||
       IncognitoModePrefs::ArePlatformParentalControlsEnabled()) {
     profile_mode_mask |= PROFILE_MODE_INCOGNITO_DISABLED;
   }
 
-  return AppendMirrorRequestHeaderIfPossible(
-      request, redirect_url, io_data->google_services_account_id()->GetValue(),
-      io_data->GetCookieSettings(), profile_mode_mask);
+  if (AppendMirrorRequestHeaderIfPossible(
+        request, redirect_url,
+        io_data->google_services_account_id()->GetValue(),
+        io_data->GetCookieSettings(), profile_mode_mask))

[mmenke, 2016/08/23 20:34:16]

AppendMirrorRequestHeaderIfPossible seems problematic.  It doesn't look like it
checks for HTTPS, and relies on HSTS being enabled for Google domains...Which
hopefully is true, but this code should not rely on it.  That also means it
exposes this information to HTTP-only extensions, even if all google domains are
HSTS, which seems weird.

A third party ServiceWorker can also serve requests to http://www.google.com
with responses from http://www.evil.com...  I believe we won't expose headers
for the request in that case, either to the ServiceWorker or over the network
but it still makes me very nervous.

[mmenke, 2016/08/23 20:44:52]

And this also assumes all Google.* TLDs are owned by Google, which also seems
like a problem.  If someone registers google.mil, or just hijacks request to it,
for instance, it looks like they could get this information.

[Ramin Halavati, 2016/08/26 17:04:31]

I have added a test for content::IsOriginSecure and don't generate a header if
the origin is not secure.

[Ramin Halavati, 2016/08/26 17:04:31]

I think this is a bigger issue of google_util::IsGoogleDomainUrl. I think that
multiple services depend on this function working properly. I think we should
consider this out of scope for this CL. How would you suggest to fix this?

[mmenke, 2016/08/26 17:42:20]

My feeling is that we should not rely on it for any security or privacy related
checks.  I don't know what project mirror is, but I don't think we should just
be arbitrarily adding privacy-relevant headers to requests based on requested
URLs.  It seems like a problematic design to me.

I do agree that this seems like a more general problem with IsGoogleDomainUrl
itself, even with an HTTPS check added to it.

+    return true;
+
+  request->RemoveRequestHeaderByName(signin::kChromeConnectedHeader);

[eroman, 2016/08/23 20:02:36]

This doesn't seem right.

This code is called for all requests flowing through resource dispatcher host.
Which means that any other consumer that tries to set their own header named
X-Chrome-Connected will have it stripped at this layer. At best that is
confusing, at worst that is a bug which prevents using this header name on
non-google domains.

[Ramin Halavati, 2016/08/26 17:04:31]

It's updated so that it is removed only when it is redirecting from a domain
which is eligible for having this header, to one that is not eligible.

+  return false;
 }
 
 void ProcessMirrorResponseHeaderIfExists(net::URLRequest* request,
                                          ProfileIOData* io_data,
                                          int child_id,
                                          int route_id) {
   ManageAccountsParams params =
       BuildManageAccountsParamsHelper(request, io_data);
   if (params.service_type == GAIA_SERVICE_TYPE_NONE)
     return;
 
   params.child_id = child_id;
   params.route_id = route_id;
   content::BrowserThread::PostTask(
       content::BrowserThread::UI, FROM_HERE,
       base::Bind(ProcessMirrorHeaderUIThread, child_id, route_id, params));
 }
 
 }  // namespace signin