Add checkbox for reporting invalid TLS/SSL cert chains

chrome/browser/ssl/ssl_blocking_page.cc

Index: chrome/browser/ssl/ssl_blocking_page.cc
diff --git a/chrome/browser/ssl/ssl_blocking_page.cc b/chrome/browser/ssl/ssl_blocking_page.cc
index ecf70291056c8b47e64f534c76b29a09aad97083..152573a65173bbf1f2441722065fb1e44e6706fe 100644
--- a/chrome/browser/ssl/ssl_blocking_page.cc
+++ b/chrome/browser/ssl/ssl_blocking_page.cc
@@ -10,6 +10,7 @@
 #include "base/i18n/time_formatting.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
+#include "base/prefs/pref_service.h"
 #include "base/process/launch.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
@@ -25,6 +26,7 @@
 #include "chrome/browser/ssl/ssl_error_classification.h"
 #include "chrome/browser/ssl/ssl_error_info.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/google/core/browser/google_util.h"
@@ -43,6 +45,9 @@
 #include "net/base/hash_value.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_util.h"
+#include "net/url_request/fraudulent_certificate_reporter.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_getter.h"
 #include "ui/base/l10n/l10n_util.h"
 
 #if defined(OS_WIN)
@@ -223,7 +228,7 @@ SSLBlockingPage::SSLBlockingPage(content::WebContents* web_contents,
                                  const GURL& request_url,
                                  int options_mask,
                                  const base::Callback<void(bool)>& callback)
-    : SecurityInterstitialPage(web_contents, request_url),
+    : SecurityInterstitialPageWithExtendedReporting(web_contents, request_url),
       callback_(callback),
       cert_error_(cert_error),
       ssl_info_(ssl_info),
@@ -430,6 +435,8 @@ void SSLBlockingPage::PopulateInterstitialStrings(
   std::vector<std::string> encoded_chain;
   ssl_info_.cert->GetPEMEncodedChain(&encoded_chain);
   load_time_data->SetString("pem", JoinString(encoded_chain, std::string()));
+
+  PopulateExtendedReportingOption(load_time_data);
 }
 
 void SSLBlockingPage::OverrideEntry(NavigationEntry* entry) {
@@ -462,6 +469,14 @@ void SSLBlockingPage::CommandReceived(const std::string& command) {
       }
       break;
     }
+    case CMD_DO_REPORT: {
+      SetReportingPreference(true);
+      break;
+    }
+    case CMD_DONT_REPORT: {
+      SetReportingPreference(false);
+      break;
+    }
     case CMD_MORE: {
       metrics_helper_->RecordUserInteraction(
           SecurityInterstitialMetricsHelper::SHOW_ADVANCED);
@@ -506,6 +521,11 @@ void SSLBlockingPage::OverrideRendererPrefs(
 void SSLBlockingPage::OnProceed() {
   metrics_helper_->RecordUserDecision(
       SecurityInterstitialMetricsHelper::PROCEED);
+
+  // Finish collection information about invalid certificates, if the
+  // user opted in to.
+  FinishCertCollection();
+
   RecordSSLExpirationPageEventState(
       expired_but_previously_allowed_, true, overridable_);
   // Accepting the certificate resumes the loading of the page.
@@ -515,11 +535,32 @@ void SSLBlockingPage::OnProceed() {
 void SSLBlockingPage::OnDontProceed() {
   metrics_helper_->RecordUserDecision(
       SecurityInterstitialMetricsHelper::DONT_PROCEED);
+
+  // Finish collection information about invalid certificates, if the
+  // user opted in to.
+  FinishCertCollection();
+
   RecordSSLExpirationPageEventState(
       expired_but_previously_allowed_, false, overridable_);
   NotifyDenyCertificate();
 }
 
+void SSLBlockingPage::FinishCertCollection() {
+  const bool enabled =
+      IsPrefEnabled(prefs::kSafeBrowsingExtendedReportingEnabled);
+  UMA_HISTOGRAM_BOOLEAN("SB2.ExtendedReportingIsEnabled", enabled);
+
+  if (enabled) {
+    net::URLRequestContext* request_context = web_contents()
+                                                  ->GetBrowserContext()
+                                                  ->GetRequestContext()
+                                                  ->GetURLRequestContext();
+    net::FraudulentCertificateReporter* reporter =
+        request_context->fraudulent_certificate_reporter();
+    reporter->SendInvalidChainReport(request_url().host(), ssl_info_);
+  }
+}
+
 void SSLBlockingPage::NotifyDenyCertificate() {
   // It's possible that callback_ may not exist if the user clicks "Proceed"
   // followed by pressing the back button before the interstitial is hidden.
@@ -567,3 +608,29 @@ bool SSLBlockingPage::IsOptionsOverridable(int options_mask) {
   return (options_mask & SSLBlockingPage::OVERRIDABLE) &&
          !(options_mask & SSLBlockingPage::STRICT_ENFORCEMENT);
 }
+
+void SSLBlockingPage::PopulateExtendedReportingOption(
+    base::DictionaryValue* load_time_data) {

[felt, 2015/02/19 00:32:32]

Should we exclude Lucas's clock interstitials, both from showing a checkbox and
from uploading?

[estark, 2015/02/19 02:32:09]

Done.

[estark, 2015/02/19 02:37:30]

On second thought, I do think at some point it could be interesting to collect
data from the clock interstitials... for example, I wonder if users are ever
getting "red herring" clock interstitials, where they received a bad cert but
saw the clock interstitial because their clock happened to be off. Still, I
think it's fine to not collect data from clock interstitials for now to avoid
overwhelming our data set with them, and we can always turn it on later.

[felt, 2015/02/20 19:20:34]

That's an interesting point, I hadn't thought of that. The weird thing though is
that the checkbox text might not make sense on the clock interstitial. Would we
want to upload the clock interstitial if the user is already opted in, but not
show the checkbox? The downside of that is that it might skew the %ages.

[estark, 2015/02/21 06:53:04]

Hmm, so I can think of two or three ways that we could possibly address this:
1. Ideally, maybe it doesn't even make sense to show the clock interstitial at
all if the certificate would fail to validate for some other reason. Or maybe we
should be showing some hybrid interstitial that says both "you should fix your
clock" and "this website is broken." But this sounds kind of hard to implement.
2. Maybe we could use special wording on the checkbox label for the clock
interstitial so that it makes more sense, though I'm not coming up with anything
great at the moment.
3. We briefly discussed today the possibility of recording the causes of the
validation failures in addition to the chains themselves (wrong name, expired
cert, pinning violation, etc.). If we do that, we could go with your idea
(report the clock interstitial if the user is already opted in) and report
inaccurate client clock as the reason for the validation failure. That could
give us some potentially useful data (e.g. reports of red herring clock
interstitials) but would allow us to easily exclude the clock interstitials when
calculating percentages.

[felt, 2015/02/24 01:57:52]

My suggestion to proceed here: include the clock interstitial for now, but take
a screenshot of the checkbox on the clock interstitial and check with Alex
Ainslie (ainslie@) about whether it looks OK.

[estark, 2015/02/24 18:47:08]

Done -- cc'ed you on the email to ainslie.

+  // Only show the checkbox if not off-the-record and if the
+  // command-line option is set.
+  const bool show = !web_contents()->GetBrowserContext()->IsOffTheRecord() &&
+                    base::CommandLine::ForCurrentProcess()->HasSwitch(
+                        switches::kEnableInvalidCertCollection);
+
+  load_time_data->SetBoolean(interstitials::kDisplayCheckBox, show);
+  if (!show)
+    return;
+
+  load_time_data->SetBoolean(
+      interstitials::kBoxChecked,
+      IsPrefEnabled(prefs::kSafeBrowsingExtendedReportingEnabled));
+
+  const std::string privacy_link = base::StringPrintf(
+      interstitials::kPrivacyLinkHtml,
+      l10n_util::GetStringUTF8(IDS_SAFE_BROWSING_PRIVACY_POLICY_PAGE).c_str());
+
+  load_time_data->SetString(
+      "optInLink",
+      l10n_util::GetStringFUTF16(IDS_SAFE_BROWSING_MALWARE_REPORTING_AGREE,
+                                 base::UTF8ToUTF16(privacy_link)));
+}