diff options
| -rw-r--r-- | chrome/browser/chrome_content_browser_client.cc | 39 | ||||
| -rw-r--r-- | chrome/browser/ssl/ssl_blocking_page.cc | 36 | ||||
| -rw-r--r-- | chrome/browser/ssl/ssl_blocking_page.h | 22 | ||||
| -rw-r--r-- | chrome/browser/ssl/ssl_browser_tests.cc | 64 | ||||
| -rw-r--r-- | chrome/browser/ssl/ssl_cert_reporter.h | 27 | ||||
| -rw-r--r-- | chrome/browser/ssl/ssl_error_handler.cc | 25 | ||||
| -rw-r--r-- | chrome/browser/ssl/ssl_error_handler.h | 9 | ||||
| -rw-r--r-- | chrome/chrome_browser.gypi | 1 |
8 files changed, 154 insertions, 69 deletions
diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc index ecac531..39f2fcd 100644 --- a/chrome/browser/chrome_content_browser_client.cc +++ b/chrome/browser/chrome_content_browser_client.cc @@ -9,6 +9,7 @@ #include <vector> #include "base/bind.h" +#include "base/bind_helpers.h" #include "base/command_line.h" #include "base/files/scoped_file.h" #include "base/i18n/icu_util.h" @@ -53,6 +54,7 @@ #include "chrome/browser/renderer_host/chrome_render_message_filter.h" #include "chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.h" #include "chrome/browser/safe_browsing/safe_browsing_service.h" +#include "chrome/browser/safe_browsing/ui_manager.h" #include "chrome/browser/search/instant_service.h" #include "chrome/browser/search/instant_service_factory.h" #include "chrome/browser/search/search.h" @@ -62,6 +64,7 @@ #include "chrome/browser/speech/tts_message_filter.h" #include "chrome/browser/ssl/ssl_add_certificate.h" #include "chrome/browser/ssl/ssl_blocking_page.h" +#include "chrome/browser/ssl/ssl_cert_reporter.h" #include "chrome/browser/ssl/ssl_client_certificate_selector.h" #include "chrome/browser/ssl/ssl_error_handler.h" #include "chrome/browser/sync_file_system/local/sync_file_system_backend.h" @@ -518,6 +521,29 @@ void HandleBlockedPopupOnUIThread(const BlockedWindowParams& params) { popup_helper->AddBlockedPopup(params); } +// An implementation of the SSLCertReporter interface used by +// SSLErrorHandler. Uses the SafeBrowsing UI manager to send invalid +// certificate reports. +class SafeBrowsingSSLCertReporter : public SSLCertReporter { + public: + explicit SafeBrowsingSSLCertReporter( + const scoped_refptr<SafeBrowsingUIManager>& safe_browsing_ui_manager) + : safe_browsing_ui_manager_(safe_browsing_ui_manager) {} + ~SafeBrowsingSSLCertReporter() override {} + + // SSLCertReporter implementation + void ReportInvalidCertificateChain(const std::string& hostname, + const net::SSLInfo& ssl_info) override { + if (safe_browsing_ui_manager_) { + safe_browsing_ui_manager_->ReportInvalidCertificateChain( + hostname, ssl_info, base::Bind(&base::DoNothing)); + } + } + + private: + const scoped_refptr<SafeBrowsingUIManager> safe_browsing_ui_manager_; +}; + #if defined(OS_ANDROID) void HandleSingleTabModeBlockOnUIThread(const BlockedWindowParams& params) { @@ -571,7 +597,7 @@ void GetGuestViewDefaultContentSettingRules( std::string(), incognito)); } -#endif // defined(ENALBE_EXTENSIONS) +#endif // defined(ENABLE_EXTENSIONS) } // namespace @@ -1711,11 +1737,12 @@ void ChromeContentBrowserClient::AllowCertificateError( SafeBrowsingService* safe_browsing_service = g_browser_process->safe_browsing_service(); - SSLErrorHandler::HandleSSLError( - tab, cert_error, ssl_info, request_url, options_mask, - safe_browsing_service ? safe_browsing_service->ui_manager().get() - : nullptr, - callback); + scoped_ptr<SafeBrowsingSSLCertReporter> cert_reporter( + new SafeBrowsingSSLCertReporter(safe_browsing_service + ? safe_browsing_service->ui_manager() + : nullptr)); + SSLErrorHandler::HandleSSLError(tab, cert_error, ssl_info, request_url, + options_mask, cert_reporter.Pass(), callback); } void ChromeContentBrowserClient::SelectClientCertificate( diff --git a/chrome/browser/ssl/ssl_blocking_page.cc b/chrome/browser/ssl/ssl_blocking_page.cc index d5f256e..717d138 100644 --- a/chrome/browser/ssl/ssl_blocking_page.cc +++ b/chrome/browser/ssl/ssl_blocking_page.cc @@ -28,7 +28,7 @@ #include "chrome/browser/interstitials/security_interstitial_metrics_helper.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/renderer_preferences_util.h" -#include "chrome/browser/safe_browsing/ui_manager.h" +#include "chrome/browser/ssl/ssl_cert_reporter.h" #include "chrome/browser/ssl/ssl_error_classification.h" #include "chrome/browser/ssl/ssl_error_info.h" #include "chrome/common/chrome_switches.h" @@ -236,15 +236,14 @@ InterstitialPageDelegate::TypeID SSLBlockingPage::kTypeForTesting = // Note that we always create a navigation entry with SSL errors. // No error happening loading a sub-resource triggers an interstitial so far. -SSLBlockingPage::SSLBlockingPage( - content::WebContents* web_contents, - int cert_error, - const net::SSLInfo& ssl_info, - const GURL& request_url, - int options_mask, - const base::Time& time_triggered, - SafeBrowsingUIManager* safe_browsing_ui_manager, - const base::Callback<void(bool)>& callback) +SSLBlockingPage::SSLBlockingPage(content::WebContents* web_contents, + int cert_error, + const net::SSLInfo& ssl_info, + const GURL& request_url, + int options_mask, + const base::Time& time_triggered, + scoped_ptr<SSLCertReporter> ssl_cert_reporter, + const base::Callback<void(bool)>& callback) : SecurityInterstitialPage(web_contents, request_url), callback_(callback), cert_error_(cert_error), @@ -255,7 +254,7 @@ SSLBlockingPage::SSLBlockingPage( expired_but_previously_allowed_( (options_mask & EXPIRED_BUT_PREVIOUSLY_ALLOWED) != 0), time_triggered_(time_triggered), - safe_browsing_ui_manager_(safe_browsing_ui_manager) { + ssl_cert_reporter_(ssl_cert_reporter.Pass()) { interstitial_reason_ = IsErrorDueToBadClock(time_triggered_, cert_error_) ? SSL_REASON_BAD_CLOCK : SSL_REASON_SSL; @@ -498,9 +497,9 @@ void SSLBlockingPage::OverrideEntry(NavigationEntry* entry) { entry->GetSSL().security_bits = ssl_info_.security_bits; } -void SSLBlockingPage::SetCertificateReportCallbackForTesting( - const base::Closure& callback) { - certificate_report_callback_for_testing_ = callback; +void SSLBlockingPage::SetSSLCertReporterForTesting( + scoped_ptr<SSLCertReporter> ssl_cert_reporter) { + ssl_cert_reporter_ = ssl_cert_reporter.Pass(); } // This handles the commands sent from the interstitial JavaScript. @@ -649,9 +648,6 @@ std::string SSLBlockingPage::GetSamplingEventName() const { } void SSLBlockingPage::FinishCertCollection() { - base::ScopedClosureRunner scoped_callback( - certificate_report_callback_for_testing_); - if (!ShouldShowCertificateReporterCheckbox()) return; @@ -665,10 +661,8 @@ void SSLBlockingPage::FinishCertCollection() { SecurityInterstitialMetricsHelper::EXTENDED_REPORTING_IS_ENABLED); if (ShouldReportCertificateError()) { - if (certificate_report_callback_for_testing_.is_null()) - scoped_callback.Reset(base::Bind(&base::DoNothing)); - safe_browsing_ui_manager_->ReportInvalidCertificateChain( - request_url().host(), ssl_info_, scoped_callback.Release()); + ssl_cert_reporter_->ReportInvalidCertificateChain(request_url().host(), + ssl_info_); } } diff --git a/chrome/browser/ssl/ssl_blocking_page.h b/chrome/browser/ssl/ssl_blocking_page.h index 6e63f54..ce694f80 100644 --- a/chrome/browser/ssl/ssl_blocking_page.h +++ b/chrome/browser/ssl/ssl_blocking_page.h @@ -13,6 +13,7 @@ #include "base/task/cancelable_task_tracker.h" #include "base/time/time.h" #include "chrome/browser/interstitials/security_interstitial_page.h" +#include "chrome/browser/ssl/ssl_cert_reporter.h" #include "net/ssl/ssl_info.h" #include "url/gurl.h" @@ -28,7 +29,6 @@ class ExperienceSamplingEvent; } #endif -class SafeBrowsingUIManager; class SSLErrorClassification; // This class is responsible for showing/hiding the interstitial page that is @@ -65,7 +65,7 @@ class SSLBlockingPage : public SecurityInterstitialPage { const GURL& request_url, int options_mask, const base::Time& time_triggered, - SafeBrowsingUIManager* safe_browsing_ui_manager, + scoped_ptr<SSLCertReporter> ssl_cert_reporter, const base::Callback<void(bool)>& callback); // InterstitialPageDelegate method: @@ -74,9 +74,8 @@ class SSLBlockingPage : public SecurityInterstitialPage { // Returns true if |options_mask| refers to an overridable SSL error. static bool IsOptionsOverridable(int options_mask); - // Allows tests to be notified when an invalid cert chain report has - // been sent (or not sent). - void SetCertificateReportCallbackForTesting(const base::Closure& callback); + void SetSSLCertReporterForTesting( + scoped_ptr<SSLCertReporter> ssl_cert_reporter); protected: // InterstitialPageDelegate implementation. @@ -100,8 +99,7 @@ class SSLBlockingPage : public SecurityInterstitialPage { std::string GetUmaHistogramPrefix() const; std::string GetSamplingEventName() const; - // Send a report about an invalid certificate to the server. Takes - // care of calling certificate_report_callback_for_testing_. + // Send a report about an invalid certificate to the server. void FinishCertCollection(); // Check whether a checkbox should be shown on the page that allows @@ -136,14 +134,8 @@ class SSLBlockingPage : public SecurityInterstitialPage { // calculates all times relative to this. const base::Time time_triggered_; - // For reporting invalid SSL certificates as part of Safe Browsing - // Extended Reporting. - SafeBrowsingUIManager* safe_browsing_ui_manager_; - - // This callback is run when an extended reporting certificate chain - // report has been sent, or when it is decided that it should not be - // sent (for example, when in incognito mode). - base::Closure certificate_report_callback_for_testing_; + // Handles reports of invalid SSL certificates. + scoped_ptr<SSLCertReporter> ssl_cert_reporter_; // Which type of interstitial this is. enum SSLInterstitialReason { diff --git a/chrome/browser/ssl/ssl_browser_tests.cc b/chrome/browser/ssl/ssl_browser_tests.cc index 83aa42b..d6eb171 100644 --- a/chrome/browser/ssl/ssl_browser_tests.cc +++ b/chrome/browser/ssl/ssl_browser_tests.cc @@ -20,6 +20,7 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/safe_browsing/ping_manager.h" #include "chrome/browser/safe_browsing/safe_browsing_service.h" +#include "chrome/browser/safe_browsing/ui_manager.h" #include "chrome/browser/ssl/ssl_blocking_page.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_commands.h" @@ -195,7 +196,9 @@ enum Proceed { SSL_INTERSTITIAL_PROCEED, SSL_INTERSTITIAL_DO_NOT_PROCEED }; enum ExpectReport { CERT_REPORT_EXPECTED, CERT_REPORT_NOT_EXPECTED }; // This class is used to test invalid certificate chain reporting when -// the user opts in to do so on the interstitial. +// the user opts in to do so on the interstitial. It keeps track of the +// most recent hostname for which a report would have been sent over the +// network. class MockReporter : public CertificateErrorReporter { public: explicit MockReporter(net::URLRequestContext* request_context, @@ -226,6 +229,40 @@ void SetUpMockReporter(SafeBrowsingService* safe_browsing_service, scoped_ptr<CertificateErrorReporter>(reporter)); } +// This is a test implementation of the interface that blocking pages +// use to send certificate reports. It checks that the blocking page +// calls the report method when a report should be sent. +class MockSSLCertReporter : public SSLCertReporter { + public: + MockSSLCertReporter( + const scoped_refptr<SafeBrowsingUIManager>& safe_browsing_ui_manager, + const base::Closure& report_sent_callback) + : safe_browsing_ui_manager_(safe_browsing_ui_manager), + reported_(false), + expect_report_(false), + report_sent_callback_(report_sent_callback) {} + + ~MockSSLCertReporter() override { EXPECT_EQ(expect_report_, reported_); } + + // SSLCertReporter implementation + void ReportInvalidCertificateChain(const std::string& hostname, + const net::SSLInfo& ssl_info) override { + reported_ = true; + if (expect_report_) { + safe_browsing_ui_manager_->ReportInvalidCertificateChain( + hostname, ssl_info, report_sent_callback_); + } + } + + void set_expect_report(bool expect_report) { expect_report_ = expect_report; } + + private: + const scoped_refptr<SafeBrowsingUIManager> safe_browsing_ui_manager_; + bool reported_; + bool expect_report_; + base::Closure report_sent_callback_; +}; + } // namespace CertificateReporting } // namespace @@ -434,6 +471,9 @@ class SSLUITest : public InProcessBrowserTest { CertificateReporting::Proceed proceed, CertificateReporting::ExpectReport expect_report, Browser* browser) { + base::RunLoop run_loop; + bool report_expected = + expect_report == CertificateReporting::CERT_REPORT_EXPECTED; ASSERT_TRUE(https_server_expired_.Start()); // Opt in to sending reports for invalid certificate chains. @@ -447,13 +487,21 @@ class SSLUITest : public InProcessBrowserTest { CheckAuthenticationBrokenState(tab, net::CERT_STATUS_DATE_INVALID, AuthState::SHOWING_INTERSTITIAL); - // Set up a callback so that the test is notified when the report - // has been sent on the IO thread (or not sent). - base::RunLoop report_run_loop; - base::Closure report_callback = report_run_loop.QuitClosure(); + // Set up a MockSSLCertReporter to keep track of when the blocking + // page invokes the cert reporter. + SafeBrowsingService* sb_service = + g_browser_process->safe_browsing_service(); + ASSERT_TRUE(sb_service); + scoped_ptr<CertificateReporting::MockSSLCertReporter> ssl_cert_reporter( + new CertificateReporting::MockSSLCertReporter( + sb_service->ui_manager(), report_expected + ? run_loop.QuitClosure() + : base::Bind(&base::DoNothing))); + ssl_cert_reporter->set_expect_report(report_expected); + SSLBlockingPage* interstitial_page = static_cast<SSLBlockingPage*>( tab->GetInterstitialPage()->GetDelegateForTesting()); - interstitial_page->SetCertificateReportCallbackForTesting(report_callback); + interstitial_page->SetSSLCertReporterForTesting(ssl_cert_reporter.Pass()); EXPECT_EQ(std::string(), reporter_->latest_hostname_reported()); @@ -467,11 +515,9 @@ class SSLUITest : public InProcessBrowserTest { interstitial_page->DontProceed(); } - // Wait until the report has been sent on the IO thread. - report_run_loop.Run(); - if (expect_report == CertificateReporting::CERT_REPORT_EXPECTED) { // Check that the mock reporter received a request to send a report. + run_loop.Run(); EXPECT_EQ(https_server_expired_.GetURL("/").host(), reporter_->latest_hostname_reported()); } else { diff --git a/chrome/browser/ssl/ssl_cert_reporter.h b/chrome/browser/ssl/ssl_cert_reporter.h new file mode 100644 index 0000000..d31c080 --- /dev/null +++ b/chrome/browser/ssl/ssl_cert_reporter.h @@ -0,0 +1,27 @@ +// 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. + +#ifndef CHROME_BROWSER_SSL_SSL_CERT_REPORTER_H_ +#define CHROME_BROWSER_SSL_SSL_CERT_REPORTER_H_ + +#include <string> + +namespace net { +class SSLInfo; +} // namespace net + +// An interface used by interstitial pages to send reports of invalid +// certificate chains. +class SSLCertReporter { + public: + virtual ~SSLCertReporter() {} + + // Send a report for |hostname| with the given |ssl_info| to the + // report collection endpoint. |callback| will be run when the report has + // been sent off (not necessarily after a response has been received). + virtual void ReportInvalidCertificateChain(const std::string& hostname, + const net::SSLInfo& ssl_info) = 0; +}; + +#endif // CHROME_BROWSER_SSL_SSL_CERT_REPORTER_H_ diff --git a/chrome/browser/ssl/ssl_error_handler.cc b/chrome/browser/ssl/ssl_error_handler.cc index 6e0cd4d..25e4b6a 100644 --- a/chrome/browser/ssl/ssl_error_handler.cc +++ b/chrome/browser/ssl/ssl_error_handler.cc @@ -9,8 +9,8 @@ #include "base/metrics/histogram.h" #include "base/time/time.h" #include "chrome/browser/profiles/profile.h" -#include "chrome/browser/safe_browsing/ui_manager.h" #include "chrome/browser/ssl/ssl_blocking_page.h" +#include "chrome/browser/ssl/ssl_cert_reporter.h" #include "content/public/browser/notification_service.h" #include "content/public/browser/notification_source.h" #include "content/public/browser/web_contents.h" @@ -90,7 +90,7 @@ void SSLErrorHandler::HandleSSLError( const net::SSLInfo& ssl_info, const GURL& request_url, int options_mask, - SafeBrowsingUIManager* safe_browsing_ui_manager, + scoped_ptr<SSLCertReporter> ssl_cert_reporter, const base::Callback<void(bool)>& callback) { #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION) CaptivePortalTabHelper* captive_portal_tab_helper = @@ -103,7 +103,7 @@ void SSLErrorHandler::HandleSSLError( web_contents->SetUserData( UserDataKey(), new SSLErrorHandler(web_contents, cert_error, ssl_info, request_url, - options_mask, safe_browsing_ui_manager, callback)); + options_mask, ssl_cert_reporter.Pass(), callback)); SSLErrorHandler* error_handler = SSLErrorHandler::FromWebContents(web_contents); @@ -123,14 +123,13 @@ void SSLErrorHandler::SetInterstitialTimerStartedCallbackForTest( g_timer_started_callback = callback; } -SSLErrorHandler::SSLErrorHandler( - content::WebContents* web_contents, - int cert_error, - const net::SSLInfo& ssl_info, - const GURL& request_url, - int options_mask, - SafeBrowsingUIManager* safe_browsing_ui_manager, - const base::Callback<void(bool)>& callback) +SSLErrorHandler::SSLErrorHandler(content::WebContents* web_contents, + int cert_error, + const net::SSLInfo& ssl_info, + const GURL& request_url, + int options_mask, + scoped_ptr<SSLCertReporter> ssl_cert_reporter, + const base::Callback<void(bool)>& callback) : content::WebContentsObserver(web_contents), web_contents_(web_contents), cert_error_(cert_error), @@ -138,7 +137,7 @@ SSLErrorHandler::SSLErrorHandler( request_url_(request_url), options_mask_(options_mask), callback_(callback), - safe_browsing_ui_manager_(safe_browsing_ui_manager) { + ssl_cert_reporter_(ssl_cert_reporter.Pass()) { #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION) Profile* profile = Profile::FromBrowserContext( web_contents->GetBrowserContext()); @@ -208,7 +207,7 @@ void SSLErrorHandler::ShowSSLInterstitial() { SHOW_SSL_INTERSTITIAL_NONOVERRIDABLE); (new SSLBlockingPage(web_contents_, cert_error_, ssl_info_, request_url_, options_mask_, base::Time::NowFromSystemTime(), - safe_browsing_ui_manager_, callback_))->Show(); + ssl_cert_reporter_.Pass(), callback_))->Show(); // Once an interstitial is displayed, no need to keep the handler around. // This is the equivalent of "delete this". web_contents_->RemoveUserData(UserDataKey()); diff --git a/chrome/browser/ssl/ssl_error_handler.h b/chrome/browser/ssl/ssl_error_handler.h index c069b47..49f0c35 100644 --- a/chrome/browser/ssl/ssl_error_handler.h +++ b/chrome/browser/ssl/ssl_error_handler.h @@ -11,6 +11,7 @@ #include "base/macros.h" #include "base/timer/timer.h" #include "chrome/browser/chrome_notification_types.h" +#include "chrome/browser/ssl/ssl_cert_reporter.h" #include "content/public/browser/notification_observer.h" #include "content/public/browser/notification_registrar.h" #include "content/public/browser/web_contents_observer.h" @@ -23,8 +24,6 @@ class RenderViewHost; class WebContents; } -class SafeBrowsingUIManager; - // This class is responsible for deciding whether to show an SSL warning or a // captive portal error page. It makes this decision by delaying the display of // SSL interstitial for a few seconds (2 by default), and waiting for a captive @@ -53,7 +52,7 @@ class SSLErrorHandler : public content::WebContentsUserData<SSLErrorHandler>, const net::SSLInfo& ssl_info, const GURL& request_url, int options_mask, - SafeBrowsingUIManager* safe_browsing_ui_manager, + scoped_ptr<SSLCertReporter> ssl_cert_reporter, const base::Callback<void(bool)>& callback); static void SetInterstitialDelayTypeForTest(InterstitialDelayType delay); @@ -68,7 +67,7 @@ class SSLErrorHandler : public content::WebContentsUserData<SSLErrorHandler>, const net::SSLInfo& ssl_info, const GURL& request_url, int options_mask, - SafeBrowsingUIManager* safe_browsing_ui_manager, + scoped_ptr<SSLCertReporter> ssl_cert_reporter, const base::Callback<void(bool)>& callback); ~SSLErrorHandler() override; @@ -112,7 +111,7 @@ class SSLErrorHandler : public content::WebContentsUserData<SSLErrorHandler>, content::NotificationRegistrar registrar_; base::OneShotTimer<SSLErrorHandler> timer_; - SafeBrowsingUIManager* safe_browsing_ui_manager_; + scoped_ptr<SSLCertReporter> ssl_cert_reporter_; DISALLOW_COPY_AND_ASSIGN(SSLErrorHandler); }; diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index 49853f5..c6548f8 100644 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -2640,6 +2640,7 @@ 'browser/ssl/chrome_ssl_host_state_delegate_factory.h', 'browser/ssl/ssl_blocking_page.cc', 'browser/ssl/ssl_blocking_page.h', + 'browser/ssl/ssl_cert_reporter.h', 'browser/ssl/ssl_client_certificate_selector.h', 'browser/ssl/ssl_error_classification.cc', 'browser/ssl/ssl_error_classification.h', |