[Backport] CVE-2021-30590: Heap buffer overflow in Bookmarks

Partial cherry-pick of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/3073100:
[M90-LTS] Fix RecentlyUsedFoldersComboModel heap overflows

This fixes a few bugs:
* RecentlyUsedFoldersComboModel::RemoveNode() would not inform its
  observers of changes.
* RecentlyUsedFoldersComboModel::GetDefaultIndex() did not behave well
  after model changes (could end up using a cached out-of-bounds index).
* BubbleDialogModelHost would not pass on selected-index updates unless
  the user changed the index by performing a combobox action (not true
  when an Extension removes a bookmark folder).

This also replaces off-by-one index correction changes with CHECKs for
index correctness inside views::Combobox. This turns security bugs into
crash bugs and also is likelier to get us better crash stacks if this
happens in the wild as well.

(cherry picked from commit d2e1d6871cf7ca9dbbc82a400be49234d20f98cf)

Bug: 1227777
Change-Id: I9b851129fee4bdd249c1db77b01312b6671784be
Commit-Queue: Peter Boström <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#904551}
Reviewed-by: Achuith Bhandarkar <[email protected]>
Commit-Queue: Zakhar Voit <[email protected]>
Owners-Override: Achuith Bhandarkar <[email protected]>
Cr-Commit-Position: refs/branch-heads/4430@{#1562}
Cr-Branched-From: e5ce7dc4f7518237b3d9bb93cccca35d25216cbe-refs/heads/master@{#857950}
Reviewed-by: Allan Sandfeld Jensen <[email protected]>

Author: zakharvoit

chromium/ui/views/bubble/bubble_dialog_model_host.cc

@@ -56,9 +56,8 @@ class CheckboxControl : public Checkbox {
       : label_line_height_(label_line_height) {
     auto* layout = SetLayoutManager(std::make_unique<BoxLayout>());
     layout->set_between_child_spacing(LayoutProvider::Get()->GetDistanceMetric(
-        views::DISTANCE_RELATED_LABEL_HORIZONTAL));
-    layout->set_cross_axis_alignment(
-        views::BoxLayout::CrossAxisAlignment::kStart);
+        DISTANCE_RELATED_LABEL_HORIZONTAL));
+    layout->set_cross_axis_alignment(BoxLayout::CrossAxisAlignment::kStart);
 
     SetAssociatedLabel(label.get());
 
@@ -254,8 +253,8 @@ BubbleDialogModelHost::BubbleDialogModelHost(
   set_close_on_deactivate(model_->close_on_deactivate(GetPassKey()));
 
   set_fixed_width(LayoutProvider::Get()->GetDistanceMetric(
-      anchor_view ? views::DISTANCE_BUBBLE_PREFERRED_WIDTH
-                  : views::DISTANCE_MODAL_DIALOG_PREFERRED_WIDTH));
+      anchor_view ? DISTANCE_BUBBLE_PREFERRED_WIDTH
+                  : DISTANCE_MODAL_DIALOG_PREFERRED_WIDTH));
 
   AddInitialFields();
 }
@@ -464,17 +463,19 @@ void BubbleDialogModelHost::AddOrUpdateCombobox(
   combobox->SetCallback(base::BindRepeating(
       [](ui::DialogModelCombobox* model_field,
          base::PassKey<DialogModelHost> pass_key, Combobox* combobox) {
-        // TODO(pbos): This should be a subscription through the Combobox
-        // directly, but Combobox right now doesn't support listening to
-        // selected-index changes.
-        model_field->OnSelectedIndexChanged(pass_key,
-                                            combobox->GetSelectedIndex());
         model_field->OnPerformAction(pass_key);
       },
       model_field, GetPassKey(), combobox.get()));
 
-  // TODO(pbos): Add subscription to combobox selected-index changes.
   combobox->SetSelectedIndex(model_field->selected_index());
+  property_changed_subscriptions_.push_back(
+      combobox->AddSelectedIndexChangedCallback(base::BindRepeating(
+          [](ui::DialogModelCombobox* model_field,
+             base::PassKey<DialogModelHost> pass_key, Combobox* combobox) {
+            model_field->OnSelectedIndexChanged(pass_key,
+                                                combobox->GetSelectedIndex());
+          },
+          model_field, GetPassKey(), combobox.get())));
   const gfx::FontList& font_list = combobox->GetFontList();
   AddViewForLabelAndField(model_field, model_field->label(GetPassKey()),
                           std::move(combobox), font_list);

chromium/ui/views/controls/combobox/combobox.cc

@@ -281,6 +281,7 @@ const gfx::FontList& Combobox::GetFontList() const {
 void Combobox::SetSelectedIndex(int index) {
   if (selected_index_ == index)
     return;
+  // TODO(pbos): Add (D)CHECKs to validate the selected index.
   selected_index_ = index;
   if (size_to_largest_label_) {
     OnPropertyChanged(&selected_index_, kPropertyEffectsPaint);
@@ -290,6 +291,11 @@ void Combobox::SetSelectedIndex(int index) {
   }
 }
 
+base::CallbackListSubscription Combobox::AddSelectedIndexChangedCallback(
+    views::PropertyChangedCallback callback) {
+  return AddPropertyChangedCallback(&selected_index_, std::move(callback));
+}
+
 bool Combobox::SelectValue(const base::string16& value) {
   for (int i = 0; i < GetModel()->GetItemCount(); ++i) {
     if (value == GetModel()->GetItemAt(i)) {
@@ -430,9 +436,11 @@ bool Combobox::OnKeyPressed(const ui::KeyEvent& e) {
   // TODO(oshima): handle IME.
   DCHECK_EQ(e.type(), ui::ET_KEY_PRESSED);
 
+  // TODO(pbos): Do we need to handle selected_index_ == -1 for unselected here?
+  // Ditto on handling an empty model?
   DCHECK_GE(selected_index_, 0);
   DCHECK_LT(selected_index_, GetModel()->GetItemCount());
-  if (selected_index_ < 0 || selected_index_ > GetModel()->GetItemCount())
+  if (selected_index_ < 0 || selected_index_ >= GetModel()->GetItemCount())
     SetSelectedIndex(0);
 
   bool show_menu = false;
@@ -618,10 +626,13 @@ void Combobox::PaintIconAndText(gfx::Canvas* canvas) {
 
   // Draw the text.
   SkColor text_color = GetTextColorForEnableState(*this, GetEnabled());
-  if (selected_index_ < 0 || selected_index_ > GetModel()->GetItemCount()) {
-    NOTREACHED();
+  // TODO(pbos): Do we need to handle selected_index_ == -1 for unselected here?
+  // Ditto on handling an empty model?
+  DCHECK_GE(selected_index_, 0);
+  DCHECK_LT(selected_index_, GetModel()->GetItemCount());
+  if (selected_index_ < 0 || selected_index_ >= GetModel()->GetItemCount())
     SetSelectedIndex(0);
-  }
+
   base::string16 text = GetModel()->GetItemAt(selected_index_);
 
   int disclosure_arrow_offset = width() - kComboboxArrowContainerWidth;

chromium/ui/views/controls/combobox/combobox.h

@@ -71,6 +71,8 @@ class VIEWS_EXPORT Combobox : public View,
   // Gets/Sets the selected index.
   int GetSelectedIndex() const { return selected_index_; }
   void SetSelectedIndex(int index);
+  base::CallbackListSubscription AddSelectedIndexChangedCallback(
+      views::PropertyChangedCallback callback) WARN_UNUSED_RESULT;
 
   // Looks for the first occurrence of |value| in |model()|. If found, selects
   // the found index and returns true. Otherwise simply noops and returns false.