From c32e054faf74ca1524860a1bc7a8cb1fd92512f7 Mon Sep 17 00:00:00 2001 From: Yoichi Osato Date: Wed, 1 Mar 2023 06:38:58 +0000 Subject: [Backport] CVE-2023-1815: Use after free in Networking APIs Manual backport of patch originally reviewed on https://chromium-review.googlesource.com/c/chromium/src/+/4280021: Call each NetworkStateObserver separately. NetworkStateNotifier used to have NetworkStateObserver list as HashMap> and call each observer on a taskrunner sequentially. That caused race condition and use-after-free: what if an observer calls wait and other observer removes all? We also should not guarantee the order of registering observers is kept as notification order: each observer should not depend on others. To fix that, this patch reconstructs the structure to HashMap and call each observer on each taskrunner separately. This implementation follows base/observer_list_threadsafe.h except the taskrunner is given by the caller. Fixed: 1278708 Change-Id: Iff5d0008d5b0d98caa5931e2806db3ffc52be6fa Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4280021 Reviewed-by: Kent Tamura Commit-Queue: Yoichi Osato Cr-Commit-Position: refs/heads/main@{#1111448} Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/475426 Reviewed-by: Allan Sandfeld Jensen --- .../platform/network/network_state_notifier.cc | 154 ++++++--------------- .../platform/network/network_state_notifier.h | 33 +---- 2 files changed, 51 insertions(+), 136 deletions(-) diff --git a/chromium/third_party/blink/renderer/platform/network/network_state_notifier.cc b/chromium/third_party/blink/renderer/platform/network/network_state_notifier.cc index f4215fc1b1a..0b681b98b9f 100644 --- a/chromium/third_party/blink/renderer/platform/network/network_state_notifier.cc +++ b/chromium/third_party/blink/renderer/platform/network/network_state_notifier.cc @@ -277,54 +277,55 @@ void NetworkStateNotifier::NotifyObservers(ObserverListMap& map, DCHECK(IsMainThread()); base::AutoLock locker(lock_); for (const auto& entry : map) { - scoped_refptr task_runner = entry.key; - PostCrossThreadTask( - *task_runner, FROM_HERE, - CrossThreadBindOnce(&NetworkStateNotifier::NotifyObserversOnTaskRunner, - CrossThreadUnretained(this), - CrossThreadUnretained(&map), type, task_runner, - state)); + entry.value->PostTask( + FROM_HERE, + base::BindOnce(&NetworkStateNotifier::NotifyObserverOnTaskRunner, + base::Unretained(this), base::UnsafeDangling(entry.key), + type, state)); } } -void NetworkStateNotifier::NotifyObserversOnTaskRunner( - ObserverListMap* map, +void NetworkStateNotifier::NotifyObserverOnTaskRunner( + NetworkStateObserver* observer, ObserverType type, - scoped_refptr task_runner, const NetworkState& state) { - ObserverList* observer_list = LockAndFindObserverList(*map, task_runner); - - // The context could have been removed before the notification task got to - // run. - if (!observer_list) - return; - - DCHECK(task_runner->RunsTasksInCurrentSequence()); - - observer_list->iterating = true; - - for (wtf_size_t i = 0; i < observer_list->observers.size(); ++i) { - // Observers removed during iteration are zeroed out, skip them. - if (!observer_list->observers[i]) - continue; - switch (type) { - case ObserverType::kOnLineState: - observer_list->observers[i]->OnLineStateChange(state.on_line); - continue; - case ObserverType::kConnectionType: - observer_list->observers[i]->ConnectionChange( - state.type, state.max_bandwidth_mbps, state.effective_type, - state.http_rtt, state.transport_rtt, state.downlink_throughput_mbps, - state.save_data); - continue; + { + base::AutoLock locker(lock_); + ObserverListMap& map = GetObserverMapFor(type); + // It's safe to pass a MayBeDangling pointer to find(). + ObserverListMap::iterator it = map.find(observer); + if (map.end() == it) { + return; } - NOTREACHED(); + DCHECK(it->value->RunsTasksInCurrentSequence()); } - observer_list->iterating = false; + switch (type) { + case ObserverType::kOnLineState: + observer->OnLineStateChange(state.on_line); + return; + case ObserverType::kConnectionType: + observer->ConnectionChange( + state.type, state.max_bandwidth_mbps, state.effective_type, + state.http_rtt, state.transport_rtt, state.downlink_throughput_mbps, + state.save_data); + return; + default: + NOTREACHED(); + } +} - if (!observer_list->zeroed_observers.empty()) - CollectZeroedObservers(*map, observer_list, std::move(task_runner)); +NetworkStateNotifier::ObserverListMap& NetworkStateNotifier::GetObserverMapFor( + ObserverType type) { + switch (type) { + case ObserverType::kConnectionType: + return connection_observers_; + case ObserverType::kOnLineState: + return on_line_state_observers_; + default: + NOTREACHED(); + return connection_observers_; + } } void NetworkStateNotifier::AddObserverToMap( @@ -336,85 +337,20 @@ void NetworkStateNotifier::AddObserverToMap( base::AutoLock locker(lock_); ObserverListMap::AddResult result = - map.insert(std::move(task_runner), nullptr); - if (result.is_new_entry) - result.stored_value->value = std::make_unique(); - - DCHECK(result.stored_value->value->observers.Find(observer) == kNotFound); - result.stored_value->value->observers.push_back(observer); + map.insert(observer, std::move(task_runner)); + DCHECK(result.is_new_entry); } void NetworkStateNotifier::RemoveObserver( ObserverType type, NetworkStateObserver* observer, scoped_refptr task_runner) { - switch (type) { - case ObserverType::kConnectionType: - RemoveObserverFromMap(connection_observers_, observer, - std::move(task_runner)); - break; - case ObserverType::kOnLineState: - RemoveObserverFromMap(on_line_state_observers_, observer, - std::move(task_runner)); - break; - } -} - -void NetworkStateNotifier::RemoveObserverFromMap( - ObserverListMap& map, - NetworkStateObserver* observer, - scoped_refptr task_runner) { DCHECK(task_runner->RunsTasksInCurrentSequence()); DCHECK(observer); - ObserverList* observer_list = LockAndFindObserverList(map, task_runner); - if (!observer_list) - return; - - Vector& observers = observer_list->observers; - wtf_size_t index = observers.Find(observer); - if (index != kNotFound) { - observers[index] = 0; - observer_list->zeroed_observers.push_back(index); - } - - if (!observer_list->iterating && !observer_list->zeroed_observers.empty()) - CollectZeroedObservers(map, observer_list, std::move(task_runner)); -} - -NetworkStateNotifier::ObserverList* -NetworkStateNotifier::LockAndFindObserverList( - ObserverListMap& map, - scoped_refptr task_runner) { - base::AutoLock locker(lock_); - ObserverListMap::iterator it = map.find(task_runner); - return it == map.end() ? nullptr : it->value.get(); -} - -void NetworkStateNotifier::CollectZeroedObservers( - ObserverListMap& map, - ObserverList* list, - scoped_refptr task_runner) { - DCHECK(task_runner->RunsTasksInCurrentSequence()); - DCHECK(!list->iterating); - - // If any observers were removed during the iteration they will have - // 0 values, clean them up. - std::sort(list->zeroed_observers.begin(), list->zeroed_observers.end()); - int removed = 0; - for (wtf_size_t i = 0; i < list->zeroed_observers.size(); ++i) { - int index_to_remove = list->zeroed_observers[i] - removed; - DCHECK_EQ(nullptr, list->observers[index_to_remove]); - list->observers.EraseAt(index_to_remove); - removed += 1; - } - - list->zeroed_observers.clear(); - - if (list->observers.empty()) { - base::AutoLock locker(lock_); - map.erase(task_runner); // deletes list - } + ObserverListMap& map = GetObserverMapFor(type); + DCHECK_NE(map.end(), map.find(observer)); + map.erase(observer); } // static diff --git a/chromium/third_party/blink/renderer/platform/network/network_state_notifier.h b/chromium/third_party/blink/renderer/platform/network/network_state_notifier.h index 7159ec45636..b291e394923 100644 --- a/chromium/third_party/blink/renderer/platform/network/network_state_notifier.h +++ b/chromium/third_party/blink/renderer/platform/network/network_state_notifier.h @@ -317,13 +317,6 @@ class PLATFORM_EXPORT NetworkStateNotifier { private: friend class NetworkStateObserverHandle; - struct ObserverList { - ObserverList() : iterating(false) {} - bool iterating; - Vector observers; - Vector zeroed_observers; // Indices in observers that are 0. - }; - // This helper scope issues required notifications when mutating the state if // something has changed. It's only possible to mutate the state on the main // thread. Note that ScopedNotifier must be destroyed when not holding a lock @@ -342,14 +335,14 @@ class PLATFORM_EXPORT NetworkStateNotifier { // The ObserverListMap is cross-thread accessed, adding/removing Observers // running on a task runner. - using ObserverListMap = HashMap, - std::unique_ptr>; + using ObserverListMap = HashMap>; void NotifyObservers(ObserverListMap&, ObserverType, const NetworkState&); - void NotifyObserversOnTaskRunner(ObserverListMap*, - ObserverType, - scoped_refptr, - const NetworkState&); + void NotifyObserverOnTaskRunner(NetworkStateObserver*, + ObserverType, + const NetworkState&); + ObserverListMap& GetObserverMapFor(ObserverType); void AddObserverToMap(ObserverListMap&, NetworkStateObserver*, @@ -357,20 +350,6 @@ class PLATFORM_EXPORT NetworkStateNotifier { void RemoveObserver(ObserverType, NetworkStateObserver*, scoped_refptr); - void RemoveObserverFromMap(ObserverListMap&, - NetworkStateObserver*, - scoped_refptr); - - ObserverList* LockAndFindObserverList( - ObserverListMap&, - scoped_refptr); - - // Removed observers are nulled out in the list in case the list is being - // iterated over. Once done iterating, call this to clean up nulled - // observers. - void CollectZeroedObservers(ObserverListMap&, - ObserverList*, - scoped_refptr); // A random number by which the RTT and downlink estimates are multiplied // with. The returned random multiplier is a function of the hostname. -- cgit v1.2.1