From 15fa9b70af99448877b1d5853dde7c97f95eba68 Mon Sep 17 00:00:00 2001
From: tomerni-island <[email protected]>
Date: Wed, 09 Sep 2026 13:43:41 -0700
Subject: [PATCH] [updater] Serve and prefer the admin-protected update service pipe

The system update service now also listens on a pipe under
"ProtectedPrefix\Administrators", where only Administrators and
LocalSystem may create pipes, with a DACL that withholds
FILE_CREATE_PIPE_INSTANCE from Authenticated Users. Together those close
the two ways an unprivileged process could end up serving that pipe:
squatting the name while no server is listening, and adding an instance
under the name the updater owns, which NPFS reports as owned by
LocalSystem because the security descriptor belongs to the name rather
than the instance. The updater first creates the protected pipe and only
then the legacy pipe to avoid race condition.

The legacy pipe keeps its name and DACL and stays live so that clients
which predate this change keep working. Clients prefer the protected
pipe and fall back to the legacy one only when the protected pipe does
not exist, which means an older updater.

Bug: 535718578
Change-Id: Iccf3641a4939bc5e7ea84c625c9dbd136d974a72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8334870
Reviewed-by: Noah Rose Ledesma <[email protected]>
Commit-Queue: S Ganesh <[email protected]>
Reviewed-by: S Ganesh <[email protected]>
Reviewed-by: Sorin Jianu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1694848}
---
diff --git a/chrome/enterprise_companion/enterprise_companion_service_stub.cc b/chrome/enterprise_companion/enterprise_companion_service_stub.cc
index cec85c47..5e0821f1 100644
--- a/chrome/enterprise_companion/enterprise_companion_service_stub.cc
+++ b/chrome/enterprise_companion/enterprise_companion_service_stub.cc
@@ -69,7 +69,7 @@
     server_.set_disconnect_handler(base::BindRepeating(
         [] { VLOG(1) << "EnterpriseCompanion client disconnected"; }));
     if (endpoint_created_listener_for_testing) {
-      server_.set_on_server_endpoint_created_callback_for_testing(
+      server_.set_on_server_endpoint_created_callback(
           endpoint_created_listener_for_testing);
     }
     server_.StartServer();
diff --git a/chrome/updater/app/server/update_service_stub.cc b/chrome/updater/app/server/update_service_stub.cc
index 2848629..6d48808 100644
--- a/chrome/updater/app/server/update_service_stub.cc
+++ b/chrome/updater/app/server/update_service_stub.cc
@@ -28,6 +28,10 @@
 #include "components/named_mojo_ipc_server/named_mojo_ipc_server.h"
 #include "mojo/public/cpp/bindings/remote.h"
 
+#if BUILDFLAG(IS_WIN)
+#include "base/win/windows_version.h"
+#endif
+
 namespace updater {
 namespace {
 
@@ -196,6 +200,18 @@
   SEQUENCE_CHECKER(sequence_checker_);
 };
 
+base::RepeatingCallback<
+    mojom::UpdateService*(const named_mojo_ipc_server::ConnectionInfo&)>
+MakeImplProvider(mojom::UpdateService* interface,
+                 mojom::UpdateService* filter) {
+  return base::BindRepeating(
+      [](mojom::UpdateService* interface, mojom::UpdateService* filter,
+         const named_mojo_ipc_server::ConnectionInfo& info) {
+        return IsConnectionTrusted(info) ? interface : filter;
+      },
+      interface, filter);
+}
+
 }  // namespace
 
 UpdateServiceStub::UpdateServiceStub(scoped_refptr<updater::UpdateService> impl,
@@ -390,23 +406,39 @@
     base::RepeatingClosure endpoint_created_listener_for_testing)
     : filter_(std::make_unique<UpdateServiceStubUntrusted>(this)),
       server_(CreateServerEndpointOptions(GetUpdateServiceServerName(scope)),
-              base::BindRepeating(base::BindRepeating(
-                  [](mojom::UpdateService* interface,
-                     mojom::UpdateService* filter,
-                     const named_mojo_ipc_server::ConnectionInfo& info) {
-                    return IsConnectionTrusted(info) ? interface : filter;
-                  },
-                  this,
-                  filter_.get()))),
+              MakeImplProvider(this, filter_.get())),
       impl_(impl),
       task_start_listener_(task_start_listener),
       task_end_listener_(task_end_listener) {
-  server_.set_disconnect_handler(base::BindRepeating(
-      [] { VLOG(1) << "UpdateService client disconnected."; }));
+  base::RepeatingClosure on_client_disconnected = base::BindRepeating(
+      [] { VLOG(1) << "UpdateService client disconnected."; });
+  server_.set_disconnect_handler(on_client_disconnected);
   if (endpoint_created_listener_for_testing) {
-    server_.set_on_server_endpoint_created_callback_for_testing(  // IN-TEST
+    server_.set_on_server_endpoint_created_callback(
         endpoint_created_listener_for_testing);
   }
+
+#if BUILDFLAG(IS_WIN)
+  if (IsSystemInstall(scope) &&
+      base::win::GetVersion() >= base::win::Version::WIN10_RS3) {
+    protected_server_.emplace(CreateProtectedServerEndpointOptions(
+                                  scope, GetUpdateServiceServerName(scope)),
+                              MakeImplProvider(this, filter_.get()));
+    protected_server_->set_disconnect_handler(on_client_disconnected);
+
+    base::RepeatingClosure on_endpoint_created = base::BindRepeating(
+        &named_mojo_ipc_server::NamedMojoIpcServerBase::StartServer,
+        base::Unretained(&server_));
+    protected_server_->set_on_server_endpoint_created_callback(
+        endpoint_created_listener_for_testing
+            ? std::move(on_endpoint_created)
+                  .Then(endpoint_created_listener_for_testing)
+            : std::move(on_endpoint_created));
+    protected_server_->StartServer();
+    return;
+  }
+#endif
+
   server_.StartServer();
 }
 
diff --git a/chrome/updater/app/server/update_service_stub.h b/chrome/updater/app/server/update_service_stub.h
index dc5658b7..119bd69 100644
--- a/chrome/updater/app/server/update_service_stub.h
+++ b/chrome/updater/app/server/update_service_stub.h
@@ -6,11 +6,13 @@
 #define CHROME_UPDATER_APP_SERVER_UPDATE_SERVICE_STUB_H_
 
 #include <memory>
+#include <optional>
 
 #include "base/functional/callback_forward.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/sequence_checker.h"
+#include "build/build_config.h"
 #include "chrome/updater/registration_data.h"
 #include "chrome/updater/update_service.h"
 #include "chrome/updater/update_service_internal.h"
@@ -91,6 +93,10 @@
 
   std::unique_ptr<mojom::UpdateService> filter_;
   named_mojo_ipc_server::NamedMojoIpcServer<mojom::UpdateService> server_;
+#if BUILDFLAG(IS_WIN)
+  std::optional<named_mojo_ipc_server::NamedMojoIpcServer<mojom::UpdateService>>
+      protected_server_;
+#endif
   scoped_refptr<updater::UpdateService> impl_;
   base::RepeatingClosure task_start_listener_;
   base::RepeatingClosure task_end_listener_;
diff --git a/chrome/updater/ipc/ipc_security.h b/chrome/updater/ipc/ipc_security.h
index 7897ca2..16808c5e 100644
--- a/chrome/updater/ipc/ipc_security.h
+++ b/chrome/updater/ipc/ipc_security.h
@@ -5,6 +5,7 @@
 #ifndef CHROME_UPDATER_IPC_IPC_SECURITY_H_
 #define CHROME_UPDATER_IPC_IPC_SECURITY_H_
 
+#include "build/build_config.h"
 #include "chrome/updater/updater_scope.h"
 #include "mojo/public/cpp/platform/named_platform_channel.h"
 
@@ -23,6 +24,14 @@
 named_mojo_ipc_server::EndpointOptions CreateServerEndpointOptions(
     const mojo::NamedPlatformChannel::ServerName& server_name);
 
+#if BUILDFLAG(IS_WIN)
+// Like above, but for the pipe under "ProtectedPrefix\Administrators", where
+// only Administrators and LocalSystem may create pipes. System scope only.
+named_mojo_ipc_server::EndpointOptions CreateProtectedServerEndpointOptions(
+    UpdaterScope scope,
+    const mojo::NamedPlatformChannel::ServerName& server_name);
+#endif
+
 }  // namespace updater
 
 #endif  // CHROME_UPDATER_IPC_IPC_SECURITY_H_
diff --git a/chrome/updater/ipc/ipc_security_win.cc b/chrome/updater/ipc/ipc_security_win.cc
index 13a5686..45f0ce3 100644
--- a/chrome/updater/ipc/ipc_security_win.cc
+++ b/chrome/updater/ipc/ipc_security_win.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/updater/ipc/ipc_security.h"
 
+#include "base/check.h"
 #include "chrome/updater/get_updater_scope.h"
 #include "components/named_mojo_ipc_server/connection_info.h"
 #include "components/named_mojo_ipc_server/endpoint_options.h"
@@ -45,4 +46,28 @@
   return options;
 }
 
+named_mojo_ipc_server::EndpointOptions CreateProtectedServerEndpointOptions(
+    UpdaterScope scope,
+    const mojo::NamedPlatformChannel::ServerName& server_name) {
+  CHECK(IsSystemInstall(scope));
+  named_mojo_ipc_server::EndpointOptions options =
+      CreateServerEndpointOptions(server_name);
+  options.pipe_name_type =
+      mojo::NamedPlatformChannel::PipeNameType::kAdminProtected;
+
+  // A DACL to grant:
+  // GA = Generic All
+  // access to:
+  // SY = LOCAL_SYSTEM
+  // BA = BUILTIN_ADMINISTRATORS
+  // 0x12019B = GENERIC_READ | GENERIC_WRITE excluding
+  //            FILE_CREATE_PIPE_INSTANCE (0x4), which would let a client add
+  //            an instance under this name and inherit its privileged owner
+  // access to:
+  // AU = AUTHENTICATED_USERS
+  options.security_descriptor =
+      L"D:P(A;;GA;;;SY)(A;;GA;;;BA)(A;;0x12019B;;;AU)";
+  return options;
+}
+
 }  // namespace updater
diff --git a/chrome/updater/ipc/update_service_dialer_win.cc b/chrome/updater/ipc/update_service_dialer_win.cc
index b138fa28..962f292 100644
--- a/chrome/updater/ipc/update_service_dialer_win.cc
+++ b/chrome/updater/ipc/update_service_dialer_win.cc
@@ -9,6 +9,7 @@
 #include <wrl/client.h>
 
 #include <optional>
+#include <string>
 #include <utility>
 
 #include "base/functional/bind.h"
@@ -89,6 +90,42 @@
              base::win::Sid(base::win::WellKnownSid::kBuiltinAdministrators);
 }
 
+std::optional<mojo::PlatformChannelEndpoint> ConnectToUpdateService(
+    UpdaterScope scope,
+    const mojo::NamedPlatformChannel::ServerName& server_name,
+    mojo::NamedPlatformChannel::PipeNameType pipe_name_type) {
+  mojo::NamedPlatformChannel::Options options;
+  options.server_name = server_name;
+  options.allow_impersonation = true;
+  options.verify_server_privilege = true;
+  options.pipe_name_type = pipe_name_type;
+  mojo::PlatformChannelEndpoint connected_endpoint =
+      named_mojo_ipc_server::ConnectToServer(options);
+  if (!connected_endpoint.is_valid() ||
+      (IsSystemInstall(scope) &&
+       !IsServerElevated(
+           connected_endpoint.platform_handle().GetHandle().get()))) {
+    return std::nullopt;
+  }
+  return connected_endpoint;
+}
+
+// `::WaitNamedPipeW` reports ERROR_FILE_NOT_FOUND only when NPFS has no pipe
+// registered under the name, and ERROR_PATH_NOT_FOUND when the prefix
+// directory itself is absent; a pipe that exists but is busy reports
+// ERROR_SEM_TIMEOUT instead.
+bool IsPipeMissing(const mojo::NamedPlatformChannel::ServerName& server_name,
+                   mojo::NamedPlatformChannel::PipeNameType pipe_name_type) {
+  const std::wstring pipe_name =
+      mojo::NamedPlatformChannel::GetPipeNameFromServerName(server_name,
+                                                            pipe_name_type);
+  if (::WaitNamedPipeW(pipe_name.c_str(), 1)) {
+    return false;
+  }
+  const DWORD error = ::GetLastError();
+  return error == ERROR_FILE_NOT_FOUND || error == ERROR_PATH_NOT_FOUND;
+}
+
 void ConnectMojoImpl(
     UpdaterScope scope,
     bool is_internal_service,
@@ -114,31 +151,33 @@
     }
 
     server = result;
-    mojo::NamedPlatformChannel::Options options;
-    options.server_name = is_internal_service
-                              ? GetUpdateServiceInternalServerName(scope)
-                              : GetUpdateServiceServerName(scope);
-    options.allow_impersonation = true;
-    options.verify_server_privilege = true;
-    mojo::PlatformChannelEndpoint connected_endpoint =
-        named_mojo_ipc_server::ConnectToServer(options);
-    if (IsSystemInstall(scope) && connected_endpoint.is_valid() &&
-        !IsServerElevated(
-            connected_endpoint.platform_handle().GetHandle().get())) {
-      return std::nullopt;
+    const mojo::NamedPlatformChannel::ServerName server_name =
+        is_internal_service ? GetUpdateServiceInternalServerName(scope)
+                            : GetUpdateServiceServerName(scope);
+
+    if (IsSystemInstall(scope) && !is_internal_service) {
+      return SelectUpdateServiceEndpoint(
+          [&](mojo::NamedPlatformChannel::PipeNameType pipe_name_type) {
+            return ConnectToUpdateService(scope, server_name, pipe_name_type);
+          },
+          [&](mojo::NamedPlatformChannel::PipeNameType pipe_name_type) {
+            return IsPipeMissing(server_name, pipe_name_type);
+          });
     }
-    return connected_endpoint;
+
+    return ConnectToUpdateService(
+        scope, server_name, mojo::NamedPlatformChannel::PipeNameType::kDefault);
   }();
 
-  if (tries >= 1 && !endpoint) {
-    VLOG(1) << "Failed to connect to remote mojo service, is_internal_service: "
-            << is_internal_service << ", scope: " << scope;
-    std::move(connected_callback).Run(std::nullopt, {});
+  if (endpoint) {
+    std::move(connected_callback).Run(std::move(endpoint), server);
     return;
   }
 
-  if (endpoint && endpoint->is_valid()) {
-    std::move(connected_callback).Run(std::move(endpoint), server);
+  if (tries >= 1) {
+    VLOG(1) << "Failed to connect to remote mojo service, is_internal_service: "
+            << is_internal_service << ", scope: " << scope;
+    std::move(connected_callback).Run(std::nullopt, {});
     return;
   }
 
@@ -151,6 +190,29 @@
 
 }  // namespace
 
+std::optional<mojo::PlatformChannelEndpoint> SelectUpdateServiceEndpoint(
+    base::FunctionRef<std::optional<mojo::PlatformChannelEndpoint>(
+        mojo::NamedPlatformChannel::PipeNameType)> connect,
+    base::FunctionRef<bool(mojo::NamedPlatformChannel::PipeNameType)>
+        is_pipe_missing) {
+  constexpr auto kProtected =
+      mojo::NamedPlatformChannel::PipeNameType::kAdminProtected;
+  constexpr auto kDefault = mojo::NamedPlatformChannel::PipeNameType::kDefault;
+
+  if (std::optional<mojo::PlatformChannelEndpoint> endpoint =
+          connect(kProtected)) {
+    return endpoint;
+  }
+
+  // A server serving the legacy pipe but not the protected one either predates
+  // it or failed to create it; both want the fallback. While the server is
+  // still starting neither pipe exists yet, so retry instead of downgrading.
+  if (is_pipe_missing(kProtected) && !is_pipe_missing(kDefault)) {
+    return connect(kDefault);
+  }
+  return std::nullopt;
+}
+
 void ConnectMojo(
     UpdaterScope scope,
     bool is_internal_service,
diff --git a/chrome/updater/ipc/update_service_dialer_win.h b/chrome/updater/ipc/update_service_dialer_win.h
index 97d978f..c4e5d5a9 100644
--- a/chrome/updater/ipc/update_service_dialer_win.h
+++ b/chrome/updater/ipc/update_service_dialer_win.h
@@ -10,7 +10,9 @@
 #include <optional>
 
 #include "base/functional/callback_forward.h"
+#include "base/functional/function_ref.h"
 #include "chrome/updater/updater_scope.h"
+#include "mojo/public/cpp/platform/named_platform_channel.h"
 
 namespace base {
 class Time;
@@ -30,6 +32,16 @@
     base::OnceCallback<void(std::optional<mojo::PlatformChannelEndpoint>,
                             Microsoft::WRL::ComPtr<IUnknown>)>
         connected_callback);
+
+// Prefers the admin-protected pipe, downgrading to the legacy one only for a
+// server that does not serve it. `connect` and `is_pipe_missing` are injected
+// so the selection can be tested without a privileged server.
+std::optional<mojo::PlatformChannelEndpoint> SelectUpdateServiceEndpoint(
+    base::FunctionRef<std::optional<mojo::PlatformChannelEndpoint>(
+        mojo::NamedPlatformChannel::PipeNameType)> connect,
+    base::FunctionRef<bool(mojo::NamedPlatformChannel::PipeNameType)>
+        is_pipe_missing);
+
 }  // namespace updater
 
 #endif  // CHROME_UPDATER_IPC_UPDATE_SERVICE_DIALER_WIN_H_
diff --git a/components/named_mojo_ipc_server/named_mojo_ipc_server.h b/components/named_mojo_ipc_server/named_mojo_ipc_server.h
index a55743a..aad8d642 100644
--- a/components/named_mojo_ipc_server/named_mojo_ipc_server.h
+++ b/components/named_mojo_ipc_server/named_mojo_ipc_server.h
@@ -38,12 +38,10 @@
   void StopServer() override;
... (file diff truncated)

diff --git a/chrome/updater/ipc/update_service_dialer_win_unittest.cc b/chrome/updater/ipc/update_service_dialer_win_unittest.cc
new file mode 100644
index 0000000..407eb12
--- /dev/null
+++ b/chrome/updater/ipc/update_service_dialer_win_unittest.cc
@@ -0,0 +1,87 @@
+// Copyright 2026 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/updater/ipc/update_service_dialer_win.h"
+
+#include <optional>
+#include <vector>
+
+#include "base/containers/flat_set.h"
+#include "mojo/public/cpp/platform/named_platform_channel.h"
+#include "mojo/public/cpp/platform/platform_channel.h"
+#include "mojo/public/cpp/platform/platform_channel_endpoint.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace updater {
+namespace {
+
+using PipeNameType = ::mojo::NamedPlatformChannel::PipeNameType;
+using ::testing::ElementsAre;
+
+class UpdateServiceDialerWinTest : public ::testing::Test {
+ protected:
+  std::optional<mojo::PlatformChannelEndpoint> Select() {
+    return SelectUpdateServiceEndpoint(
+        [&](PipeNameType pipe_name_type) { return Connect(pipe_name_type); },
+        [&](PipeNameType pipe_name_type) {
+          return !present_.contains(pipe_name_type);
+        });
+  }
+
+  std::optional<mojo::PlatformChannelEndpoint> Connect(
+      PipeNameType pipe_name_type) {
+    connect_attempts_.push_back(pipe_name_type);
+    if (!connectable_.contains(pipe_name_type)) {
+      return std::nullopt;
+    }
+    mojo::PlatformChannel channel;
+    return channel.TakeLocalEndpoint();
+  }
+
+  base::flat_set<PipeNameType> connectable_;
+  base::flat_set<PipeNameType> present_;
+  std::vector<PipeNameType> connect_attempts_;
+};
+
+TEST_F(UpdateServiceDialerWinTest, PrefersProtectedPipeWhenServerServesIt) {
+  connectable_ = {PipeNameType::kAdminProtected};
+  present_ = {PipeNameType::kAdminProtected, PipeNameType::kDefault};
+
+  EXPECT_TRUE(Select().has_value());
+  EXPECT_THAT(connect_attempts_, ElementsAre(PipeNameType::kAdminProtected));
+}
+
+TEST_F(UpdateServiceDialerWinTest, FallsBackWhenServerHasNoProtectedPipe) {
+  connectable_ = {PipeNameType::kDefault};
+  present_ = {PipeNameType::kDefault};
+
+  EXPECT_TRUE(Select().has_value());
+  EXPECT_THAT(connect_attempts_, ElementsAre(PipeNameType::kAdminProtected,
+                                             PipeNameType::kDefault));
+}
+
+TEST_F(UpdateServiceDialerWinTest, DoesNotFallBackWhenProtectedPipeRefuses) {
+  connectable_ = {PipeNameType::kDefault};
+  present_ = {PipeNameType::kAdminProtected, PipeNameType::kDefault};
+
+  EXPECT_FALSE(Select().has_value());
+  EXPECT_THAT(connect_attempts_, ElementsAre(PipeNameType::kAdminProtected));
+}
+
+TEST_F(UpdateServiceDialerWinTest, ConnectsWhenOnlyProtectedPipeIsPresent) {
+  connectable_ = {PipeNameType::kAdminProtected};
+  present_ = {PipeNameType::kAdminProtected};
+
+  EXPECT_TRUE(Select().has_value());
+  EXPECT_THAT(connect_attempts_, ElementsAre(PipeNameType::kAdminProtected));
+}
+
+TEST_F(UpdateServiceDialerWinTest, DoesNotFallBackWhileServerIsStarting) {
+  EXPECT_FALSE(Select().has_value());
+  EXPECT_THAT(connect_attempts_, ElementsAre(PipeNameType::kAdminProtected));
+}
+
+}  // namespace
+}  // namespace updater
diff --git a/components/named_mojo_ipc_server/named_mojo_ipc_server_unittest.cc b/components/named_mojo_ipc_server/named_mojo_ipc_server_unittest.cc
index 739a0df..b973f83 100644
--- a/components/named_mojo_ipc_server/named_mojo_ipc_server_unittest.cc
+++ b/components/named_mojo_ipc_server/named_mojo_ipc_server_unittest.cc
@@ -194,7 +194,7 @@
       options, base::BindRepeating([](test::mojom::Echo* impl,
                                       const ConnectionInfo&) { return impl; },
                                    this));
-  ipc_server_->set_on_server_endpoint_created_callback_for_testing(
+  ipc_server_->set_on_server_endpoint_created_callback(
       base::BindRepeating(&NamedMojoIpcServerTest::OnServerEndpointCreated,
                           base::Unretained(this)));
 }