Prohibit web payments on sites with bad SSL certificates

content/browser/web_contents/web_contents_impl.cc

Index: content/browser/web_contents/web_contents_impl.cc
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index deee5480624927d91392edcc4f98dc3d9a510024..149d636b693a32981bbc22604bcac6cff28136dd 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -120,6 +120,7 @@
 #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"
@@ -818,6 +819,24 @@ const GURL& WebContentsImpl::GetLastCommittedURL() const {
   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_;
 }
@@ -3153,7 +3172,7 @@ int WebContentsImpl::DownloadImage(
     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) {
@@ -5405,18 +5424,18 @@ void WebContentsImpl::ShowInsecureLocalhostWarningIfNeeded() {
   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 "