From e36b9582aac21b6f861ff389c61e941db381f257 Mon Sep 17 00:00:00 2001
From: Joe Downing <[email protected]>
Date: Mon, 24 Aug 2026 15:16:54 -0700
Subject: [PATCH] [M153] Validate Mojo message header in FileChooserWindows

Original change's description:
> Validate Mojo message header in FileChooserWindows
>
> When prompting the user for a file on Windows, the SYSTEM-privileged
> remoting_desktop process spawns a low-privileged child process
> (remoting_host.exe --type=file_chooser) and reads the serialized
> FileChooserResult response over an anonymous pipe.
>
> Previously, the SYSTEM process wrapped the raw pipe bytes in a
> mojo::Message and called
> mojom::FileChooserResult::DeserializeFromMessage directly. Because
> manual deserialization bypasses the standard Mojo
> MessageHeaderValidator and payload bounds checks rely on DCHECKs
> in release builds, a compromised child could supply crafted Mojo
> headers (such as invalid num_bytes or manipulated relative payload
> pointers) leading to out-of-bounds heap reads in the SYSTEM process.
>
> This CL:
> 1. Extracts ParseFileChooserResponse() to validate incoming message
>    bytes with mojo::MessageHeaderValidator before deserialization.
> 2. Returns a FileTransfer_Error if validation fails, preventing OOB
>    reads.
> 3. Adds FileChooserWinTest to verify that valid responses deserialize
>    correctly and malformed headers are rejected safely.
> 4. Adds a TODO to replace raw pipe serialization with a standard Mojo
>    IPC channel across this privilege boundary.
>
> Bug: 517336350
>
> Change-Id: Ic65bbf494e6058a7b01aed74effa8a55203ce551
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8281486
> Reviewed-by: Yuwei Huang <[email protected]>
> Commit-Queue: Joe Downing <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#1684315}

(cherry picked from commit fbce880a18489ef2eae947127722f97dfb9a7062)

Bug: 551225850,517336350
Change-Id: Ic65bbf494e6058a7b01aed74effa8a55203ce551
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8279136
Bot-Commit: [email protected] <[email protected]>
Auto-Submit: chrome-cherry-picker@chops-service-accounts.iam.gserviceaccount.com <chrome-cherry-picker@chops-service-accounts.iam.gserviceaccount.com>
Commit-Queue: [email protected] <[email protected]>
Cr-Commit-Position: refs/branch-heads/8010@{#277}
Cr-Branched-From: 86cee6df69e0463a839c0cc8435c9d4c259434d3-refs/heads/main@{#1681091}
---

diff --git a/remoting/host/file_transfer/BUILD.gn b/remoting/host/file_transfer/BUILD.gn
index 0775683..fcf54e2 100644
--- a/remoting/host/file_transfer/BUILD.gn
+++ b/remoting/host/file_transfer/BUILD.gn
@@ -20,7 +20,6 @@
     frameworks = [ "AppKit.framework" ]
   } else if (is_win) {
     sources += [
-      "file_chooser_common_win.h",
       "file_chooser_main_win.cc",
       "file_chooser_win.cc",
     ]
@@ -91,9 +90,11 @@
       "ensure_user_mac.cc",
     ]
   } else if (is_win) {
+    public += [ "file_chooser_common_win.h" ]
     sources += [
       "directory_helpers_win.cc",
       "ensure_user_win.cc",
+      "file_chooser_common_win.cc",
     ]
   } else if (is_chromeos) {
     sources += [
@@ -182,6 +183,9 @@
     ]
   } else {
     sources += [ "local_file_operations_unittest.cc" ]
+    if (is_win) {
+      sources += [ "file_chooser_win_unittest.cc" ]
+    }
     if (remoting_multi_process) {
       sources += [ "ipc_file_operations_unittest.cc" ]
     }
diff --git a/remoting/host/file_transfer/file_chooser_common_win.cc b/remoting/host/file_transfer/file_chooser_common_win.cc
new file mode 100644
index 0000000..f65677c
--- /dev/null
+++ b/remoting/host/file_transfer/file_chooser_common_win.cc
@@ -0,0 +1,43 @@
+// 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 "remoting/host/file_transfer/file_chooser_common_win.h"
+
+#include <utility>
+
+#include "base/logging.h"
+#include "mojo/public/cpp/bindings/message.h"
+#include "mojo/public/cpp/bindings/message_header_validator.h"
+#include "remoting/host/mojom/desktop_session.mojom.h"
+#include "remoting/protocol/file_transfer_helpers.h"
+
+namespace remoting {
+
+FileChooser::Result ParseFileChooserResponse(
+    base::span<const uint8_t> response_bytes) {
+  // TODO(crbug.com/517336350): Replace raw pipe serialization with a standard
+  // Mojo IPC channel to avoid manual deserialization across privilege
+  // boundaries.
+  mojo::Message serialized_message(response_bytes,
+                                   base::span<mojo::ScopedHandle>());
+
+  mojo::MessageHeaderValidator validator;
+  if (!validator.Accept(&serialized_message)) {
+    LOG(ERROR) << "Failed to validate message header from file chooser.";
+    return protocol::MakeFileTransferError(
+        FROM_HERE, protocol::FileTransfer_Error_Type_UNEXPECTED_ERROR);
+  }
+
+  FileChooser::Result result;
+  if (!mojom::FileChooserResult::DeserializeFromMessage(
+          std::move(serialized_message), &result)) {
+    LOG(ERROR) << "Failed to deserialize response.";
+    return protocol::MakeFileTransferError(
+        FROM_HERE, protocol::FileTransfer_Error_Type_UNEXPECTED_ERROR);
+  }
+
+  return result;
+}
+
+}  // namespace remoting
diff --git a/remoting/host/file_transfer/file_chooser_common_win.h b/remoting/host/file_transfer/file_chooser_common_win.h
index 5d82d5c..cf97266 100644
--- a/remoting/host/file_transfer/file_chooser_common_win.h
+++ b/remoting/host/file_transfer/file_chooser_common_win.h
@@ -6,6 +6,10 @@
 #define REMOTING_HOST_FILE_TRANSFER_FILE_CHOOSER_COMMON_WIN_H_
 
 #include <cstddef>
+#include <cstdint>
+
+#include "base/containers/span.h"
+#include "remoting/host/file_transfer/file_chooser.h"
 
 namespace remoting {
 
@@ -17,6 +21,12 @@
 // likely to see on Windows.
 constexpr std::size_t kFileChooserPipeBufferSize = 4096;
 
+// Parses and deserializes the response bytes received from the file chooser
+// child process. Validates the Mojo message header to ensure safe handling
+// across privilege boundaries.
+FileChooser::Result ParseFileChooserResponse(
+    base::span<const uint8_t> response_bytes);
+
 }  // namespace remoting
 
 #endif  // REMOTING_HOST_FILE_TRANSFER_FILE_CHOOSER_COMMON_WIN_H_
diff --git a/remoting/host/file_transfer/file_chooser_win.cc b/remoting/host/file_transfer/file_chooser_win.cc
index 5c5915eb..40e3ce0b 100644
--- a/remoting/host/file_transfer/file_chooser_win.cc
+++ b/remoting/host/file_transfer/file_chooser_win.cc
@@ -23,11 +23,9 @@
 #include "base/task/sequenced_task_runner.h"
 #include "base/win/object_watcher.h"
 #include "base/win/scoped_handle.h"
-#include "mojo/public/cpp/bindings/message.h"
 #include "remoting/host/base/host_exit_codes.h"
 #include "remoting/host/base/switches.h"
 #include "remoting/host/file_transfer/file_chooser_common_win.h"
-#include "remoting/host/mojom/desktop_session.mojom.h"
 
 namespace remoting {
 
@@ -171,18 +169,8 @@
     return;
   }
 
-  mojo::Message serialized_message(base::span(response_bytes).first(bytes_read),
-                                   base::span<mojo::ScopedHandle>());
-
-  FileChooser::Result result;
-  if (!mojom::FileChooserResult::DeserializeFromMessage(
-          std::move(serialized_message), &result)) {
-    LOG(ERROR) << "Failed to deserialize response.";
-    std::move(callback_).Run(MakeFileTransferError(
-        FROM_HERE, protocol::FileTransfer_Error_Type_UNEXPECTED_ERROR));
-    return;
-  }
-
+  FileChooser::Result result =
+      ParseFileChooserResponse(base::span(response_bytes).first(bytes_read));
   std::move(callback_).Run(std::move(result));
 }
 

diff --git a/remoting/host/file_transfer/file_chooser_win_unittest.cc b/remoting/host/file_transfer/file_chooser_win_unittest.cc
new file mode 100644
index 0000000..364d40a
--- /dev/null
+++ b/remoting/host/file_transfer/file_chooser_win_unittest.cc
@@ -0,0 +1,106 @@
+// 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 "remoting/host/file_transfer/file_chooser.h"
+
+#include <cstdint>
+#include <vector>
+
+#include "base/containers/span.h"
+#include "base/files/file_path.h"
+#include "base/test/task_environment.h"
+#include "mojo/public/cpp/bindings/message.h"
+#include "remoting/host/file_transfer/file_chooser_common_win.h"
+#include "remoting/host/mojom/desktop_session.mojom.h"
+#include "remoting/protocol/file_transfer_helpers.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace remoting {
+
+class FileChooserWinTest : public testing::Test {
+ protected:
+  base::test::TaskEnvironment task_environment_;
+};
+
+TEST_F(FileChooserWinTest, ParseValidSuccessResponse) {
+  base::FilePath test_path(FILE_PATH_LITERAL("C:\\test\\file.txt"));
+  FileChooser::Result input(test_path);
+  mojo::Message serialized_message =
+      mojom::FileChooserResult::SerializeAsMessage(&input);
+
+  FileChooser::Result result =
+      ParseFileChooserResponse(serialized_message.data_as_span());
+
+  ASSERT_TRUE(result.is_success());
+  EXPECT_EQ(result.success(), test_path);
+}
+
+TEST_F(FileChooserWinTest, ParseValidErrorResponse) {
+  protocol::FileTransfer_Error error = MakeFileTransferError(
+      FROM_HERE, protocol::FileTransfer_Error_Type_CANCELED);
+  FileChooser::Result input(error);
+  mojo::Message serialized_message =
+      mojom::FileChooserResult::SerializeAsMessage(&input);
+
+  FileChooser::Result result =
+      ParseFileChooserResponse(serialized_message.data_as_span());
+
+  ASSERT_TRUE(result.is_error());
+  EXPECT_EQ(result.error().type(), protocol::FileTransfer_Error_Type_CANCELED);
+}
+
+TEST_F(FileChooserWinTest, RejectMalformedHeaderWithInvalidNumBytes) {
+  // Vector A: 8-byte V0 header with num_bytes (0x28 = 40) larger than buffer.
+  const uint8_t malformed_bytes[8] = {0x28, 0x00, 0x00, 0x00,
+                                      0x00, 0x00, 0x00, 0x00};
+  FileChooser::Result result = ParseFileChooserResponse(malformed_bytes);
+
+  ASSERT_TRUE(result.is_error());
+  EXPECT_EQ(result.error().type(),
+            protocol::FileTransfer_Error_Type_UNEXPECTED_ERROR);
+}
+
+TEST_F(FileChooserWinTest, RejectMalformedHeaderWithV2PayloadPointerOOB) {
+  // Vector B: 48-byte V2 header with payload offset pointing outside buffer.
+  uint8_t malformed_bytes[48] = {0};
+  // num_bytes = 48
+  malformed_bytes[0] = 48;
+  // version = 2
+  malformed_bytes[4] = 2;
+  // payload offset pointing 0x1000 bytes away
+  malformed_bytes[32] = 0x00;
+  malformed_bytes[33] = 0x10;
+  malformed_bytes[40] = 0x00;
+  malformed_bytes[41] = 0x10;
+
+  FileChooser::Result result = ParseFileChooserResponse(malformed_bytes);
+
+  ASSERT_TRUE(result.is_error());
+  EXPECT_EQ(result.error().type(),
+            protocol::FileTransfer_Error_Type_UNEXPECTED_ERROR);
+}
+
+TEST_F(FileChooserWinTest, RejectEmptyBuffer) {
+  FileChooser::Result result =
+      ParseFileChooserResponse(base::span<const uint8_t>());
+
+  ASSERT_TRUE(result.is_error());
+  EXPECT_EQ(result.error().type(),
+            protocol::FileTransfer_Error_Type_UNEXPECTED_ERROR);
+}
+
+TEST_F(FileChooserWinTest, RejectTruncatedHeaders) {
+  // Test various truncated buffer sizes smaller than full Mojo message headers.
+  for (size_t size : {1, 4, 7, 16, 24}) {
+    std::vector<uint8_t> truncated_bytes(size, 0);
+    FileChooser::Result result = ParseFileChooserResponse(truncated_bytes);
+
+    ASSERT_TRUE(result.is_error()) << "Failed for size: " << size;
+    EXPECT_EQ(result.error().type(),
+              protocol::FileTransfer_Error_Type_UNEXPECTED_ERROR)
+        << "Failed for size: " << size;
+  }
+}
+
+}  // namespace remoting