From 78e86d3a69e3242605f4cadd258317e834e56138 Mon Sep 17 00:00:00 2001
From: Sangbaek Park <[email protected]>
Date: Wed, 13 May 2026 13:07:01 -0700
Subject: [PATCH] media: Fix site isolation bypass in CDM storage

A compromised MediaFoundation CDM utility process could potentially
bypass site isolation by enumerating the shared CDM storage root
directory. This directory stores the persistent DRM state (such as
PlayReady/Widevine licenses) of visited sites, keyed by unguessable
origin IDs.

Previously, the lpacMediaFoundationCdmData capability SID was granted
FILE_GENERIC_READ on the root directory. Because FILE_GENERIC_READ
includes the FILE_LIST_DIRECTORY access right, a compromised CDM could
list the root directory, discover the origin IDs of other sites the
user had visited, and use the inherited permissions to read or modify
their DRM state.

This CL fixes the vulnerability by modifying the applied ACLs:
1. The root directory is now strictly granted FILE_TRAVERSE with
   NO_INHERITANCE. This prevents directory enumeration while still
   allowing processes to traverse into known subdirectories.
2. The broad FILE_GENERIC_READ/WRITE permissions are now applied with
   an INHERIT_ONLY_ACE flag, ensuring they only apply to the scoped
   subdirectories and files created within the root.
3. Pre-creating the origin-specific CDM store subdirectory within the
   fully privileged browser process, which allows the sandboxed utility
   process to automatically inherit the necessary read/write access
   without needing to weaken the sandbox's security policies.

Tests added: { CdmDocumentServiceImplTest.VerifyCdmStorePathRootAcl }

Bug: 495848160
Change-Id: I03f96c3375818f3a8fa5ec64fc0f4850fdfd9ca8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7814332
Reviewed-by: Vikram Pasupathy <[email protected]>
Commit-Queue: Sangbaek Park <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1630151}
---

diff --git a/chrome/browser/media/cdm_document_service_impl.cc b/chrome/browser/media/cdm_document_service_impl.cc
index f455fa4..c8059b8 100644
--- a/chrome/browser/media/cdm_document_service_impl.cc
+++ b/chrome/browser/media/cdm_document_service_impl.cc
@@ -35,6 +35,8 @@
 #if BUILDFLAG(IS_WIN)
 #include <windows.h>
 
+#include <aclapi.h>
+
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/metrics/histogram_functions.h"
@@ -102,10 +104,23 @@
 
   auto sids = base::win::Sid::FromNamedCapabilityVector(
       {sandbox::policy::kMediaFoundationCdmData});
+
+  // Grant traverse access to the root directory itself to allow processes to
+  // access known subdirectories but prevent directory listing.
+  if (!base::win::GrantAccessToPath(cdm_store_path_root, sids, FILE_TRAVERSE,
+                                    NO_INHERITANCE,
+                                    /*recursive=*/false)) {
+    DLOG(ERROR) << "Failed to grant traverse access to the root directory.";
+    return false;
+  }
+
+  // Grant full access to children via inheritance, so that subdirectories
+  // corresponding to specific origin IDs are accessible.
   return base::win::GrantAccessToPath(
       cdm_store_path_root, sids,
-      FILE_GENERIC_READ | FILE_GENERIC_WRITE | GENERIC_EXECUTE | DELETE,
-      CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE);
+      FILE_GENERIC_READ | FILE_GENERIC_WRITE | DELETE,
+      CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE | INHERIT_ONLY_ACE,
+      /*recursive=*/false);
 }
 
 std::unique_ptr<media::MediaFoundationCdmData>
@@ -118,7 +133,20 @@
     return nullptr;
   }
 
-  std::unique_ptr<media::MediaFoundationCdmData> cdm_data;
+  // The root directory is only granted FILE_TRAVERSE access, which doesn't
+  // allow the utility process to create new subdirectories. So we should have
+  // the browser process pre-create the origin-specific subdirectory before
+  // passing the root path to the CDM. This way, the utility process never needs
+  // directory creation permissions at the root; it only accesses the explicit
+  // subdirectory made for it.
+  base::FilePath cdm_store_path =
+      cdm_store_path_root.AppendASCII(pref_data->origin_id().ToString());
+  base::File::Error file_error;
+  if (!base::CreateDirectoryAndGetError(cdm_store_path, &file_error)) {
+    DLOG(ERROR) << "Create CDM store path failed with " << file_error;
+    return nullptr;
+  }
+
   return std::make_unique<media::MediaFoundationCdmData>(
       pref_data->origin_id(), pref_data->client_token(), cdm_store_path_root);
 }
diff --git a/chrome/browser/media/cdm_document_service_impl_test.cc b/chrome/browser/media/cdm_document_service_impl_test.cc
index 68bb7cf..1bfa9f9 100644
--- a/chrome/browser/media/cdm_document_service_impl_test.cc
+++ b/chrome/browser/media/cdm_document_service_impl_test.cc
@@ -34,6 +34,16 @@
 #include "url/gurl.h"
 #include "url/origin.h"
 
+#if BUILDFLAG(IS_WIN)
+#include <windows.h>
+
+#include <aclapi.h>
+
+#include "base/win/security_util.h"
+#include "base/win/sid.h"
+#include "sandbox/policy/win/lpac_capability.h"
+#endif  // BUILDFLAG(IS_WIN)
+
 using testing::_;
 using testing::DoAll;
 using testing::SaveArg;
@@ -366,4 +376,75 @@
   ASSERT_NE(origin_id_1, new_origin_id);
 }
 
+#if BUILDFLAG(IS_WIN)
+TEST_F(CdmDocumentServiceImplTest, VerifyCdmStorePathRootAcl) {
+  NavigateToUrlAndCreateCdmDocumentService(GURL(kTestOrigin));
+  auto data = GetMediaFoundationCdmData();
+
+  auto sids = base::win::Sid::FromNamedCapabilityVector(
+      {sandbox::policy::kMediaFoundationCdmData});
+  ASSERT_FALSE(sids.empty());
+
+  // The root path should have traverse permissions.
+  EXPECT_TRUE(base::win::HasAccessToPath(data->cdm_store_path_root, sids,
+                                         FILE_TRAVERSE, NO_INHERITANCE));
+
+  // The root path should NOT have list directory permissions to prevent
+  // cross-origin enumeration. base::win::HasAccessToPath cannot be used here
+  // because it matches inherit-only ACEs when querying with NO_INHERITANCE.
+  PACL dacl = nullptr;
+  PSECURITY_DESCRIPTOR sd = nullptr;
+  // Manually retrieve the Discretionary Access Control List (DACL) to inspect
+  // its entries directly.
+  ASSERT_EQ(ERROR_SUCCESS,
+            ::GetNamedSecurityInfo(data->cdm_store_path_root.value().c_str(),
+                                   SE_FILE_OBJECT, DACL_SECURITY_INFORMATION,
+                                   nullptr, nullptr, &dacl, nullptr, &sd));
+  bool has_list_directory = false;
+  if (dacl) {
+    // Iterate over each Access Control Entry (ACE) in the DACL.
+    for (DWORD i = 0; i < dacl->AceCount; ++i) {
+      PVOID ace_ptr = nullptr;
+      if (::GetAce(dacl, i, &ace_ptr)) {
+        PACE_HEADER ace_header = static_cast<PACE_HEADER>(ace_ptr);
+        // We only care about ACEs that grant access and are not marked as
+        // "inherit only". INHERIT_ONLY_ACEs apply to child objects, not the
+        // directory itself.
+        if (ace_header->AceType == ACCESS_ALLOWED_ACE_TYPE &&
+            !(ace_header->AceFlags & INHERIT_ONLY_ACE)) {
+          PACCESS_ALLOWED_ACE allowed_ace =
+              static_cast<PACCESS_ALLOWED_ACE>(ace_ptr);
+          // Check if this ACE grants the FILE_LIST_DIRECTORY permission.
+          if (allowed_ace->Mask & FILE_LIST_DIRECTORY) {
+            PSID ace_sid = reinterpret_cast<PSID>(&allowed_ace->SidStart);
+            // Check if the SID in the ACE matches our LPAC SID.
+            for (const auto& sid : sids) {
+              if (::EqualSid(ace_sid, sid.GetPSID())) {
+                has_list_directory = true;
+                break;
+              }
+            }
+          }
+        }
+      }
+      if (has_list_directory) {
+        break;
+      }
+    }
+  }
+  // A compromised MF utility process can't enumerate other origin-specific
+  // subdirectories anymore, which is the main goal of this test.
+  EXPECT_FALSE(has_list_directory);
+  ::LocalFree(sd);
+
+  // And the inherited permissions should grant full access to subdirectories.
+  // (Note: HasAccessToPath requires the exact inheritance flags to match the
+  // ACE).
+  EXPECT_TRUE(base::win::HasAccessToPath(
+      data->cdm_store_path_root, sids,
+      FILE_GENERIC_READ | FILE_GENERIC_WRITE | DELETE,
+      CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE | INHERIT_ONLY_ACE));
+}
+#endif  // BUILDFLAG(IS_WIN)
+
 }  // namespace content

diff --git a/chrome/browser/media/cdm_document_service_impl_test.cc b/chrome/browser/media/cdm_document_service_impl_test.cc
index 68bb7cf..1bfa9f9 100644
--- a/chrome/browser/media/cdm_document_service_impl_test.cc
+++ b/chrome/browser/media/cdm_document_service_impl_test.cc
@@ -34,6 +34,16 @@
 #include "url/gurl.h"
 #include "url/origin.h"
 
+#if BUILDFLAG(IS_WIN)
+#include <windows.h>
+
+#include <aclapi.h>
+
+#include "base/win/security_util.h"
+#include "base/win/sid.h"
+#include "sandbox/policy/win/lpac_capability.h"
+#endif  // BUILDFLAG(IS_WIN)
+
 using testing::_;
 using testing::DoAll;
 using testing::SaveArg;
@@ -366,4 +376,75 @@
   ASSERT_NE(origin_id_1, new_origin_id);
 }
 
+#if BUILDFLAG(IS_WIN)
+TEST_F(CdmDocumentServiceImplTest, VerifyCdmStorePathRootAcl) {
+  NavigateToUrlAndCreateCdmDocumentService(GURL(kTestOrigin));
+  auto data = GetMediaFoundationCdmData();
+
+  auto sids = base::win::Sid::FromNamedCapabilityVector(
+      {sandbox::policy::kMediaFoundationCdmData});
+  ASSERT_FALSE(sids.empty());
+
+  // The root path should have traverse permissions.
+  EXPECT_TRUE(base::win::HasAccessToPath(data->cdm_store_path_root, sids,
+                                         FILE_TRAVERSE, NO_INHERITANCE));
+
+  // The root path should NOT have list directory permissions to prevent
+  // cross-origin enumeration. base::win::HasAccessToPath cannot be used here
+  // because it matches inherit-only ACEs when querying with NO_INHERITANCE.
+  PACL dacl = nullptr;
+  PSECURITY_DESCRIPTOR sd = nullptr;
+  // Manually retrieve the Discretionary Access Control List (DACL) to inspect
+  // its entries directly.
+  ASSERT_EQ(ERROR_SUCCESS,
+            ::GetNamedSecurityInfo(data->cdm_store_path_root.value().c_str(),
+                                   SE_FILE_OBJECT, DACL_SECURITY_INFORMATION,
+                                   nullptr, nullptr, &dacl, nullptr, &sd));
+  bool has_list_directory = false;
+  if (dacl) {
+    // Iterate over each Access Control Entry (ACE) in the DACL.
+    for (DWORD i = 0; i < dacl->AceCount; ++i) {
+      PVOID ace_ptr = nullptr;
+      if (::GetAce(dacl, i, &ace_ptr)) {
+        PACE_HEADER ace_header = static_cast<PACE_HEADER>(ace_ptr);
+        // We only care about ACEs that grant access and are not marked as
+        // "inherit only". INHERIT_ONLY_ACEs apply to child objects, not the
+        // directory itself.
+        if (ace_header->AceType == ACCESS_ALLOWED_ACE_TYPE &&
+            !(ace_header->AceFlags & INHERIT_ONLY_ACE)) {
+          PACCESS_ALLOWED_ACE allowed_ace =
+              static_cast<PACCESS_ALLOWED_ACE>(ace_ptr);
+          // Check if this ACE grants the FILE_LIST_DIRECTORY permission.
+          if (allowed_ace->Mask & FILE_LIST_DIRECTORY) {
+            PSID ace_sid = reinterpret_cast<PSID>(&allowed_ace->SidStart);
+            // Check if the SID in the ACE matches our LPAC SID.
+            for (const auto& sid : sids) {
+              if (::EqualSid(ace_sid, sid.GetPSID())) {
+                has_list_directory = true;
+                break;
+              }
+            }
+          }
+        }
+      }
+      if (has_list_directory) {
+        break;
+      }
+    }
+  }
+  // A compromised MF utility process can't enumerate other origin-specific
+  // subdirectories anymore, which is the main goal of this test.
+  EXPECT_FALSE(has_list_directory);
+  ::LocalFree(sd);
+
+  // And the inherited permissions should grant full access to subdirectories.
+  // (Note: HasAccessToPath requires the exact inheritance flags to match the
+  // ACE).
+  EXPECT_TRUE(base::win::HasAccessToPath(
+      data->cdm_store_path_root, sids,
+      FILE_GENERIC_READ | FILE_GENERIC_WRITE | DELETE,
+      CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE | INHERIT_ONLY_ACE));
+}
+#endif  // BUILDFLAG(IS_WIN)
+
 }  // namespace content