Prohibit web payments on sites with bad SSL certificates

content/browser/web_contents/web_contents_impl.cc

 // Copyright (c) 2012 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 "content/browser/web_contents/web_contents_impl.h"
 
 #include <stddef.h>
 
 #include <cmath>
 #include <utility>
@@ skipping 102 matching lines @@
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_contents_binding_set.h"
 #include "content/public/browser/web_contents_delegate.h"
 #include "content/public/browser/web_contents_unresponsive_state.h"
 #include "content/public/common/bindings_policy.h"
 #include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/child_process_host.h"
 #include "content/public/common/content_constants.h"
 #include "content/public/common/content_switches.h"
+#include "content/public/common/origin_util.h"
 #include "content/public/common/page_zoom.h"
 #include "content/public/common/result_codes.h"
 #include "content/public/common/url_utils.h"
 #include "content/public/common/web_preferences.h"
 #include "device/geolocation/geolocation_service_context.h"
 #include "device/nfc/nfc.mojom.h"
 #include "device/wake_lock/wake_lock_service_context.h"
 #include "net/base/url_util.h"
 #include "net/http/http_cache.h"
 #include "net/http/http_transaction_factory.h"
@@ skipping 678 matching lines @@
   NavigationEntry* entry = controller_.GetVisibleEntry();
   return entry ? entry->GetVirtualURL() : GURL::EmptyGURL();
 }
 
 const GURL& WebContentsImpl::GetLastCommittedURL() const {
   // We may not have a navigation entry yet.
   NavigationEntry* entry = controller_.GetLastCommittedEntry();
   return entry ? entry->GetVirtualURL() : GURL::EmptyGURL();
 }
 
+bool WebContentsImpl::IsContextSecure() const {
+  NavigationEntry* navigation_entry = controller_.GetLastCommittedEntry();
+  if (!navigation_entry)
+    return false;
+
+  if (!IsOriginSecure(navigation_entry->GetURL()))
+    return false;
+
+  if (!navigation_entry->GetURL().SchemeIsCryptographic())
+    return true;

[palmer, 2017/01/05 22:55:06]

Why return true if the scheme is *not* cryptographic?

[lgarron, 2017/01/05 23:00:20]

I was about to comment the same.

In general, is there a need to introduce a new IsContextSecure() function in so
many places?

palmer@ created IsOriginSecure() to unify such calculations, and if there is a
need for a new function we should think that through.

[please use gerrit instead, 2017/01/06 18:07:15]

I would love to have a single IsOriginSecure() function that's approved by the
security team and used by credit card autofill, password autofill, and web
payments API. The file "content/public/common/origin_util.h" is supposed to
provide this functionality, but it does not have SSL validity checks. Is that
intentional?

The SSL validity check that you see below is moved from
chrome_autofill_client.cc. The same code was also present in
aw_autofill_client.cc, so I've unified them.

One caveat is that ChromeAutofillClient::IsContextSecure() returned false for
localhost and file://, which hampers web developers and our browser/integration
tests. That's why in this common version of IsContextSecure(), I added
IsOriginSecure() as well.

[please use gerrit instead, 2017/01/06 18:07:15]

To prevent checking SSL status of localhost and file:// scheme.

[lgarron, 2017/01/06 23:21:25]

IsOriginSecure() can't have SSL validity checks because you only pass it an
origin/URL.
In general, we don't try to prevent features in secure contexts when the
connection is invalid. If the page is non-secure but loads, the user often still
expects the behaviour of a secure page.

In any case, IsOriginSecure() returns true for localhost and file://.

+
+  SSLStatus ssl_status = navigation_entry->GetSSL();
+  return ssl_status.certificate &&
+         (!net::IsCertStatusError(ssl_status.cert_status) ||
+          net::IsCertStatusMinorError(ssl_status.cert_status)) &&
+         !(ssl_status.content_status & SSLStatus::RAN_INSECURE_CONTENT);

[lgarron, 2017/01/05 23:00:20]

This looks like an ad-hoc implementation of something between "is this
connection secure" and "do we display it as secure".

Do we prevent any other "secure context" features based on things like this? If
so, where are they calculated now. If not, are there strong reasons to add this
here for payments? Are there plans to unify calculations?

[please use gerrit instead, 2017/01/06 18:07:15]

I also don't like this ad-hoc thing and would like to remove it to tighten the
validity check.
Before this patch, only credit card autofill and password autofill checked the
validity of the SSL certificate:

https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil...

https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil...

https://cs.chromium.org/chromium/src/components/password_manager/core/browser...

https://cs.chromium.org/chromium/src/components/password_manager/core/browser...

Note that credit card autofill also checks the target URL of the <form>, but
password autofill does not. Is it an oversight?

[lgarron, 2017/01/06 23:21:25]

Interesting. Do you know how these decisions were made, and when?
I'll start an Enamel thread about this.

+}
+
 WebContentsDelegate* WebContentsImpl::GetDelegate() {
   return delegate_;
 }
 
 void WebContentsImpl::SetDelegate(WebContentsDelegate* delegate) {
   // TODO(cbentzel): remove this debugging code?
   if (delegate == delegate_)
     return;
   if (delegate_)
     delegate_->Detach(this);
@@ skipping 2315 matching lines @@
 }
 
 int WebContentsImpl::DownloadImage(
     const GURL& url,
     bool is_favicon,
     uint32_t max_bitmap_size,
     bool bypass_cache,
     const WebContents::ImageDownloadCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   static int next_image_download_id = 0;
-  const content::mojom::ImageDownloaderPtr& mojo_image_downloader =
+  const mojom::ImageDownloaderPtr& mojo_image_downloader =
       GetMainFrame()->GetMojoImageDownloader();
   const int download_id = ++next_image_download_id;
   if (!mojo_image_downloader) {
     // If the renderer process is dead (i.e. crash, or memory pressure on
     // Android), the downloader service will be invalid. Pre-Mojo, this would
     // hang the callback indefinetly since the IPC would be dropped. Now,
     // respond with a 400 HTTP error code to indicate that something went wrong.
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
         base::Bind(&WebContentsImpl::OnDidDownloadImage,
@@ skipping 2231 matching lines @@
   view->FocusedNodeTouched(location_dips_screen, editable);
 #endif
 }
 
 void WebContentsImpl::ShowInsecureLocalhostWarningIfNeeded() {
   bool allow_localhost = base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kAllowInsecureLocalhost);
   if (!allow_localhost)
     return;
 
-  content::NavigationEntry* entry = GetController().GetLastCommittedEntry();
+  NavigationEntry* entry = GetController().GetLastCommittedEntry();
   if (!entry || !net::IsLocalhost(entry->GetURL().host()))
     return;
 
-  content::SSLStatus ssl_status = entry->GetSSL();
+  SSLStatus ssl_status = entry->GetSSL();
   bool is_cert_error = net::IsCertStatusError(ssl_status.cert_status) &&
                        !net::IsCertStatusMinorError(ssl_status.cert_status);
   if (!is_cert_error)
     return;
 
   GetMainFrame()->AddMessageToConsole(
-      content::CONSOLE_MESSAGE_LEVEL_WARNING,
+      CONSOLE_MESSAGE_LEVEL_WARNING,
       base::StringPrintf("This site does not have a valid SSL "
                          "certificate! Without SSL, your site's and "
                          "visitors' data is vulnerable to theft and "
                          "tampering. Get a valid SSL certificate before"
                          " releasing your website to the public."));
 }
 
 }  // namespace content