[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 998b05a) Bug: 1291254 Change-Id: I33b193b570081562b1156ac62fd37bb5acc8a80d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3422230 Reviewed-by: Xinghui Lu <xinghuilu@chromium.org> Commit-Queue: Rohit Bhatia <bhatiarohit@google.com> Cr-Original-Commit-Position: refs/heads/main@{#964714} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3449373 Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Reviewed-by: Daniel Rubery <drubery@chromium.org> Cr-Commit-Position: refs/branch-heads/4758@{#1115} Cr-Branched-From: 4a2cf4b-refs/heads/main@{#950365}
chromium · Feb 9, 2022 · f5de310 · f5de310
1 parent d3dcbbe
commit f5de310
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 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
@@ -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<TailoredSecurityConsentedModalAndroid>();
-  message_->DisplayMessage(
+  message_ = std::make_unique<TailoredSecurityConsentedModalAndroid>(
       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
@@ -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<messages::MessageWrapper>(
       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
@@ -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);