From 295a4a1b14b8fe12929bb61e6e00a74ac43098e8 Mon Sep 17 00:00:00 2001
From: Alex Gough <[email protected]>
Date: Mon, 05 May 2025 19:09:15 -0700
Subject: [PATCH] Drop transitive trust from transports

Untrusted nodes could reflect a broker initiated transport back to
a broker. This ultimately allows for handle leaks if the reflected
transport was later used to deserialize another transport containing
handles in the broker.

This CL addresses this along several axes:

1. untrusted transports cannot return new links to brokers.
2. process trustiness on Windows is propagated when a transport is
deserialized from a transport.

Windows has a special additional level of trustiness associated with
mojo peers via the is_remote_process_untrusted attribute (the
MOJO_SEND_INVITATION_FLAG_UNTRUSTED_PROCESS in invitations). This
affects how handles are sent between processes. This was a bool on all
platforms which was confusing.

This CL makes this attribute clearer. On Windows it is now a bi-state
enum, while on other platforms it is simply kUntracked. This makes it
easier to use default constructed values, and the same API on all
platforms without using too many buildflag differences.

This state was not being propagated correctly during transport
deserialization, and is now set as the same trust as the process from
which a deserialized transport came. Processes currently default to
being kTrusted, which matches the current behavior of the bool flag.

Finally, this CL turns a DCHECK into a CHECK to ensure peers are only
elevated when expected.

Bug: 412578726
Change-Id: I6741a3f53b26c3df854731177cdc886e9c8f7f11
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6497400
Reviewed-by: Daniel Cheng <[email protected]>
Commit-Queue: Alex Gough <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1456055}
---

diff --git a/mojo/core/ipcz_driver/invitation.cc b/mojo/core/ipcz_driver/invitation.cc
index 4b3f717d..001b146 100644
--- a/mojo/core/ipcz_driver/invitation.cc
+++ b/mojo/core/ipcz_driver/invitation.cc
@@ -90,7 +90,7 @@
     base::Process remote_process = base::Process(),
     MojoProcessErrorHandler error_handler = nullptr,
     uintptr_t error_handler_context = 0,
-    bool is_remote_process_untrusted = false) {
+    Transport::ProcessTrust remote_process_trust = Transport::ProcessTrust{}) {
   CHECK_EQ(endpoint.num_platform_handles, 1u);
   auto handle =
       PlatformHandle::FromMojoPlatformHandle(&endpoint.platform_handles[0]);
@@ -100,7 +100,7 @@
 
   auto transport = base::MakeRefCounted<Transport>(
       endpoint_types, PlatformChannelEndpoint(std::move(handle)),
-      std::move(remote_process), is_remote_process_untrusted);
+      std::move(remote_process), remote_process_trust);
   transport->SetErrorHandler(error_handler, error_handler_context);
   transport->set_leak_channel_on_shutdown(options.leak_channel_on_shutdown);
   transport->set_is_peer_trusted(options.is_peer_trusted);
@@ -268,15 +268,21 @@
   // bit essentially means that the remote process is especially untrustworthy
   // (e.g. a Chrome renderer) and should be subject to additional constraints
   // regarding what types of objects can be transferred to it.
-  const bool is_remote_process_untrusted =
-      options &&
-      (options->flags & MOJO_SEND_INVITATION_FLAG_UNTRUSTED_PROCESS) != 0;
+  Transport::ProcessTrust remote_process_trust{};
+#if BUILDFLAG(IS_WIN)
+  if (options &&
+      (options->flags & MOJO_SEND_INVITATION_FLAG_UNTRUSTED_PROCESS) != 0) {
+    remote_process_trust = Transport::ProcessTrust::kUntrusted;
+  } else {
+    remote_process_trust = Transport::ProcessTrust::kTrusted;
+  }
+#endif
 
   const bool is_peer_elevated =
       options && (options->flags & MOJO_SEND_INVITATION_FLAG_ELEVATED);
 #if !BUILDFLAG(IS_WIN)
   // For now, the concept of an elevated process is only meaningful on Windows.
-  DCHECK(!is_peer_elevated);
+  CHECK(!is_peer_elevated);
 #endif
 
 #if BUILDFLAG(IS_WIN)
@@ -296,7 +302,7 @@
       *transport_endpoint,
       {.is_peer_trusted = is_peer_elevated, .is_trusted_by_peer = true},
       std::move(remote_process), error_handler, error_handler_context,
-      is_remote_process_untrusted);
+      remote_process_trust);
   if (transport == IPCZ_INVALID_DRIVER_HANDLE) {
     return MOJO_RESULT_INVALID_ARGUMENT;
   }
diff --git a/mojo/core/ipcz_driver/transport.cc b/mojo/core/ipcz_driver/transport.cc
index c1516bae..bca98fd 100644
--- a/mojo/core/ipcz_driver/transport.cc
+++ b/mojo/core/ipcz_driver/transport.cc
@@ -136,7 +136,7 @@
                   const base::Process& remote_process,
                   HandleOwner handle_owner,
                   HandleData& out_handle_data,
-                  bool is_remote_process_untrusted) {
+                  Transport::ProcessTrust remote_process_trust) {
   CHECK(handle.is_valid());
   // Duplicating INVALID_HANDLE_VALUE passes a process handle. If you intend to
   // do this, you must open a valid process handle, not pass the result of
@@ -158,7 +158,7 @@
   DCHECK_EQ(handle_owner, HandleOwner::kRecipient);
   DCHECK(remote_process.IsValid());
 #if BUILDFLAG(IS_WIN)
-  if (is_remote_process_untrusted) {
+  if (remote_process_trust == Transport::ProcessTrust::kUntrusted) {
     DcheckIfFileHandleIsUnsafe(handle.GetHandle().get());
   }
 #endif
@@ -242,23 +242,20 @@
 Transport::Transport(EndpointTypes endpoint_types,
                      PlatformChannelEndpoint endpoint,
                      base::Process remote_process,
-                     bool is_remote_process_untrusted)
+                     ProcessTrust remote_process_trust)
     : endpoint_types_(endpoint_types),
       remote_process_(std::move(remote_process)),
-#if BUILDFLAG(IS_WIN)
-      is_remote_process_untrusted_(is_remote_process_untrusted),
-#endif
-      inactive_endpoint_(std::move(endpoint)) {
-}
+      remote_process_trust_(remote_process_trust),
+      inactive_endpoint_(std::move(endpoint)) {}
 
 // static
 scoped_refptr<Transport> Transport::Create(EndpointTypes endpoint_types,
                                            PlatformChannelEndpoint endpoint,
                                            base::Process remote_process,
-                                           bool is_remote_process_untrusted) {
+                                           ProcessTrust remote_process_trust) {
   return base::MakeRefCounted<Transport>(endpoint_types, std::move(endpoint),
                                          std::move(remote_process),
-                                         is_remote_process_untrusted);
+                                         remote_process_trust);
 }
 
 // static
@@ -495,7 +492,7 @@
   for (size_t i = 0; i < object_num_handles; ++i) {
 #if BUILDFLAG(IS_WIN)
     ok &= EncodeHandle(platform_handles[i], remote_process_, handle_owner,
-                       handle_data[i], is_remote_process_untrusted_);
+                       handle_data[i], remote_process_trust());
 #else
     handles[i] = TransmissiblePlatformHandle::ReleaseAsHandle(
         base::MakeRefCounted<TransmissiblePlatformHandle>(
@@ -654,23 +651,39 @@
     process = base::Process(handles[1].ReleaseHandle());
   }
 #endif
+  // Reject transports with out of range enum value in destination_type.
+  if (!(header.destination_type == kBroker ||
+        header.destination_type == kNonBroker)) {
+    return nullptr;
+  }
+
   const bool is_source_trusted = from_transport.is_peer_trusted() ||
                                  from_transport.destination_type() == kBroker;
+
   const bool is_new_peer_trusted = header.is_peer_trusted;
+  const bool is_trusted_by_peer = header.is_trusted_by_peer;
+
   if (is_new_peer_trusted && !is_source_trusted) {
     // Untrusted transports cannot send us trusted transports.
     return nullptr;
   }
+
+  if (header.destination_type == kBroker && !is_source_trusted) {
+    // Do not accept broker connections from untrusted transports.
+    return nullptr;
+  }
+
   if (header.is_same_remote_process &&
       from_transport.remote_process().IsValid()) {
     process = from_transport.remote_process().Duplicate();
   }
-  auto transport = Create({.source = from_transport.source_type(),
-                           .destination = header.destination_type},
-                          PlatformChannelEndpoint(std::move(handles[0])),
-                          std::move(process));
+  auto transport =
+      Create({.source = from_transport.source_type(),
+              .destination = header.destination_type},
+             PlatformChannelEndpoint(std::move(handles[0])), std::move(process),
+             from_transport.remote_process_trust());
   transport->set_is_peer_trusted(is_new_peer_trusted);

diff --git a/mojo/core/ipcz_driver/transport_test.cc b/mojo/core/ipcz_driver/transport_test.cc
index b9a1e994..f37157e 100644
--- a/mojo/core/ipcz_driver/transport_test.cc
+++ b/mojo/core/ipcz_driver/transport_test.cc
@@ -72,7 +72,13 @@
   static scoped_refptr<Transport> CreateAndSendTransport(
       MojoHandle pipe,
       const base::Process& process,
-      bool untrusted = false) {
+#if BUILDFLAG(IS_WIN)
+      Transport::ProcessTrust process_trust = Transport::ProcessTrust::kTrusted
+#else
+      // Parameter is not tracked on non-Windows platforms.
+      Transport::ProcessTrust process_trust = Transport::ProcessTrust{}
+#endif
+  ) {
     PlatformChannel channel;
     MojoHandle transport_for_client =
         WrapPlatformHandle(channel.TakeRemoteEndpoint().TakePlatformHandle())
@@ -81,7 +87,7 @@
     WriteMessageWithHandles(pipe, "", &transport_for_client, 1);
     return Transport::Create(
         {.source = Transport::kBroker, .destination = Transport::kNonBroker},
-        channel.TakeLocalEndpoint(), process.Duplicate(), untrusted);
+        channel.TakeLocalEndpoint(), process.Duplicate(), process_trust);
   }
 
   // Retrieves a PlatformChannel endpoint from `pipe` and returns a newly
@@ -263,6 +269,93 @@
   });
 }
 
+DEFINE_TEST_CLIENT_TEST_WITH_PIPE(MalformedTransportClient,
+                                  MojoIpczTransportTest,
+                                  h) {
+  // Offsets of enums that should be validated on receipt. Serialized objects
+  // use types internal to transport.cc e.g. [ObjectHeader][TransportHeader]...
+  // so supply direct offsets here.
+
+  // offsetof(ObjectHeader, type).
+  constexpr size_t object_type_offset = 4;
+  // offsetof(TransportHeader, destination_type) + sizeof(ObjectHeader)
+#if BUILDFLAG(IS_WIN)
+  constexpr size_t transport_destination_type_offset = 0x18;
+#else
+  constexpr size_t transport_destination_type_offset = 0x08;
+#endif
+
+  scoped_refptr<Transport> transport = ReceiveTransport(h);
+
+  TransportListener listener(*transport);
+  EXPECT_EQ("ready", listener.WaitForNextMessage().as_string());
+
+  {
+    auto [our_new_transport, their_new_transport] =
+        Transport::CreatePair(Transport::kNonBroker, Transport::kNonBroker);
+
+    TestMessage msg =
+        SerializeObjectFor(*transport, std::move(their_new_transport));
+    // Peek into the message to break the encoded object type by using an out
+    // of range enum value. This is uint32_t sized.
+    msg.bytes[object_type_offset] = 22;
+    msg.Transmit(*transport);
+
+    EXPECT_EQ("got null", listener.WaitForNextMessage().as_string());
+  }
+
+  {
+    auto [our_new_transport, their_new_transport] =
+        Transport::CreatePair(Transport::kNonBroker, Transport::kNonBroker);
+
+    TestMessage msg =
+        SerializeObjectFor(*transport, std::move(their_new_transport));
+    // Peek into the message to break the encoded transport type by using an out
+    // of range enum value. This is uint8_t sized.
+    msg.bytes[transport_destination_type_offset] = 22;
+    msg.Transmit(*transport);
+
+    EXPECT_EQ("got null", listener.WaitForNextMessage().as_string());
+  }
+
+  TestMessage("done").Transmit(*transport);
+  EXPECT_EQ(MOJO_RESULT_OK, MojoClose(h));
+}
+
+TEST_F(MojoIpczTransportTest, MalformedTransport) {
+  RunTestClientWithController(
+      "MalformedTransportClient", [&](ClientController& c) {
+        scoped_refptr<Transport> transport =
+            CreateAndSendTransport(c.pipe(), c.process());
+
+        TransportListener listener(*transport);
+        TestMessage("ready").Transmit(*transport);
+
+        {
+          // Object type is invalid so the object should be rejected.
+          TestMessage message = listener.WaitForNextMessage();
+          scoped_refptr<ObjectBase> object;
+          const IpczResult result = transport->DeserializeObject(
+              base::span(message.bytes), base::span(message.handles), object);
+          EXPECT_EQ(result, IPCZ_RESULT_UNIMPLEMENTED);
+          TestMessage("got null").Transmit(*transport);
+        }
+
+        {
+          // Transport type is invalid so the object should be rejected.
+          TestMessage message = listener.WaitForNextMessage();
+          scoped_refptr<ObjectBase> object;
+          const IpczResult result = transport->DeserializeObject(
+              base::span(message.bytes), base::span(message.handles), object);
+          EXPECT_EQ(result, IPCZ_RESULT_INVALID_ARGUMENT);
+          TestMessage("got null").Transmit(*transport);
+        }
+
+        EXPECT_EQ("done", listener.WaitForNextMessage().as_string());
+        listener.WaitForDisconnect();
+      });
+}
+
 // Transport on Windows does not support out-of-band handle transfer, so this
 // test is impossible there. Windows handle transmission is instead covered by
 // tests which more broadly cover driver object serialization.
@@ -415,13 +508,22 @@
     return false;
 #endif
   }
+  Transport::ProcessTrust TransportProcessTrust() {
+// Enforcement only happens on Windows.
+#if BUILDFLAG(IS_WIN)
+    return IsEnforcementEnabled() ? Transport::ProcessTrust::kUntrusted
+                                  : Transport::ProcessTrust::kTrusted;
+#else
+    return Transport::ProcessTrust::kUntracked;
+#endif
+  }
   bool ShouldMarkNoExecute() { return std::get<1>(GetParam()); }
 };
 
 TEST_P(MojoIpczTransportSecurityTest, TransmitFile) {
   RunTestClientWithController("TransmitFileClient", [&](ClientController& c) {
     scoped_refptr<Transport> transport =
-        CreateAndSendTransport(c.pipe(), c.process(), IsEnforcementEnabled());
+        CreateAndSendTransport(c.pipe(), c.process(), TransportProcessTrust());
     base::ScopedTempDir temp_dir;
     CHECK(temp_dir.CreateUniqueTempDir());
     int32_t flags = base::File::FLAG_CREATE | base::File::FLAG_READ |
@@ -644,8 +746,8 @@
 TEST_F(MojoIpczTransportTest, InvalidHandleUntrusted) {
   RunTestClientWithController(
       "InvalidHandleUntrustedClient", [&](ClientController& c) {
-        scoped_refptr<Transport> transport =
-            CreateAndSendTransport(c.pipe(), c.process(), /*untrusted=*/true);
+        scoped_refptr<Transport> transport = CreateAndSendTransport(
+            c.pipe(), c.process(), Transport::ProcessTrust{});
 
         TransportListener listener(*transport);
         TestMessage(kFromTrusted).Transmit(*transport);
@@ -666,5 +768,98 @@
 
 #endif  // BUILDFLAG(IS_WIN)
 
+DEFINE_TEST_CLIENT_TEST_WITH_PIPE(TransportFromUntrustedClient,
+                                  MojoIpczTransportTest,
+                                  h) {
+  scoped_refptr<Transport> transport = ReceiveTransport(h);
+  TransportListener listener(*transport);
+  EXPECT_EQ("ready", listener.WaitForNextMessage().as_string());
+
+  for (int i = 0; i < 2; i++) {
+    auto ours = i == 0 ? Transport::kNonBroker : Transport::kBroker;
+    auto theirs = i == 0 ? Transport::kBroker : Transport::kNonBroker;
+    {
+      auto [our_new_transport, their_new_transport] =
+          Transport::CreatePair(ours, theirs);
+
+      their_new_transport->set_is_peer_trusted(true);
+
+      SerializeObjectFor(*transport, std::move(their_new_transport))
+          .Transmit(*transport);
+      EXPECT_EQ("got null", listener.WaitForNextMessage().as_string());
+    }
+
+    {
+      auto [our_new_transport, their_new_transport] =
+          Transport::CreatePair(ours, theirs);
+
+      their_new_transport->set_is_trusted_by_peer(true);
+
+      SerializeObjectFor(*transport, std::move(their_new_transport))
+          .Transmit(*transport);
+      if (ours == Transport::kNonBroker) {
+        EXPECT_EQ("got untrusted", listener.WaitForNextMessage().as_string());
+      } else {
+        EXPECT_EQ("got null", listener.WaitForNextMessage().as_string());
+      }
+    }
+  }
+
+  EXPECT_EQ(MOJO_RESULT_OK, MojoClose(h));
+}
+
+TEST_F(MojoIpczTransportTest, TransportFromUntrusted) {
+#if BUILDFLAG(IS_WIN)
+  // TODO(crbug.com/414392683) default to untrusted/untracked.
+  Transport::ProcessTrust process_trust = Transport::ProcessTrust::kUntrusted;
+#else
+  Transport::ProcessTrust process_trust{};
+#endif
+  RunTestClientWithController(
+      "TransportFromUntrustedClient", [&](ClientController& c) {
+        scoped_refptr<Transport> transport =
+            CreateAndSendTransport(c.pipe(), c.process(), process_trust);
+
+        TransportListener listener(*transport);
+        TestMessage("ready").Transmit(*transport);
+
+        // A broker (this process) should reject transports from untrusted
+        // clients if they claim the transport's peer is trusted or is a broker.
+        // It is ok to allow transports from a client that indicates they trust
+        // the peer, as a broker will not make trust decisions based on that.
+        for (int i = 0; i < 2; i++) {
+          auto theirs = i == 0 ? Transport::kNonBroker : Transport::kBroker;
+          {
+            TestMessage message = listener.WaitForNextMessage();
+            scoped_refptr<ObjectBase> object;
+            const IpczResult result = transport->DeserializeObject(
+                base::span(message.bytes), base::span(message.handles), object);
+            EXPECT_EQ(result, IPCZ_RESULT_INVALID_ARGUMENT);
+            TestMessage("got null").Transmit(*transport);
+          }
+
+          {
+            TestMessage message = listener.WaitForNextMessage();
+            if (theirs == Transport::kNonBroker) {
+              scoped_refptr<Transport> transport2 =
+                  DeserializeObjectFrom<Transport>(*transport, message);
+              EXPECT_TRUE(transport2->is_trusted_by_peer());
+              EXPECT_FALSE(transport2->is_peer_trusted());
+              TestMessage("got untrusted").Transmit(*transport);
+            } else {
+              scoped_refptr<ObjectBase> object;
+              const IpczResult result = transport->DeserializeObject(
+                  base::span(message.bytes), base::span(message.handles),
+                  object);
+              EXPECT_EQ(result, IPCZ_RESULT_INVALID_ARGUMENT);
+              TestMessage("got null").Transmit(*transport);
+            }
+          }
+        }
+
+        listener.WaitForDisconnect();
+      });
+}
+
 }  // namespace
 }  // namespace mojo::core::ipcz_driver