Downgrade security state after user clicks through SB interstitial

chrome/browser/ssl/chrome_security_state_model_client.cc

Index: chrome/browser/ssl/chrome_security_state_model_client.cc
diff --git a/chrome/browser/ssl/chrome_security_state_model_client.cc b/chrome/browser/ssl/chrome_security_state_model_client.cc
index bc917b7136d150468c3f5081bc11c7ae12363d06..0d398d53ff880af93026ca1985c6ce7a466b1e44 100644
--- a/chrome/browser/ssl/chrome_security_state_model_client.cc
+++ b/chrome/browser/ssl/chrome_security_state_model_client.cc
@@ -9,12 +9,17 @@
 #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"
@@ -26,6 +31,7 @@
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(ChromeSecurityStateModelClient);
 
+using safe_browsing::SafeBrowsingUIManager;
 using security_state::SecurityStateModel;
 
 namespace {
@@ -254,4 +260,23 @@ void ChromeSecurityStateModelClient::GetVisibleSecurityState(
          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.

+  }
 }