[Backport] CVE-2021-30512: Use after free in Notifications

Partial cherry-pick of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/2838205:
Notifications: crash if improper action icons sent from renderer.

Previously, the code only called DCHECK but as this data is from a
renderer we should probably crash the browser.

Bug: 1200019
Change-Id: If4d9d48c8e18a3ed9c8bb3a50b952591259e0db5
Commit-Queue: Justin DeWitt <[email protected]>
Reviewed-by: Peter Beverloo <[email protected]>
Cr-Commit-Position: refs/heads/master@{#875788}
Reviewed-by: Allan Sandfeld Jensen <[email protected]>

Author: mrdewitt

chromium/content/browser/notifications/blink_notification_service_impl.cc

@@ -39,6 +39,9 @@ const char kBadMessageImproperNotificationImage[] =
     "disabled.";
 const char kBadMessageInvalidNotificationTriggerTimestamp[] =
     "Received an invalid notification trigger timestamp.";
+const char kBadMessageInvalidNotificationActionButtons[] =
+    "Received a notification with a number of action images that does not "
+    "match the number of actions.";
 
 // Returns the implementation of the PlatformNotificationService. May be NULL.
 PlatformNotificationService* GetNotificationService(
@@ -132,7 +135,8 @@ void BlinkNotificationServiceImpl::DisplayNonPersistentNotification(
     mojo::PendingRemote<blink::mojom::NonPersistentNotificationListener>
         event_listener_remote) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (!ValidateNotificationResources(notification_resources))
+  if (!ValidateNotificationDataAndResources(platform_notification_data,
+                                            notification_resources))
     return;
 
   if (!GetNotificationService(browser_context_))
@@ -187,28 +191,31 @@ BlinkNotificationServiceImpl::CheckPermissionStatus() {
                             origin_.GetURL());
 }
 
-bool BlinkNotificationServiceImpl::ValidateNotificationResources(
+bool BlinkNotificationServiceImpl::ValidateNotificationDataAndResources(
+    const blink::PlatformNotificationData& platform_notification_data,
     const blink::NotificationResources& notification_resources) {
-  if (notification_resources.image.drawsNothing() ||
-      base::FeatureList::IsEnabled(features::kNotificationContentImage))
-    return true;
-  receiver_.ReportBadMessage(kBadMessageImproperNotificationImage);
-  // The above ReportBadMessage() closes |binding_| but does not trigger its
-  // connection error handler, so we need to call the error handler explicitly
-  // here to do some necessary work.
-  OnConnectionError();
-  return false;
-}
+  if (platform_notification_data.actions.size() !=
+      notification_resources.action_icons.size()) {
+    receiver_.ReportBadMessage(kBadMessageInvalidNotificationActionButtons);
+    OnConnectionError();
+    return false;
+  }
 
-// Checks if this notification has a valid trigger.
-bool BlinkNotificationServiceImpl::ValidateNotificationData(
-    const blink::PlatformNotificationData& notification_data) {
-  if (!CheckNotificationTriggerRange(notification_data)) {
+  if (!CheckNotificationTriggerRange(platform_notification_data)) {
     receiver_.ReportBadMessage(kBadMessageInvalidNotificationTriggerTimestamp);
     OnConnectionError();
     return false;
   }
 
+  if (!notification_resources.image.drawsNothing() &&
+      !base::FeatureList::IsEnabled(features::kNotificationContentImage)) {
+    receiver_.ReportBadMessage(kBadMessageImproperNotificationImage);
+    // The above ReportBadMessage() closes |binding_| but does not trigger its
+    // connection error handler, so we need to call the error handler explicitly
+    // here to do some necessary work.
+    OnConnectionError();
+    return false;
+  }
   return true;
 }
 
@@ -218,10 +225,8 @@ void BlinkNotificationServiceImpl::DisplayPersistentNotification(
     const blink::NotificationResources& notification_resources,
     DisplayPersistentNotificationCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (!ValidateNotificationResources(notification_resources))
-    return;
-
-  if (!ValidateNotificationData(platform_notification_data))
+  if (!ValidateNotificationDataAndResources(platform_notification_data,
+                                            notification_resources))
     return;
 
   if (!GetNotificationService(browser_context_)) {

chromium/content/browser/notifications/blink_notification_service_impl.h

@@ -71,20 +71,15 @@ class CONTENT_EXPORT BlinkNotificationServiceImpl
   // Check the permission status for the current |origin_|.
   blink::mojom::PermissionStatus CheckPermissionStatus();
 
-  // Validate |notification_resources| received in a Mojo IPC message.
-  // If the validation failed, we'd close the Mojo connection |binding_| and
-  // destroy |this| by calling OnConnectionError() directly, then return false.
-  // So, please do not touch |this| again after you got a false return value.
-  bool ValidateNotificationResources(
+  // Validate |notification_data| and |notification_resources| received in a
+  // Mojo IPC message. If the validation failed, we'd close the Mojo connection
+  // |binding_| and destroy |this| by calling OnConnectionError() directly, then
+  // return false. So, please do not touch |this| again after you got a false
+  // return value.
+  bool ValidateNotificationDataAndResources(
+      const blink::PlatformNotificationData& notification_data,
       const blink::NotificationResources& notification_resources);
 
-  // Validate |notification_data| received in a Mojo IPC message.
-  // If the validation failed, we'd close the Mojo connection |binding_| and
-  // destroy |this| by calling OnConnectionError() directly, then return false.
-  // So, please do not touch |this| again after you got a false return value.
-  bool ValidateNotificationData(
-      const blink::PlatformNotificationData& notification_data);
-
   void DidWriteNotificationData(DisplayPersistentNotificationCallback callback,
                                 bool success,
                                 const std::string& notification_id);