From b8b43e307495d9946f5a6dad02bbcff925d4eeb1 Mon Sep 17 00:00:00 2001
From: Joe Downing <joedow@google.com>
Date: Thu, 16 Apr 2026 23:12:07 -0700
Subject: [PATCH] CRD: Use safe buffer access in EventTraceData::Create

The Chrome Remote Desktop daemon on Windows parses ETW logs.
An integer truncation bug in EventTraceData::Create allowed a
malicious ETW event to specify an out-of-bounds read offset,
potentially leading to heap memory leakage into log files.

This patch refactors the parsing logic to use base::span and
base::SpanReader. This prevents truncation by using safe arithmetic
(base::CheckedNumeric) and ensures all reads are bounds-checked by
the underlying abstractions.

Key changes:
 - Replaced raw pointer arithmetic with base::SpanReader for
   sequential field extraction.
  - Used base::CheckedNumeric to safely calculate stack data skip
    sizes, preventing truncation and overflow on 32-bit/64-bit
    systems.
  - Replaced strnlen_s and manual char pointer casts with
    base::as_string_view and std::string_view::find for safer string
    parsing.
  - Added an integrity check (DCHECK) to ensure the entire MofData
    buffer is consumed.
  - Added a regression test case to verify safe handling of malformed
    ETW payloads with excessive stack depth.

Bug: 501900366
Change-Id: If10ca8b965be8c648cc2a4acc785a5d231a5e183
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7769113
Reviewed-by: Yuwei Huang <yuweih@chromium.org>
Commit-Queue: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1616350}
---

diff --git a/remoting/host/win/event_trace_data.cc b/remoting/host/win/event_trace_data.cc
index 4ae3b2d..26ca871 100644
--- a/remoting/host/win/event_trace_data.cc
+++ b/remoting/host/win/event_trace_data.cc
@@ -4,12 +4,18 @@
 
 #include "remoting/host/win/event_trace_data.h"
 
+#include <string_view>
+
 #include "base/check.h"
 #include "base/compiler_specific.h"
+#include "base/containers/span.h"
+#include "base/containers/span_reader.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/logging_win.h"
 #include "base/notreached.h"
+#include "base/numerics/checked_math.h"
+#include "base/strings/string_view_util.h"
 #include "base/strings/utf_string_conversions.h"
 
 namespace remoting {
@@ -70,38 +76,60 @@
   // - For LOG_MESSAGE_FULL events, the MofData buffer is comprised of 5 fields
   //   which must be parsed (or skipped) in sequence.
   if (data.event_type == logging::LOG_MESSAGE) {
-    data.message.assign(reinterpret_cast<const char*>(event->MofData),
-                        event->MofLength);
+    // SAFETY: `event->MofData` and `event->MofLength` are provided by the
+    // Windows ETW subsystem. We trust these values to define the valid memory
+    // range for the event payload.
+    auto message_span = UNSAFE_BUFFERS(base::span(
+        reinterpret_cast<const uint8_t*>(event->MofData), event->MofLength));
+    std::string_view message_view = base::as_string_view(message_span);
+    data.message.assign(message_view.substr(0, message_view.find('\0')));
   } else if (data.event_type == logging::LOG_MESSAGE_FULL) {
-    const uint8_t* mof_data = reinterpret_cast<const uint8_t*>(event->MofData);
-    uint32_t offset = 0;
+    // SAFETY: `event->MofData` and `event->MofLength` are provided by the
+    // Windows ETW subsystem. We trust these values to define the valid memory
+    // range for the event payload.
+    base::SpanReader reader(UNSAFE_BUFFERS(base::span(
+        reinterpret_cast<const uint8_t*>(event->MofData), event->MofLength)));
 
     // Read the size, skip past the stack info, and move the cursor.
-    DWORD stack_depth = *reinterpret_cast<const DWORD*>(mof_data);
-    int bytes_to_skip = sizeof(DWORD) + stack_depth * sizeof(intptr_t);
-    offset += bytes_to_skip;
+    uint32_t stack_depth;
+    if (!reader.ReadU32NativeEndian(stack_depth)) {
+      return data;
+    }
+    base::CheckedNumeric<size_t> bytes_to_skip = stack_depth;
+    bytes_to_skip *= sizeof(intptr_t);
+    if (!bytes_to_skip.IsValid() || !reader.Skip(bytes_to_skip.ValueOrDie())) {
+      return data;
+    }
 
     // Read the line info and move the cursor.
-    data.line =
-        *reinterpret_cast<const int32_t*>(UNSAFE_TODO(mof_data + offset));
-    offset += sizeof(int32_t);
+    if (!reader.ReadI32NativeEndian(data.line)) {
+      return data;
+    }
 
     // Read the file info and move the cursor.
-    const char* file_info =
-        reinterpret_cast<const char*>(UNSAFE_TODO(mof_data + offset));
-    size_t str_len = strnlen_s(file_info, event->MofLength - offset);
-    base::FilePath file_path(base::UTF8ToWide(file_info));
+    std::string_view file_info_view =
+        base::as_string_view(reader.remaining_span());
+    size_t nul_pos = file_info_view.find('\0');
+    if (nul_pos == std::string_view::npos) {
+      return data;
+    }
+    base::FilePath file_path(
+        base::UTF8ToWide(file_info_view.substr(0, nul_pos)));
     data.file_name = base::WideToUTF8(file_path.BaseName().value());
-    offset += (str_len + 1);
+    reader.Skip(nul_pos + 1);
 
     // Read the message and move the cursor.
-    const char* message =
-        reinterpret_cast<const char*>(UNSAFE_TODO(mof_data + offset));
-    str_len = strnlen_s(message, event->MofLength - offset);
-    data.message.assign(message);
-    offset += (str_len + 1);
+    std::string_view message_view =
+        base::as_string_view(reader.remaining_span());
+    nul_pos = message_view.find('\0');
+    if (nul_pos == std::string_view::npos) {
+      return data;
+    }
+    data.message.assign(message_view.substr(0, nul_pos));
+    reader.Skip(nul_pos + 1);
 
-    DCHECK_EQ(event->MofLength, offset);
+    // Ensure that the entire buffer was consumed.
+    DCHECK_EQ(reader.remaining(), 0u);
   } else {
     NOTREACHED() << "Unknown event type: " << data.event_type;
   }
diff --git a/remoting/host/win/event_trace_data_unittest.cc b/remoting/host/win/event_trace_data_unittest.cc
index f47447a..243c0ac6 100644
--- a/remoting/host/win/event_trace_data_unittest.cc
+++ b/remoting/host/win/event_trace_data_unittest.cc
@@ -4,6 +4,9 @@
 
 #include "remoting/host/win/event_trace_data.h"
 
+#include <string>
+#include <vector>
+
 #include "base/check.h"
 #include "base/compiler_specific.h"
 #include "base/logging.h"
@@ -27,15 +30,13 @@
  protected:
   void InitForLogMessage();
   void InitForLogMessageFull();
+  void InitSharedFields(uint8_t type);
 
   size_t ReserveBufferSpace(size_t space_needed);
 
   FILETIME time_ = {};
   EVENT_TRACE event_trace_ = {};
   std::vector<uint8_t> buffer_;
-
- private:
-  void InitSharedFields(uint8_t type);
 };
 
 void EventTraceDataTest::InitSharedFields(uint8_t type) {
@@ -110,6 +111,7 @@
   EXPECT_EQ(data.process_id, kProcessId);
   EXPECT_EQ(data.thread_id, kThreadId);
   EXPECT_STREQ(data.message.c_str(), kTestLogMessage);
+  EXPECT_EQ(data.message.length(), strlen(kTestLogMessage));
 
   // File and line data should not be filled in for this log message type.
   EXPECT_EQ(data.file_name, std::string());
@@ -131,4 +133,29 @@
   EXPECT_STREQ(data.message.c_str(), kTestLogMessage);
 }
 
+TEST_F(EventTraceDataTest, LogFullMessage_LargeStackDepth) {
+  InitSharedFields(static_cast<uint8_t>(logging::LOG_MESSAGE_FULL));
+
+  // A large stack depth that would require more memory than is available in
+  // the buffer should be handled safely.
+  DWORD large_stack_depth = 0x3FFFFFFE;
+  size_t data_size = sizeof(DWORD);
+  size_t offset = ReserveBufferSpace(data_size);
+  UNSAFE_TODO(memcpy(buffer_.data() + offset, &large_stack_depth, data_size));
+
+  // Set the MofLength to the current buffer size, which only contains the
+  // large stack depth value.
+  event_trace_.MofData = buffer_.data();
+  event_trace_.MofLength = buffer_.size();
+

diff --git a/remoting/host/win/event_trace_data_unittest.cc b/remoting/host/win/event_trace_data_unittest.cc
index f47447a..243c0ac6 100644
--- a/remoting/host/win/event_trace_data_unittest.cc
+++ b/remoting/host/win/event_trace_data_unittest.cc
@@ -4,6 +4,9 @@
 
 #include "remoting/host/win/event_trace_data.h"
 
+#include <string>
+#include <vector>
+
 #include "base/check.h"
 #include "base/compiler_specific.h"
 #include "base/logging.h"
@@ -27,15 +30,13 @@
  protected:
   void InitForLogMessage();
   void InitForLogMessageFull();
+  void InitSharedFields(uint8_t type);
 
   size_t ReserveBufferSpace(size_t space_needed);
 
   FILETIME time_ = {};
   EVENT_TRACE event_trace_ = {};
   std::vector<uint8_t> buffer_;
-
- private:
-  void InitSharedFields(uint8_t type);
 };
 
 void EventTraceDataTest::InitSharedFields(uint8_t type) {
@@ -110,6 +111,7 @@
   EXPECT_EQ(data.process_id, kProcessId);
   EXPECT_EQ(data.thread_id, kThreadId);
   EXPECT_STREQ(data.message.c_str(), kTestLogMessage);
+  EXPECT_EQ(data.message.length(), strlen(kTestLogMessage));
 
   // File and line data should not be filled in for this log message type.
   EXPECT_EQ(data.file_name, std::string());
@@ -131,4 +133,29 @@
   EXPECT_STREQ(data.message.c_str(), kTestLogMessage);
 }
 
+TEST_F(EventTraceDataTest, LogFullMessage_LargeStackDepth) {
+  InitSharedFields(static_cast<uint8_t>(logging::LOG_MESSAGE_FULL));
+
+  // A large stack depth that would require more memory than is available in
+  // the buffer should be handled safely.
+  DWORD large_stack_depth = 0x3FFFFFFE;
+  size_t data_size = sizeof(DWORD);
+  size_t offset = ReserveBufferSpace(data_size);
+  UNSAFE_TODO(memcpy(buffer_.data() + offset, &large_stack_depth, data_size));
+
+  // Set the MofLength to the current buffer size, which only contains the
+  // large stack depth value.
+  event_trace_.MofData = buffer_.data();
+  event_trace_.MofLength = buffer_.size();
+
+  // The malformed data should be detected and handled without crashing or
+  // reading out of bounds.
+  EventTraceData data = EventTraceData::Create(&event_trace_);
+
+  // Since the payload is malformed, the line number and message should not
+  // be populated.
+  EXPECT_EQ(data.line, 0);
+  EXPECT_TRUE(data.message.empty());
+}
+
 }  // namespace remoting