From aac48dd2cd1f34beb4d29a409c24f1ff4f98213c Mon Sep 17 00:00:00 2001 From: Josh Karlin Date: Tue, 2 Mar 2021 03:00:18 +0000 Subject: [Backport] CVE-2021-21214: Use after free in Network API Cherry-pick of patch originally reviewed on https://chromium-review.googlesource.com/c/chromium/src/+/2727306: Fix removal of observers in NetworkStateNotifier The NetworkStateNotifier has a per-thread list of observer pointers. If one is deleted mid-iteration, what we do is replace the pointer in the list with a 0, and add the index to the zeroed list of observers to remove after iteration completes. Well, the removal step was broken for cases where there were multiple elements to remove. It didn't adjust for the fact that the indexes shifted after each removal. Bug: 1170148 Change-Id: I446acaae5f8a805a58142848634a0ee8c5f90882 Reviewed-by: Kentaro Hara Commit-Queue: Josh Karlin Cr-Commit-Position: refs/heads/master@{#858853} Reviewed-by: Allan Sandfeld Jensen --- .../platform/network/network_state_notifier.cc | 10 ++++- .../network/network_state_notifier_test.cc | 47 ++++++++++++++++++++++ 2 files changed, 55 insertions(+), 2 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 32328e88187..c7227f63483 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 @@ -395,8 +395,14 @@ void NetworkStateNotifier::CollectZeroedObservers( // If any observers were removed during the iteration they will have // 0 values, clean them up. - for (wtf_size_t i = 0; i < list->zeroed_observers.size(); ++i) - list->observers.EraseAt(list->zeroed_observers[i]); + 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(); diff --git a/chromium/third_party/blink/renderer/platform/network/network_state_notifier_test.cc b/chromium/third_party/blink/renderer/platform/network/network_state_notifier_test.cc index eb2bd791529..f7c359235a8 100644 --- a/chromium/third_party/blink/renderer/platform/network/network_state_notifier_test.cc +++ b/chromium/third_party/blink/renderer/platform/network/network_state_notifier_test.cc @@ -528,6 +528,53 @@ TEST_F(NetworkStateNotifierTest, RemoveFutureObserverWhileNotifying) { kUnknownThroughputMbps, SaveData::kOff)); } +// It should be safe to remove multiple observers in one iteration. +TEST_F(NetworkStateNotifierTest, RemoveMultipleObserversWhileNotifying) { + StateObserver observer1, observer2, observer3; + std::unique_ptr handle1 = + notifier_.AddConnectionObserver(&observer1, GetTaskRunner()); + std::unique_ptr handle2 = + notifier_.AddConnectionObserver(&observer2, GetTaskRunner()); + std::unique_ptr handle3 = + notifier_.AddConnectionObserver(&observer3, GetTaskRunner()); + observer1.RemoveObserverOnNotification(std::move(handle1)); + observer3.RemoveObserverOnNotification(std::move(handle3)); + + // Running the first time should delete observers 1 and 3. + SetConnection(kWebConnectionTypeBluetooth, kBluetoothMaxBandwidthMbps, + WebEffectiveConnectionType::kTypeUnknown, kUnknownRtt, + kUnknownRtt, kUnknownThroughputMbps, SaveData::kOff); + EXPECT_TRUE(VerifyObservations( + observer1, kWebConnectionTypeBluetooth, kBluetoothMaxBandwidthMbps, + WebEffectiveConnectionType::kTypeUnknown, kUnknownRtt, kUnknownRtt, + kUnknownThroughputMbps, SaveData::kOff)); + EXPECT_TRUE(VerifyObservations( + observer2, kWebConnectionTypeBluetooth, kBluetoothMaxBandwidthMbps, + WebEffectiveConnectionType::kTypeUnknown, kUnknownRtt, kUnknownRtt, + kUnknownThroughputMbps, SaveData::kOff)); + EXPECT_TRUE(VerifyObservations( + observer3, kWebConnectionTypeBluetooth, kBluetoothMaxBandwidthMbps, + WebEffectiveConnectionType::kTypeUnknown, kUnknownRtt, kUnknownRtt, + kUnknownThroughputMbps, SaveData::kOff)); + + // Run again and only observer 2 should have been updated. + SetConnection(kWebConnectionTypeEthernet, kEthernetMaxBandwidthMbps, + WebEffectiveConnectionType::kTypeUnknown, kUnknownRtt, + kUnknownRtt, kUnknownThroughputMbps, SaveData::kOff); + EXPECT_TRUE(VerifyObservations( + observer1, kWebConnectionTypeBluetooth, kBluetoothMaxBandwidthMbps, + WebEffectiveConnectionType::kTypeUnknown, kUnknownRtt, kUnknownRtt, + kUnknownThroughputMbps, SaveData::kOff)); + EXPECT_TRUE(VerifyObservations( + observer2, kWebConnectionTypeEthernet, kEthernetMaxBandwidthMbps, + WebEffectiveConnectionType::kTypeUnknown, kUnknownRtt, kUnknownRtt, + kUnknownThroughputMbps, SaveData::kOff)); + EXPECT_TRUE(VerifyObservations( + observer3, kWebConnectionTypeBluetooth, kBluetoothMaxBandwidthMbps, + WebEffectiveConnectionType::kTypeUnknown, kUnknownRtt, kUnknownRtt, + kUnknownThroughputMbps, SaveData::kOff)); +} + TEST_F(NetworkStateNotifierTest, MultipleContextsAddObserver) { StateObserver observer1, observer2; std::unique_ptr handle1 = -- cgit v1.2.3