From 3fe58100b42fb607e3c62a5cd010ce44d9fa22c1 Mon Sep 17 00:00:00 2001
From: Noah Rose Ledesma <noahrose@google.com>
Date: Tue, 23 Jun 2026 14:26:37 -0700
Subject: [PATCH] Stream downloads from OOP fetcher

The updater relies on an out of process network fetcher on macOS to
authenticate with some proxies when running as root. Previously, this
worker operated in the bootstrap namespace of a logged-in user but with
root's uid. A recent change to mitigate a vulnerability made this
fetcher run as the "nobody" user instead.

This created a separate gap by which the insecure "nobody" process could
retain a file handle and modify content after CRX validation, resulting
in a TOCTOU error in the root process that could lead to LPE.

The TOCTOU error is resolved separately by the root process by cloning
the file provided by the less-priviledged process. However, it is still
prudent to limit the capabilities of the fetcher by not allowing it to
write directly to sensitive file system paths.

Refactor the out of process network fetcher's API to stream bytes to the
parent process which writes the file.

Bug: 522314940
Change-Id: I8e26d4f3ea921bb6a1de2aba0316cf326a6a6964
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7946001
Commit-Queue: Noah Rose Ledesma <noahrose@google.com>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Adam Norberg <norberg@google.com>
Cr-Commit-Position: refs/heads/main@{#1651283}
---

diff --git a/chrome/updater/BUILD.gn b/chrome/updater/BUILD.gn
index 6b48ea28..fb1d21ec 100644
--- a/chrome/updater/BUILD.gn
+++ b/chrome/updater/BUILD.gn
@@ -275,6 +275,8 @@
         "mac/setup/mac_setup.mm",
         "mac/setup/wake_task.h",
         "mac/setup/wake_task.mm",
+        "net/chunk_queue.h",
+        "net/chunk_queue.mm",
         "net/fetcher_callback_adapter.cc",
         "net/fetcher_callback_adapter.h",
         "net/network_fetcher_mac.mm",
@@ -1146,6 +1148,7 @@
         "mac/setup/ks_tickets_unittest.mm",
         "mac/setup/setup_unittest.mm",
         "mac/setup/wake_task_unittest.mm",
+        "net/chunk_queue_unittest.mm",
         "policy/mac/managed_preference_policy_manager_impl_unittest.mm",
         "policy/mac/managed_preference_policy_manager_unittest.cc",
         "test/integration_tests_mac.h",
diff --git a/chrome/updater/app/app_net_worker.cc b/chrome/updater/app/app_net_worker.cc
index 1786606c..a4e2e38 100644
--- a/chrome/updater/app/app_net_worker.cc
+++ b/chrome/updater/app/app_net_worker.cc
@@ -20,6 +20,7 @@
 #include "base/functional/bind.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
 #include "base/task/bind_post_task.h"
@@ -37,6 +38,7 @@
 #include "mojo/public/cpp/bindings/remote.h"
 #include "mojo/public/cpp/bindings/self_owned_receiver.h"
 #include "mojo/public/cpp/platform/platform_channel.h"
+#include "mojo/public/cpp/system/data_pipe.h"
 #include "mojo/public/cpp/system/invitation.h"
 #include "mojo/public/cpp/system/message_pipe.h"
 #include "url/gurl.h"
@@ -92,7 +94,7 @@
     : public base::RefCountedThreadSafe<FileDownloadObserverWrapper> {
  public:
   explicit FileDownloadObserverWrapper(
-      mojom::FetchService::DownloadToFileCallback callback) {
+      mojom::FetchService::DownloadToStreamCallback callback) {
     std::move(callback).Run(observer_.BindNewPipeAndPassReceiver());
   }
 
@@ -101,11 +103,6 @@
     observer_->OnResponseStarted(http_status_code, content_length);
   }
 
-  void OnProgress(int64_t current) {
-    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-    observer_->OnProgress(current);
-  }
-
   void OnDownloadComplete(int32_t net_error, int64_t content_length) {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     observer_->OnDownloadComplete(net_error, content_length);
@@ -138,10 +135,10 @@
                    std::vector<mojom::HttpHeaderPtr> additional_headers,
                    mojom::FetchService::PostRequestCallback callback) override;
 
-  void DownloadToFile(
+  void DownloadToStream(
       const ::GURL& url,
-      ::base::File output_file,
-      mojom::FetchService::DownloadToFileCallback callback) override;
+      mojo::ScopedDataPipeProducerHandle response_stream,
+      mojom::FetchService::DownloadToStreamCallback callback) override;
 
  private:
   SEQUENCE_CHECKER(sequence_checker_);
@@ -202,28 +199,25 @@
           std::move(fetcher), wrapper));
 }
 
-void FetchServiceImpl::DownloadToFile(
-    const ::GURL& url,
-    ::base::File output_file,
-    mojom::FetchService::DownloadToFileCallback callback) {
+void FetchServiceImpl::DownloadToStream(
+    const GURL& url,
+    mojo::ScopedDataPipeProducerHandle response_stream,
+    mojom::FetchService::DownloadToStreamCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   auto wrapper =
       base::MakeRefCounted<FileDownloadObserverWrapper>(std::move(callback));
-  std::unique_ptr<NetworkFileFetcher> file_fetcher =
-      std::make_unique<NetworkFileFetcher>();
-  NetworkFileFetcher* file_fetcher_ptr = file_fetcher.get();
-  file_fetcher_ptr->Download(
-      url, std::move(output_file),
+  auto stream_fetcher = base::MakeRefCounted<NetworkStreamFetcher>();
+  stream_fetcher->Download(
+      url, std::move(response_stream),
       base::BindRepeating(&FileDownloadObserverWrapper::OnResponseStarted,
                           wrapper),
-      base::BindRepeating(&FileDownloadObserverWrapper::OnProgress, wrapper),
       base::BindOnce(
-          [](std::unique_ptr<NetworkFileFetcher> /*file_fetcher*/,
+          [](scoped_refptr<NetworkStreamFetcher> /*stream_fetcher*/,
              scoped_refptr<FileDownloadObserverWrapper> wrapper,
              int32_t net_error, int64_t content_length) {
             wrapper->OnDownloadComplete(net_error, content_length);
           },
-          std::move(file_fetcher), wrapper));
+          stream_fetcher, wrapper));
 }
 
 // AppNetWorker runs networking tasks in a dedicated process.
diff --git a/chrome/updater/app/app_net_worker_unittest.cc b/chrome/updater/app/app_net_worker_unittest.cc
index a25b498a..e9f3541 100644
--- a/chrome/updater/app/app_net_worker_unittest.cc
+++ b/chrome/updater/app/app_net_worker_unittest.cc
@@ -11,6 +11,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/barrier_closure.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_file.h"
 #include "base/functional/bind.h"
@@ -38,8 +39,11 @@
 #include "mojo/public/cpp/bindings/remote.h"
 #include "mojo/public/cpp/bindings/self_owned_receiver.h"
 #include "mojo/public/cpp/platform/platform_channel.h"
+#include "mojo/public/cpp/system/data_pipe.h"
+#include "mojo/public/cpp/system/data_pipe_drainer.h"
 #include "mojo/public/cpp/system/invitation.h"
 #include "mojo/public/cpp/system/message_pipe.h"
+#include "mojo/public/cpp/system/wait.h"
 #include "net/base/net_errors.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_status_code.h"
@@ -51,6 +55,31 @@
 
 namespace updater {
 
+namespace {
+
+class TestDrainer : public mojo::DataPipeDrainer::Client {
+ public:
+  TestDrainer(mojo::ScopedDataPipeConsumerHandle consumer,
+              base::OnceClosure on_complete)
+      : drainer_(this, std::move(consumer)),
+        on_complete_(std::move(on_complete)) {}
+
+  // mojo::DataPipeDrainer::Client:
+  void OnDataAvailable(base::span<const uint8_t> data) override {
+    data_.append(reinterpret_cast<const char*>(data.data()), data.size());
+  }
+  void OnDataComplete() override { std::move(on_complete_).Run(); }
+
+  const std::string& data() const { return data_; }
+
+ private:
+  mojo::DataPipeDrainer drainer_;
+  base::OnceClosure on_complete_;
+  std::string data_;
+};
+
+}  // namespace
+
 class AppNetWorkerTest : public ::testing::Test {

diff --git a/chrome/updater/app/app_net_worker_unittest.cc b/chrome/updater/app/app_net_worker_unittest.cc
index a25b498a..e9f3541 100644
--- a/chrome/updater/app/app_net_worker_unittest.cc
+++ b/chrome/updater/app/app_net_worker_unittest.cc
@@ -11,6 +11,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/barrier_closure.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_file.h"
 #include "base/functional/bind.h"
@@ -38,8 +39,11 @@
 #include "mojo/public/cpp/bindings/remote.h"
 #include "mojo/public/cpp/bindings/self_owned_receiver.h"
 #include "mojo/public/cpp/platform/platform_channel.h"
+#include "mojo/public/cpp/system/data_pipe.h"
+#include "mojo/public/cpp/system/data_pipe_drainer.h"
 #include "mojo/public/cpp/system/invitation.h"
 #include "mojo/public/cpp/system/message_pipe.h"
+#include "mojo/public/cpp/system/wait.h"
 #include "net/base/net_errors.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_status_code.h"
@@ -51,6 +55,31 @@
 
 namespace updater {
 
+namespace {
+
+class TestDrainer : public mojo::DataPipeDrainer::Client {
+ public:
+  TestDrainer(mojo::ScopedDataPipeConsumerHandle consumer,
+              base::OnceClosure on_complete)
+      : drainer_(this, std::move(consumer)),
+        on_complete_(std::move(on_complete)) {}
+
+  // mojo::DataPipeDrainer::Client:
+  void OnDataAvailable(base::span<const uint8_t> data) override {
+    data_.append(reinterpret_cast<const char*>(data.data()), data.size());
+  }
+  void OnDataComplete() override { std::move(on_complete_).Run(); }
+
+  const std::string& data() const { return data_; }
+
+ private:
+  mojo::DataPipeDrainer drainer_;
+  base::OnceClosure on_complete_;
+  std::string data_;
+};
+
+}  // namespace
+
 class AppNetWorkerTest : public ::testing::Test {
  protected:
   void SetUp() override {
@@ -155,14 +184,17 @@
       }));
   ASSERT_TRUE(test_server.Start());
 
-  base::ScopedTempFile output;
-  ASSERT_TRUE(output.Create());
-  base::File output_file(output.path(),
-                         base::File::FLAG_OPEN_ALWAYS | base::File::FLAG_WRITE);
-  ASSERT_TRUE(output_file.IsValid());
+  mojo::ScopedDataPipeProducerHandle producer;
+  mojo::ScopedDataPipeConsumerHandle consumer;
+  ASSERT_EQ(mojo::CreateDataPipe(nullptr, producer, consumer), MOJO_RESULT_OK);
+
   base::RunLoop run_loop;
-  remote_->DownloadToFile(
-      test_server.GetURL("/"), std::move(output_file),
+
+  base::RepeatingClosure barrier =
+      base::BarrierClosure(2, run_loop.QuitClosure());
+
+  remote_->DownloadToStream(
+      test_server.GetURL("/"), std::move(producer),
       MakeFileDownloadObserver(
           base::BindLambdaForTesting(
               [&](int32_t http_status_code, int64_t content_length) {
@@ -171,20 +203,89 @@
                   EXPECT_EQ(content_length, payload_size.value());
                 }
               }),
-          base::BindLambdaForTesting([&](int64_t current) {
-            EXPECT_LE(current, payload_size.value());
-          }),
           base::BindLambdaForTesting(
               [&](int32_t net_error, int64_t content_length) {
                 EXPECT_EQ(net_error, 0);
                 EXPECT_EQ(content_length, payload_size.value());
-                run_loop.Quit();
+                barrier.Run();
               })));
+
+  TestDrainer drainer(std::move(consumer), barrier);
   run_loop.Run();
-  EXPECT_TRUE(base::ContentsEqual(output.path(), payload_path));
+
+  EXPECT_EQ(drainer.data(), payload);
 }
 
-TEST_F(AppNetWorkerTest, DownloadMultipleFiles) {
+TEST_F(AppNetWorkerTest, DownloadFileRecoversFromFullPipe) {
+  std::string payload(5000, 'A');
+  net::EmbeddedTestServer test_server;
+  test_server.RegisterRequestHandler(base::BindLambdaForTesting(
+      [&](const net::test_server::HttpRequest& request)
+          -> std::unique_ptr<net::test_server::HttpResponse> {
+        auto http_response =
+            std::make_unique<net::test_server::BasicHttpResponse>();
+        http_response->set_code(net::HTTP_OK);
+        http_response->set_content(payload);
+        http_response->set_content_type("application/octet-stream");
+        return http_response;
+      }));
+  ASSERT_TRUE(test_server.Start());
+
+  mojo::ScopedDataPipeProducerHandle producer;
+  mojo::ScopedDataPipeConsumerHandle consumer;
+  constexpr static size_t kPipeCapacity = 1024;
+  ASSERT_EQ(mojo::CreateDataPipe(kPipeCapacity, producer, consumer),
+            MOJO_RESULT_OK);
+
+  base::RunLoop run_loop;
+
+  base::RepeatingClosure barrier =
+      base::BarrierClosure(2, run_loop.QuitClosure());
+
+  base::RunLoop response_wait_loop;
+  bool response_started = false;
+
+  remote_->DownloadToStream(
+      test_server.GetURL("/"), std::move(producer),
+      MakeFileDownloadObserver(
+          base::BindLambdaForTesting(
+              [&](int32_t http_status_code, int64_t content_length) {
+                EXPECT_EQ(http_status_code, net::HTTP_OK);
+                EXPECT_EQ(content_length, 5000);
+                response_started = true;
+                response_wait_loop.Quit();
+              }),
+          base::BindLambdaForTesting(
+              [&](int32_t net_error, int64_t content_length) {
+                EXPECT_EQ(net_error, 0);
+                EXPECT_EQ(content_length, 5000);
+                barrier.Run();
+              })));
+
+  response_wait_loop.Run();
+  EXPECT_TRUE(response_started);
+
+  // Wait until the writer in the child process has filled the pipe.
+  while (true) {
+    size_t bytes_available = 0;
+    MojoResult result = consumer->ReadData(
+        MOJO_READ_DATA_FLAG_QUERY, base::span<uint8_t>(), bytes_available);
+    if (result == MOJO_RESULT_OK && bytes_available >= kPipeCapacity) {
+      break;
+    }
+    base::RunLoop loop;
+    base::ThreadPool::PostDelayedTask(FROM_HERE, loop.QuitClosure(),
+                                      base::Milliseconds(10));
+    loop.Run();
+  }
+
+  TestDrainer drainer(std::move(consumer), barrier);
+  run_loop.Run();
+
+  EXPECT_EQ(drainer.data(), payload);
+}
+
+TEST_F(AppNetWorkerTest, DownloadMultiple) {
   base::FilePath payload_path = updater::test::GetTestFilePath("signed.exe.gz");
   std::optional<int64_t> payload_size = base::GetFileSize(payload_path);
   ASSERT_TRUE(payload_size.has_value());
@@ -207,14 +308,17 @@
   ASSERT_TRUE(test_server.Start());
 
   for (int i = 0; i < 2; ++i) {
-    base::ScopedTempFile output;
-    ASSERT_TRUE(output.Create());
-    base::File output_file(
-        output.path(), base::File::FLAG_OPEN_ALWAYS | base::File::FLAG_WRITE);
-    ASSERT_TRUE(output_file.IsValid());
+    mojo::ScopedDataPipeProducerHandle producer;
+    mojo::ScopedDataPipeConsumerHandle consumer;
+    ASSERT_EQ(mojo::CreateDataPipe(nullptr, producer, consumer),
+              MOJO_RESULT_OK);
+
     base::RunLoop loop;
-    remote_->DownloadToFile(
-        test_server.GetURL("/payload"), std::move(output_file),
+    base::RepeatingClosure barrier =
+        base::BarrierClosure(2, loop.QuitClosure());
+
+    remote_->DownloadToStream(
+        test_server.GetURL("/payload"), std::move(producer),
         MakeFileDownloadObserver(
             base::BindLambdaForTesting(
                 [&](int32_t http_status_code, int64_t content_length) {
@@ -223,17 +327,16 @@
                     EXPECT_EQ(content_length, payload_size.value());
                   }
                 }),
-            base::BindLambdaForTesting([&](int64_t current) {
-              EXPECT_LE(current, payload_size.value());
-            }),
             base::BindLambdaForTesting(
                 [&](int32_t net_error, int64_t content_length) {
                   EXPECT_EQ(net_error, 0);
                   EXPECT_EQ(content_length, payload_size.value());
-                  loop.Quit();
+                  barrier.Run();
                 })));
+
+    TestDrainer drainer(std::move(consumer), barrier);
     loop.Run();
-    EXPECT_TRUE(base::ContentsEqual(output.path(), payload_path));
+    EXPECT_EQ(drainer.data(), payload);
   }
 }
diff --git a/chrome/updater/net/chunk_queue_unittest.mm b/chrome/updater/net/chunk_queue_unittest.mm
new file mode 100644
index 0000000..f6df393
--- /dev/null
+++ b/chrome/updater/net/chunk_queue_unittest.mm
@@ -0,0 +1,188 @@
+// 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/net/chunk_queue.h"
+
+#import <Foundation/Foundation.h>
+
+#include <algorithm>
+#include <cstddef>
+#include <cstdint>
+#include <string>
+
+#include "base/functional/bind.h"
+#include "base/notreached.h"
+#include "base/test/bind.h"
+#include "mojo/public/c/system/types.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+@interface MultiRangeData : NSData {
+ @private
+  std::vector<std::string> _ranges;
+}
+
+- (instancetype)initWithRanges:(std::vector<std::string>)ranges;
+@end
+
+@implementation MultiRangeData
+
+- (instancetype)initWithRanges:(std::vector<std::string>)ranges {
+  if ((self = [super init])) {
+    _ranges = std::move(ranges);
+  }
+  return self;
+}
+
+- (id)copyWithZone:(NSZone*)zone {
+  return self;
+}
+
+- (NSUInteger)length {
+  NSUInteger total = 0;
+  for (const auto& r : _ranges) {
+    total += r.length();
+  }
+  return total;
+}
+
+- (const void*)bytes {
+  NOTREACHED();
+}
+
+- (void)enumerateByteRangesUsingBlock:
+    (void(NS_NOESCAPE ^)(const void* bytes, NSRange byteRange, BOOL* stop))
+        block {
+  NSUInteger current_offset = 0;
+  for (const auto& r : _ranges) {
+    BOOL stop = NO;
+    block(r.data(), NSMakeRange(current_offset, r.length()), &stop);
+    if (stop) {
+      break;
+    }
+    current_offset += r.length();
+  }
+}
+@end
+
+namespace updater {
+
+TEST(ChunkQueueTest, PushAndEmpty) {
+  ChunkQueue queue;
... (truncated)