[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 <haraken@chromium.org>
Commit-Queue: Josh Karlin <jkarlin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#858853}
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

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<NetworkStateNotifier::NetworkStateObserverHandle> handle1 =
+      notifier_.AddConnectionObserver(&observer1, GetTaskRunner());
+  std::unique_ptr<NetworkStateNotifier::NetworkStateObserverHandle> handle2 =
+      notifier_.AddConnectionObserver(&observer2, GetTaskRunner());
+  std::unique_ptr<NetworkStateNotifier::NetworkStateObserverHandle> 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<NetworkStateNotifier::NetworkStateObserverHandle> handle1 =