commit 62c335a645add717009aca1657952c81dade921b
author Ken Rockot <rockot@google.com> Thu Nov 28 19:26:41 2024
committer Copybara-Service <copybara-worker@google.com> Thu Nov 28 19:34:35 2024
tree d0efbe30976a2a0bd4b2d11a19989564d05d9b3f
parent 948fb200d4f9513c2064c5878a480bd97f4a45ea

ipcz: Validate new router sublinks

This fixes a bug where it was possible to craft a parcel which routes
itself to one of its own attached routers, thus creating an unreachable
circular Router reference and leaking memory.

Fixed: 380927241
Change-Id: I5263f9cf808028aa31fd7bcad6cc140fc2d865c3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6055325
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1389513}
NOKEYCHECK=True
GitOrigin-RevId: 45f8235319dfbe42eddbf363526e5a9e7e240afb

src/ipcz/node_link.cc

diff --git a/src/ipcz/node_link.cc b/src/ipcz/node_link.cc
index 0a6ea32..250cd25 100644
--- a/src/ipcz/node_link.cc
+++ b/src/ipcz/node_link.cc
@@ -528,6 +528,7 @@
       accept.GetArrayView<HandleType>(accept.v0()->handle_types);
   absl::Span<const RouterDescriptor> new_routers =
       accept.GetArrayView<RouterDescriptor>(accept.v0()->new_routers);
+  const SublinkId for_sublink = accept.v0()->sublink;
   auto driver_objects = accept.driver_objects();
 
   // Note that on any validation failure below, we defer rejection at least
@@ -544,7 +545,8 @@
           continue;
         }
 
-        Ref<Router> new_router = Router::Deserialize(new_routers[0], *this);
+        Ref<Router> new_router =
+            Router::Deserialize(new_routers[0], *this, for_sublink);
         if (!new_router) {
           parcel_valid = false;
           continue;
@@ -597,7 +599,6 @@
     return false;
   }
 
-  const SublinkId for_sublink = accept.v0()->sublink;
   auto parcel = std::make_unique<Parcel>(accept.v0()->sequence_number);
   parcel->set_num_subparcels(num_subparcels);
   parcel->set_subparcel_index(subparcel_index);

src/ipcz/router.cc

diff --git a/src/ipcz/router.cc b/src/ipcz/router.cc
index bcd1970..a84f055 100644
--- a/src/ipcz/router.cc
+++ b/src/ipcz/router.cc
@@ -700,7 +700,21 @@
 
 // static
 Ref<Router> Router::Deserialize(const RouterDescriptor& descriptor,
-                                NodeLink& from_node_link) {
+                                NodeLink& from_node_link,
+                                SublinkId receiving_sublink) {
+  std::optional<SublinkId> new_decaying_sublink;
+  if (descriptor.proxy_already_bypassed) {
+    new_decaying_sublink = descriptor.new_decaying_sublink;
+  }
+
+  if (descriptor.new_sublink == receiving_sublink ||
+      descriptor.new_sublink == new_decaying_sublink ||
+      new_decaying_sublink == receiving_sublink) {
+    // New sublink IDs must be unique among each other, and must not identify
+    // the new Router as its own recipient.
+    return nullptr;
+  }
+
   auto router = MakeRefCounted<Router>();
   Ref<RemoteRouterLink> new_outward_link;
   {
@@ -721,7 +735,7 @@
       }
     }
 
-    if (descriptor.proxy_already_bypassed) {
+    if (new_decaying_sublink) {
       // When split from a local peer, our remote counterpart (our remote peer's
       // former local peer) will use this link to forward parcels it already
       // received from our peer. This link decays like any other decaying link
@@ -732,9 +746,9 @@
       // The sequence length from the link is whatever had already been sent
       // to our counterpart back on the peer's node.
       Ref<RemoteRouterLink> new_decaying_link =
-          from_node_link.AddRemoteRouterLink(
-              descriptor.new_decaying_sublink, nullptr,
-              LinkType::kPeripheralOutward, LinkSide::kB, router);
+          from_node_link.AddRemoteRouterLink(*new_decaying_sublink, nullptr,
+                                             LinkType::kPeripheralOutward,
+                                             LinkSide::kB, router);
       if (!new_decaying_link) {
         return nullptr;
       }
@@ -766,7 +780,7 @@
                << from_node_link.remote_node_name().ToString() << " to "
                << from_node_link.local_node_name().ToString() << " via sublink "
                << descriptor.new_sublink << " and decaying sublink "
-               << descriptor.new_decaying_sublink;
+               << *new_decaying_sublink;
     } else {
       if (!descriptor.new_link_state_fragment.is_null()) {
         // No RouterLinkState fragment should be provided for this new

src/ipcz/router.h

diff --git a/src/ipcz/router.h b/src/ipcz/router.h
index 5d414d5..3c537f4 100644
--- a/src/ipcz/router.h
+++ b/src/ipcz/router.h
@@ -168,9 +168,11 @@
   // via Put or Get APIs.
   IpczResult MergeRoute(const Ref<Router>& other);
 
-  // Deserializes a new Router from `descriptor` received over `from_node_link`.
+  // Deserializes a new Router from `descriptor` received over `from_node_link`
+  // on `receiving_sublink`.
   static Ref<Router> Deserialize(const RouterDescriptor& descriptor,
-                                 NodeLink& from_node_link);
+                                 NodeLink& from_node_link,
+                                 SublinkId receiving_sublink);
 
   // Serializes a description of a new Router which will be used to extend this
   // Router's route across `to_node_link` by introducing a new Router on the