commit 6d5d50670f8582476a9de2058dc6b702010db6eb
author Ken Rockot <rockot@google.com> Fri Aug 26 17:02:24 2022
committer Takuto Ikuta <tikuta@google.com> Fri Dec 02 02:03:19 2022
tree 80302fad32272e84c706ad4950ae63798c4ad794
parent 2234c3b5ec51e537b9b2b3b9470353d5068f03b5

ipcz: Fix portal transmission in parcels

This corrects two bugs in remote portal transmission:

- A TOCTOU bug where we were reading back a serialized RouterDescriptor
  for input to some computations after transmitting the parcel. This is
  corrected by serializing the descriptors into local memory first and
  then copying them into the outgoing message. The local copy is used
  for subsequent computations.

- An accounting error where portal serialization could go out of bounds
  of the destination RouterDescriptor storage, because descriptor
  storage was indexed by overall object attachment index rather than
  portal index.

The first case was already racy and covered by existing tests. A new
test is added to cover the second issue.

Bug: 1299283
Change-Id: I81c9b200d05a14b1ef06f14b01db47361005bed3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3856791
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/main@{#1039841}
NOKEYCHECK=True
GitOrigin-RevId: 854e71aa33201b4d5054af340d3bbcc14fd9e4b6

src/box_test.cc

diff --git a/src/box_test.cc b/src/box_test.cc
index cdbfce0..e7eb707 100644
--- a/src/box_test.cc
+++ b/src/box_test.cc
@@ -56,6 +56,7 @@
 
 constexpr const char kMessage1[] = "Hello, world?";
 constexpr const char kMessage2[] = "Hello, world!";
+constexpr const char kMessage3[] = "Hello. World.";
 
 MULTINODE_TEST_NODE(BoxTestNode, TransferBoxClient) {
   IpczHandle b = ConnectToBroker();
@@ -75,6 +76,32 @@
   Close(c);
 }
 
+MULTINODE_TEST_NODE(BoxTestNode, TransferBoxAndPortalClient) {
+  IpczHandle b = ConnectToBroker();
+
+  IpczHandle handles[2];
+  std::string message;
+  EXPECT_EQ(IPCZ_RESULT_OK, WaitToGet(b, &message, handles));
+  EXPECT_EQ(kMessage2, message);
+  EXPECT_EQ(IPCZ_RESULT_OK, Put(handles[1], kMessage3));
+  EXPECT_EQ(kMessage1, UnboxBlob(handles[0]));
+  CloseAll({b, handles[1]});
+}
+
+MULTINODE_TEST(BoxTest, TransferBoxAndPortal) {
+  IpczHandle c = SpawnTestNode<TransferBoxAndPortalClient>();
+
+  auto [q, p] = OpenPortals();
+  IpczHandle box = BoxBlob(kMessage1);
+  IpczHandle handles[] = {box, p};
+  EXPECT_EQ(IPCZ_RESULT_OK, Put(c, kMessage2, absl::MakeSpan(handles)));
+
+  std::string message;
+  EXPECT_EQ(IPCZ_RESULT_OK, WaitToGet(q, &message));
+  EXPECT_EQ(kMessage3, message);
+  CloseAll({c, q});
+}
+
 constexpr size_t TransferBoxBetweenNonBrokersNumIterations = 50;
 
 MULTINODE_TEST_NODE(BoxTestNode, TransferBoxBetweenNonBrokersClient1) {

src/ipcz/remote_router_link.cc

diff --git a/src/ipcz/remote_router_link.cc b/src/ipcz/remote_router_link.cc
index ac46861..e35a1b9 100644
--- a/src/ipcz/remote_router_link.cc
+++ b/src/ipcz/remote_router_link.cc
@@ -195,7 +195,14 @@
   // Serialize attached objects. We accumulate the Routers of all attached
   // portals, because we need to reference them again after transmission, with
   // a 1:1 correspondence to the serialized RouterDescriptors.
-  absl::InlinedVector<Ref<Router>, 4> routers_to_proxy;
+  absl::InlinedVector<Ref<Router>, 4> routers_to_proxy(num_portals);
+  absl::InlinedVector<RouterDescriptor, 4> descriptors(num_portals);
+
+  // Explicitly zero the descriptor memory since there may be padding bits
+  // within and we'll be copying the full contents into message data below.
+  memset(descriptors.data(), 0, descriptors.size() * sizeof(descriptors[0]));
+
+  size_t portal_index = 0;
   for (size_t i = 0; i < objects.size(); ++i) {
     APIObject& object = *objects[i];
 
@@ -204,8 +211,10 @@
         handle_types[i] = HandleType::kPortal;
 
         Ref<Router> router = Portal::FromObject(&object)->router();
-        router->SerializeNewRouter(*node_link(), new_routers[i]);
-        routers_to_proxy.push_back(std::move(router));
+        ABSL_ASSERT(portal_index < num_portals);
+        router->SerializeNewRouter(*node_link(), descriptors[portal_index]);
+        routers_to_proxy[portal_index] = std::move(router);
+        ++portal_index;
         break;
       }
 
@@ -220,6 +229,11 @@
     }
   }
 
+  // Copy all the serialized router descriptors into the message. Our local
+  // copy will supply inputs for BeginProxyingToNewRouter() calls below.
+  memcpy(new_routers.data(), descriptors.data(),
+         new_routers.size() * sizeof(new_routers[0]));
+
   if (must_split_parcel) {
     msg::AcceptParcelDriverObjects accept_objects;
     accept_objects.params().sublink = sublink_;
@@ -241,9 +255,9 @@
 
   // Now that the parcel has been transmitted, it's safe to start proxying from
   // any routers whose routes have just been extended to the destination.
-  ABSL_ASSERT(routers_to_proxy.size() == new_routers.size());
+  ABSL_ASSERT(routers_to_proxy.size() == descriptors.size());
   for (size_t i = 0; i < routers_to_proxy.size(); ++i) {
-    routers_to_proxy[i]->BeginProxyingToNewRouter(*node_link(), new_routers[i]);
+    routers_to_proxy[i]->BeginProxyingToNewRouter(*node_link(), descriptors[i]);
   }
 
   // Finally, a Parcel will normally close all attached objects when destroyed.