From ac28a391ebb35f55e977838e178f245d5086dcc2 Mon Sep 17 00:00:00 2001 From: cdesouza-chromium Date: Thu, 31 Oct 2024 03:35:27 +0000 Subject: [PATCH] [DanglingPtr] Fix UaFs with `SidePanelEntry` observations (#26288) [DanglingPtr] Fix UaFs with `SidePanelEntry` observation This change addresses the uaf cases we were having with the multisource observation for `SidePanelEntry` on `SidebarContainerView`. The main issue we were having is that we can't precisely be notified of when a given `SidePanelEntry` is going out of scope, and in many cases the instances would be destroyed before the lifetime of `SidebarContainerView`. In such cases, the observation would be kept around in the multisource observer, and eventually would cause an UaF, as the multisource observer was being destroyed and trying to remove the outstanding observations that were dangling. `SidePanelEntry` observations do dangle in upstream. That's the model they have for the lifetimes: FeatureFoo is destroyed before `SidePanelEntry`. There are exactly 2 ways for a `SidePanelEntry` to be destroyed in chromium: FeatureFoo destroyes it via Deregister, or `SidePanelRegistry` is destroyed before `FeatureFoo`. `SidebarContainerView` cannot in a reliable way keep track of these guarantees. This change is predicated on the fact that `SidebarContainerView` will always outlive all the use of events by any living `SidePanelEntry` instance. In this change we introduce a customisation point to `SidePanelEntry` that allows us to check if a certain entry is being observed or not. If it is we don't double-observe. Observations are then removed when a tab is destroyed, which includes when it gets moved to another Browser window. Resolves brave/brave-browser#39053 Resolves brave/brave-browser#41924 --- browser/ui/sidebar/sidebar_browsertest.cc | 16 ---- browser/ui/views/frame/brave_browser_view.cc | 4 +- browser/ui/views/frame/brave_browser_view.h | 2 +- .../brave_side_panel_coordinator.cc | 17 +--- .../views/sidebar/sidebar_container_view.cc | 86 +++++++------------ .../ui/views/sidebar/sidebar_container_view.h | 16 +--- .../views/side_panel/side_panel_coordinator.h | 2 - .../ui/views/side_panel/side_panel_entry.cc | 12 +++ .../ui/views/side_panel/side_panel_entry.h | 17 ++++ 9 files changed, 66 insertions(+), 106 deletions(-) create mode 100644 chromium_src/chrome/browser/ui/views/side_panel/side_panel_entry.cc create mode 100644 chromium_src/chrome/browser/ui/views/side_panel/side_panel_entry.h diff --git a/browser/ui/sidebar/sidebar_browsertest.cc b/browser/ui/sidebar/sidebar_browsertest.cc index 92bc7ba2f977..031382a43a8d 100644 --- a/browser/ui/sidebar/sidebar_browsertest.cc +++ b/browser/ui/sidebar/sidebar_browsertest.cc @@ -286,18 +286,6 @@ class SidebarBrowserTest : public InProcessBrowserTest { return std::distance(items.cbegin(), iter); } - bool SidebarContainerObserving(SidePanelEntryId id) { - auto* sidebar_container_view = - static_cast(controller()->sidebar()); - for (const auto& entry : - sidebar_container_view->panel_entry_observations_.sources()) { - if (entry->key().id() == id) { - return true; - } - } - return false; - } - raw_ptr item_added_bubble_anchor_ = nullptr; std::unique_ptr run_loop_; base::WeakPtrFactory weak_factory_{this}; @@ -931,10 +919,6 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, TabSpecificAndGlobalPanelsTest) { WaitUntil(base::BindLambdaForTesting([&]() { return panel_ui->GetCurrentEntryId() == SidePanelEntryId::kBookmarks; })); - // As previous customize panel per-url panel, it's closed by deregistering - // after loading another url. Then, sidebar container should stop observing - // its entry. - EXPECT_FALSE(SidebarContainerObserving(SidePanelEntryId::kCustomizeChrome)); } IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, DisabledItemsTest) { diff --git a/browser/ui/views/frame/brave_browser_view.cc b/browser/ui/views/frame/brave_browser_view.cc index 90bfd2d96b2d..967be2c34919 100644 --- a/browser/ui/views/frame/brave_browser_view.cc +++ b/browser/ui/views/frame/brave_browser_view.cc @@ -778,8 +778,8 @@ WalletButton* BraveBrowserView::GetWalletButton() { return static_cast(toolbar())->wallet_button(); } -void BraveBrowserView::WillShowSidePanel(bool show_on_deregistered) { - sidebar_container_view_->WillShowSidePanel(show_on_deregistered); +void BraveBrowserView::WillShowSidePanel() { + sidebar_container_view_->WillShowSidePanel(); } void BraveBrowserView::NotifyDialogPositionRequiresUpdate() { diff --git a/browser/ui/views/frame/brave_browser_view.h b/browser/ui/views/frame/brave_browser_view.h index 9460756d2e89..86faa00d12e2 100644 --- a/browser/ui/views/frame/brave_browser_view.h +++ b/browser/ui/views/frame/brave_browser_view.h @@ -81,7 +81,7 @@ class BraveBrowserView : public BrowserView, void CloseWalletBubble(); WalletButton* GetWalletButton(); views::View* GetWalletButtonAnchorView(); - void WillShowSidePanel(bool show_on_deregistered); + void WillShowSidePanel(); // Triggers layout of web modal dialogs void NotifyDialogPositionRequiresUpdate(); diff --git a/browser/ui/views/side_panel/brave_side_panel_coordinator.cc b/browser/ui/views/side_panel/brave_side_panel_coordinator.cc index 4750019371db..78198d32c9d5 100644 --- a/browser/ui/views/side_panel/brave_side_panel_coordinator.cc +++ b/browser/ui/views/side_panel/brave_side_panel_coordinator.cc @@ -152,22 +152,7 @@ void BraveSidePanelCoordinator::PopulateSidePanel( // global or active tab's contextual registry. auto* brave_browser_view = static_cast(browser_view_); CHECK(browser_view_->unified_side_panel()->children().size() == 1); - const auto content_wrapper = - browser_view_->unified_side_panel()->children()[0]; - bool show_on_deregistered = false; - // If current entry is not null and its content is already removed from - // wrapper when new panel is about to be shown, new panel is shown by - // deregistering current panel. See the comment of - // SidebarContainerView::WillShowSidePanel() about why |show_on_deregistered| - // is needed. - // Instead, we can override base class' Show(const UniqueKey& entry, ...) - // method as we can know whether it's shown by deregistering with - // |open_trigger| arg. However, need patching to make it virtual as base class - // already have overridden same name methods. - if (current_entry_.get() && content_wrapper->children().empty()) { - show_on_deregistered = true; - } - brave_browser_view->WillShowSidePanel(show_on_deregistered); + brave_browser_view->WillShowSidePanel(); SidePanelCoordinator::PopulateSidePanel(supress_animations, unique_key, entry, std::move(content_view)); } diff --git a/browser/ui/views/sidebar/sidebar_container_view.cc b/browser/ui/views/sidebar/sidebar_container_view.cc index 78445c941098..19851744c6aa 100644 --- a/browser/ui/views/sidebar/sidebar_container_view.cc +++ b/browser/ui/views/sidebar/sidebar_container_view.cc @@ -175,9 +175,7 @@ bool SidebarContainerView::IsSidebarVisible() const { return sidebar_control_view_ && sidebar_control_view_->GetVisible(); } -void SidebarContainerView::WillShowSidePanel(bool show_on_deregistered) { - deregister_when_hidden_ = show_on_deregistered; - +void SidebarContainerView::WillShowSidePanel() { // It's good timing to start observing any panel entries // from global and contextual if not yet observed. auto* tab_model = browser_->tab_strip_model(); @@ -189,7 +187,7 @@ void SidebarContainerView::WillShowSidePanel(bool show_on_deregistered) { auto* global_registry = side_panel_coordinator_->GetWindowRegistry(); for (const auto& entry : global_registry->entries()) { - StartObservingForEntry(entry.get()); + AddSidePanelEntryObservation(entry.get()); } } @@ -773,11 +771,6 @@ void SidebarContainerView::OnEntryShown(SidePanelEntry* entry) { void SidebarContainerView::OnEntryHidden(SidePanelEntry* entry) { DVLOG(1) << "Panel hidden: " << SidePanelEntryIdToString(entry->key().id()); - if (deregister_when_hidden_) { - StopObservingForEntry(entry); - deregister_when_hidden_ = false; - } - auto* controller = GetBraveBrowser()->sidebar_controller(); // Handling if |entry| is managed one. @@ -809,26 +802,22 @@ void SidebarContainerView::OnEntryHidden(SidePanelEntry* entry) { } } -void SidebarContainerView::OnEntryWillHide(SidePanelEntry* entry, - SidePanelEntryHideReason reason) { - DVLOG(1) << "Panel will hide: " - << SidePanelEntryIdToString(entry->key().id()); - - // If |reason| is panel closing, we could deregister. And it'll be - // re-registered when panel is shown if that entry is still live in tab's - // registry. - // We only stop observing when |entry|'s panel is hidden by closing. - // If it's hidden by replacing with other panel, we should not stop - // to know the timing that it's shown again. - if (reason == SidePanelEntryHideReason::kSidePanelClosed) { - deregister_when_hidden_ = true; - } -} - void SidebarContainerView::OnTabWillBeRemoved(content::WebContents* contents, int index) { // At this time, we can stop observing as TabFeatures is available. - StopObservingContextualSidePanelEntry(contents); + auto* tab = tabs::TabInterface::GetFromContents(contents); + if (!tab->GetTabFeatures()) { + return; + } + + auto* registry = tab->GetTabFeatures()->side_panel_registry(); + if (!registry) { + return; + } + + for (const auto& entry : registry->entries()) { + RemoveSidePanelEntryObservation(entry.get()); + } } void SidebarContainerView::UpdateActiveItemState() { @@ -885,23 +874,6 @@ void SidebarContainerView::OnTabStripModelChanged( } } -void SidebarContainerView::StopObservingContextualSidePanelEntry( - content::WebContents* contents) { - auto* tab = tabs::TabInterface::GetFromContents(contents); - if (!tab->GetTabFeatures()) { - return; - } - - auto* registry = tab->GetTabFeatures()->side_panel_registry(); - if (!registry) { - return; - } - - for (const auto& entry : registry->entries()) { - StopObservingForEntry(entry.get()); - } -} - void SidebarContainerView::StartObservingContextualSidePanelEntry( content::WebContents* contents) { auto* tab = tabs::TabInterface::GetFromContents(contents); @@ -914,8 +886,9 @@ void SidebarContainerView::StartObservingContextualSidePanelEntry( return; } + // Adding observations for the side panel entries from tab not seen before. for (const auto& entry : registry->entries()) { - StartObservingForEntry(entry.get()); + AddSidePanelEntryObservation(entry.get()); } SharedPinnedTabService* shared_pinned_tab_service = @@ -937,24 +910,23 @@ void SidebarContainerView::StartObservingContextualSidePanelEntry( } } -void SidebarContainerView::StartObservingForEntry(SidePanelEntry* entry) { - if (!panel_entry_observations_.IsObservingSource(entry)) { - DVLOG(1) << "Observing panel entry: " - << SidePanelEntryIdToString(entry->key().id()); - panel_entry_observations_.AddObservation(entry); - } +BraveBrowser* SidebarContainerView::GetBraveBrowser() const { + return static_cast(browser_.get()); } -void SidebarContainerView::StopObservingForEntry(SidePanelEntry* entry) { - if (panel_entry_observations_.IsObservingSource(entry)) { - DVLOG(1) << "Removing panel entry observation: " - << SidePanelEntryIdToString(entry->key().id()); - panel_entry_observations_.RemoveObservation(entry); +void SidebarContainerView::AddSidePanelEntryObservation(SidePanelEntry* entry) { + if (entry->IsBeingObservedBy(this)) { + return; } + entry->AddObserver(this); } -BraveBrowser* SidebarContainerView::GetBraveBrowser() const { - return static_cast(browser_.get()); +void SidebarContainerView::RemoveSidePanelEntryObservation( + SidePanelEntry* entry) { + if (!entry->IsBeingObservedBy(this)) { + return; + } + entry->RemoveObserver(this); } BEGIN_METADATA(SidebarContainerView) diff --git a/browser/ui/views/sidebar/sidebar_container_view.h b/browser/ui/views/sidebar/sidebar_container_view.h index 6cd395c95bee..b5a3b4ebac49 100644 --- a/browser/ui/views/sidebar/sidebar_container_view.h +++ b/browser/ui/views/sidebar/sidebar_container_view.h @@ -10,8 +10,6 @@ #include #include "base/memory/raw_ptr.h" -#include "base/scoped_multi_source_observation.h" -#include "base/scoped_observation.h" #include "base/timer/timer.h" #include "brave/browser/ui/sidebar/sidebar.h" #include "brave/browser/ui/sidebar/sidebar_model.h" @@ -86,8 +84,7 @@ class SidebarContainerView // By using OnEntryWillHide()'s arg, we can know this hidden is from // closing or not. Fortunately, we can know when WillShowSidePanel() // is called whether it's closing from deregistration. - // With |show_on_deregistered|, we can stop observing OnEntryHidden(). - void WillShowSidePanel(bool show_on_deregistered); + void WillShowSidePanel(); bool IsFullscreenForCurrentEntry() const; void set_operation_from_active_tab_change(bool tab_change) { @@ -130,8 +127,6 @@ class SidebarContainerView // SidePanelEntryObserver: void OnEntryShown(SidePanelEntry* entry) override; void OnEntryHidden(SidePanelEntry* entry) override; - void OnEntryWillHide(SidePanelEntry* entry, - SidePanelEntryHideReason reason) override; // TabStripModelObserver: void OnTabStripModelChanged( @@ -192,15 +187,15 @@ class SidebarContainerView void StopBrowserWindowEventMonitoring(); void StartObservingContextualSidePanelEntry(content::WebContents* contents); - void StopObservingContextualSidePanelEntry(content::WebContents* contents); - void StartObservingForEntry(SidePanelEntry* entry); - void StopObservingForEntry(SidePanelEntry* entry); void UpdateActiveItemState(); // Casts |browser_| to BraveBrowser, as storing it as BraveBrowser would cause // a precocious downcast. BraveBrowser* GetBraveBrowser() const; + void AddSidePanelEntryObservation(SidePanelEntry* entry); + void RemoveSidePanelEntryObservation(SidePanelEntry* entry); + raw_ptr browser_ = nullptr; raw_ptr side_panel_coordinator_ = nullptr; raw_ptr side_panel_ = nullptr; @@ -209,7 +204,6 @@ class SidebarContainerView bool initialized_ = false; bool sidebar_on_left_ = true; bool operation_from_active_tab_change_ = false; - bool deregister_when_hidden_ = false; base::OneShotTimer sidebar_hide_timer_; sidebar::SidebarService::ShowSidebarOption show_sidebar_option_ = sidebar::SidebarService::ShowSidebarOption::kShowAlways; @@ -225,8 +219,6 @@ class SidebarContainerView base::ScopedObservation sidebar_model_observation_{this}; - base::ScopedMultiSourceObservation - panel_entry_observations_{this}; }; #endif // BRAVE_BROWSER_UI_VIEWS_SIDEBAR_SIDEBAR_CONTAINER_VIEW_H_ diff --git a/chromium_src/chrome/browser/ui/views/side_panel/side_panel_coordinator.h b/chromium_src/chrome/browser/ui/views/side_panel/side_panel_coordinator.h index 9dbafde3487e..09cb0bdb9730 100644 --- a/chromium_src/chrome/browser/ui/views/side_panel/side_panel_coordinator.h +++ b/chromium_src/chrome/browser/ui/views/side_panel/side_panel_coordinator.h @@ -34,11 +34,9 @@ virtual NotifyPinnedContainerOfActiveStateChange #define PopulateSidePanel virtual PopulateSidePanel -#define OnEntryWillDeregister virtual OnEntryWillDeregister #include "src/chrome/browser/ui/views/side_panel/side_panel_coordinator.h" // IWYU pragma: export -#undef OnEntryWillDeregister #undef PopulateSidePanel #undef NotifyPinnedContainerOfActiveStateChange #undef CreateHeader diff --git a/chromium_src/chrome/browser/ui/views/side_panel/side_panel_entry.cc b/chromium_src/chrome/browser/ui/views/side_panel/side_panel_entry.cc new file mode 100644 index 000000000000..cf0ce8fc21aa --- /dev/null +++ b/chromium_src/chrome/browser/ui/views/side_panel/side_panel_entry.cc @@ -0,0 +1,12 @@ +/* Copyright (c) 2024 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "chrome/browser/ui/views/side_panel/side_panel_entry.h" + +#include "src/chrome/browser/ui/views/side_panel/side_panel_entry.cc" + +bool SidePanelEntry::IsBeingObservedBy(SidePanelEntryObserver* observer) { + return observers_.HasObserver(observer); +} diff --git a/chromium_src/chrome/browser/ui/views/side_panel/side_panel_entry.h b/chromium_src/chrome/browser/ui/views/side_panel/side_panel_entry.h new file mode 100644 index 000000000000..25b056890129 --- /dev/null +++ b/chromium_src/chrome/browser/ui/views/side_panel/side_panel_entry.h @@ -0,0 +1,17 @@ +/* Copyright (c) 2024 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_VIEWS_SIDE_PANEL_SIDE_PANEL_ENTRY_H_ +#define BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_VIEWS_SIDE_PANEL_SIDE_PANEL_ENTRY_H_ + +#define SupportsNewTabButton(...) \ + SupportsNewTabButton(__VA_ARGS__); \ + bool IsBeingObservedBy(SidePanelEntryObserver* observer) + +#include "src/chrome/browser/ui/views/side_panel/side_panel_entry.h" // IWYU pragma: export + +#undef SupportsNewTabButton + +#endif // BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_VIEWS_SIDE_PANEL_SIDE_PANEL_ENTRY_H_