PatchGap
Chrome · Tracing

CVE-2025-0438

OOB in Tracing
High chrome OOB 📄 Reporter bug report 🔧 Commit mapped

Overview

High
Severity
—
CVSS
No
Exploited ITW
Fixed
Fix Status
ImpactStack buffer overflow in Tracing
DescriptionStack buffer overflow in Tracing
ComponentTracing
Bug ClassOOB
Tracker384186539
Fix commitb6ec777da385 (chromium/src) +51/-10
CISA KEVNot listed
CreditedHan Zheng (HexHive)
Disclosed2025-01-14

Changed Functions

FunctionChangeNotes
if
services/tracing/perfetto/system_test_utils.cc
 modified
if
services/tracing/public/cpp/system_tracing_service_unittest.cc
 modified
TEST_F
services/tracing/public/cpp/system_tracing_service_unittest.cc
 modified

Files Changed

  • services/tracing/perfetto/system_test_utils.cc
  • services/tracing/perfetto/system_test_utils.h
  • services/tracing/public/cpp/system_tracing_service.cc
  • services/tracing/public/cpp/system_tracing_service_unittest.cc

Patch

view commit
From b6ec777da385a63fd8c57f2a86acdb3e43420eb0 Mon Sep 17 00:00:00 2001
From: Chinglin Yu <[email protected]>
Date: Sun, 22 Dec 2024 19:48:09 -0800
Subject: [PATCH] Reland "tracing: handle invalid producer socket name"

This reverts commit 3d784689fc8ac31c97f03712f2efba5eb5a1512a.

Reason for revert: fixed unittest crash in Mac ASAN build.

The test case saves/restores environment variables using
setenv()/getenv(). The original environment variable value is saved as
const char* to the internal buffer allocated by setenv(), which is prone to UAF if the same environment variable is set again. Fix by copying the value from getenv() to avoid the crash. Also fixed system_test_utils.cc that has a similar usage pattern and has suppressed ASAN failure in Mac
ASAN build.

Original change's description:
> Revert "tracing: handle invalid producer socket name"
>
> This reverts commit 8fe29b08f181fda986f9d58c1f3edc283457dc7a.
>
> Reason for revert: b/384862278
>
> Original change's description:
> > tracing: handle invalid producer socket name
> >
> > Don't make the socket connection on seeing an invalid socket name and
> > return an invalid FD to the opener.
> >
> > Bug: 384186539
> > Test: Unit test.
> > Change-Id: I3fce049c4bbe5a078dfe5456208697afa71262ea
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6100572
> > Reviewed-by: Charles Dick <[email protected]>
> > Reviewed-by: Mikhail Khokhlov <[email protected]>
> > Reviewed-by: Stephen Nusko <[email protected]>
> > Commit-Queue: Chinglin Yu <[email protected]>
> > Cr-Commit-Position: refs/heads/main@{#1397727}
>
> Bug: 384186539
> Change-Id: Ia54883fc1e9d60a17f8224cf0270a2e32ad04c05
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6097780
> Auto-Submit: Adem Derinel <[email protected]>
> Owners-Override: Adem Derinel <[email protected]>
> Commit-Queue: Rubber Stamper <[email protected]>
> Bot-Commit: Rubber Stamper <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#1397801}

Bug: 384186539, 384862278
Change-Id: Ib0c63080d13afa958a127166dd1fa15f9b122859
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6108519
Reviewed-by: Mikhail Khokhlov <[email protected]>
Reviewed-by: Charles Dick <[email protected]>
Commit-Queue: Chinglin Yu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1399697}
---

diff --git a/services/tracing/perfetto/system_test_utils.cc b/services/tracing/perfetto/system_test_utils.cc
index 24370e5c..92c9c257 100644
--- a/services/tracing/perfetto/system_test_utils.cc
+++ b/services/tracing/perfetto/system_test_utils.cc
@@ -44,7 +44,10 @@
   // multiple tests run we need to restore the value so each test is
   // hermetic.
 
-  old_tmpdir_ = getenv("TMPDIR");
+  const auto* old_tmpdir = getenv("TMPDIR");
+  if (old_tmpdir) {
+    old_tmpdir_ = old_tmpdir;
+  }
   setenv("TMPDIR", tmp_dir.GetPath().value().c_str(), true);
   // Set up the system socket locations in a valid tmp directory.
   producer_ = tmp_dir.GetPath().Append(FILE_PATH_LITERAL("producer")).value();
@@ -59,7 +62,7 @@
   if (used_tmpdir_) {
     if (old_tmpdir_) {
       // Restore the old value back to its initial value.
-      setenv("TMPDIR", old_tmpdir_, true);
+      setenv("TMPDIR", old_tmpdir_->c_str(), true);
     } else {
       // TMPDIR wasn't set originally so unset it.
       unsetenv("TMPDIR");
diff --git a/services/tracing/perfetto/system_test_utils.h b/services/tracing/perfetto/system_test_utils.h
index fd3c56f..06bef90 100644
--- a/services/tracing/perfetto/system_test_utils.h
+++ b/services/tracing/perfetto/system_test_utils.h
@@ -40,7 +40,7 @@
   void StartService();
 
   const bool used_tmpdir_;
-  const char* old_tmpdir_ = nullptr;
+  std::optional<std::string> old_tmpdir_;
   std::string consumer_;
   std::string producer_;
   std::unique_ptr<perfetto::ServiceIPCHost> service_;
diff --git a/services/tracing/public/cpp/system_tracing_service.cc b/services/tracing/public/cpp/system_tracing_service.cc
index 57ca7d9..86df2a5e 100644
--- a/services/tracing/public/cpp/system_tracing_service.cc
+++ b/services/tracing/public/cpp/system_tracing_service.cc
@@ -68,6 +68,10 @@
     if (socket_fd_.get() == -1) {
       return base::unexpected(errno);
     }
+    if (producer_sock_name.size() > sizeof(sockaddr_un::sun_path) - 1) {
+      DVLOG(3) << "Oversized producer socket name " << producer_sock_name;
+      return base::unexpected(EINVAL);
+    }
 
     struct sockaddr_un saddr;
     memset(&saddr, 0, sizeof(saddr));
diff --git a/services/tracing/public/cpp/system_tracing_service_unittest.cc b/services/tracing/public/cpp/system_tracing_service_unittest.cc
index bf6358c7..b98b9ab 100644
--- a/services/tracing/public/cpp/system_tracing_service_unittest.cc
+++ b/services/tracing/public/cpp/system_tracing_service_unittest.cc
@@ -4,8 +4,10 @@
 
 #include "services/tracing/public/cpp/system_tracing_service.h"
 
+#include <sys/un.h>
 #include <unistd.h>
 
+#include <cstdint>
 #include <optional>
 
 #include "base/files/scoped_temp_dir.h"
@@ -33,21 +35,26 @@
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     system_service_ = std::make_unique<MockSystemService>(temp_dir_);
 
+    // Save the current producer socket name from env if currently set.
+    const auto* producer_sock_name_env = getenv(kProducerSockEnvName);
+    if (producer_sock_name_env) {
+      saved_producer_sock_env_ = producer_sock_name_env;
+    }
     // Override the default system producer socket.
-    saved_producer_sock_env_ = getenv(kProducerSockEnvName);
     ASSERT_EQ(0, setenv(kProducerSockEnvName,
                         system_service_->producer().c_str(), 1));
   }
 
   void TearDown() override {
-    system_service_ = nullptr;
     // Restore the value of Perfetto producer socket name env variable.
     if (saved_producer_sock_env_) {
-      ASSERT_EQ(0,
-                setenv(kProducerSockEnvName, saved_producer_sock_env_, true));
+      ASSERT_EQ(0, setenv(kProducerSockEnvName,
+                          saved_producer_sock_env_->c_str(), 1));
     } else {
       ASSERT_EQ(0, unsetenv(kProducerSockEnvName));
     }
+
+    system_service_ = nullptr;
     task_environment_.RunUntilIdle();
   }
 
@@ -57,7 +64,7 @@
       base::SingleThreadTaskRunner::GetCurrentDefault()};
   base::ScopedTempDir temp_dir_;
   std::unique_ptr<MockSystemService> system_service_;
-  const char* saved_producer_sock_env_ = nullptr;
+  std::optional<std::string> saved_producer_sock_env_;
 };
 
 // Test the OpenProducerSocket implementation. Expect a valid socket file
@@ -93,7 +100,6 @@
   bool callback_called = false;
 
   // Set the producer socket name to a nonexistent path.
-  saved_producer_sock_env_ = getenv(kProducerSockEnvName);
   ASSERT_EQ(0, setenv(kProducerSockEnvName, "nonexistent_socket", 1));
 
   base::RunLoop run_loop;
@@ -147,7 +153,6 @@
   bool callback_called = false;
 
   // Set the producer socket name to a nonexistent path.
-  saved_producer_sock_env_ = getenv(kProducerSockEnvName);
   ASSERT_EQ(0, setenv(kProducerSockEnvName, "nonexistent_socket", 1));
 
   // Bind the pending remote on the current thread.
@@ -167,5 +172,34 @@
   ASSERT_TRUE(callback_called);
 }
 
+TEST_F(SystemTracingServiceTest, BindAndPassPendingRemote_NonexistentLong) {
+  auto sts = std::make_unique<SystemTracingService>();
+  bool callback_called = false;
+
+  // Set the producer socket name to a long nonexistent path.
+  std::string sock_name;
+  while (sock_name.size() < 1024) {
+    sock_name.append("nonexistent/");
+  }
+  ASSERT_GT(sock_name.size(), sizeof(sockaddr_un::sun_path));
+  ASSERT_EQ(0, setenv(kProducerSockEnvName, sock_name.c_str(), 1));
+
+  // Bind the pending remote on the current thread.
+  mojo::Remote<mojom::SystemTracingService> remote;
Loading diff…

Regression Test / PoC

shipped with the fix
diff --git a/services/tracing/public/cpp/system_tracing_service_unittest.cc b/services/tracing/public/cpp/system_tracing_service_unittest.cc
index bf6358c7..b98b9ab 100644
--- a/services/tracing/public/cpp/system_tracing_service_unittest.cc
+++ b/services/tracing/public/cpp/system_tracing_service_unittest.cc
@@ -4,8 +4,10 @@
 
 #include "services/tracing/public/cpp/system_tracing_service.h"
 
+#include <sys/un.h>
 #include <unistd.h>
 
+#include <cstdint>
 #include <optional>
 
 #include "base/files/scoped_temp_dir.h"
@@ -33,21 +35,26 @@
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     system_service_ = std::make_unique<MockSystemService>(temp_dir_);
 
+    // Save the current producer socket name from env if currently set.
+    const auto* producer_sock_name_env = getenv(kProducerSockEnvName);
+    if (producer_sock_name_env) {
+      saved_producer_sock_env_ = producer_sock_name_env;
+    }
     // Override the default system producer socket.
-    saved_producer_sock_env_ = getenv(kProducerSockEnvName);
     ASSERT_EQ(0, setenv(kProducerSockEnvName,
                         system_service_->producer().c_str(), 1));
   }
 
   void TearDown() override {
-    system_service_ = nullptr;
     // Restore the value of Perfetto producer socket name env variable.
     if (saved_producer_sock_env_) {
-      ASSERT_EQ(0,
-                setenv(kProducerSockEnvName, saved_producer_sock_env_, true));
+      ASSERT_EQ(0, setenv(kProducerSockEnvName,
+                          saved_producer_sock_env_->c_str(), 1));
     } else {
       ASSERT_EQ(0, unsetenv(kProducerSockEnvName));
     }
+
+    system_service_ = nullptr;
     task_environment_.RunUntilIdle();
   }
 
@@ -57,7 +64,7 @@
       base::SingleThreadTaskRunner::GetCurrentDefault()};
   base::ScopedTempDir temp_dir_;
   std::unique_ptr<MockSystemService> system_service_;
-  const char* saved_producer_sock_env_ = nullptr;
+  std::optional<std::string> saved_producer_sock_env_;
 };
 
 // Test the OpenProducerSocket implementation. Expect a valid socket file
@@ -93,7 +100,6 @@
   bool callback_called = false;
 
   // Set the producer socket name to a nonexistent path.
-  saved_producer_sock_env_ = getenv(kProducerSockEnvName);
   ASSERT_EQ(0, setenv(kProducerSockEnvName, "nonexistent_socket", 1));
 
   base::RunLoop run_loop;
@@ -147,7 +153,6 @@
   bool callback_called = false;
 
   // Set the producer socket name to a nonexistent path.
-  saved_producer_sock_env_ = getenv(kProducerSockEnvName);
   ASSERT_EQ(0, setenv(kProducerSockEnvName, "nonexistent_socket", 1));
 
   // Bind the pending remote on the current thread.
@@ -167,5 +172,34 @@
   ASSERT_TRUE(callback_called);
 }
 
+TEST_F(SystemTracingServiceTest, BindAndPassPendingRemote_NonexistentLong) {
+  auto sts = std::make_unique<SystemTracingService>();
+  bool callback_called = false;
+
+  // Set the producer socket name to a long nonexistent path.
+  std::string sock_name;
+  while (sock_name.size() < 1024) {
+    sock_name.append("nonexistent/");
+  }
+  ASSERT_GT(sock_name.size(), sizeof(sockaddr_un::sun_path));
+  ASSERT_EQ(0, setenv(kProducerSockEnvName, sock_name.c_str(), 1));
+
+  // Bind the pending remote on the current thread.
+  mojo::Remote<mojom::SystemTracingService> remote;
+  remote.Bind(sts->BindAndPassPendingRemote(), nullptr);
+
+  base::RunLoop run_loop;
+  auto callback = base::BindLambdaForTesting([&](base::File file) {
+    callback_called = true;
+    ASSERT_FALSE(file.IsValid());
+    run_loop.Quit();
+  });
+
+  remote->OpenProducerSocket(std::move(callback));
+  ASSERT_FALSE(callback_called);
+  run_loop.Run();
+  ASSERT_TRUE(callback_called);
+}
+
 }  // namespace
 }  // namespace tracing
Loading diff…

Original Bug Report

reported by [email protected]

Security: Potential Stack-Buffer-Overflow in ProducerSocketConnector::ConnectSocket

Steps to reproduce the problem

  1. apply the patch (equal to setting the environment variable)
  2. run ./services_unittests –gtest_filter=SystemTracingServiceTest.OpenProducerSocket_Nonexistent

Problem Description

I’m not sure if it’s a valid assumption (attacker controls the environment variable), but to be safe I reported it.

RCA

In ProducerSocketConnector::ConnectSocket, the producer_sock_name [1] can be read from a environment variable PERFETTO_PRODUCER_SOCK_NAME[3], so if adversary can control the environment variable, the size of the name can be arbitary long.

Then at [2], the producer_sock_name is copied to the struct sockaddr_un’s sun_path, which is fixed for 108 bytes. Therefore, the attacker can craft the PERFETTO_PRODUCER_SOCK_NAME to overwrite the stack, as much as they want.

// services/tracing/public/cpp/system_tracing_service.cc
  base::expected<bool, int> ConnectSocket() {
    std::string producer_sock_name = perfetto::GetProducerSocket(); // [1]
    socket_fd_.reset(socket(AF_UNIX, SOCK_STREAM, 0));
    if (socket_fd_.get() == -1) {
      return base::unexpected(errno);
    }

    struct sockaddr_un saddr;
    memset(&saddr, 0, sizeof(saddr));
    memcpy(saddr.sun_path, producer_sock_name.data(), // [2]
           producer_sock_name.size());

  }

// third_party/perfetto/src/tracing/ipc/default_socket.cc
const char* GetProducerSocket() {
  const char* name = getenv("PERFETTO_PRODUCER_SOCK_NAME"); // [3]
  if (name == nullptr) {
    // ...
  }
  base::ignore_result(UseRunPerfettoBaseDir);  // Silence unused func warnings.
  return name;
}

struct sockaddr_un {
  sa_family_t sun_family; /* AF_UNIX */
  char sun_path[108];     /* Pathname */ //[4]
};

Summary

Security: Potential Stack-Buffer-Overflow in ProducerSocketConnector::ConnectSocket

Custom Questions

Type of crash:

browser

Reporter credit:

Han Zheng (HexHive)

Additional Data

Category: Security
Chrome Channel: Not sure
Regression: N/A

View on issue tracker
On This Page