Downgrade security state after user clicks through SB interstitial

chrome/browser/ssl/chrome_security_state_model_client.cc

 // Copyright 2015 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/ssl/chrome_security_state_model_client.h"
 
 #include "base/command_line.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/policy/policy_cert_service.h"
 #include "chrome/browser/chromeos/policy/policy_cert_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/browser/safe_browsing/ui_manager.h"
 #include "chrome/grit/generated_resources.h"
 #include "content/public/browser/cert_store.h"
 #include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_process_host.h"
 #include "content/public/browser/security_style_explanation.h"
 #include "content/public/browser/security_style_explanations.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/origin_util.h"
 #include "content/public/common/ssl_status.h"
 #include "net/base/net_errors.h"
 #include "net/cert/x509_certificate.h"
 #include "ui/base/l10n/l10n_util.h"
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(ChromeSecurityStateModelClient);
 
+using safe_browsing::SafeBrowsingUIManager;
 using security_state::SecurityStateModel;
 
 namespace {
 
 // Converts a content::SecurityStyle (an indicator of a request's
 // overall security level computed by //content) into a
 // SecurityStateModel::SecurityLevel (a finer-grained SecurityStateModel
 // concept that can express all of SecurityStateModel's policies that
 // //content doesn't necessarily know about).
 SecurityStateModel::SecurityLevel GetSecurityLevelForSecurityStyle(
@@ skipping 208 matching lines @@
                                     ssl.sct_statuses.end());
   state->displayed_mixed_content =
       !!(ssl.content_status & content::SSLStatus::DISPLAYED_INSECURE_CONTENT);
   state->ran_mixed_content =
       !!(ssl.content_status & content::SSLStatus::RAN_INSECURE_CONTENT);
   state->displayed_content_with_cert_errors =
       !!(ssl.content_status &
          content::SSLStatus::DISPLAYED_CONTENT_WITH_CERT_ERRORS);
   state->ran_content_with_cert_errors =
       !!(ssl.content_status & content::SSLStatus::RAN_CONTENT_WITH_CERT_ERRORS);
+
+  // Check to see whether the security state should be downgraded to reflect
+  // a Safe Browsing verdict.
+  safe_browsing::SafeBrowsingService* sb_service =
+      g_browser_process->safe_browsing_service();
+  if (!sb_service)
+    return;
+  scoped_refptr<SafeBrowsingUIManager> sb_ui_manager = sb_service->ui_manager();

[estark, 2016/08/24 05:15:39]

optional: it might be simpler to add a new SBUIManager method that takes a
WebContents and a URL as arguments, and factor that method out of
IsWhitelisted(). Something like:

SBUIManager::IsWhitelisted(const UnsafeResource& resource) {
  ...
  return IsURLWhitelistedForWebContents(resource.web_contents_getter.Run(),
maybe_whitelisted_url);
}

SBUIManager::IsURLWhitelistedForWebContents(WebContents* wc, const GURL& url) {
  site_list = wc->GetUserData(...);
  return site_list->Contains(url);
}

and then you can just call IsURLWhitelistedForWebContents(web_contents_,
entry->GetURL()) here.

(Besides simpler code, the other reason I'm thinking about this is that
originally GetVisibleSecurityState() was supposed to be super fast, and its
purpose was to zippily provide an object that SecurityStateModel memoizes on, to
avoid computing a whole SecurityInfo every time the omnibox wants to redraw. To
that end, I'm not sure if this WebContentsGetter stuff is fast or slow, so it
would be nice to cut it out if possible. OTOH, I'm increasingly thinking that
this whole memoize-on-VisibleSecurityState thing is more complicated than it's
worth and quite possibly doesn't even actually buy us any performance.)

[Jialiu Lin, 2016/08/24 06:27:20]

I agree with estark@.

[felt, 2016/08/24 19:27:19]

I was trying to avoid introducing a new method to the UIManager just to wrap
this, but done.

+  SafeBrowsingUIManager::UnsafeResource test_resource;
+  test_resource.url = entry->GetURL();
+  test_resource.is_subresource = false;
+  test_resource.web_contents_getter =
+      SafeBrowsingUIManager::UnsafeResource::GetWebContentsGetter(
+          web_contents_->GetRenderProcessHost()->GetID(),
+          web_contents_->GetMainFrame()->GetRoutingID());
+  if (sb_ui_manager->IsWhitelisted(test_resource)) {
+    state->fails_malware_check = true;
+    state->initial_security_level = SecurityStateModel::SECURITY_ERROR;

[estark, 2016/08/24 05:15:39]

Strictly speaking, you don't need this, right? The actual security level in the
page's SecurityInfo will get set to SECURITY_ERROR in
GetSecurityLevelForRequest() by virtue of having fails_malware_check == true.

[felt, 2016/08/24 19:27:19]

Strictly speaking, no. But it seems potentially confusing to leave the initial
security level at a higher value, to me.

+  }
 }