From f5de31070b686aa6b252dfe4b364bd4577641938 Mon Sep 17 00:00:00 2001 From: Rohit Bhatia Date: Wed, 9 Feb 2022 01:35:58 +0000 Subject: [PATCH] [M98] Fix UAF in TailoredSecurity on Android Fix UAF error by not calling dismiss callback in accepted and dismiss internal. Both will call HandleMessageDismissed later. Also, fix lifetime issue for TailoredSecurityConsentedModalAndroid. The class was written for a persistent lifetime. Now it is managed with a similar per-message lifetime as TailoredSecurityUnconsentedMessageAndroid. Also, turn off Enhanced Protection for sync user disable flow on Android. (cherry picked from commit 998b05a4cc0713a24124bff0e786426855ae9a6c) Bug: 1291254 Change-Id: I33b193b570081562b1156ac62fd37bb5acc8a80d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3422230 Reviewed-by: Xinghui Lu Commit-Queue: Rohit Bhatia Cr-Original-Commit-Position: refs/heads/main@{#964714} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3449373 Bot-Commit: Rubber Stamper Reviewed-by: Daniel Rubery Cr-Commit-Position: refs/branch-heads/4758@{#1115} Cr-Branched-From: 4a2cf4baf90326df19c3ee70ff987960d59a386e-refs/heads/main@{#950365} --- .../chrome_tailored_security_service.cc | 11 +++---- .../consented_message_android.cc | 31 +++++++------------ .../consented_message_android.h | 10 +++--- 3 files changed, 21 insertions(+), 31 deletions(-) diff --git a/chrome/browser/safe_browsing/tailored_security/chrome_tailored_security_service.cc b/chrome/browser/safe_browsing/tailored_security/chrome_tailored_security_service.cc index 0da5979d24cd60..ad44684b46b363 100644 --- a/chrome/browser/safe_browsing/tailored_security/chrome_tailored_security_service.cc +++ b/chrome/browser/safe_browsing/tailored_security/chrome_tailored_security_service.cc @@ -76,14 +76,13 @@ void ChromeTailoredSecurityService::ShowSyncNotification(bool is_enabled) { if (!web_contents) return; - // Since the Android UX is a notice, we simply enable Enhanced Protection - // here. + // Since the Android UX is a notice, we simply set Safe Browsing state. SetSafeBrowsingState(profile_->GetPrefs(), - SafeBrowsingState::ENHANCED_PROTECTION, - /*is_esb_enabled_in_sync=*/true); + is_enabled ? SafeBrowsingState::ENHANCED_PROTECTION + : SafeBrowsingState::STANDARD_PROTECTION, + /*is_esb_enabled_in_sync=*/is_enabled); - message_ = std::make_unique(); - message_->DisplayMessage( + message_ = std::make_unique( web_contents, is_enabled, base::BindOnce(&ChromeTailoredSecurityService::MessageDismissed, // Unretained is safe because |this| owns |message_|. diff --git a/chrome/browser/safe_browsing/tailored_security/consented_message_android.cc b/chrome/browser/safe_browsing/tailored_security/consented_message_android.cc index a50dbbccd5a7ca..b3e8b374a2890e 100644 --- a/chrome/browser/safe_browsing/tailored_security/consented_message_android.cc +++ b/chrome/browser/safe_browsing/tailored_security/consented_message_android.cc @@ -27,24 +27,13 @@ void LogOutcome(TailoredSecurityOutcome outcome, bool enable) { } // namespace -TailoredSecurityConsentedModalAndroid::TailoredSecurityConsentedModalAndroid() = - default; - -TailoredSecurityConsentedModalAndroid:: - ~TailoredSecurityConsentedModalAndroid() { - DismissMessageInternal(messages::DismissReason::UNKNOWN); -} - -void TailoredSecurityConsentedModalAndroid::DisplayMessage( +TailoredSecurityConsentedModalAndroid::TailoredSecurityConsentedModalAndroid( content::WebContents* web_contents, bool enable, - base::OnceClosure dismiss_callback) { - if (message_) { - return; - } - web_contents_ = web_contents; - is_enable_message_ = enable; - dismiss_callback_ = std::move(dismiss_callback); + base::OnceClosure dismiss_callback) + : web_contents_(web_contents), + dismiss_callback_(std::move(dismiss_callback)), + is_enable_message_(enable) { message_ = std::make_unique( is_enable_message_ ? messages::MessageIdentifier::TAILORED_SECURITY_ENABLED @@ -90,13 +79,17 @@ void TailoredSecurityConsentedModalAndroid::DisplayMessage( LogOutcome(TailoredSecurityOutcome::kShown, is_enable_message_); } +TailoredSecurityConsentedModalAndroid:: + ~TailoredSecurityConsentedModalAndroid() { + DismissMessageInternal(messages::DismissReason::UNKNOWN); +} + void TailoredSecurityConsentedModalAndroid::DismissMessageInternal( messages::DismissReason dismiss_reason) { if (!message_) return; messages::MessageDispatcherBridge::Get()->DismissMessage(message_.get(), dismiss_reason); - std::move(dismiss_callback_).Run(); } void TailoredSecurityConsentedModalAndroid::HandleSettingsClicked() { @@ -110,12 +103,12 @@ void TailoredSecurityConsentedModalAndroid::HandleMessageDismissed( messages::DismissReason dismiss_reason) { LogOutcome(TailoredSecurityOutcome::kDismissed, is_enable_message_); message_.reset(); - std::move(dismiss_callback_).Run(); + if (dismiss_callback_) + std::move(dismiss_callback_).Run(); } void TailoredSecurityConsentedModalAndroid::HandleMessageAccepted() { LogOutcome(TailoredSecurityOutcome::kAccepted, is_enable_message_); - std::move(dismiss_callback_).Run(); } } // namespace safe_browsing diff --git a/chrome/browser/safe_browsing/tailored_security/consented_message_android.h b/chrome/browser/safe_browsing/tailored_security/consented_message_android.h index 55f07e4f0eaf1d..80a4d0fd05e830 100644 --- a/chrome/browser/safe_browsing/tailored_security/consented_message_android.h +++ b/chrome/browser/safe_browsing/tailored_security/consented_message_android.h @@ -19,14 +19,12 @@ namespace safe_browsing { class TailoredSecurityConsentedModalAndroid { public: - TailoredSecurityConsentedModalAndroid(); - ~TailoredSecurityConsentedModalAndroid(); - // Show the message for the given `web_contents`, when the Tailored security // setting has been `enabled`. - void DisplayMessage(content::WebContents* web_contents, - bool enabled, - base::OnceClosure dismiss_callback); + TailoredSecurityConsentedModalAndroid(content::WebContents* web_contents, + bool enabled, + base::OnceClosure dismiss_callback); + ~TailoredSecurityConsentedModalAndroid(); private: void DismissMessageInternal(messages::DismissReason dismiss_reason);