From df8a9e84c24be5d630c5ff1a1b6309110e3e2d71 Mon Sep 17 00:00:00 2001 From: boocmp Date: Wed, 20 Nov 2024 22:57:53 +0700 Subject: [PATCH 01/14] Added ComponentContentsVerifier that checks verified_contents.json shipped with a component. --- browser/brave_browser_main_parts.cc | 2 + browser/component_updater/BUILD.gn | 9 + .../brave_component_contents_verifier.cc | 196 ++++++++++++++++++ .../brave_component_contents_verifier.h | 15 ++ .../brave_component_updater/browser/BUILD.gn | 3 + .../brave_component_updater/browser/DEPS | 2 +- .../browser/component_contents_verifier.cc | 86 ++++++++ .../browser/component_contents_verifier.h | 79 +++++++ .../browser/ad_block_component_installer.cc | 18 +- .../browser/ad_block_component_installer.h | 9 +- .../ad_block_component_service_manager.cc | 13 -- .../ad_block_component_service_manager.h | 1 - .../ad_block_default_resource_provider.cc | 33 ++- .../ad_block_default_resource_provider.h | 11 +- 14 files changed, 436 insertions(+), 41 deletions(-) create mode 100644 browser/component_updater/brave_component_contents_verifier.cc create mode 100644 browser/component_updater/brave_component_contents_verifier.h create mode 100644 components/brave_component_updater/browser/component_contents_verifier.cc create mode 100644 components/brave_component_updater/browser/component_contents_verifier.h diff --git a/browser/brave_browser_main_parts.cc b/browser/brave_browser_main_parts.cc index 069500aac7d2..5fce234e6111 100644 --- a/browser/brave_browser_main_parts.cc +++ b/browser/brave_browser_main_parts.cc @@ -11,6 +11,7 @@ #include "base/command_line.h" #include "base/path_service.h" #include "brave/browser/browsing_data/brave_clear_browsing_data.h" +#include "brave/browser/component_updater/brave_component_contents_verifier.h" #include "brave/browser/ethereum_remote_client/buildflags/buildflags.h" #include "brave/components/brave_component_updater/browser/brave_on_demand_updater.h" #include "brave/components/brave_rewards/common/rewards_flags.h" @@ -81,6 +82,7 @@ ChromeBrowserMainParts::ChromeBrowserMainParts(bool is_integration_test, ChromeBrowserMainParts::~ChromeBrowserMainParts() = default; int ChromeBrowserMainParts::PreMainMessageLoopRun() { + component_updater::SetupComponentContentsVerifier(); brave_component_updater::BraveOnDemandUpdater::GetInstance() ->RegisterOnDemandUpdater( &g_browser_process->component_updater()->GetOnDemandUpdater()); diff --git a/browser/component_updater/BUILD.gn b/browser/component_updater/BUILD.gn index 21c8458b48eb..e4d2a240699e 100644 --- a/browser/component_updater/BUILD.gn +++ b/browser/component_updater/BUILD.gn @@ -3,14 +3,19 @@ # License, v. 2.0. If a copy of the MPL was not distributed with this file, # You can obtain one at http://mozilla.org/MPL/2.0/. */ +import("//extensions/buildflags/buildflags.gni") + source_set("component_updater") { sources = [ + "brave_component_contents_verifier.cc", + "brave_component_contents_verifier.h", "brave_component_updater_configurator.cc", "brave_component_updater_configurator.h", ] deps = [ "//base", + "//brave/components/brave_component_updater/browser", "//brave/components/constants", "//chrome/common", "//components/component_updater", @@ -25,6 +30,10 @@ source_set("component_updater") { "//services/network/public/cpp", ] + if (enable_extensions) { + deps += [ "//extensions/browser" ] + } + if (is_win) { deps += [ "//chrome/installer/util:with_no_strings" ] } diff --git a/browser/component_updater/brave_component_contents_verifier.cc b/browser/component_updater/brave_component_contents_verifier.cc new file mode 100644 index 000000000000..88d1e9614a97 --- /dev/null +++ b/browser/component_updater/brave_component_contents_verifier.cc @@ -0,0 +1,196 @@ +// Copyright (c) 2024 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +#include "brave/browser/component_updater/brave_component_contents_verifier.h" + +#include +#include +#include +#include +#include + +#include "base/files/file_path.h" +#include "base/functional/bind.h" +#include "brave/components/brave_component_updater/browser/component_contents_verifier.h" +#include "crypto/secure_hash.h" +#include "crypto/sha2.h" +#include "extensions/buildflags/buildflags.h" + +#if BUILDFLAG(ENABLE_EXTENSIONS) + +#include "extensions/browser/content_hash_tree.h" +#include "extensions/browser/verified_contents.h" + +namespace { + +constexpr const uint8_t kComponentContentsVerifierPublicKey[] = { + 0x30, 0x82, 0x01, 0x22, 0x30, 0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, + 0xf7, 0x0d, 0x01, 0x01, 0x01, 0x05, 0x00, 0x03, 0x82, 0x01, 0x0f, 0x00, + 0x30, 0x82, 0x01, 0x0a, 0x02, 0x82, 0x01, 0x01, 0x00, 0xc3, 0x8a, 0x2d, + 0x68, 0x10, 0xb3, 0x52, 0xf1, 0xcc, 0xae, 0xd7, 0x70, 0xb7, 0xc9, 0xc7, + 0xe3, 0x1c, 0x91, 0x4c, 0x1a, 0x73, 0xf4, 0xbd, 0xb9, 0x54, 0x01, 0xb2, + 0xd0, 0x03, 0xfa, 0x23, 0x43, 0xb9, 0x52, 0xf4, 0xc8, 0x27, 0xbf, 0xed, + 0x6c, 0x9b, 0xba, 0x26, 0xa6, 0xe9, 0x43, 0x0f, 0x71, 0xf6, 0xa2, 0x2c, + 0x1e, 0xf9, 0xb3, 0xfc, 0xec, 0xfb, 0x12, 0xe1, 0xea, 0xbb, 0x71, 0x6d, + 0x80, 0x2b, 0x26, 0x5f, 0xfb, 0x20, 0x91, 0x26, 0x5a, 0x6d, 0xdd, 0x0e, + 0xd8, 0x88, 0xc2, 0x11, 0x7e, 0xf7, 0x5d, 0xfc, 0xd6, 0x99, 0x50, 0x9b, + 0x70, 0xe9, 0xa7, 0xcf, 0x7f, 0x4c, 0xf0, 0x80, 0x2d, 0x8d, 0x97, 0x9f, + 0xfc, 0x5c, 0x56, 0xc3, 0xff, 0x37, 0xf8, 0x3a, 0x44, 0x3a, 0x44, 0x3c, + 0xbe, 0x96, 0xf9, 0x14, 0x44, 0xb1, 0x8e, 0x08, 0xc2, 0x55, 0x49, 0xf1, + 0xe4, 0x3a, 0xa6, 0x63, 0x23, 0x26, 0x00, 0xa4, 0x8b, 0xdd, 0xaf, 0x45, + 0xb0, 0xf8, 0xb8, 0x24, 0x80, 0x85, 0x85, 0x95, 0x2e, 0xc5, 0xd4, 0xf1, + 0xdc, 0xfb, 0xeb, 0x95, 0xb6, 0x13, 0x3a, 0x46, 0x9d, 0x49, 0xb1, 0x15, + 0x72, 0xfd, 0x85, 0xc6, 0x0b, 0x7e, 0xe2, 0x97, 0xf1, 0x66, 0xa6, 0x0b, + 0x67, 0xe8, 0x72, 0xb9, 0xf1, 0x3b, 0x30, 0x1a, 0x77, 0x6d, 0xf3, 0xc4, + 0x99, 0xc0, 0x86, 0xb2, 0x4e, 0x1d, 0xde, 0x34, 0x86, 0x15, 0x41, 0x68, + 0x1e, 0x94, 0x48, 0x0a, 0x54, 0x05, 0x52, 0x01, 0x04, 0xd2, 0xef, 0x57, + 0x89, 0x47, 0xd3, 0xae, 0xd9, 0xc1, 0xf5, 0xa9, 0xbf, 0x60, 0x96, 0x4b, + 0xa0, 0x12, 0x9c, 0xf3, 0x00, 0xe6, 0x32, 0x40, 0xc7, 0x6e, 0x29, 0xa8, + 0x81, 0xfd, 0x2f, 0x1e, 0x92, 0x4a, 0x5e, 0xed, 0xef, 0x13, 0x9f, 0xed, + 0x88, 0x77, 0x2c, 0x84, 0xdd, 0x00, 0x87, 0x03, 0x49, 0x09, 0xb7, 0x4b, + 0xc7, 0x02, 0x03, 0x01, 0x00, 0x01}; + +constexpr const char kVerifiedContentsPath[] = + "_metadata/verified_contents.json"; + +std::string GetRootHasheForContent(base::span contents, + size_t block_size) { + size_t offset = 0; + std::vector hashes; + // Even when the contents is empty, we want to output at least one hash + // block (the hash of the empty string). + do { + const size_t bytes_to_read = std::min(contents.size() - offset, block_size); + std::unique_ptr hash( + crypto::SecureHash::Create(crypto::SecureHash::SHA256)); + hash->Update(contents.subspan(offset, bytes_to_read)); + + std::string buffer; + buffer.resize(crypto::kSHA256Length); + hash->Finish(base::as_writable_byte_span(buffer)); + hashes.push_back(std::move(buffer)); + + // If |contents| is empty, then we want to just exit here. + if (bytes_to_read == 0) { + break; + } + + offset += bytes_to_read; + } while (offset < contents.size()); + + CHECK(block_size % crypto::kSHA256Length == 0); + return extensions::ComputeTreeHashRoot(hashes, + block_size / crypto::kSHA256Length); +} + +// Only proxies the file access. +class ComponentNoChecksContentsAccessorImpl + : public brave_component_updater::ComponentContentsAccessor { + public: + explicit ComponentNoChecksContentsAccessorImpl( + const base::FilePath& component_root) + : component_root_(component_root) {} + + const base::FilePath& GetComponentRoot() const override { + return component_root_; + } + + bool IsComponentSignatureValid() const override { return true; } + + void IgnoreInvalidSignature(bool) override {} + + bool VerifyContents(const base::FilePath& relative_path, + base::span contents) override { + return true; + } + + protected: + ~ComponentNoChecksContentsAccessorImpl() override = default; + + const base::FilePath component_root_; +}; + +// Proxies the file access and checks the verified_contents.json. +class ComponentContentsAccessorImpl + : public ComponentNoChecksContentsAccessorImpl { + public: + explicit ComponentContentsAccessorImpl(const base::FilePath& component_root) + : ComponentNoChecksContentsAccessorImpl(component_root) { + verified_contents_ = extensions::VerifiedContents::CreateFromFile( + base::make_span(kComponentContentsVerifierPublicKey), + component_root.AppendASCII(kVerifiedContentsPath)); + if (verified_contents_->block_size() % crypto::kSHA256Length != 0) { + // Unsupported block size. + verified_contents_.reset(); + } + } + + bool IsComponentSignatureValid() const override { + return !!verified_contents_.get(); + } + + void IgnoreInvalidSignature(bool ignore) override { + ignore_invalid_signature_ = ignore; + } + + bool VerifyContents(const base::FilePath& relative_path, + base::span contents) override { + if (!IsComponentSignatureValid()) { + return ignore_invalid_signature_; + } + + if (!verified_contents_->HasTreeHashRoot(relative_path)) { + return true; + } + + const auto root_hash = + GetRootHasheForContent(contents, verified_contents_->block_size()); + if (!verified_contents_->TreeHashRootEquals(relative_path, root_hash)) { + return false; + } + return true; + } + + private: + ~ComponentContentsAccessorImpl() override = default; + + std::unique_ptr verified_contents_; + bool ignore_invalid_signature_ = false; +}; + +} // namespace + +namespace component_updater { +void SetupComponentContentsVerifier() { + auto factory = base::BindRepeating( + [](const base::FilePath& component_root) + -> scoped_refptr { + return base::MakeRefCounted( + component_root); + }); + brave_component_updater::ComponentContentsVerifier::Setup(std::move(factory)); +} +} // namespace component_updater + +#else // BUILDFLAG(ENABLE_EXTENSIONS) + +namespace component_updater { + +// We expect that on these platforms the component files are +// protected by the OS. + +void SetupComponentContentsVerifier() { + auto factory = base::BindRepeating( + [](const base::FilePath& component_root) + -> scoped_refptr { + return base::MakeRefCounted( + component_root); + }); + brave_component_updater::ComponentContentsVerifier::Setup(std::move(factory)); +} + +} // namespace component_updater + +#endif // BUILDFLAG(ENABLE_EXTENSIONS) diff --git a/browser/component_updater/brave_component_contents_verifier.h b/browser/component_updater/brave_component_contents_verifier.h new file mode 100644 index 000000000000..cc2177cce036 --- /dev/null +++ b/browser/component_updater/brave_component_contents_verifier.h @@ -0,0 +1,15 @@ +// Copyright (c) 2024 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +#ifndef BRAVE_BROWSER_COMPONENT_UPDATER_BRAVE_COMPONENT_CONTENTS_VERIFIER_H_ +#define BRAVE_BROWSER_COMPONENT_UPDATER_BRAVE_COMPONENT_CONTENTS_VERIFIER_H_ + +namespace component_updater { + +void SetupComponentContentsVerifier(); + +} + +#endif // BRAVE_BROWSER_COMPONENT_UPDATER_BRAVE_COMPONENT_CONTENTS_VERIFIER_H_ diff --git a/components/brave_component_updater/browser/BUILD.gn b/components/brave_component_updater/browser/BUILD.gn index 21d693049875..f2a8b907a40d 100644 --- a/components/brave_component_updater/browser/BUILD.gn +++ b/components/brave_component_updater/browser/BUILD.gn @@ -15,6 +15,8 @@ static_library("browser") { "brave_component_updater_delegate.h", "brave_on_demand_updater.cc", "brave_on_demand_updater.h", + "component_contents_verifier.cc", + "component_contents_verifier.h", "dat_file_util.cc", "dat_file_util.h", "features.cc", @@ -33,6 +35,7 @@ static_library("browser") { "//components/prefs", "//components/update_client", "//crypto", + "//extensions/buildflags", ] } diff --git a/components/brave_component_updater/browser/DEPS b/components/brave_component_updater/browser/DEPS index 08b52cb8cbf3..2919c1791d12 100644 --- a/components/brave_component_updater/browser/DEPS +++ b/components/brave_component_updater/browser/DEPS @@ -1,3 +1,3 @@ include_rules = [ - "+extensions/common", + "+extensions/buildflags", ] diff --git a/components/brave_component_updater/browser/component_contents_verifier.cc b/components/brave_component_updater/browser/component_contents_verifier.cc new file mode 100644 index 000000000000..e877fdc472ac --- /dev/null +++ b/components/brave_component_updater/browser/component_contents_verifier.cc @@ -0,0 +1,86 @@ +// Copyright (c) 2024 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +#include "brave/components/brave_component_updater/browser/component_contents_verifier.h" + +#include +#include + +#include "base/check_is_test.h" +#include "base/files/file_util.h" +#include "base/no_destructor.h" +#include "extensions/buildflags/buildflags.h" + +namespace brave_component_updater { + +base::expected +ComponentContentsAccessor::ReadFileToString( + const base::FilePath& relative_path) { + std::string contents; + if (!base::ReadFileToString(GetComponentRoot().Append(relative_path), + &contents)) { + return base::unexpected(Error::kFileReadFailed); + } + if (!VerifyContents(relative_path, base::as_byte_span(contents))) { + return base::unexpected(Error::kInvalidSignature); + } + + return base::ok(std::move(contents)); +} + +base::expected, ComponentContentsAccessor::Error> +ComponentContentsAccessor::ReadFileToBytes( + const base::FilePath& relative_path) { + auto contents = + base::ReadFileToBytes(GetComponentRoot().Append(relative_path)); + if (!contents) { + return base::unexpected(Error::kFileReadFailed); + } + if (!VerifyContents(relative_path, base::as_byte_span(*contents))) { + return base::unexpected(Error::kInvalidSignature); + } + + return base::ok(*std::move(contents)); +} + +std::string ComponentContentsAccessor::GetFileAsString( + const base::FilePath& relative_path, + const std::string& default_value) { + return ReadFileToString(relative_path).value_or(default_value); +} + +std::vector ComponentContentsAccessor::GetFileAsBytes( + const base::FilePath& relative_path, + const std::vector& default_value) { + return ReadFileToBytes(relative_path).value_or(default_value); +} + +ComponentContentsVerifier::ComponentContentsVerifier() = default; +ComponentContentsVerifier::~ComponentContentsVerifier() = default; + +// static +void ComponentContentsVerifier::Setup( + ComponentContentsVerifier::ComponentContentsAccessorFactory cca_factory) { + if (GetInstance()->cca_factory_) { + CHECK_IS_TEST(); + } + GetInstance()->cca_factory_ = std::move(cca_factory); +} + +// static +ComponentContentsVerifier* ComponentContentsVerifier::GetInstance() { + static base::NoDestructor instance; + return instance.get(); +} + +// Must be called on the MAY_BLOCK sequence. +scoped_refptr +ComponentContentsVerifier::GetContentsAccessor( + const base::FilePath& component_root) { + CHECK(cca_factory_); + return cca_factory_.Run(component_root); +} + +} // namespace brave_component_updater diff --git a/components/brave_component_updater/browser/component_contents_verifier.h b/components/brave_component_updater/browser/component_contents_verifier.h new file mode 100644 index 000000000000..cb80180b22ee --- /dev/null +++ b/components/brave_component_updater/browser/component_contents_verifier.h @@ -0,0 +1,79 @@ +// Copyright (c) 2024 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +#ifndef BRAVE_COMPONENTS_BRAVE_COMPONENT_UPDATER_BROWSER_COMPONENT_CONTENTS_VERIFIER_H_ +#define BRAVE_COMPONENTS_BRAVE_COMPONENT_UPDATER_BROWSER_COMPONENT_CONTENTS_VERIFIER_H_ + +#include +#include + +#include "base/files/file_path.h" +#include "base/functional/callback.h" +#include "base/memory/ref_counted.h" +#include "base/types/expected.h" + +namespace base { +template +class NoDestructor; +} // namespace base + +namespace brave_component_updater { + +// Use on MAY_BLOCK sequence. +class ComponentContentsAccessor + : public base::RefCountedThreadSafe { + public: + enum class Error { + kFileReadFailed, + kInvalidSignature, + }; + + virtual const base::FilePath& GetComponentRoot() const = 0; + virtual bool IsComponentSignatureValid() const = 0; + virtual void IgnoreInvalidSignature(bool ignore) = 0; + virtual bool VerifyContents(const base::FilePath& relative_path, + base::span contents) = 0; + + base::expected ReadFileToString( + const base::FilePath& relative_path); + base::expected, Error> ReadFileToBytes( + const base::FilePath& relative_path); + + std::string GetFileAsString(const base::FilePath& relative_path, + const std::string& default_value); + std::vector GetFileAsBytes( + const base::FilePath& relative_path, + const std::vector& default_value); + + protected: + friend class base::RefCountedThreadSafe; + virtual ~ComponentContentsAccessor() = default; +}; + +class ComponentContentsVerifier { + public: + using ComponentContentsAccessorFactory = + base::RepeatingCallback( + const base::FilePath& component_root)>; + + static void Setup(ComponentContentsAccessorFactory cca_factory); + static ComponentContentsVerifier* GetInstance(); + + // Must be called on the MAY_BLOCK sequence. + scoped_refptr GetContentsAccessor( + const base::FilePath& component_root); + + private: + friend base::NoDestructor; + + ComponentContentsAccessorFactory cca_factory_; + + ComponentContentsVerifier(); + ~ComponentContentsVerifier(); +}; + +} // namespace brave_component_updater + +#endif // BRAVE_COMPONENTS_BRAVE_COMPONENT_UPDATER_BROWSER_COMPONENT_CONTENTS_VERIFIER_H_ diff --git a/components/brave_shields/core/browser/ad_block_component_installer.cc b/components/brave_shields/core/browser/ad_block_component_installer.cc index 0a2cb2d40f51..fcfd5b1eb44a 100644 --- a/components/brave_shields/core/browser/ad_block_component_installer.cc +++ b/components/brave_shields/core/browser/ad_block_component_installer.cc @@ -7,12 +7,13 @@ #include #include +#include #include #include "base/base64.h" #include "base/functional/bind.h" -#include "base/functional/callback.h" #include "brave/components/brave_component_updater/browser/brave_on_demand_updater.h" +#include "brave/components/brave_component_updater/browser/component_contents_verifier.h" #include "brave/components/brave_shields/core/common/brave_shield_constants.h" #include "components/component_updater/component_installer.h" #include "components/component_updater/component_updater_service.h" @@ -104,7 +105,7 @@ void AdBlockComponentInstallerPolicy::ComponentReady( const base::Version& version, const base::FilePath& path, base::Value::Dict manifest) { - ready_callback_.Run(path); + ready_callback_.Run(std::move(path)); } bool AdBlockComponentInstallerPolicy::VerifyInstallation( @@ -147,16 +148,25 @@ void OnRegistered(const std::string& component_id) { void RegisterAdBlockDefaultResourceComponent( component_updater::ComponentUpdateService* cus, - OnComponentReadyCallback callback) { + OnSecureComponentReadyCallback callback) { // In test, |cus| could be nullptr. if (!cus) { return; } + auto on_ready = base::BindRepeating( + [](OnSecureComponentReadyCallback callback, const base::FilePath& path) { + auto accessor = + brave_component_updater::ComponentContentsVerifier::GetInstance() + ->GetContentsAccessor(path); + callback.Run(std::move(accessor)); + }, + std::move(callback)); + auto installer = base::MakeRefCounted( std::make_unique( kAdBlockResourceComponentBase64PublicKey, kAdBlockResourceComponentId, - kAdBlockResourceComponentName, callback)); + kAdBlockResourceComponentName, std::move(on_ready))); installer->Register( cus, base::BindOnce(&OnRegistered, kAdBlockResourceComponentId)); } diff --git a/components/brave_shields/core/browser/ad_block_component_installer.h b/components/brave_shields/core/browser/ad_block_component_installer.h index 2f73b36eca0f..afc0bf121580 100644 --- a/components/brave_shields/core/browser/ad_block_component_installer.h +++ b/components/brave_shields/core/browser/ad_block_component_installer.h @@ -6,10 +6,12 @@ #ifndef BRAVE_COMPONENTS_BRAVE_SHIELDS_CORE_BROWSER_AD_BLOCK_COMPONENT_INSTALLER_H_ #define BRAVE_COMPONENTS_BRAVE_SHIELDS_CORE_BROWSER_AD_BLOCK_COMPONENT_INSTALLER_H_ +#include #include #include "base/files/file_path.h" #include "base/functional/callback.h" +#include "brave/components/brave_component_updater/browser/component_contents_verifier.h" namespace component_updater { class ComponentUpdateService; @@ -18,11 +20,14 @@ class ComponentUpdateService; namespace brave_shields { using OnComponentReadyCallback = - base::RepeatingCallback; + base::RepeatingCallback; + +using OnSecureComponentReadyCallback = base::RepeatingCallback)>; void RegisterAdBlockDefaultResourceComponent( component_updater::ComponentUpdateService* cus, - OnComponentReadyCallback callback); + OnSecureComponentReadyCallback callback); void RegisterAdBlockFilterListCatalogComponent( component_updater::ComponentUpdateService* cus, diff --git a/components/brave_shields/core/browser/ad_block_component_service_manager.cc b/components/brave_shields/core/browser/ad_block_component_service_manager.cc index 334c76680af5..8669e924d7e3 100644 --- a/components/brave_shields/core/browser/ad_block_component_service_manager.cc +++ b/components/brave_shields/core/browser/ad_block_component_service_manager.cc @@ -189,19 +189,6 @@ bool AdBlockComponentServiceManager::IsFilterListAvailable( return catalog_entry != filter_list_catalog_.end(); } -base::FilePath AdBlockComponentServiceManager::GetFilterSetPath( - const std::string& uuid) { - DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); - DCHECK(!uuid.empty()); - auto it = component_filters_providers_.find(uuid); - - if (it == component_filters_providers_.end()) { - return base::FilePath(); - } - - return it->second->GetFilterSetPath(); -} - bool AdBlockComponentServiceManager::IsFilterListEnabled( const std::string& uuid) const { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); diff --git a/components/brave_shields/core/browser/ad_block_component_service_manager.h b/components/brave_shields/core/browser/ad_block_component_service_manager.h index 8d9696082f1c..c8ac89830b0c 100644 --- a/components/brave_shields/core/browser/ad_block_component_service_manager.h +++ b/components/brave_shields/core/browser/ad_block_component_service_manager.h @@ -55,7 +55,6 @@ class AdBlockComponentServiceManager // Get the filter set path for a given filter list. // If the filter list is not available, an empty path is returned. - base::FilePath GetFilterSetPath(const std::string& uuid); bool IsFilterListAvailable(const std::string& uuid) const; bool IsFilterListEnabled(const std::string& uuid) const; void EnableFilterList(const std::string& uuid, bool enabled); diff --git a/components/brave_shields/core/browser/ad_block_default_resource_provider.cc b/components/brave_shields/core/browser/ad_block_default_resource_provider.cc index e2e649c4418e..99bc80a58bee 100644 --- a/components/brave_shields/core/browser/ad_block_default_resource_provider.cc +++ b/components/brave_shields/core/browser/ad_block_default_resource_provider.cc @@ -10,7 +10,7 @@ #include "base/files/file_path.h" #include "base/task/thread_pool.h" -#include "brave/components/brave_component_updater/browser/dat_file_util.h" +#include "brave/components/brave_component_updater/browser/component_contents_verifier.h" #include "brave/components/brave_shields/core/browser/ad_block_component_installer.h" namespace { @@ -37,37 +37,32 @@ AdBlockDefaultResourceProvider::AdBlockDefaultResourceProvider( AdBlockDefaultResourceProvider::~AdBlockDefaultResourceProvider() = default; base::FilePath AdBlockDefaultResourceProvider::GetResourcesPath() { - if (component_path_.empty()) { - // Since we know it's empty return it as is. - return component_path_; + if (!component_accessor_) { + return base::FilePath(); } - return component_path_.AppendASCII(kAdBlockResourcesFilename); + return component_accessor_->GetComponentRoot().AppendASCII( + kAdBlockResourcesFilename); } void AdBlockDefaultResourceProvider::OnComponentReady( - const base::FilePath& path) { - component_path_ = path; - base::FilePath resources_path = GetResourcesPath(); - - if (resources_path.empty()) { + scoped_refptr + accessor) { + component_accessor_ = std::move(accessor); + if (!component_accessor_) { // This should not happen, but if it does, we should not proceed. return; } // Load the resources (as a string) - base::ThreadPool::PostTaskAndReplyWithResult( - FROM_HERE, {base::MayBlock()}, - base::BindOnce(&brave_component_updater::GetDATFileAsString, - resources_path), + LoadResources( base::BindOnce(&AdBlockDefaultResourceProvider::NotifyResourcesLoaded, weak_factory_.GetWeakPtr())); } void AdBlockDefaultResourceProvider::LoadResources( base::OnceCallback cb) { - base::FilePath resources_path = GetResourcesPath(); - if (resources_path.empty()) { + if (!component_accessor_) { // If the path is not ready yet, run the callback with empty resources to // avoid blocking filter data loads. std::move(cb).Run("[]"); @@ -76,8 +71,10 @@ void AdBlockDefaultResourceProvider::LoadResources( base::ThreadPool::PostTaskAndReplyWithResult( FROM_HERE, {base::MayBlock()}, - base::BindOnce(&brave_component_updater::GetDATFileAsString, - resources_path), + base::BindOnce( + &brave_component_updater::ComponentContentsAccessor::GetFileAsString, + base::RetainedRef(component_accessor_), + base::FilePath::FromASCII(kAdBlockResourcesFilename), "[]"), std::move(cb)); } diff --git a/components/brave_shields/core/browser/ad_block_default_resource_provider.h b/components/brave_shields/core/browser/ad_block_default_resource_provider.h index bbade2db1661..d82437109f50 100644 --- a/components/brave_shields/core/browser/ad_block_default_resource_provider.h +++ b/components/brave_shields/core/browser/ad_block_default_resource_provider.h @@ -15,6 +15,10 @@ namespace component_updater { class ComponentUpdateService; } // namespace component_updater +namespace brave_component_updater { +class ComponentContentsAccessor; +} + namespace base { class FilePath; } @@ -42,9 +46,12 @@ class AdBlockDefaultResourceProvider : public AdBlockResourceProvider { private: friend class ::AdBlockServiceTest; - void OnComponentReady(const base::FilePath&); + void OnComponentReady( + scoped_refptr + accessor); - base::FilePath component_path_; + scoped_refptr + component_accessor_; base::WeakPtrFactory weak_factory_{this}; }; From 1cd84a9e05394b7f2010eddcf9599cc3ec22eea8 Mon Sep 17 00:00:00 2001 From: boocmp Date: Fri, 22 Nov 2024 21:47:07 +0700 Subject: [PATCH 02/14] Tests compilation. --- .../ad_block_service_browsertest.cc | 6 +- .../brave_component_contents_verifier.cc | 101 +++++++++--------- .../brave_component_contents_verifier.h | 13 ++- .../ad_block_component_service_manager.cc | 13 +++ .../ad_block_component_service_manager.h | 1 + test/BUILD.gn | 1 + 6 files changed, 81 insertions(+), 54 deletions(-) diff --git a/browser/brave_shields/ad_block_service_browsertest.cc b/browser/brave_shields/ad_block_service_browsertest.cc index c34c88e79915..6dc2f1d30d9e 100644 --- a/browser/brave_shields/ad_block_service_browsertest.cc +++ b/browser/brave_shields/ad_block_service_browsertest.cc @@ -24,6 +24,7 @@ #include "base/threading/thread_restrictions.h" #include "brave/app/brave_command_ids.h" #include "brave/browser/brave_browser_process.h" +#include "brave/browser/component_updater/brave_component_contents_verifier.h" #include "brave/browser/net/brave_ad_block_tp_network_delegate_helper.h" #include "brave/components/brave_shields/content/browser/ad_block_custom_filters_provider.h" #include "brave/components/brave_shields/content/browser/ad_block_engine.h" @@ -251,7 +252,10 @@ void AdBlockServiceTest::UpdateAdBlockResources(const std::string& resources) { brave_shields::AdBlockService* service = g_brave_browser_process->ad_block_service(); - service->default_resource_provider()->OnComponentReady(component_path); + static_cast( + service->resource_provider()) + ->OnComponentReady(component_updater::CreateComponentContentsAccessor( + false, component_path)); } void AdBlockServiceTest::UpdateAdBlockInstanceWithRules( diff --git a/browser/component_updater/brave_component_contents_verifier.cc b/browser/component_updater/brave_component_contents_verifier.cc index 88d1e9614a97..ea85ea8d5682 100644 --- a/browser/component_updater/brave_component_contents_verifier.cc +++ b/browser/component_updater/brave_component_contents_verifier.cc @@ -13,15 +13,47 @@ #include "base/files/file_path.h" #include "base/functional/bind.h" -#include "brave/components/brave_component_updater/browser/component_contents_verifier.h" #include "crypto/secure_hash.h" #include "crypto/sha2.h" #include "extensions/buildflags/buildflags.h" #if BUILDFLAG(ENABLE_EXTENSIONS) - #include "extensions/browser/content_hash_tree.h" #include "extensions/browser/verified_contents.h" +#endif + +namespace { + +// Only proxies the file access. +class ComponentNoChecksContentsAccessorImpl + : public brave_component_updater::ComponentContentsAccessor { + public: + explicit ComponentNoChecksContentsAccessorImpl( + const base::FilePath& component_root) + : component_root_(component_root) {} + + const base::FilePath& GetComponentRoot() const override { + return component_root_; + } + + bool IsComponentSignatureValid() const override { return true; } + + void IgnoreInvalidSignature(bool) override {} + + bool VerifyContents(const base::FilePath& relative_path, + base::span contents) override { + return true; + } + + protected: + ~ComponentNoChecksContentsAccessorImpl() override = default; + + const base::FilePath component_root_; +}; + +} // namespace + +#if BUILDFLAG(ENABLE_EXTENSIONS) namespace { @@ -85,33 +117,6 @@ std::string GetRootHasheForContent(base::span contents, block_size / crypto::kSHA256Length); } -// Only proxies the file access. -class ComponentNoChecksContentsAccessorImpl - : public brave_component_updater::ComponentContentsAccessor { - public: - explicit ComponentNoChecksContentsAccessorImpl( - const base::FilePath& component_root) - : component_root_(component_root) {} - - const base::FilePath& GetComponentRoot() const override { - return component_root_; - } - - bool IsComponentSignatureValid() const override { return true; } - - void IgnoreInvalidSignature(bool) override {} - - bool VerifyContents(const base::FilePath& relative_path, - base::span contents) override { - return true; - } - - protected: - ~ComponentNoChecksContentsAccessorImpl() override = default; - - const base::FilePath component_root_; -}; - // Proxies the file access and checks the verified_contents.json. class ComponentContentsAccessorImpl : public ComponentNoChecksContentsAccessorImpl { @@ -162,35 +167,27 @@ class ComponentContentsAccessorImpl } // namespace -namespace component_updater { -void SetupComponentContentsVerifier() { - auto factory = base::BindRepeating( - [](const base::FilePath& component_root) - -> scoped_refptr { - return base::MakeRefCounted( - component_root); - }); - brave_component_updater::ComponentContentsVerifier::Setup(std::move(factory)); -} -} // namespace component_updater - -#else // BUILDFLAG(ENABLE_EXTENSIONS) +#endif // BUILDFLAG(ENABLE_EXTENSIONS) namespace component_updater { -// We expect that on these platforms the component files are -// protected by the OS. +scoped_refptr +CreateComponentContentsAccessor(bool with_verifier, + const base::FilePath& component_root) { +#if BUILDFLAG(ENABLE_EXTENSIONS) + if (with_verifier) { + return base::MakeRefCounted(component_root); + } +#endif + // if there is no extensions enabled then we expect that on these platforms + // the component files are protected by the OS. + return base::MakeRefCounted( + component_root); +} void SetupComponentContentsVerifier() { - auto factory = base::BindRepeating( - [](const base::FilePath& component_root) - -> scoped_refptr { - return base::MakeRefCounted( - component_root); - }); + auto factory = base::BindRepeating(CreateComponentContentsAccessor, true); brave_component_updater::ComponentContentsVerifier::Setup(std::move(factory)); } } // namespace component_updater - -#endif // BUILDFLAG(ENABLE_EXTENSIONS) diff --git a/browser/component_updater/brave_component_contents_verifier.h b/browser/component_updater/brave_component_contents_verifier.h index cc2177cce036..70e9a2a3f291 100644 --- a/browser/component_updater/brave_component_contents_verifier.h +++ b/browser/component_updater/brave_component_contents_verifier.h @@ -6,10 +6,21 @@ #ifndef BRAVE_BROWSER_COMPONENT_UPDATER_BRAVE_COMPONENT_CONTENTS_VERIFIER_H_ #define BRAVE_BROWSER_COMPONENT_UPDATER_BRAVE_COMPONENT_CONTENTS_VERIFIER_H_ +#include "base/memory/scoped_refptr.h" +#include "brave/components/brave_component_updater/browser/component_contents_verifier.h" + +namespace base { +class FilePath; +} + namespace component_updater { +scoped_refptr +CreateComponentContentsAccessor(bool with_verifier, + const base::FilePath& component_root); + void SetupComponentContentsVerifier(); -} +} // namespace component_updater #endif // BRAVE_BROWSER_COMPONENT_UPDATER_BRAVE_COMPONENT_CONTENTS_VERIFIER_H_ diff --git a/components/brave_shields/core/browser/ad_block_component_service_manager.cc b/components/brave_shields/core/browser/ad_block_component_service_manager.cc index 8669e924d7e3..334c76680af5 100644 --- a/components/brave_shields/core/browser/ad_block_component_service_manager.cc +++ b/components/brave_shields/core/browser/ad_block_component_service_manager.cc @@ -189,6 +189,19 @@ bool AdBlockComponentServiceManager::IsFilterListAvailable( return catalog_entry != filter_list_catalog_.end(); } +base::FilePath AdBlockComponentServiceManager::GetFilterSetPath( + const std::string& uuid) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + DCHECK(!uuid.empty()); + auto it = component_filters_providers_.find(uuid); + + if (it == component_filters_providers_.end()) { + return base::FilePath(); + } + + return it->second->GetFilterSetPath(); +} + bool AdBlockComponentServiceManager::IsFilterListEnabled( const std::string& uuid) const { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); diff --git a/components/brave_shields/core/browser/ad_block_component_service_manager.h b/components/brave_shields/core/browser/ad_block_component_service_manager.h index c8ac89830b0c..8d9696082f1c 100644 --- a/components/brave_shields/core/browser/ad_block_component_service_manager.h +++ b/components/brave_shields/core/browser/ad_block_component_service_manager.h @@ -55,6 +55,7 @@ class AdBlockComponentServiceManager // Get the filter set path for a given filter list. // If the filter list is not available, an empty path is returned. + base::FilePath GetFilterSetPath(const std::string& uuid); bool IsFilterListAvailable(const std::string& uuid) const; bool IsFilterListEnabled(const std::string& uuid) const; void EnableFilterList(const std::string& uuid, bool enabled); diff --git a/test/BUILD.gn b/test/BUILD.gn index 25b5f54c8acc..e2a68f951479 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -838,6 +838,7 @@ test("brave_browser_tests") { "//brave/browser/brave_wallet:browser_tests", "//brave/browser/brave_wallet:tab_helper", "//brave/browser/browsing_data:browser_tests", + "//brave/browser/component_updater", "//brave/browser/decentralized_dns/test:browser_tests", "//brave/browser/ephemeral_storage:browser_tests", "//brave/browser/ethereum_remote_client/buildflags", From b2cc1c46268bea87a1fc52cb97d4f8bc90b8575a Mon Sep 17 00:00:00 2001 From: boocmp Date: Mon, 25 Nov 2024 19:30:42 +0700 Subject: [PATCH 03/14] Added test. --- .../ad_block_service_browsertest.cc | 64 +++++++++++++++++++ .../ad_block_service_browsertest.h | 3 + .../_metadata/verified_contents.json | 1 + .../resources_corrupted/resources.json | 1 + .../_metadata/verified_contents.json | 1 + .../resources_ok/resources.json | 1 + 6 files changed, 71 insertions(+) create mode 100644 test/data/adblock-components/resources_corrupted/_metadata/verified_contents.json create mode 100644 test/data/adblock-components/resources_corrupted/resources.json create mode 100644 test/data/adblock-components/resources_ok/_metadata/verified_contents.json create mode 100644 test/data/adblock-components/resources_ok/resources.json diff --git a/browser/brave_shields/ad_block_service_browsertest.cc b/browser/brave_shields/ad_block_service_browsertest.cc index 6dc2f1d30d9e..a209867bf443 100644 --- a/browser/brave_shields/ad_block_service_browsertest.cc +++ b/browser/brave_shields/ad_block_service_browsertest.cc @@ -19,6 +19,7 @@ #include "base/path_service.h" #include "base/strings/stringprintf.h" #include "base/task/sequenced_task_runner.h" +#include "base/test/bind.h" #include "base/test/scoped_feature_list.h" #include "base/test/thread_test_helper.h" #include "base/threading/thread_restrictions.h" @@ -246,6 +247,32 @@ base::FilePath AdBlockServiceTest::MakeTestDataCopy( return temp_path.Append(source_location.BaseName()); } +brave_shields::AdBlockResourceProvider* +AdBlockServiceTest::IntsallDefaultAdBlockResources( + const base::FilePath& component_path) { + brave_shields::AdBlockService* service = + g_brave_browser_process->ad_block_service(); + auto* resource_provider = + static_cast( + service->resource_provider()); + base::RunLoop run_loop; + base::ThreadPool::PostTaskAndReplyWithResult( + FROM_HERE, {base::MayBlock()}, + base::BindLambdaForTesting([&component_path]() { + return component_updater::CreateComponentContentsAccessor( + true, component_path); + }), + base::BindLambdaForTesting( + [resource_provider, &run_loop]( + scoped_refptr + accessor) { + resource_provider->OnComponentReady(std::move(accessor)); + run_loop.Quit(); + })); + run_loop.Run(); + return resource_provider; +} + void AdBlockServiceTest::UpdateAdBlockResources(const std::string& resources) { auto component_path = MakeFileInTempDir("resources.json", resources); @@ -1608,6 +1635,43 @@ IN_PROC_BROWSER_TEST_F(Default1pBlockingFlagDisabledTest, Custom1pBlocking) { EXPECT_EQ(profile()->GetPrefs()->GetUint64(kAdsBlocked), 2ULL); } +#if BUILDFLAG(ENABLE_EXTENSIONS) +#define MAYBE_SafeResourcesComponentAccess SafeResourcesComponentAccess +#else +#define MAYBE_SafeResourcesComponentAccess DISABLED_SafeResourcesComponentAccess +#endif +IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, MAYBE_SafeResourcesComponentAccess) { + const auto components_path = + GetTestDataDir().AppendASCII("adblock-components"); + { + auto* resource_provider = IntsallDefaultAdBlockResources( + components_path.AppendASCII("resources_ok")); + base::RunLoop run_loop; + resource_provider->LoadResources( + base::BindLambdaForTesting([&run_loop](const std::string& resources) { + constexpr const char kExpectedResources[] = + "[{\"name\":\"brave-fix.js\",\"aliases\":[],\"kind\":{\"mime\":" + "\"application/javascript\"}]"; + EXPECT_EQ(kExpectedResources, resources); + run_loop.Quit(); + })); + run_loop.Run(); + } + + { + auto* resource_provider = IntsallDefaultAdBlockResources( + components_path.AppendASCII("resources_corrupted")); + base::RunLoop run_loop; + resource_provider->LoadResources( + base::BindLambdaForTesting([&run_loop](const std::string& resources) { + constexpr const char kExpectedResources[] = "[]"; + EXPECT_EQ(kExpectedResources, resources); + run_loop.Quit(); + })); + run_loop.Run(); + } +} + // Load a page with a script which uses a redirect data URL. IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, RedirectRulesAreRespected) { UpdateAdBlockResources(R"( diff --git a/browser/brave_shields/ad_block_service_browsertest.h b/browser/brave_shields/ad_block_service_browsertest.h index b4e3c9f7b869..5f7af1e8ceca 100644 --- a/browser/brave_shields/ad_block_service_browsertest.h +++ b/browser/brave_shields/ad_block_service_browsertest.h @@ -22,6 +22,7 @@ class HostContentSettingsMap; namespace brave_shields { class AdBlockService; +class AdBlockResourceProvider; class FilterListCatalogEntry; } // namespace brave_shields @@ -47,6 +48,8 @@ class AdBlockServiceTest : public PlatformBrowserTest { void AddNewRules(const std::string& rules, uint8_t permission_mask = 0, bool first_party_protections = false); + brave_shields::AdBlockResourceProvider* IntsallDefaultAdBlockResources( + const base::FilePath& component_path); void UpdateAdBlockResources(const std::string& resources); void UpdateAdBlockInstanceWithRules(const std::string& rules); void EnableDeveloperMode(bool enabled); diff --git a/test/data/adblock-components/resources_corrupted/_metadata/verified_contents.json b/test/data/adblock-components/resources_corrupted/_metadata/verified_contents.json new file mode 100644 index 000000000000..aa9a40a0b038 --- /dev/null +++ b/test/data/adblock-components/resources_corrupted/_metadata/verified_contents.json @@ -0,0 +1 @@ +[{"description":"treehash per file","signed_content":{"payload":"eyJjb250ZW50X2hhc2hlcyI6W3siYmxvY2tfc2l6ZSI6NDA5NiwiZGlnZXN0Ijoic2hhMjU2IiwiZmlsZXMiOlt7InBhdGgiOiJyZXNvdXJjZXMuanNvbiIsInJvb3RfaGFzaCI6Ik9oM3pERVJjT3U0NWE2N21WQkFqeWcyNzEyUGxvek04UWtBeVNnZzVUMlkifV0sImZvcm1hdCI6InRyZWVoYXNoIiwiaGFzaF9ibG9ja19zaXplIjo0MDk2fV0sIml0ZW1faWQiOiJtZmRkaWJtYmxtYmNjcGFkZm5kZ2FraW9wbW1oZWJvcCIsIml0ZW1fdmVyc2lvbiI6IjEuMC4xMDMiLCJwcm90b2NvbF92ZXJzaW9uIjoxfQ","signatures":[{"protected":"eyJhbGciOiJSUzI1NiJ9","header":{"kid":"webstore"},"signature":"gxnzPORvPtzdFbXbaOD-MOa93ED7LFNA8bAswg7dCXBXe9CP5Loyjsj3ch1XxVSzCfyLQ-lai0LN7TlTjxTZocbXD7MRfwo8hR9Qn2iyTjc8QswSiWl_F0tuV7fg6b6oTiX7Cc7-EPoyngWAA9nX7_HqjfG8XspnoEWs_-lER-DPDCX7sbWdSZuZaPoEOgDaujILJgcYr264n0nZNzc213wVzKxwfQeZAPUZAnTofulO03QQquKuzjZURS8ZZdSW2vlc72duxylo3Xr40lERk60VGv35ZwYW1UZpt8PZH_XOame3vfyBrbrxylRySu7nAQ8GuyEuDP8ucIAHUpf5Hg"}]}}] \ No newline at end of file diff --git a/test/data/adblock-components/resources_corrupted/resources.json b/test/data/adblock-components/resources_corrupted/resources.json new file mode 100644 index 000000000000..9f2c0f1717b1 --- /dev/null +++ b/test/data/adblock-components/resources_corrupted/resources.json @@ -0,0 +1 @@ +[{"name":"malware.js","aliases":[],"kind":{"mime":"application/javascript"}] \ No newline at end of file diff --git a/test/data/adblock-components/resources_ok/_metadata/verified_contents.json b/test/data/adblock-components/resources_ok/_metadata/verified_contents.json new file mode 100644 index 000000000000..aa9a40a0b038 --- /dev/null +++ b/test/data/adblock-components/resources_ok/_metadata/verified_contents.json @@ -0,0 +1 @@ +[{"description":"treehash per file","signed_content":{"payload":"eyJjb250ZW50X2hhc2hlcyI6W3siYmxvY2tfc2l6ZSI6NDA5NiwiZGlnZXN0Ijoic2hhMjU2IiwiZmlsZXMiOlt7InBhdGgiOiJyZXNvdXJjZXMuanNvbiIsInJvb3RfaGFzaCI6Ik9oM3pERVJjT3U0NWE2N21WQkFqeWcyNzEyUGxvek04UWtBeVNnZzVUMlkifV0sImZvcm1hdCI6InRyZWVoYXNoIiwiaGFzaF9ibG9ja19zaXplIjo0MDk2fV0sIml0ZW1faWQiOiJtZmRkaWJtYmxtYmNjcGFkZm5kZ2FraW9wbW1oZWJvcCIsIml0ZW1fdmVyc2lvbiI6IjEuMC4xMDMiLCJwcm90b2NvbF92ZXJzaW9uIjoxfQ","signatures":[{"protected":"eyJhbGciOiJSUzI1NiJ9","header":{"kid":"webstore"},"signature":"gxnzPORvPtzdFbXbaOD-MOa93ED7LFNA8bAswg7dCXBXe9CP5Loyjsj3ch1XxVSzCfyLQ-lai0LN7TlTjxTZocbXD7MRfwo8hR9Qn2iyTjc8QswSiWl_F0tuV7fg6b6oTiX7Cc7-EPoyngWAA9nX7_HqjfG8XspnoEWs_-lER-DPDCX7sbWdSZuZaPoEOgDaujILJgcYr264n0nZNzc213wVzKxwfQeZAPUZAnTofulO03QQquKuzjZURS8ZZdSW2vlc72duxylo3Xr40lERk60VGv35ZwYW1UZpt8PZH_XOame3vfyBrbrxylRySu7nAQ8GuyEuDP8ucIAHUpf5Hg"}]}}] \ No newline at end of file diff --git a/test/data/adblock-components/resources_ok/resources.json b/test/data/adblock-components/resources_ok/resources.json new file mode 100644 index 000000000000..eccd68bcad87 --- /dev/null +++ b/test/data/adblock-components/resources_ok/resources.json @@ -0,0 +1 @@ +[{"name":"brave-fix.js","aliases":[],"kind":{"mime":"application/javascript"}] \ No newline at end of file From 9935d8ff836d48545e44c3b174008396d7b11426 Mon Sep 17 00:00:00 2001 From: boocmp Date: Tue, 26 Nov 2024 20:35:01 +0700 Subject: [PATCH 04/14] Some review issues are addressed. --- .../brave_component_contents_verifier.cc | 31 ++++------- .../brave_component_updater/browser/DEPS | 3 -- .../browser/component_contents_verifier.cc | 51 ++++++++----------- .../browser/component_contents_verifier.h | 12 ++--- .../browser/ad_block_component_installer.cc | 8 ++- .../ad_block_default_resource_provider.cc | 4 -- 6 files changed, 38 insertions(+), 71 deletions(-) delete mode 100644 components/brave_component_updater/browser/DEPS diff --git a/browser/component_updater/brave_component_contents_verifier.cc b/browser/component_updater/brave_component_contents_verifier.cc index ea85ea8d5682..f408e2d05195 100644 --- a/browser/component_updater/brave_component_contents_verifier.cc +++ b/browser/component_updater/brave_component_contents_verifier.cc @@ -11,6 +11,7 @@ #include #include +#include "base/containers/span_reader.h" #include "base/files/file_path.h" #include "base/functional/bind.h" #include "crypto/secure_hash.h" @@ -57,7 +58,7 @@ class ComponentNoChecksContentsAccessorImpl namespace { -constexpr const uint8_t kComponentContentsVerifierPublicKey[] = { +constexpr uint8_t kComponentContentsVerifierPublicKey[] = { 0x30, 0x82, 0x01, 0x22, 0x30, 0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01, 0x05, 0x00, 0x03, 0x82, 0x01, 0x0f, 0x00, 0x30, 0x82, 0x01, 0x0a, 0x02, 0x82, 0x01, 0x01, 0x00, 0xc3, 0x8a, 0x2d, @@ -84,33 +85,25 @@ constexpr const uint8_t kComponentContentsVerifierPublicKey[] = { 0x88, 0x77, 0x2c, 0x84, 0xdd, 0x00, 0x87, 0x03, 0x49, 0x09, 0xb7, 0x4b, 0xc7, 0x02, 0x03, 0x01, 0x00, 0x01}; -constexpr const char kVerifiedContentsPath[] = - "_metadata/verified_contents.json"; +constexpr char kVerifiedContentsPath[] = "_metadata/verified_contents.json"; std::string GetRootHasheForContent(base::span contents, size_t block_size) { - size_t offset = 0; std::vector hashes; // Even when the contents is empty, we want to output at least one hash // block (the hash of the empty string). + base::SpanReader reader(contents); do { - const size_t bytes_to_read = std::min(contents.size() - offset, block_size); + auto chunk = reader.Read(std::min(block_size, reader.remaining())); std::unique_ptr hash( crypto::SecureHash::Create(crypto::SecureHash::SHA256)); - hash->Update(contents.subspan(offset, bytes_to_read)); + hash->Update(chunk.value()); std::string buffer; buffer.resize(crypto::kSHA256Length); hash->Finish(base::as_writable_byte_span(buffer)); hashes.push_back(std::move(buffer)); - - // If |contents| is empty, then we want to just exit here. - if (bytes_to_read == 0) { - break; - } - - offset += bytes_to_read; - } while (offset < contents.size()); + } while (reader.remaining() > 0); CHECK(block_size % crypto::kSHA256Length == 0); return extensions::ComputeTreeHashRoot(hashes, @@ -124,9 +117,10 @@ class ComponentContentsAccessorImpl explicit ComponentContentsAccessorImpl(const base::FilePath& component_root) : ComponentNoChecksContentsAccessorImpl(component_root) { verified_contents_ = extensions::VerifiedContents::CreateFromFile( - base::make_span(kComponentContentsVerifierPublicKey), + kComponentContentsVerifierPublicKey, component_root.AppendASCII(kVerifiedContentsPath)); - if (verified_contents_->block_size() % crypto::kSHA256Length != 0) { + if (verified_contents_ && + verified_contents_->block_size() % crypto::kSHA256Length != 0) { // Unsupported block size. verified_contents_.reset(); } @@ -152,10 +146,7 @@ class ComponentContentsAccessorImpl const auto root_hash = GetRootHasheForContent(contents, verified_contents_->block_size()); - if (!verified_contents_->TreeHashRootEquals(relative_path, root_hash)) { - return false; - } - return true; + return verified_contents_->TreeHashRootEquals(relative_path, root_hash); } private: diff --git a/components/brave_component_updater/browser/DEPS b/components/brave_component_updater/browser/DEPS deleted file mode 100644 index 2919c1791d12..000000000000 --- a/components/brave_component_updater/browser/DEPS +++ /dev/null @@ -1,3 +0,0 @@ -include_rules = [ - "+extensions/buildflags", -] diff --git a/components/brave_component_updater/browser/component_contents_verifier.cc b/components/brave_component_updater/browser/component_contents_verifier.cc index e877fdc472ac..319934a0a751 100644 --- a/components/brave_component_updater/browser/component_contents_verifier.cc +++ b/components/brave_component_updater/browser/component_contents_verifier.cc @@ -11,50 +11,37 @@ #include "base/check_is_test.h" #include "base/files/file_util.h" #include "base/no_destructor.h" -#include "extensions/buildflags/buildflags.h" +#include "base/task/thread_pool.h" namespace brave_component_updater { -base::expected -ComponentContentsAccessor::ReadFileToString( - const base::FilePath& relative_path) { +std::string ComponentContentsAccessor::GetFileAsString( + const base::FilePath& relative_path, + const std::string& default_value) { std::string contents; if (!base::ReadFileToString(GetComponentRoot().Append(relative_path), &contents)) { - return base::unexpected(Error::kFileReadFailed); + return default_value; } if (!VerifyContents(relative_path, base::as_byte_span(contents))) { - return base::unexpected(Error::kInvalidSignature); + return default_value; } - return base::ok(std::move(contents)); + return contents; } -base::expected, ComponentContentsAccessor::Error> -ComponentContentsAccessor::ReadFileToBytes( - const base::FilePath& relative_path) { +std::vector ComponentContentsAccessor::GetFileAsBytes( + const base::FilePath& relative_path, + const std::vector& default_value) { auto contents = base::ReadFileToBytes(GetComponentRoot().Append(relative_path)); if (!contents) { - return base::unexpected(Error::kFileReadFailed); + return default_value; } if (!VerifyContents(relative_path, base::as_byte_span(*contents))) { - return base::unexpected(Error::kInvalidSignature); + return default_value; } - - return base::ok(*std::move(contents)); -} - -std::string ComponentContentsAccessor::GetFileAsString( - const base::FilePath& relative_path, - const std::string& default_value) { - return ReadFileToString(relative_path).value_or(default_value); -} - -std::vector ComponentContentsAccessor::GetFileAsBytes( - const base::FilePath& relative_path, - const std::vector& default_value) { - return ReadFileToBytes(relative_path).value_or(default_value); + return std::move(*contents); } ComponentContentsVerifier::ComponentContentsVerifier() = default; @@ -75,12 +62,14 @@ ComponentContentsVerifier* ComponentContentsVerifier::GetInstance() { return instance.get(); } -// Must be called on the MAY_BLOCK sequence. -scoped_refptr -ComponentContentsVerifier::GetContentsAccessor( - const base::FilePath& component_root) { +void ComponentContentsVerifier::CreateContentsAccessor( + const base::FilePath& component_root, + base::OnceCallback)> + on_created) { CHECK(cca_factory_); - return cca_factory_.Run(component_root); + base::ThreadPool::PostTaskAndReplyWithResult( + FROM_HERE, {base::MayBlock()}, + base::BindOnce(cca_factory_, component_root), std::move(on_created)); } } // namespace brave_component_updater diff --git a/components/brave_component_updater/browser/component_contents_verifier.h b/components/brave_component_updater/browser/component_contents_verifier.h index cb80180b22ee..03ae3c26d582 100644 --- a/components/brave_component_updater/browser/component_contents_verifier.h +++ b/components/brave_component_updater/browser/component_contents_verifier.h @@ -36,11 +36,6 @@ class ComponentContentsAccessor virtual bool VerifyContents(const base::FilePath& relative_path, base::span contents) = 0; - base::expected ReadFileToString( - const base::FilePath& relative_path); - base::expected, Error> ReadFileToBytes( - const base::FilePath& relative_path); - std::string GetFileAsString(const base::FilePath& relative_path, const std::string& default_value); std::vector GetFileAsBytes( @@ -61,9 +56,10 @@ class ComponentContentsVerifier { static void Setup(ComponentContentsAccessorFactory cca_factory); static ComponentContentsVerifier* GetInstance(); - // Must be called on the MAY_BLOCK sequence. - scoped_refptr GetContentsAccessor( - const base::FilePath& component_root); + void CreateContentsAccessor( + const base::FilePath& component_root, + base::OnceCallback)> + on_created); private: friend base::NoDestructor; diff --git a/components/brave_shields/core/browser/ad_block_component_installer.cc b/components/brave_shields/core/browser/ad_block_component_installer.cc index fcfd5b1eb44a..37ac398a3c40 100644 --- a/components/brave_shields/core/browser/ad_block_component_installer.cc +++ b/components/brave_shields/core/browser/ad_block_component_installer.cc @@ -105,7 +105,7 @@ void AdBlockComponentInstallerPolicy::ComponentReady( const base::Version& version, const base::FilePath& path, base::Value::Dict manifest) { - ready_callback_.Run(std::move(path)); + ready_callback_.Run(path); } bool AdBlockComponentInstallerPolicy::VerifyInstallation( @@ -156,10 +156,8 @@ void RegisterAdBlockDefaultResourceComponent( auto on_ready = base::BindRepeating( [](OnSecureComponentReadyCallback callback, const base::FilePath& path) { - auto accessor = - brave_component_updater::ComponentContentsVerifier::GetInstance() - ->GetContentsAccessor(path); - callback.Run(std::move(accessor)); + brave_component_updater::ComponentContentsVerifier::GetInstance() + ->CreateContentsAccessor(path, std::move(callback)); }, std::move(callback)); diff --git a/components/brave_shields/core/browser/ad_block_default_resource_provider.cc b/components/brave_shields/core/browser/ad_block_default_resource_provider.cc index 99bc80a58bee..6304e106c2f9 100644 --- a/components/brave_shields/core/browser/ad_block_default_resource_provider.cc +++ b/components/brave_shields/core/browser/ad_block_default_resource_provider.cc @@ -49,10 +49,6 @@ void AdBlockDefaultResourceProvider::OnComponentReady( scoped_refptr accessor) { component_accessor_ = std::move(accessor); - if (!component_accessor_) { - // This should not happen, but if it does, we should not proceed. - return; - } // Load the resources (as a string) LoadResources( From e7bce5de03854ec1545f15b2ff28a24fa9331f9f Mon Sep 17 00:00:00 2001 From: boocmp Date: Wed, 27 Nov 2024 08:46:55 +0700 Subject: [PATCH 05/14] Returning optional instead of passing default value. --- .../browser/component_contents_verifier.cc | 18 ++++++++---------- .../browser/component_contents_verifier.h | 11 +++++------ .../ad_block_default_resource_provider.cc | 11 +++++++++-- 3 files changed, 22 insertions(+), 18 deletions(-) diff --git a/components/brave_component_updater/browser/component_contents_verifier.cc b/components/brave_component_updater/browser/component_contents_verifier.cc index 319934a0a751..7862b4c82cbf 100644 --- a/components/brave_component_updater/browser/component_contents_verifier.cc +++ b/components/brave_component_updater/browser/component_contents_verifier.cc @@ -15,31 +15,29 @@ namespace brave_component_updater { -std::string ComponentContentsAccessor::GetFileAsString( - const base::FilePath& relative_path, - const std::string& default_value) { +std::optional ComponentContentsAccessor::GetFileAsString( + const base::FilePath& relative_path) { std::string contents; if (!base::ReadFileToString(GetComponentRoot().Append(relative_path), &contents)) { - return default_value; + return std::nullopt; } if (!VerifyContents(relative_path, base::as_byte_span(contents))) { - return default_value; + return std::nullopt; } return contents; } -std::vector ComponentContentsAccessor::GetFileAsBytes( - const base::FilePath& relative_path, - const std::vector& default_value) { +std::optional> ComponentContentsAccessor::GetFileAsBytes( + const base::FilePath& relative_path) { auto contents = base::ReadFileToBytes(GetComponentRoot().Append(relative_path)); if (!contents) { - return default_value; + return std::nullopt; } if (!VerifyContents(relative_path, base::as_byte_span(*contents))) { - return default_value; + return std::nullopt; } return std::move(*contents); } diff --git a/components/brave_component_updater/browser/component_contents_verifier.h b/components/brave_component_updater/browser/component_contents_verifier.h index 03ae3c26d582..f6ca70beeed0 100644 --- a/components/brave_component_updater/browser/component_contents_verifier.h +++ b/components/brave_component_updater/browser/component_contents_verifier.h @@ -6,13 +6,13 @@ #ifndef BRAVE_COMPONENTS_BRAVE_COMPONENT_UPDATER_BROWSER_COMPONENT_CONTENTS_VERIFIER_H_ #define BRAVE_COMPONENTS_BRAVE_COMPONENT_UPDATER_BROWSER_COMPONENT_CONTENTS_VERIFIER_H_ +#include #include #include #include "base/files/file_path.h" #include "base/functional/callback.h" #include "base/memory/ref_counted.h" -#include "base/types/expected.h" namespace base { template @@ -36,11 +36,10 @@ class ComponentContentsAccessor virtual bool VerifyContents(const base::FilePath& relative_path, base::span contents) = 0; - std::string GetFileAsString(const base::FilePath& relative_path, - const std::string& default_value); - std::vector GetFileAsBytes( - const base::FilePath& relative_path, - const std::vector& default_value); + std::optional GetFileAsString( + const base::FilePath& relative_path); + std::optional> GetFileAsBytes( + const base::FilePath& relative_path); protected: friend class base::RefCountedThreadSafe; diff --git a/components/brave_shields/core/browser/ad_block_default_resource_provider.cc b/components/brave_shields/core/browser/ad_block_default_resource_provider.cc index 6304e106c2f9..35413706891b 100644 --- a/components/brave_shields/core/browser/ad_block_default_resource_provider.cc +++ b/components/brave_shields/core/browser/ad_block_default_resource_provider.cc @@ -65,13 +65,20 @@ void AdBlockDefaultResourceProvider::LoadResources( return; } + auto handle_file_content = base::BindOnce( + [](base::OnceCallback cb, + std::optional content) { + std::move(cb).Run(content.value_or("[]")); + }, + std::move(cb)); + base::ThreadPool::PostTaskAndReplyWithResult( FROM_HERE, {base::MayBlock()}, base::BindOnce( &brave_component_updater::ComponentContentsAccessor::GetFileAsString, base::RetainedRef(component_accessor_), - base::FilePath::FromASCII(kAdBlockResourcesFilename), "[]"), - std::move(cb)); + base::FilePath::FromASCII(kAdBlockResourcesFilename)), + std::move(handle_file_content)); } } // namespace brave_shields From 29d238bdeeb87fa4f437cb0d745c8dc58983405e Mon Sep 17 00:00:00 2001 From: boocmp Date: Wed, 27 Nov 2024 09:16:12 +0700 Subject: [PATCH 06/14] Removed ComponentNoChecksContentsAccessorImpl. --- .../brave_component_contents_verifier.cc | 42 ++----------------- .../browser/component_contents_verifier.cc | 20 +++++++++ .../browser/component_contents_verifier.h | 23 ++++++---- 3 files changed, 38 insertions(+), 47 deletions(-) diff --git a/browser/component_updater/brave_component_contents_verifier.cc b/browser/component_updater/brave_component_contents_verifier.cc index f408e2d05195..ce69b4344b2b 100644 --- a/browser/component_updater/brave_component_contents_verifier.cc +++ b/browser/component_updater/brave_component_contents_verifier.cc @@ -21,40 +21,6 @@ #if BUILDFLAG(ENABLE_EXTENSIONS) #include "extensions/browser/content_hash_tree.h" #include "extensions/browser/verified_contents.h" -#endif - -namespace { - -// Only proxies the file access. -class ComponentNoChecksContentsAccessorImpl - : public brave_component_updater::ComponentContentsAccessor { - public: - explicit ComponentNoChecksContentsAccessorImpl( - const base::FilePath& component_root) - : component_root_(component_root) {} - - const base::FilePath& GetComponentRoot() const override { - return component_root_; - } - - bool IsComponentSignatureValid() const override { return true; } - - void IgnoreInvalidSignature(bool) override {} - - bool VerifyContents(const base::FilePath& relative_path, - base::span contents) override { - return true; - } - - protected: - ~ComponentNoChecksContentsAccessorImpl() override = default; - - const base::FilePath component_root_; -}; - -} // namespace - -#if BUILDFLAG(ENABLE_EXTENSIONS) namespace { @@ -112,10 +78,10 @@ std::string GetRootHasheForContent(base::span contents, // Proxies the file access and checks the verified_contents.json. class ComponentContentsAccessorImpl - : public ComponentNoChecksContentsAccessorImpl { + : public brave_component_updater::ComponentContentsAccessor { public: explicit ComponentContentsAccessorImpl(const base::FilePath& component_root) - : ComponentNoChecksContentsAccessorImpl(component_root) { + : brave_component_updater::ComponentContentsAccessor(component_root) { verified_contents_ = extensions::VerifiedContents::CreateFromFile( kComponentContentsVerifierPublicKey, component_root.AppendASCII(kVerifiedContentsPath)); @@ -172,8 +138,8 @@ CreateComponentContentsAccessor(bool with_verifier, #endif // if there is no extensions enabled then we expect that on these platforms // the component files are protected by the OS. - return base::MakeRefCounted( - component_root); + return base::MakeRefCounted< + brave_component_updater::ComponentContentsAccessor>(component_root); } void SetupComponentContentsVerifier() { diff --git a/components/brave_component_updater/browser/component_contents_verifier.cc b/components/brave_component_updater/browser/component_contents_verifier.cc index 7862b4c82cbf..9a43160dc99e 100644 --- a/components/brave_component_updater/browser/component_contents_verifier.cc +++ b/components/brave_component_updater/browser/component_contents_verifier.cc @@ -15,6 +15,26 @@ namespace brave_component_updater { +ComponentContentsAccessor::ComponentContentsAccessor( + const base::FilePath component_root) + : component_root_(component_root) {} + +bool ComponentContentsAccessor::IsComponentSignatureValid() const { + return true; +} + +void ComponentContentsAccessor::IgnoreInvalidSignature(bool ignore) {} + +bool ComponentContentsAccessor::VerifyContents( + const base::FilePath& relative_path, + base::span contents) { + return true; +} + +const base::FilePath& ComponentContentsAccessor::GetComponentRoot() const { + return component_root_; +} + std::optional ComponentContentsAccessor::GetFileAsString( const base::FilePath& relative_path) { std::string contents; diff --git a/components/brave_component_updater/browser/component_contents_verifier.h b/components/brave_component_updater/browser/component_contents_verifier.h index f6ca70beeed0..ad83d2475790 100644 --- a/components/brave_component_updater/browser/component_contents_verifier.h +++ b/components/brave_component_updater/browser/component_contents_verifier.h @@ -25,16 +25,19 @@ namespace brave_component_updater { class ComponentContentsAccessor : public base::RefCountedThreadSafe { public: - enum class Error { - kFileReadFailed, - kInvalidSignature, - }; - - virtual const base::FilePath& GetComponentRoot() const = 0; - virtual bool IsComponentSignatureValid() const = 0; - virtual void IgnoreInvalidSignature(bool ignore) = 0; + explicit ComponentContentsAccessor(const base::FilePath component_root); + ComponentContentsAccessor(const ComponentContentsAccessor&) = delete; + ComponentContentsAccessor(ComponentContentsAccessor&&) = delete; + ComponentContentsAccessor& operator=(const ComponentContentsAccessor&) = + delete; + ComponentContentsAccessor& operator=(ComponentContentsAccessor&&) = delete; + + virtual bool IsComponentSignatureValid() const; + virtual void IgnoreInvalidSignature(bool ignore); virtual bool VerifyContents(const base::FilePath& relative_path, - base::span contents) = 0; + base::span contents); + + const base::FilePath& GetComponentRoot() const; std::optional GetFileAsString( const base::FilePath& relative_path); @@ -44,6 +47,8 @@ class ComponentContentsAccessor protected: friend class base::RefCountedThreadSafe; virtual ~ComponentContentsAccessor() = default; + + const base::FilePath component_root_; }; class ComponentContentsVerifier { From 8a7e230c1d6d26461e8f4cc6962f480d21fa23b3 Mon Sep 17 00:00:00 2001 From: boocmp Date: Wed, 27 Nov 2024 10:59:06 +0700 Subject: [PATCH 07/14] Check integrigity of all ad-block components. --- .../ad_block_service_browsertest.cc | 7 ++- .../brave_component_contents_verifier.cc | 16 ++++- .../ad_block_component_filters_provider.cc | 43 +++++++------ .../ad_block_component_filters_provider.h | 12 +++- .../browser/ad_block_component_installer.cc | 60 +++++++++---------- .../browser/ad_block_component_installer.h | 9 +-- .../ad_block_filter_list_catalog_provider.cc | 34 ++++++----- .../ad_block_filter_list_catalog_provider.h | 12 ++-- 8 files changed, 108 insertions(+), 85 deletions(-) diff --git a/browser/brave_shields/ad_block_service_browsertest.cc b/browser/brave_shields/ad_block_service_browsertest.cc index a209867bf443..c7e439e6be8c 100644 --- a/browser/brave_shields/ad_block_service_browsertest.cc +++ b/browser/brave_shields/ad_block_service_browsertest.cc @@ -298,7 +298,8 @@ void AdBlockServiceTest::UpdateAdBlockInstanceWithRules( std::string uuid = "default"; auto& provider = component_providers.at(uuid); EXPECT_TRUE(provider); - provider->OnComponentReady(component_path); + provider->OnComponentReady(component_updater::CreateComponentContentsAccessor( + false, component_path)); auto* engine = service->default_engine_.get(); EngineTestObserver engine_observer(engine); @@ -373,7 +374,9 @@ void AdBlockServiceTest::InstallComponent( auto& provider = component_providers.at(catalog_entry.uuid); EXPECT_TRUE(provider); - provider->OnComponentReady(component_path); + provider->OnComponentReady( + component_updater::CreateComponentContentsAccessor(false, + component_path)); auto* engine = catalog_entry.first_party_protections ? service->default_engine_.get() diff --git a/browser/component_updater/brave_component_contents_verifier.cc b/browser/component_updater/brave_component_contents_verifier.cc index ce69b4344b2b..c1a14e67c1e2 100644 --- a/browser/component_updater/brave_component_contents_verifier.cc +++ b/browser/component_updater/brave_component_contents_verifier.cc @@ -13,6 +13,7 @@ #include "base/containers/span_reader.h" #include "base/files/file_path.h" +#include "base/files/file_util.h" #include "base/functional/bind.h" #include "crypto/secure_hash.h" #include "crypto/sha2.h" @@ -52,6 +53,8 @@ constexpr uint8_t kComponentContentsVerifierPublicKey[] = { 0xc7, 0x02, 0x03, 0x01, 0x00, 0x01}; constexpr char kVerifiedContentsPath[] = "_metadata/verified_contents.json"; +constexpr char kBraveVerifiedContentsPath[] = + "brave_metadata/verified_contents.json"; std::string GetRootHasheForContent(base::span contents, size_t block_size) { @@ -82,9 +85,16 @@ class ComponentContentsAccessorImpl public: explicit ComponentContentsAccessorImpl(const base::FilePath& component_root) : brave_component_updater::ComponentContentsAccessor(component_root) { - verified_contents_ = extensions::VerifiedContents::CreateFromFile( - kComponentContentsVerifierPublicKey, - component_root.AppendASCII(kVerifiedContentsPath)); + if (base::PathExists( + component_root.AppendASCII(kBraveVerifiedContentsPath))) { + verified_contents_ = extensions::VerifiedContents::CreateFromFile( + kComponentContentsVerifierPublicKey, + component_root.AppendASCII(kBraveVerifiedContentsPath)); + } else { + verified_contents_ = extensions::VerifiedContents::CreateFromFile( + kComponentContentsVerifierPublicKey, + component_root.AppendASCII(kVerifiedContentsPath)); + } if (verified_contents_ && verified_contents_->block_size() % crypto::kSHA256Length != 0) { // Unsupported block size. diff --git a/components/brave_shields/core/browser/ad_block_component_filters_provider.cc b/components/brave_shields/core/browser/ad_block_component_filters_provider.cc index 16d8b9f0d7d5..b9c9ff611203 100644 --- a/components/brave_shields/core/browser/ad_block_component_filters_provider.cc +++ b/components/brave_shields/core/browser/ad_block_component_filters_provider.cc @@ -24,8 +24,6 @@ namespace brave_shields { namespace { -void AddNothingToFilterSet(rust::Box*) {} - // static void AddDATBufferToFilterSet(uint8_t permission_mask, DATFileDataBuffer buffer, @@ -38,9 +36,9 @@ void OnReadDATFileData( base::OnceCallback< void(base::OnceCallback*)>)> cb, uint8_t permission_mask, - DATFileDataBuffer buffer) { - std::move(cb).Run( - base::BindOnce(&AddDATBufferToFilterSet, permission_mask, buffer)); + std::optional buffer) { + std::move(cb).Run(base::BindOnce(&AddDATBufferToFilterSet, permission_mask, + buffer.value_or(DATFileDataBuffer()))); } } // namespace @@ -90,30 +88,28 @@ void AdBlockComponentFiltersProvider::UnregisterComponent() { } void AdBlockComponentFiltersProvider::OnComponentReady( - const base::FilePath& path) { - base::FilePath old_path = component_path_; - component_path_ = path; - - NotifyObservers(engine_is_default_); - - if (!old_path.empty()) { + scoped_refptr + accessor) { + if (component_accessor_) { base::ThreadPool::PostTask( FROM_HERE, {base::TaskPriority::BEST_EFFORT, base::MayBlock()}, - base::BindOnce(IgnoreResult(&base::DeletePathRecursively), old_path)); + base::GetDeletePathRecursivelyCallback( + component_accessor_->GetComponentRoot())); } + component_accessor_ = std::move(accessor); + + NotifyObservers(engine_is_default_); } bool AdBlockComponentFiltersProvider::IsInitialized() const { - return !component_path_.empty(); + return !!component_accessor_; } base::FilePath AdBlockComponentFiltersProvider::GetFilterSetPath() { - if (component_path_.empty()) { - // Since we know it's empty return it as is. - return component_path_; + if (!component_accessor_) { + return base::FilePath(); } - - return component_path_.AppendASCII(kListFile); + return component_accessor_->GetComponentRoot().AppendASCII(kListFile); } void AdBlockComponentFiltersProvider::LoadFilterSet( @@ -121,16 +117,19 @@ void AdBlockComponentFiltersProvider::LoadFilterSet( void(base::OnceCallback*)>)> cb) { base::FilePath list_file_path = GetFilterSetPath(); - if (list_file_path.empty()) { + if (!component_accessor_) { // If the path is not ready yet, provide a no-op callback immediately. An // update will be pushed later to notify about the newly available list. - std::move(cb).Run(base::BindOnce(AddNothingToFilterSet)); + std::move(cb).Run(base::DoNothing()); return; } base::ThreadPool::PostTaskAndReplyWithResult( FROM_HERE, {base::MayBlock()}, - base::BindOnce(&brave_component_updater::ReadDATFileData, list_file_path), + base::BindOnce( + &brave_component_updater::ComponentContentsAccessor::GetFileAsBytes, + base::RetainedRef(component_accessor_), + base::FilePath::FromASCII(kListFile)), base::BindOnce(&OnReadDATFileData, std::move(cb), permission_mask_)); } diff --git a/components/brave_shields/core/browser/ad_block_component_filters_provider.h b/components/brave_shields/core/browser/ad_block_component_filters_provider.h index 6e810c87916f..b17026c1e68a 100644 --- a/components/brave_shields/core/browser/ad_block_component_filters_provider.h +++ b/components/brave_shields/core/browser/ad_block_component_filters_provider.h @@ -19,6 +19,10 @@ namespace component_updater { class ComponentUpdateService; } // namespace component_updater +namespace brave_component_updater { +class ComponentContentsAccessor; +} + namespace base { class FilePath; } @@ -73,9 +77,13 @@ class AdBlockComponentFiltersProvider : public AdBlockFiltersProvider { friend class ::AdBlockServiceTest; friend class ::DebounceBrowserTest; - void OnComponentReady(const base::FilePath&); + void OnComponentReady( + scoped_refptr + accessor); + + scoped_refptr + component_accessor_; - base::FilePath component_path_; std::string component_id_; uint8_t permission_mask_; const raw_ptr diff --git a/components/brave_shields/core/browser/ad_block_component_installer.cc b/components/brave_shields/core/browser/ad_block_component_installer.cc index 37ac398a3c40..d9a39aceb1d5 100644 --- a/components/brave_shields/core/browser/ad_block_component_installer.cc +++ b/components/brave_shields/core/browser/ad_block_component_installer.cc @@ -11,6 +11,7 @@ #include #include "base/base64.h" +#include "base/files/file_path.h" #include "base/functional/bind.h" #include "brave/components/brave_component_updater/browser/brave_on_demand_updater.h" #include "brave/components/brave_component_updater/browser/component_contents_verifier.h" @@ -25,6 +26,9 @@ namespace brave_shields { namespace { +using OnComponentReadyCallback = + base::RepeatingCallback; + constexpr size_t kHashSize = 32; class AdBlockComponentInstallerPolicy @@ -144,10 +148,11 @@ void OnRegistered(const std::string& component_id) { component_id, component_updater::OnDemandUpdater::Priority::FOREGROUND); } -} // namespace - -void RegisterAdBlockDefaultResourceComponent( +void RegisterAdBlockComponentInternal( component_updater::ComponentUpdateService* cus, + const std::string& component_public_key, + const std::string& component_id, + const std::string& component_name, OnSecureComponentReadyCallback callback) { // In test, |cus| could be nullptr. if (!cus) { @@ -163,27 +168,29 @@ void RegisterAdBlockDefaultResourceComponent( auto installer = base::MakeRefCounted( std::make_unique( - kAdBlockResourceComponentBase64PublicKey, kAdBlockResourceComponentId, - kAdBlockResourceComponentName, std::move(on_ready))); - installer->Register( - cus, base::BindOnce(&OnRegistered, kAdBlockResourceComponentId)); + component_public_key, component_id, component_name, + std::move(on_ready))); + installer->Register(cus, base::BindOnce(&OnRegistered, component_id)); } -void RegisterAdBlockFilterListCatalogComponent( +} // namespace + +void RegisterAdBlockDefaultResourceComponent( component_updater::ComponentUpdateService* cus, - OnComponentReadyCallback callback) { - // In test, |cus| could be nullptr. - if (!cus) { - return; - } + OnSecureComponentReadyCallback callback) { + RegisterAdBlockComponentInternal( + cus, kAdBlockResourceComponentBase64PublicKey, + kAdBlockResourceComponentId, kAdBlockResourceComponentName, + std::move(callback)); +} - auto installer = base::MakeRefCounted( - std::make_unique( - kAdBlockFilterListCatalogComponentBase64PublicKey, - kAdBlockFilterListCatalogComponentId, - kAdBlockFilterListCatalogComponentName, callback)); - installer->Register( - cus, base::BindOnce(&OnRegistered, kAdBlockFilterListCatalogComponentId)); +void RegisterAdBlockFilterListCatalogComponent( + component_updater::ComponentUpdateService* cus, + OnSecureComponentReadyCallback callback) { + RegisterAdBlockComponentInternal( + cus, kAdBlockFilterListCatalogComponentBase64PublicKey, + kAdBlockFilterListCatalogComponentId, + kAdBlockFilterListCatalogComponentName, std::move(callback)); } void RegisterAdBlockFiltersComponent( @@ -191,16 +198,9 @@ void RegisterAdBlockFiltersComponent( const std::string& component_public_key, const std::string& component_id, const std::string& component_name, - OnComponentReadyCallback callback) { - // In test, |cus| could be nullptr. - if (!cus) { - return; - } - - auto installer = base::MakeRefCounted( - std::make_unique( - component_public_key, component_id, component_name, callback)); - installer->Register(cus, base::BindOnce(&OnRegistered, component_id)); + OnSecureComponentReadyCallback callback) { + RegisterAdBlockComponentInternal(cus, component_public_key, component_id, + component_name, std::move(callback)); } } // namespace brave_shields diff --git a/components/brave_shields/core/browser/ad_block_component_installer.h b/components/brave_shields/core/browser/ad_block_component_installer.h index afc0bf121580..40dc6b885f70 100644 --- a/components/brave_shields/core/browser/ad_block_component_installer.h +++ b/components/brave_shields/core/browser/ad_block_component_installer.h @@ -6,10 +6,8 @@ #ifndef BRAVE_COMPONENTS_BRAVE_SHIELDS_CORE_BROWSER_AD_BLOCK_COMPONENT_INSTALLER_H_ #define BRAVE_COMPONENTS_BRAVE_SHIELDS_CORE_BROWSER_AD_BLOCK_COMPONENT_INSTALLER_H_ -#include #include -#include "base/files/file_path.h" #include "base/functional/callback.h" #include "brave/components/brave_component_updater/browser/component_contents_verifier.h" @@ -19,9 +17,6 @@ class ComponentUpdateService; namespace brave_shields { -using OnComponentReadyCallback = - base::RepeatingCallback; - using OnSecureComponentReadyCallback = base::RepeatingCallback)>; @@ -31,14 +26,14 @@ void RegisterAdBlockDefaultResourceComponent( void RegisterAdBlockFilterListCatalogComponent( component_updater::ComponentUpdateService* cus, - OnComponentReadyCallback callback); + OnSecureComponentReadyCallback callback); void RegisterAdBlockFiltersComponent( component_updater::ComponentUpdateService* cus, const std::string& component_public_key, const std::string& component_id, const std::string& component_name, - OnComponentReadyCallback callback); + OnSecureComponentReadyCallback callback); } // namespace brave_shields diff --git a/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.cc b/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.cc index 96e39958cd29..930c0efecfca 100644 --- a/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.cc +++ b/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.cc @@ -10,6 +10,7 @@ #include "base/files/file_path.h" #include "base/task/thread_pool.h" +#include "brave/components/brave_component_updater/browser/component_contents_verifier.h" #include "brave/components/brave_shields/core/browser/ad_block_component_installer.h" constexpr char kListCatalogFile[] = "list_catalog.json"; @@ -47,32 +48,37 @@ void AdBlockFilterListCatalogProvider::OnFilterListCatalogLoaded( } void AdBlockFilterListCatalogProvider::OnComponentReady( - const base::FilePath& path) { - component_path_ = path; + scoped_refptr + accessor) { + component_accessor_ = std::move(accessor); - // Load the filter list catalog (as a string) - base::ThreadPool::PostTaskAndReplyWithResult( - FROM_HERE, {base::MayBlock()}, - base::BindOnce(&brave_component_updater::GetDATFileAsString, - component_path_.AppendASCII(kListCatalogFile)), - base::BindOnce( - &AdBlockFilterListCatalogProvider::OnFilterListCatalogLoaded, - weak_factory_.GetWeakPtr())); + LoadFilterListCatalog(base::BindOnce( + &AdBlockFilterListCatalogProvider::OnFilterListCatalogLoaded, + weak_factory_.GetWeakPtr())); } void AdBlockFilterListCatalogProvider::LoadFilterListCatalog( base::OnceCallback cb) { - if (component_path_.empty()) { + if (!component_accessor_) { // If the path is not ready yet, don't run the callback. An update should be // pushed soon. return; } + auto on_load = base::BindOnce( + [](base::OnceCallback cb, + std::optional data) { + std::move(cb).Run(data.value_or("")); + }, + std::move(cb)); + base::ThreadPool::PostTaskAndReplyWithResult( FROM_HERE, {base::MayBlock()}, - base::BindOnce(&brave_component_updater::GetDATFileAsString, - component_path_.AppendASCII(kListCatalogFile)), - std::move(cb)); + base::BindOnce( + &brave_component_updater::ComponentContentsAccessor::GetFileAsString, + base::RetainedRef(component_accessor_), + base::FilePath::FromASCII(kListCatalogFile)), + std::move(on_load)); } } // namespace brave_shields diff --git a/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.h b/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.h index 92d0beb255e8..5ae3719a01d0 100644 --- a/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.h +++ b/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.h @@ -11,14 +11,13 @@ #include "base/functional/callback.h" #include "base/observer_list.h" #include "base/observer_list_types.h" -#include "brave/components/brave_component_updater/browser/dat_file_util.h" namespace component_updater { class ComponentUpdateService; } // namespace component_updater -namespace base { -class FilePath; +namespace brave_component_updater { +class ComponentContentsAccessor; } namespace brave_shields { @@ -46,9 +45,12 @@ class AdBlockFilterListCatalogProvider { private: void OnFilterListCatalogLoaded(const std::string& catalog_json); - void OnComponentReady(const base::FilePath&); + void OnComponentReady( + scoped_refptr + accessor); - base::FilePath component_path_; + scoped_refptr + component_accessor_; base::ObserverList observers_; From e43bc7431c02ef177b36fb6c193e84dcbe6417c0 Mon Sep 17 00:00:00 2001 From: boocmp Date: Tue, 3 Dec 2024 09:16:13 +0700 Subject: [PATCH 08/14] Added bypassing mechanism. --- .../ad_block_service_browsertest.cc | 15 ++++++++- .../brave_component_contents_verifier.cc | 33 ++++++++++++------- .../brave_component_contents_verifier.h | 6 ++++ .../browser/ui/startup/bad_flags_prompt.cc | 14 ++++++++ .../browser/component_contents_verifier.cc | 2 -- .../browser/component_contents_verifier.h | 1 - 6 files changed, 56 insertions(+), 15 deletions(-) create mode 100644 chromium_src/chrome/browser/ui/startup/bad_flags_prompt.cc diff --git a/browser/brave_shields/ad_block_service_browsertest.cc b/browser/brave_shields/ad_block_service_browsertest.cc index c7e439e6be8c..57e68d278866 100644 --- a/browser/brave_shields/ad_block_service_browsertest.cc +++ b/browser/brave_shields/ad_block_service_browsertest.cc @@ -1638,12 +1638,25 @@ IN_PROC_BROWSER_TEST_F(Default1pBlockingFlagDisabledTest, Custom1pBlocking) { EXPECT_EQ(profile()->GetPrefs()->GetUint64(kAdsBlocked), 2ULL); } +class AdBlockServiceSignedComponentsTest : public AdBlockServiceTest { + public: + AdBlockServiceSignedComponentsTest() { + feature_list_.InitAndEnableFeature( + component_updater::kComponentContentsVerifier); + } + ~AdBlockServiceSignedComponentsTest() override = default; + + private: + base::test::ScopedFeatureList feature_list_; +}; + #if BUILDFLAG(ENABLE_EXTENSIONS) #define MAYBE_SafeResourcesComponentAccess SafeResourcesComponentAccess #else #define MAYBE_SafeResourcesComponentAccess DISABLED_SafeResourcesComponentAccess #endif -IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, MAYBE_SafeResourcesComponentAccess) { +IN_PROC_BROWSER_TEST_F(AdBlockServiceSignedComponentsTest, + MAYBE_SafeResourcesComponentAccess) { const auto components_path = GetTestDataDir().AppendASCII("adblock-components"); { diff --git a/browser/component_updater/brave_component_contents_verifier.cc b/browser/component_updater/brave_component_contents_verifier.cc index c1a14e67c1e2..96cdb68b97fc 100644 --- a/browser/component_updater/brave_component_contents_verifier.cc +++ b/browser/component_updater/brave_component_contents_verifier.cc @@ -11,15 +11,17 @@ #include #include -#include "base/containers/span_reader.h" #include "base/files/file_path.h" #include "base/files/file_util.h" #include "base/functional/bind.h" -#include "crypto/secure_hash.h" -#include "crypto/sha2.h" #include "extensions/buildflags/buildflags.h" #if BUILDFLAG(ENABLE_EXTENSIONS) + +#include "base/command_line.h" +#include "base/containers/span_reader.h" +#include "crypto/secure_hash.h" +#include "crypto/sha2.h" #include "extensions/browser/content_hash_tree.h" #include "extensions/browser/verified_contents.h" @@ -106,18 +108,14 @@ class ComponentContentsAccessorImpl return !!verified_contents_.get(); } - void IgnoreInvalidSignature(bool ignore) override { - ignore_invalid_signature_ = ignore; - } - bool VerifyContents(const base::FilePath& relative_path, base::span contents) override { if (!IsComponentSignatureValid()) { - return ignore_invalid_signature_; + return false; } if (!verified_contents_->HasTreeHashRoot(relative_path)) { - return true; + return false; } const auto root_hash = @@ -129,20 +127,33 @@ class ComponentContentsAccessorImpl ~ComponentContentsAccessorImpl() override = default; std::unique_ptr verified_contents_; - bool ignore_invalid_signature_ = false; }; +bool ShouldBypassSignature() { + static const bool kBypass = + !base::FeatureList::IsEnabled( + component_updater::kComponentContentsVerifier) || + base::CommandLine::ForCurrentProcess()->HasSwitch( + component_updater::kBypassComponentContentsVerifier); + return kBypass; +} + } // namespace #endif // BUILDFLAG(ENABLE_EXTENSIONS) namespace component_updater { +BASE_FEATURE(kComponentContentsVerifier, + "ComponentContentsVerifier", + base::FEATURE_DISABLED_BY_DEFAULT); + scoped_refptr CreateComponentContentsAccessor(bool with_verifier, const base::FilePath& component_root) { #if BUILDFLAG(ENABLE_EXTENSIONS) - if (with_verifier) { + static const bool kBypassSignature = ShouldBypassSignature(); + if (with_verifier && !kBypassSignature) { return base::MakeRefCounted(component_root); } #endif diff --git a/browser/component_updater/brave_component_contents_verifier.h b/browser/component_updater/brave_component_contents_verifier.h index 70e9a2a3f291..4a5f85849ada 100644 --- a/browser/component_updater/brave_component_contents_verifier.h +++ b/browser/component_updater/brave_component_contents_verifier.h @@ -6,6 +6,7 @@ #ifndef BRAVE_BROWSER_COMPONENT_UPDATER_BRAVE_COMPONENT_CONTENTS_VERIFIER_H_ #define BRAVE_BROWSER_COMPONENT_UPDATER_BRAVE_COMPONENT_CONTENTS_VERIFIER_H_ +#include "base/feature_list.h" #include "base/memory/scoped_refptr.h" #include "brave/components/brave_component_updater/browser/component_contents_verifier.h" @@ -15,6 +16,11 @@ class FilePath; namespace component_updater { +BASE_DECLARE_FEATURE(kComponentContentsVerifier); + +inline constexpr char kBypassComponentContentsVerifier[] = + "bypass-component-contents-verifier"; + scoped_refptr CreateComponentContentsAccessor(bool with_verifier, const base::FilePath& component_root); diff --git a/chromium_src/chrome/browser/ui/startup/bad_flags_prompt.cc b/chromium_src/chrome/browser/ui/startup/bad_flags_prompt.cc new file mode 100644 index 000000000000..b897697bb5a2 --- /dev/null +++ b/chromium_src/chrome/browser/ui/startup/bad_flags_prompt.cc @@ -0,0 +1,14 @@ +// Copyright (c) 2024 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +#include "brave/browser/component_updater/brave_component_contents_verifier.h" +#include "services/network/public/cpp/network_switches.h" + +#define kHostResolverRules \ + kHostResolverRules, component_updater::kBypassComponentContentsVerifier + +#include "src/chrome/browser/ui/startup/bad_flags_prompt.cc" + +#undef kHostResolverRules diff --git a/components/brave_component_updater/browser/component_contents_verifier.cc b/components/brave_component_updater/browser/component_contents_verifier.cc index 9a43160dc99e..2ed047b1157c 100644 --- a/components/brave_component_updater/browser/component_contents_verifier.cc +++ b/components/brave_component_updater/browser/component_contents_verifier.cc @@ -23,8 +23,6 @@ bool ComponentContentsAccessor::IsComponentSignatureValid() const { return true; } -void ComponentContentsAccessor::IgnoreInvalidSignature(bool ignore) {} - bool ComponentContentsAccessor::VerifyContents( const base::FilePath& relative_path, base::span contents) { diff --git a/components/brave_component_updater/browser/component_contents_verifier.h b/components/brave_component_updater/browser/component_contents_verifier.h index ad83d2475790..e0d8faa14435 100644 --- a/components/brave_component_updater/browser/component_contents_verifier.h +++ b/components/brave_component_updater/browser/component_contents_verifier.h @@ -33,7 +33,6 @@ class ComponentContentsAccessor ComponentContentsAccessor& operator=(ComponentContentsAccessor&&) = delete; virtual bool IsComponentSignatureValid() const; - virtual void IgnoreInvalidSignature(bool ignore); virtual bool VerifyContents(const base::FilePath& relative_path, base::span contents); From 678e074e8bb0f4c13f9bab33af1f571c068a97b5 Mon Sep 17 00:00:00 2001 From: boocmp Date: Thu, 5 Dec 2024 17:03:55 +0700 Subject: [PATCH 09/14] Moved accsessor creation to VerifyIntalltion stage. --- .../browser/component_contents_verifier.cc | 11 +++----- .../browser/component_contents_verifier.h | 7 +++-- .../browser/ad_block_component_installer.cc | 26 ++++++++++--------- 3 files changed, 21 insertions(+), 23 deletions(-) diff --git a/components/brave_component_updater/browser/component_contents_verifier.cc b/components/brave_component_updater/browser/component_contents_verifier.cc index 2ed047b1157c..d2c288725e29 100644 --- a/components/brave_component_updater/browser/component_contents_verifier.cc +++ b/components/brave_component_updater/browser/component_contents_verifier.cc @@ -78,14 +78,11 @@ ComponentContentsVerifier* ComponentContentsVerifier::GetInstance() { return instance.get(); } -void ComponentContentsVerifier::CreateContentsAccessor( - const base::FilePath& component_root, - base::OnceCallback)> - on_created) { +scoped_refptr +ComponentContentsVerifier::CreateContentsAccessor( + const base::FilePath& component_root) { CHECK(cca_factory_); - base::ThreadPool::PostTaskAndReplyWithResult( - FROM_HERE, {base::MayBlock()}, - base::BindOnce(cca_factory_, component_root), std::move(on_created)); + return cca_factory_.Run(component_root); } } // namespace brave_component_updater diff --git a/components/brave_component_updater/browser/component_contents_verifier.h b/components/brave_component_updater/browser/component_contents_verifier.h index e0d8faa14435..2232f9d3074d 100644 --- a/components/brave_component_updater/browser/component_contents_verifier.h +++ b/components/brave_component_updater/browser/component_contents_verifier.h @@ -59,10 +59,9 @@ class ComponentContentsVerifier { static void Setup(ComponentContentsAccessorFactory cca_factory); static ComponentContentsVerifier* GetInstance(); - void CreateContentsAccessor( - const base::FilePath& component_root, - base::OnceCallback)> - on_created); + // Should be called on MAY_BLOCK sequence. + scoped_refptr CreateContentsAccessor( + const base::FilePath& component_root); private: friend base::NoDestructor; diff --git a/components/brave_shields/core/browser/ad_block_component_installer.cc b/components/brave_shields/core/browser/ad_block_component_installer.cc index d9a39aceb1d5..c076c83dad00 100644 --- a/components/brave_shields/core/browser/ad_block_component_installer.cc +++ b/components/brave_shields/core/browser/ad_block_component_installer.cc @@ -22,12 +22,16 @@ using brave_component_updater::BraveOnDemandUpdater; +namespace brave_component_updater { +class ComponentContentsAccessor; +} + namespace brave_shields { namespace { -using OnComponentReadyCallback = - base::RepeatingCallback; +using OnComponentReadyCallback = base::RepeatingCallback)>; constexpr size_t kHashSize = 32; @@ -69,6 +73,8 @@ class AdBlockComponentInstallerPolicy const std::string component_name_; OnComponentReadyCallback ready_callback_; uint8_t component_hash_[kHashSize]; + mutable scoped_refptr + accessor_; }; AdBlockComponentInstallerPolicy::AdBlockComponentInstallerPolicy( @@ -109,12 +115,16 @@ void AdBlockComponentInstallerPolicy::ComponentReady( const base::Version& version, const base::FilePath& path, base::Value::Dict manifest) { - ready_callback_.Run(path); + CHECK(accessor_); + CHECK_EQ(accessor_->GetComponentRoot(), path); + ready_callback_.Run(std::move(accessor_)); } bool AdBlockComponentInstallerPolicy::VerifyInstallation( const base::Value::Dict& manifest, const base::FilePath& install_dir) const { + accessor_ = brave_component_updater::ComponentContentsVerifier::GetInstance() + ->CreateContentsAccessor(install_dir); return true; } @@ -158,18 +168,10 @@ void RegisterAdBlockComponentInternal( if (!cus) { return; } - - auto on_ready = base::BindRepeating( - [](OnSecureComponentReadyCallback callback, const base::FilePath& path) { - brave_component_updater::ComponentContentsVerifier::GetInstance() - ->CreateContentsAccessor(path, std::move(callback)); - }, - std::move(callback)); - auto installer = base::MakeRefCounted( std::make_unique( component_public_key, component_id, component_name, - std::move(on_ready))); + std::move(callback))); installer->Register(cus, base::BindOnce(&OnRegistered, component_id)); } From e5d062005c88e239de2110169cd3111800bb6f0c Mon Sep 17 00:00:00 2001 From: boocmp Date: Thu, 12 Dec 2024 17:27:42 +0700 Subject: [PATCH 10/14] Review issues are addressed. --- .../ad_block_service_browsertest.cc | 31 +++------ .../brave_component_contents_verifier.cc | 62 ++++++----------- .../brave_component_contents_verifier.h | 10 --- .../browser/component_contents_verifier.cc | 69 +++++++++---------- .../browser/component_contents_verifier.h | 51 +++++--------- .../ad_block_component_filters_provider.cc | 5 +- .../ad_block_component_filters_provider.h | 10 +-- .../browser/ad_block_component_installer.cc | 8 +-- .../browser/ad_block_component_installer.h | 2 +- .../ad_block_default_resource_provider.cc | 5 +- .../ad_block_default_resource_provider.h | 10 +-- .../ad_block_filter_list_catalog_provider.cc | 5 +- .../ad_block_filter_list_catalog_provider.h | 10 +-- 13 files changed, 100 insertions(+), 178 deletions(-) diff --git a/browser/brave_shields/ad_block_service_browsertest.cc b/browser/brave_shields/ad_block_service_browsertest.cc index 57e68d278866..8bb32c09b684 100644 --- a/browser/brave_shields/ad_block_service_browsertest.cc +++ b/browser/brave_shields/ad_block_service_browsertest.cc @@ -27,6 +27,7 @@ #include "brave/browser/brave_browser_process.h" #include "brave/browser/component_updater/brave_component_contents_verifier.h" #include "brave/browser/net/brave_ad_block_tp_network_delegate_helper.h" +#include "brave/components/brave_component_updater/browser/component_contents_verifier.h" #include "brave/components/brave_shields/content/browser/ad_block_custom_filters_provider.h" #include "brave/components/brave_shields/content/browser/ad_block_engine.h" #include "brave/components/brave_shields/content/browser/ad_block_service.h" @@ -255,21 +256,10 @@ AdBlockServiceTest::IntsallDefaultAdBlockResources( auto* resource_provider = static_cast( service->resource_provider()); - base::RunLoop run_loop; - base::ThreadPool::PostTaskAndReplyWithResult( - FROM_HERE, {base::MayBlock()}, - base::BindLambdaForTesting([&component_path]() { - return component_updater::CreateComponentContentsAccessor( - true, component_path); - }), - base::BindLambdaForTesting( - [resource_provider, &run_loop]( - scoped_refptr - accessor) { - resource_provider->OnComponentReady(std::move(accessor)); - run_loop.Quit(); - })); - run_loop.Run(); + + base::ScopedAllowBlockingForTesting allow_blocking; + resource_provider->OnComponentReady( + component_updater::ComponentContentsAccessor::Create(component_path)); return resource_provider; } @@ -281,8 +271,8 @@ void AdBlockServiceTest::UpdateAdBlockResources(const std::string& resources) { static_cast( service->resource_provider()) - ->OnComponentReady(component_updater::CreateComponentContentsAccessor( - false, component_path)); + ->OnComponentReady( + component_updater::ComponentContentsAccessor::Create(component_path)); } void AdBlockServiceTest::UpdateAdBlockInstanceWithRules( @@ -298,8 +288,8 @@ void AdBlockServiceTest::UpdateAdBlockInstanceWithRules( std::string uuid = "default"; auto& provider = component_providers.at(uuid); EXPECT_TRUE(provider); - provider->OnComponentReady(component_updater::CreateComponentContentsAccessor( - false, component_path)); + provider->OnComponentReady( + component_updater::ComponentContentsAccessor::Create(component_path)); auto* engine = service->default_engine_.get(); EngineTestObserver engine_observer(engine); @@ -375,8 +365,7 @@ void AdBlockServiceTest::InstallComponent( auto& provider = component_providers.at(catalog_entry.uuid); EXPECT_TRUE(provider); provider->OnComponentReady( - component_updater::CreateComponentContentsAccessor(false, - component_path)); + component_updater::ComponentContentsAccessor::Create(component_path)); auto* engine = catalog_entry.first_party_protections ? service->default_engine_.get() diff --git a/browser/component_updater/brave_component_contents_verifier.cc b/browser/component_updater/brave_component_contents_verifier.cc index 96cdb68b97fc..b23797b4fc13 100644 --- a/browser/component_updater/brave_component_contents_verifier.cc +++ b/browser/component_updater/brave_component_contents_verifier.cc @@ -14,13 +14,13 @@ #include "base/files/file_path.h" #include "base/files/file_util.h" #include "base/functional/bind.h" +#include "brave/components/brave_component_updater/browser/component_contents_verifier.h" #include "extensions/buildflags/buildflags.h" #if BUILDFLAG(ENABLE_EXTENSIONS) #include "base/command_line.h" #include "base/containers/span_reader.h" -#include "crypto/secure_hash.h" #include "crypto/sha2.h" #include "extensions/browser/content_hash_tree.h" #include "extensions/browser/verified_contents.h" @@ -58,35 +58,27 @@ constexpr char kVerifiedContentsPath[] = "_metadata/verified_contents.json"; constexpr char kBraveVerifiedContentsPath[] = "brave_metadata/verified_contents.json"; -std::string GetRootHasheForContent(base::span contents, - size_t block_size) { +std::string GetRootHashForContent(base::span contents, + size_t block_size) { + CHECK(block_size % crypto::kSHA256Length == 0); + std::vector hashes; // Even when the contents is empty, we want to output at least one hash // block (the hash of the empty string). base::SpanReader reader(contents); do { - auto chunk = reader.Read(std::min(block_size, reader.remaining())); - std::unique_ptr hash( - crypto::SecureHash::Create(crypto::SecureHash::SHA256)); - hash->Update(chunk.value()); - - std::string buffer; - buffer.resize(crypto::kSHA256Length); - hash->Finish(base::as_writable_byte_span(buffer)); - hashes.push_back(std::move(buffer)); + const auto chunk = reader.Read(std::min(block_size, reader.remaining())); + const auto hash = crypto::SHA256Hash(chunk.value()); + hashes.emplace_back(hash.begin(), hash.end()); } while (reader.remaining() > 0); - CHECK(block_size % crypto::kSHA256Length == 0); return extensions::ComputeTreeHashRoot(hashes, block_size / crypto::kSHA256Length); } -// Proxies the file access and checks the verified_contents.json. -class ComponentContentsAccessorImpl - : public brave_component_updater::ComponentContentsAccessor { +class SecureContentsVerifier : public component_updater::ContentsVerifier { public: - explicit ComponentContentsAccessorImpl(const base::FilePath& component_root) - : brave_component_updater::ComponentContentsAccessor(component_root) { + explicit SecureContentsVerifier(const base::FilePath& component_root) { if (base::PathExists( component_root.AppendASCII(kBraveVerifiedContentsPath))) { verified_contents_ = extensions::VerifiedContents::CreateFromFile( @@ -104,28 +96,19 @@ class ComponentContentsAccessorImpl } } - bool IsComponentSignatureValid() const override { - return !!verified_contents_.get(); - } - bool VerifyContents(const base::FilePath& relative_path, base::span contents) override { - if (!IsComponentSignatureValid()) { - return false; - } - - if (!verified_contents_->HasTreeHashRoot(relative_path)) { + if (!verified_contents_ || + !verified_contents_->HasTreeHashRoot(relative_path)) { return false; } const auto root_hash = - GetRootHasheForContent(contents, verified_contents_->block_size()); + GetRootHashForContent(contents, verified_contents_->block_size()); return verified_contents_->TreeHashRootEquals(relative_path, root_hash); } private: - ~ComponentContentsAccessorImpl() override = default; - std::unique_ptr verified_contents_; }; @@ -139,7 +122,6 @@ bool ShouldBypassSignature() { } } // namespace - #endif // BUILDFLAG(ENABLE_EXTENSIONS) namespace component_updater { @@ -148,24 +130,22 @@ BASE_FEATURE(kComponentContentsVerifier, "ComponentContentsVerifier", base::FEATURE_DISABLED_BY_DEFAULT); -scoped_refptr -CreateComponentContentsAccessor(bool with_verifier, - const base::FilePath& component_root) { +std::unique_ptr CreateContentsVerifier( + const base::FilePath& component_root) { #if BUILDFLAG(ENABLE_EXTENSIONS) - static const bool kBypassSignature = ShouldBypassSignature(); - if (with_verifier && !kBypassSignature) { - return base::MakeRefCounted(component_root); + const bool bypass = ShouldBypassSignature(); + if (!bypass) { + return std::make_unique(component_root); } #endif // if there is no extensions enabled then we expect that on these platforms // the component files are protected by the OS. - return base::MakeRefCounted< - brave_component_updater::ComponentContentsAccessor>(component_root); + return std::make_unique(); } void SetupComponentContentsVerifier() { - auto factory = base::BindRepeating(CreateComponentContentsAccessor, true); - brave_component_updater::ComponentContentsVerifier::Setup(std::move(factory)); + auto factory = base::BindRepeating(CreateContentsVerifier); + SetupContentsVerifierFactory(std::move(factory)); } } // namespace component_updater diff --git a/browser/component_updater/brave_component_contents_verifier.h b/browser/component_updater/brave_component_contents_verifier.h index 4a5f85849ada..f0d42a8b9fb9 100644 --- a/browser/component_updater/brave_component_contents_verifier.h +++ b/browser/component_updater/brave_component_contents_verifier.h @@ -7,12 +7,6 @@ #define BRAVE_BROWSER_COMPONENT_UPDATER_BRAVE_COMPONENT_CONTENTS_VERIFIER_H_ #include "base/feature_list.h" -#include "base/memory/scoped_refptr.h" -#include "brave/components/brave_component_updater/browser/component_contents_verifier.h" - -namespace base { -class FilePath; -} namespace component_updater { @@ -21,10 +15,6 @@ BASE_DECLARE_FEATURE(kComponentContentsVerifier); inline constexpr char kBypassComponentContentsVerifier[] = "bypass-component-contents-verifier"; -scoped_refptr -CreateComponentContentsAccessor(bool with_verifier, - const base::FilePath& component_root); - void SetupComponentContentsVerifier(); } // namespace component_updater diff --git a/components/brave_component_updater/browser/component_contents_verifier.cc b/components/brave_component_updater/browser/component_contents_verifier.cc index d2c288725e29..4bf3402db9d6 100644 --- a/components/brave_component_updater/browser/component_contents_verifier.cc +++ b/components/brave_component_updater/browser/component_contents_verifier.cc @@ -11,24 +11,41 @@ #include "base/check_is_test.h" #include "base/files/file_util.h" #include "base/no_destructor.h" -#include "base/task/thread_pool.h" -namespace brave_component_updater { +namespace component_updater { -ComponentContentsAccessor::ComponentContentsAccessor( - const base::FilePath component_root) - : component_root_(component_root) {} +namespace { +base::NoDestructor g_verifier_factory; +} -bool ComponentContentsAccessor::IsComponentSignatureValid() const { - return true; +// static +void SetupContentsVerifierFactory(ContentsVerifierFactory factory) { + *g_verifier_factory = std::move(factory); } -bool ComponentContentsAccessor::VerifyContents( - const base::FilePath& relative_path, - base::span contents) { +bool ContentsVerifier::VerifyContents(const base::FilePath& relative_path, + base::span contents) { return true; } +ComponentContentsAccessor::ComponentContentsAccessor( + const base::FilePath& component_root) + : component_root_(component_root), + verifier_(*g_verifier_factory ? g_verifier_factory->Run(component_root) + : std::make_unique()) { + if (!*g_verifier_factory) { + CHECK_IS_TEST(); + } +} + +ComponentContentsAccessor::~ComponentContentsAccessor() = default; + +// static +scoped_refptr ComponentContentsAccessor::Create( + const base::FilePath& component_root) { + return base::WrapRefCounted(new ComponentContentsAccessor(component_root)); +} + const base::FilePath& ComponentContentsAccessor::GetComponentRoot() const { return component_root_; } @@ -40,7 +57,7 @@ std::optional ComponentContentsAccessor::GetFileAsString( &contents)) { return std::nullopt; } - if (!VerifyContents(relative_path, base::as_byte_span(contents))) { + if (!verifier_->VerifyContents(relative_path, base::as_byte_span(contents))) { return std::nullopt; } @@ -54,35 +71,11 @@ std::optional> ComponentContentsAccessor::GetFileAsBytes( if (!contents) { return std::nullopt; } - if (!VerifyContents(relative_path, base::as_byte_span(*contents))) { + if (!verifier_->VerifyContents(relative_path, + base::as_byte_span(*contents))) { return std::nullopt; } return std::move(*contents); } -ComponentContentsVerifier::ComponentContentsVerifier() = default; -ComponentContentsVerifier::~ComponentContentsVerifier() = default; - -// static -void ComponentContentsVerifier::Setup( - ComponentContentsVerifier::ComponentContentsAccessorFactory cca_factory) { - if (GetInstance()->cca_factory_) { - CHECK_IS_TEST(); - } - GetInstance()->cca_factory_ = std::move(cca_factory); -} - -// static -ComponentContentsVerifier* ComponentContentsVerifier::GetInstance() { - static base::NoDestructor instance; - return instance.get(); -} - -scoped_refptr -ComponentContentsVerifier::CreateContentsAccessor( - const base::FilePath& component_root) { - CHECK(cca_factory_); - return cca_factory_.Run(component_root); -} - -} // namespace brave_component_updater +} // namespace component_updater diff --git a/components/brave_component_updater/browser/component_contents_verifier.h b/components/brave_component_updater/browser/component_contents_verifier.h index 2232f9d3074d..8a522fac5ea9 100644 --- a/components/brave_component_updater/browser/component_contents_verifier.h +++ b/components/brave_component_updater/browser/component_contents_verifier.h @@ -6,6 +6,7 @@ #ifndef BRAVE_COMPONENTS_BRAVE_COMPONENT_UPDATER_BROWSER_COMPONENT_CONTENTS_VERIFIER_H_ #define BRAVE_COMPONENTS_BRAVE_COMPONENT_UPDATER_BROWSER_COMPONENT_CONTENTS_VERIFIER_H_ +#include #include #include #include @@ -14,27 +15,27 @@ #include "base/functional/callback.h" #include "base/memory/ref_counted.h" -namespace base { -template -class NoDestructor; -} // namespace base +namespace component_updater { -namespace brave_component_updater { +class ContentsVerifier { + public: + virtual ~ContentsVerifier() = default; + virtual bool VerifyContents(const base::FilePath& relative_path, + base::span contents); +}; // Use on MAY_BLOCK sequence. class ComponentContentsAccessor : public base::RefCountedThreadSafe { public: - explicit ComponentContentsAccessor(const base::FilePath component_root); ComponentContentsAccessor(const ComponentContentsAccessor&) = delete; ComponentContentsAccessor(ComponentContentsAccessor&&) = delete; ComponentContentsAccessor& operator=(const ComponentContentsAccessor&) = delete; ComponentContentsAccessor& operator=(ComponentContentsAccessor&&) = delete; - virtual bool IsComponentSignatureValid() const; - virtual bool VerifyContents(const base::FilePath& relative_path, - base::span contents); + static scoped_refptr Create( + const base::FilePath& component_root); const base::FilePath& GetComponentRoot() const; @@ -43,35 +44,21 @@ class ComponentContentsAccessor std::optional> GetFileAsBytes( const base::FilePath& relative_path); - protected: + private: friend class base::RefCountedThreadSafe; - virtual ~ComponentContentsAccessor() = default; + explicit ComponentContentsAccessor(const base::FilePath& component_root); + ~ComponentContentsAccessor(); const base::FilePath component_root_; + std::unique_ptr verifier_; }; -class ComponentContentsVerifier { - public: - using ComponentContentsAccessorFactory = - base::RepeatingCallback( - const base::FilePath& component_root)>; - - static void Setup(ComponentContentsAccessorFactory cca_factory); - static ComponentContentsVerifier* GetInstance(); - - // Should be called on MAY_BLOCK sequence. - scoped_refptr CreateContentsAccessor( - const base::FilePath& component_root); +using ContentsVerifierFactory = + base::RepeatingCallback( + const base::FilePath& component_root)>; - private: - friend base::NoDestructor; - - ComponentContentsAccessorFactory cca_factory_; - - ComponentContentsVerifier(); - ~ComponentContentsVerifier(); -}; +void SetupContentsVerifierFactory(ContentsVerifierFactory factory); -} // namespace brave_component_updater +} // namespace component_updater #endif // BRAVE_COMPONENTS_BRAVE_COMPONENT_UPDATER_BROWSER_COMPONENT_CONTENTS_VERIFIER_H_ diff --git a/components/brave_shields/core/browser/ad_block_component_filters_provider.cc b/components/brave_shields/core/browser/ad_block_component_filters_provider.cc index b9c9ff611203..6d8b08e044be 100644 --- a/components/brave_shields/core/browser/ad_block_component_filters_provider.cc +++ b/components/brave_shields/core/browser/ad_block_component_filters_provider.cc @@ -88,8 +88,7 @@ void AdBlockComponentFiltersProvider::UnregisterComponent() { } void AdBlockComponentFiltersProvider::OnComponentReady( - scoped_refptr - accessor) { + scoped_refptr accessor) { if (component_accessor_) { base::ThreadPool::PostTask( FROM_HERE, {base::TaskPriority::BEST_EFFORT, base::MayBlock()}, @@ -127,7 +126,7 @@ void AdBlockComponentFiltersProvider::LoadFilterSet( base::ThreadPool::PostTaskAndReplyWithResult( FROM_HERE, {base::MayBlock()}, base::BindOnce( - &brave_component_updater::ComponentContentsAccessor::GetFileAsBytes, + &component_updater::ComponentContentsAccessor::GetFileAsBytes, base::RetainedRef(component_accessor_), base::FilePath::FromASCII(kListFile)), base::BindOnce(&OnReadDATFileData, std::move(cb), permission_mask_)); diff --git a/components/brave_shields/core/browser/ad_block_component_filters_provider.h b/components/brave_shields/core/browser/ad_block_component_filters_provider.h index b17026c1e68a..feedde499a7c 100644 --- a/components/brave_shields/core/browser/ad_block_component_filters_provider.h +++ b/components/brave_shields/core/browser/ad_block_component_filters_provider.h @@ -17,11 +17,8 @@ using brave_component_updater::DATFileDataBuffer; namespace component_updater { class ComponentUpdateService; -} // namespace component_updater - -namespace brave_component_updater { class ComponentContentsAccessor; -} +} // namespace component_updater namespace base { class FilePath; @@ -78,10 +75,9 @@ class AdBlockComponentFiltersProvider : public AdBlockFiltersProvider { friend class ::DebounceBrowserTest; void OnComponentReady( - scoped_refptr - accessor); + scoped_refptr accessor); - scoped_refptr + scoped_refptr component_accessor_; std::string component_id_; diff --git a/components/brave_shields/core/browser/ad_block_component_installer.cc b/components/brave_shields/core/browser/ad_block_component_installer.cc index c076c83dad00..723358c0212a 100644 --- a/components/brave_shields/core/browser/ad_block_component_installer.cc +++ b/components/brave_shields/core/browser/ad_block_component_installer.cc @@ -31,7 +31,7 @@ namespace brave_shields { namespace { using OnComponentReadyCallback = base::RepeatingCallback)>; + scoped_refptr)>; constexpr size_t kHashSize = 32; @@ -73,8 +73,7 @@ class AdBlockComponentInstallerPolicy const std::string component_name_; OnComponentReadyCallback ready_callback_; uint8_t component_hash_[kHashSize]; - mutable scoped_refptr - accessor_; + mutable scoped_refptr accessor_; }; AdBlockComponentInstallerPolicy::AdBlockComponentInstallerPolicy( @@ -123,8 +122,7 @@ void AdBlockComponentInstallerPolicy::ComponentReady( bool AdBlockComponentInstallerPolicy::VerifyInstallation( const base::Value::Dict& manifest, const base::FilePath& install_dir) const { - accessor_ = brave_component_updater::ComponentContentsVerifier::GetInstance() - ->CreateContentsAccessor(install_dir); + accessor_ = component_updater::ComponentContentsAccessor::Create(install_dir); return true; } diff --git a/components/brave_shields/core/browser/ad_block_component_installer.h b/components/brave_shields/core/browser/ad_block_component_installer.h index 40dc6b885f70..bca6f5860ad7 100644 --- a/components/brave_shields/core/browser/ad_block_component_installer.h +++ b/components/brave_shields/core/browser/ad_block_component_installer.h @@ -18,7 +18,7 @@ class ComponentUpdateService; namespace brave_shields { using OnSecureComponentReadyCallback = base::RepeatingCallback)>; + scoped_refptr)>; void RegisterAdBlockDefaultResourceComponent( component_updater::ComponentUpdateService* cus, diff --git a/components/brave_shields/core/browser/ad_block_default_resource_provider.cc b/components/brave_shields/core/browser/ad_block_default_resource_provider.cc index 35413706891b..b52313b83262 100644 --- a/components/brave_shields/core/browser/ad_block_default_resource_provider.cc +++ b/components/brave_shields/core/browser/ad_block_default_resource_provider.cc @@ -46,8 +46,7 @@ base::FilePath AdBlockDefaultResourceProvider::GetResourcesPath() { } void AdBlockDefaultResourceProvider::OnComponentReady( - scoped_refptr - accessor) { + scoped_refptr accessor) { component_accessor_ = std::move(accessor); // Load the resources (as a string) @@ -75,7 +74,7 @@ void AdBlockDefaultResourceProvider::LoadResources( base::ThreadPool::PostTaskAndReplyWithResult( FROM_HERE, {base::MayBlock()}, base::BindOnce( - &brave_component_updater::ComponentContentsAccessor::GetFileAsString, + &component_updater::ComponentContentsAccessor::GetFileAsString, base::RetainedRef(component_accessor_), base::FilePath::FromASCII(kAdBlockResourcesFilename)), std::move(handle_file_content)); diff --git a/components/brave_shields/core/browser/ad_block_default_resource_provider.h b/components/brave_shields/core/browser/ad_block_default_resource_provider.h index d82437109f50..0ada3af56333 100644 --- a/components/brave_shields/core/browser/ad_block_default_resource_provider.h +++ b/components/brave_shields/core/browser/ad_block_default_resource_provider.h @@ -13,11 +13,8 @@ namespace component_updater { class ComponentUpdateService; -} // namespace component_updater - -namespace brave_component_updater { class ComponentContentsAccessor; -} +} // namespace component_updater namespace base { class FilePath; @@ -47,10 +44,9 @@ class AdBlockDefaultResourceProvider : public AdBlockResourceProvider { friend class ::AdBlockServiceTest; void OnComponentReady( - scoped_refptr - accessor); + scoped_refptr accessor); - scoped_refptr + scoped_refptr component_accessor_; base::WeakPtrFactory weak_factory_{this}; diff --git a/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.cc b/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.cc index 930c0efecfca..a1256d9843ba 100644 --- a/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.cc +++ b/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.cc @@ -48,8 +48,7 @@ void AdBlockFilterListCatalogProvider::OnFilterListCatalogLoaded( } void AdBlockFilterListCatalogProvider::OnComponentReady( - scoped_refptr - accessor) { + scoped_refptr accessor) { component_accessor_ = std::move(accessor); LoadFilterListCatalog(base::BindOnce( @@ -75,7 +74,7 @@ void AdBlockFilterListCatalogProvider::LoadFilterListCatalog( base::ThreadPool::PostTaskAndReplyWithResult( FROM_HERE, {base::MayBlock()}, base::BindOnce( - &brave_component_updater::ComponentContentsAccessor::GetFileAsString, + &component_updater::ComponentContentsAccessor::GetFileAsString, base::RetainedRef(component_accessor_), base::FilePath::FromASCII(kListCatalogFile)), std::move(on_load)); diff --git a/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.h b/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.h index 5ae3719a01d0..3c0a518a4aac 100644 --- a/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.h +++ b/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.h @@ -14,11 +14,8 @@ namespace component_updater { class ComponentUpdateService; -} // namespace component_updater - -namespace brave_component_updater { class ComponentContentsAccessor; -} +} // namespace component_updater namespace brave_shields { @@ -46,10 +43,9 @@ class AdBlockFilterListCatalogProvider { private: void OnFilterListCatalogLoaded(const std::string& catalog_json); void OnComponentReady( - scoped_refptr - accessor); + scoped_refptr accessor); - scoped_refptr + scoped_refptr component_accessor_; base::ObserverList observers_; From 4aca6a91dc9b1f335a2c282553968481a48ba930 Mon Sep 17 00:00:00 2001 From: boocmp Date: Fri, 13 Dec 2024 22:22:23 +0700 Subject: [PATCH 11/14] Review. --- .../ad_block_service_browsertest.cc | 39 +++++------ .../ad_block_service_browsertest.h | 2 +- .../brave_component_contents_verifier.cc | 17 ++--- .../brave_component_updater/browser/BUILD.gn | 2 + .../browser/component_contents_accessor.cc | 62 +++++++++++++++++ .../browser/component_contents_accessor.h | 50 ++++++++++++++ .../browser/component_contents_verifier.cc | 66 ++++--------------- .../browser/component_contents_verifier.h | 39 ++--------- .../content/browser/ad_block_service.h | 2 - .../ad_block_component_filters_provider.cc | 12 ++-- .../browser/ad_block_component_installer.cc | 17 ++--- .../browser/ad_block_component_installer.h | 10 +-- .../ad_block_default_resource_provider.cc | 8 +-- .../ad_block_filter_list_catalog_provider.cc | 8 +-- .../_metadata/verified_contents.json | 2 +- .../resources_corrupted/resources.json | 2 +- .../_metadata/verified_contents.json | 2 +- .../resources_ok/resources.json | 2 +- 18 files changed, 186 insertions(+), 156 deletions(-) create mode 100644 components/brave_component_updater/browser/component_contents_accessor.cc create mode 100644 components/brave_component_updater/browser/component_contents_accessor.h diff --git a/browser/brave_shields/ad_block_service_browsertest.cc b/browser/brave_shields/ad_block_service_browsertest.cc index 8bb32c09b684..35a2f1237365 100644 --- a/browser/brave_shields/ad_block_service_browsertest.cc +++ b/browser/brave_shields/ad_block_service_browsertest.cc @@ -27,7 +27,7 @@ #include "brave/browser/brave_browser_process.h" #include "brave/browser/component_updater/brave_component_contents_verifier.h" #include "brave/browser/net/brave_ad_block_tp_network_delegate_helper.h" -#include "brave/components/brave_component_updater/browser/component_contents_verifier.h" +#include "brave/components/brave_component_updater/browser/component_contents_accessor.h" #include "brave/components/brave_shields/content/browser/ad_block_custom_filters_provider.h" #include "brave/components/brave_shields/content/browser/ad_block_engine.h" #include "brave/components/brave_shields/content/browser/ad_block_service.h" @@ -249,18 +249,14 @@ base::FilePath AdBlockServiceTest::MakeTestDataCopy( } brave_shields::AdBlockResourceProvider* -AdBlockServiceTest::IntsallDefaultAdBlockResources( +AdBlockServiceTest::InstallDefaultAdBlockResources( const base::FilePath& component_path) { brave_shields::AdBlockService* service = g_brave_browser_process->ad_block_service(); - auto* resource_provider = - static_cast( - service->resource_provider()); - base::ScopedAllowBlockingForTesting allow_blocking; - resource_provider->OnComponentReady( + service->default_resource_provider()->OnComponentReady( component_updater::ComponentContentsAccessor::Create(component_path)); - return resource_provider; + return service->default_resource_provider(); } void AdBlockServiceTest::UpdateAdBlockResources(const std::string& resources) { @@ -269,10 +265,8 @@ void AdBlockServiceTest::UpdateAdBlockResources(const std::string& resources) { brave_shields::AdBlockService* service = g_brave_browser_process->ad_block_service(); - static_cast( - service->resource_provider()) - ->OnComponentReady( - component_updater::ComponentContentsAccessor::Create(component_path)); + service->default_resource_provider()->OnComponentReady( + component_updater::ComponentContentsAccessor::Create(component_path)); } void AdBlockServiceTest::UpdateAdBlockInstanceWithRules( @@ -1639,24 +1633,19 @@ class AdBlockServiceSignedComponentsTest : public AdBlockServiceTest { base::test::ScopedFeatureList feature_list_; }; -#if BUILDFLAG(ENABLE_EXTENSIONS) -#define MAYBE_SafeResourcesComponentAccess SafeResourcesComponentAccess -#else -#define MAYBE_SafeResourcesComponentAccess DISABLED_SafeResourcesComponentAccess -#endif IN_PROC_BROWSER_TEST_F(AdBlockServiceSignedComponentsTest, - MAYBE_SafeResourcesComponentAccess) { + SafeResourcesComponentAccess) { const auto components_path = GetTestDataDir().AppendASCII("adblock-components"); { - auto* resource_provider = IntsallDefaultAdBlockResources( + auto* resource_provider = InstallDefaultAdBlockResources( components_path.AppendASCII("resources_ok")); base::RunLoop run_loop; resource_provider->LoadResources( base::BindLambdaForTesting([&run_loop](const std::string& resources) { constexpr const char kExpectedResources[] = "[{\"name\":\"brave-fix.js\",\"aliases\":[],\"kind\":{\"mime\":" - "\"application/javascript\"}]"; + "\"application/javascript\"}}]"; EXPECT_EQ(kExpectedResources, resources); run_loop.Quit(); })); @@ -1664,12 +1653,20 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceSignedComponentsTest, } { - auto* resource_provider = IntsallDefaultAdBlockResources( + auto* resource_provider = InstallDefaultAdBlockResources( components_path.AppendASCII("resources_corrupted")); base::RunLoop run_loop; resource_provider->LoadResources( base::BindLambdaForTesting([&run_loop](const std::string& resources) { +#if BUILDFLAG(ENABLE_EXTENSIONS) constexpr const char kExpectedResources[] = "[]"; +#else + // In case of extensions are disabled we don't check the signature. + constexpr const char kExpectedResources[] = + "[{\"name\":\"edited.js\",\"aliases\":[],\"kind\":{\"mime\":" + "\"application/javascript\"}}]"; +#endif + EXPECT_EQ(kExpectedResources, resources); run_loop.Quit(); })); diff --git a/browser/brave_shields/ad_block_service_browsertest.h b/browser/brave_shields/ad_block_service_browsertest.h index 5f7af1e8ceca..be4b476c7644 100644 --- a/browser/brave_shields/ad_block_service_browsertest.h +++ b/browser/brave_shields/ad_block_service_browsertest.h @@ -48,7 +48,7 @@ class AdBlockServiceTest : public PlatformBrowserTest { void AddNewRules(const std::string& rules, uint8_t permission_mask = 0, bool first_party_protections = false); - brave_shields::AdBlockResourceProvider* IntsallDefaultAdBlockResources( + brave_shields::AdBlockResourceProvider* InstallDefaultAdBlockResources( const base::FilePath& component_path); void UpdateAdBlockResources(const std::string& resources); void UpdateAdBlockInstanceWithRules(const std::string& rules); diff --git a/browser/component_updater/brave_component_contents_verifier.cc b/browser/component_updater/brave_component_contents_verifier.cc index b23797b4fc13..b62fdc03653b 100644 --- a/browser/component_updater/brave_component_contents_verifier.cc +++ b/browser/component_updater/brave_component_contents_verifier.cc @@ -76,9 +76,11 @@ std::string GetRootHashForContent(base::span contents, block_size / crypto::kSHA256Length); } -class SecureContentsVerifier : public component_updater::ContentsVerifier { +class ExtensionsTreeHashContentsVerifier + : public component_updater::ContentsVerifier { public: - explicit SecureContentsVerifier(const base::FilePath& component_root) { + explicit ExtensionsTreeHashContentsVerifier( + const base::FilePath& component_root) { if (base::PathExists( component_root.AppendASCII(kBraveVerifiedContentsPath))) { verified_contents_ = extensions::VerifiedContents::CreateFromFile( @@ -130,21 +132,20 @@ BASE_FEATURE(kComponentContentsVerifier, "ComponentContentsVerifier", base::FEATURE_DISABLED_BY_DEFAULT); -std::unique_ptr CreateContentsVerifier( +std::unique_ptr CreateExtensionsTreeHashContentsVerifier( const base::FilePath& component_root) { #if BUILDFLAG(ENABLE_EXTENSIONS) - const bool bypass = ShouldBypassSignature(); - if (!bypass) { - return std::make_unique(component_root); + if (!ShouldBypassSignature()) { + return std::make_unique(component_root); } #endif // if there is no extensions enabled then we expect that on these platforms // the component files are protected by the OS. - return std::make_unique(); + return nullptr; } void SetupComponentContentsVerifier() { - auto factory = base::BindRepeating(CreateContentsVerifier); + auto factory = base::BindRepeating(CreateExtensionsTreeHashContentsVerifier); SetupContentsVerifierFactory(std::move(factory)); } diff --git a/components/brave_component_updater/browser/BUILD.gn b/components/brave_component_updater/browser/BUILD.gn index f2a8b907a40d..24e36722b262 100644 --- a/components/brave_component_updater/browser/BUILD.gn +++ b/components/brave_component_updater/browser/BUILD.gn @@ -15,6 +15,8 @@ static_library("browser") { "brave_component_updater_delegate.h", "brave_on_demand_updater.cc", "brave_on_demand_updater.h", + "component_contents_accessor.cc", + "component_contents_accessor.h", "component_contents_verifier.cc", "component_contents_verifier.h", "dat_file_util.cc", diff --git a/components/brave_component_updater/browser/component_contents_accessor.cc b/components/brave_component_updater/browser/component_contents_accessor.cc new file mode 100644 index 000000000000..21d522f14824 --- /dev/null +++ b/components/brave_component_updater/browser/component_contents_accessor.cc @@ -0,0 +1,62 @@ +// Copyright (c) 2024 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +#include "brave/components/brave_component_updater/browser/component_contents_accessor.h" + +#include +#include + +#include "base/files/file_util.h" +#include "brave/components/brave_component_updater/browser/component_contents_verifier.h" + +namespace component_updater { + +ComponentContentsAccessor::ComponentContentsAccessor( + const base::FilePath& component_root) + : component_root_(component_root), + verifier_(CreateContentsVerifier(component_root)) {} + +ComponentContentsAccessor::~ComponentContentsAccessor() = default; + +// static +scoped_refptr ComponentContentsAccessor::Create( + const base::FilePath& component_root) { + return base::WrapRefCounted(new ComponentContentsAccessor(component_root)); +} + +const base::FilePath& ComponentContentsAccessor::GetComponentRoot() const { + return component_root_; +} + +std::optional ComponentContentsAccessor::GetFileAsString( + const base::FilePath& relative_path) { + std::string contents; + if (!base::ReadFileToString(GetComponentRoot().Append(relative_path), + &contents)) { + return std::nullopt; + } + if (verifier_ && + !verifier_->VerifyContents(relative_path, base::as_byte_span(contents))) { + return std::nullopt; + } + + return contents; +} + +std::optional> ComponentContentsAccessor::GetFileAsBytes( + const base::FilePath& relative_path) { + auto contents = + base::ReadFileToBytes(GetComponentRoot().Append(relative_path)); + if (!contents) { + return std::nullopt; + } + if (verifier_ && !verifier_->VerifyContents(relative_path, + base::as_byte_span(*contents))) { + return std::nullopt; + } + return std::move(*contents); +} + +} // namespace component_updater diff --git a/components/brave_component_updater/browser/component_contents_accessor.h b/components/brave_component_updater/browser/component_contents_accessor.h new file mode 100644 index 000000000000..14c04878e58c --- /dev/null +++ b/components/brave_component_updater/browser/component_contents_accessor.h @@ -0,0 +1,50 @@ +// Copyright (c) 2024 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +#ifndef BRAVE_COMPONENTS_BRAVE_COMPONENT_UPDATER_BROWSER_COMPONENT_CONTENTS_ACCESSOR_H_ +#define BRAVE_COMPONENTS_BRAVE_COMPONENT_UPDATER_BROWSER_COMPONENT_CONTENTS_ACCESSOR_H_ + +#include +#include +#include +#include + +#include "base/files/file_path.h" +#include "base/memory/ref_counted.h" + +namespace component_updater { + +// Use on MAY_BLOCK sequence. +class ComponentContentsAccessor + : public base::RefCountedThreadSafe { + public: + ComponentContentsAccessor(const ComponentContentsAccessor&) = delete; + ComponentContentsAccessor(ComponentContentsAccessor&&) = delete; + ComponentContentsAccessor& operator=(const ComponentContentsAccessor&) = + delete; + ComponentContentsAccessor& operator=(ComponentContentsAccessor&&) = delete; + + static scoped_refptr Create( + const base::FilePath& component_root); + + const base::FilePath& GetComponentRoot() const; + + std::optional GetFileAsString( + const base::FilePath& relative_path); + std::optional> GetFileAsBytes( + const base::FilePath& relative_path); + + private: + friend class base::RefCountedThreadSafe; + explicit ComponentContentsAccessor(const base::FilePath& component_root); + ~ComponentContentsAccessor(); + + const base::FilePath component_root_; + const std::unique_ptr verifier_; +}; + +} // namespace component_updater + +#endif // BRAVE_COMPONENTS_BRAVE_COMPONENT_UPDATER_BROWSER_COMPONENT_CONTENTS_ACCESSOR_H_ diff --git a/components/brave_component_updater/browser/component_contents_verifier.cc b/components/brave_component_updater/browser/component_contents_verifier.cc index 4bf3402db9d6..f466050541b7 100644 --- a/components/brave_component_updater/browser/component_contents_verifier.cc +++ b/components/brave_component_updater/browser/component_contents_verifier.cc @@ -6,76 +6,32 @@ #include "brave/components/brave_component_updater/browser/component_contents_verifier.h" #include -#include #include "base/check_is_test.h" -#include "base/files/file_util.h" #include "base/no_destructor.h" namespace component_updater { namespace { -base::NoDestructor g_verifier_factory; -} -// static -void SetupContentsVerifierFactory(ContentsVerifierFactory factory) { - *g_verifier_factory = std::move(factory); +ContentsVerifierFactory& GetContentsVerifierFactory() { + static base::NoDestructor factory; + return *factory; } -bool ContentsVerifier::VerifyContents(const base::FilePath& relative_path, - base::span contents) { - return true; -} +} // namespace -ComponentContentsAccessor::ComponentContentsAccessor( - const base::FilePath& component_root) - : component_root_(component_root), - verifier_(*g_verifier_factory ? g_verifier_factory->Run(component_root) - : std::make_unique()) { - if (!*g_verifier_factory) { - CHECK_IS_TEST(); - } +void SetupContentsVerifierFactory(ContentsVerifierFactory factory) { + GetContentsVerifierFactory() = std::move(factory); } -ComponentContentsAccessor::~ComponentContentsAccessor() = default; - -// static -scoped_refptr ComponentContentsAccessor::Create( +std::unique_ptr CreateContentsVerifier( const base::FilePath& component_root) { - return base::WrapRefCounted(new ComponentContentsAccessor(component_root)); -} - -const base::FilePath& ComponentContentsAccessor::GetComponentRoot() const { - return component_root_; -} - -std::optional ComponentContentsAccessor::GetFileAsString( - const base::FilePath& relative_path) { - std::string contents; - if (!base::ReadFileToString(GetComponentRoot().Append(relative_path), - &contents)) { - return std::nullopt; - } - if (!verifier_->VerifyContents(relative_path, base::as_byte_span(contents))) { - return std::nullopt; - } - - return contents; -} - -std::optional> ComponentContentsAccessor::GetFileAsBytes( - const base::FilePath& relative_path) { - auto contents = - base::ReadFileToBytes(GetComponentRoot().Append(relative_path)); - if (!contents) { - return std::nullopt; - } - if (!verifier_->VerifyContents(relative_path, - base::as_byte_span(*contents))) { - return std::nullopt; + if (!GetContentsVerifierFactory()) { + CHECK_IS_TEST(); + return nullptr; } - return std::move(*contents); + return GetContentsVerifierFactory().Run(component_root); } } // namespace component_updater diff --git a/components/brave_component_updater/browser/component_contents_verifier.h b/components/brave_component_updater/browser/component_contents_verifier.h index 8a522fac5ea9..e32f2ba7c210 100644 --- a/components/brave_component_updater/browser/component_contents_verifier.h +++ b/components/brave_component_updater/browser/component_contents_verifier.h @@ -7,13 +7,9 @@ #define BRAVE_COMPONENTS_BRAVE_COMPONENT_UPDATER_BROWSER_COMPONENT_CONTENTS_VERIFIER_H_ #include -#include -#include -#include #include "base/files/file_path.h" #include "base/functional/callback.h" -#include "base/memory/ref_counted.h" namespace component_updater { @@ -21,36 +17,7 @@ class ContentsVerifier { public: virtual ~ContentsVerifier() = default; virtual bool VerifyContents(const base::FilePath& relative_path, - base::span contents); -}; - -// Use on MAY_BLOCK sequence. -class ComponentContentsAccessor - : public base::RefCountedThreadSafe { - public: - ComponentContentsAccessor(const ComponentContentsAccessor&) = delete; - ComponentContentsAccessor(ComponentContentsAccessor&&) = delete; - ComponentContentsAccessor& operator=(const ComponentContentsAccessor&) = - delete; - ComponentContentsAccessor& operator=(ComponentContentsAccessor&&) = delete; - - static scoped_refptr Create( - const base::FilePath& component_root); - - const base::FilePath& GetComponentRoot() const; - - std::optional GetFileAsString( - const base::FilePath& relative_path); - std::optional> GetFileAsBytes( - const base::FilePath& relative_path); - - private: - friend class base::RefCountedThreadSafe; - explicit ComponentContentsAccessor(const base::FilePath& component_root); - ~ComponentContentsAccessor(); - - const base::FilePath component_root_; - std::unique_ptr verifier_; + base::span contents) = 0; }; using ContentsVerifierFactory = @@ -59,6 +26,10 @@ using ContentsVerifierFactory = void SetupContentsVerifierFactory(ContentsVerifierFactory factory); +// Uses factory set above to create the verifier. +std::unique_ptr CreateContentsVerifier( + const base::FilePath& component_root); + } // namespace component_updater #endif // BRAVE_COMPONENTS_BRAVE_COMPONENT_UPDATER_BROWSER_COMPONENT_CONTENTS_VERIFIER_H_ diff --git a/components/brave_shields/content/browser/ad_block_service.h b/components/brave_shields/content/browser/ad_block_service.h index 801eea0d2117..b09ca5f1fbe8 100644 --- a/components/brave_shields/content/browser/ad_block_service.h +++ b/components/brave_shields/content/browser/ad_block_service.h @@ -152,8 +152,6 @@ class AdBlockService { friend class brave_shields::CosmeticResourceMergeTest; friend class brave_shields::StripProceduralFiltersTest; - static std::string g_ad_block_dat_file_version_; - AdBlockDefaultResourceProvider* default_resource_provider(); AdBlockComponentFiltersProvider* default_filters_provider() { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); diff --git a/components/brave_shields/core/browser/ad_block_component_filters_provider.cc b/components/brave_shields/core/browser/ad_block_component_filters_provider.cc index 6d8b08e044be..0390951895fc 100644 --- a/components/brave_shields/core/browser/ad_block_component_filters_provider.cc +++ b/components/brave_shields/core/browser/ad_block_component_filters_provider.cc @@ -12,6 +12,7 @@ #include "base/files/file_path.h" #include "base/files/file_util.h" #include "base/task/thread_pool.h" +#include "brave/components/brave_component_updater/browser/component_contents_accessor.h" #include "brave/components/brave_shields/core/browser/ad_block_component_installer.h" #include "brave/components/brave_shields/core/browser/ad_block_filters_provider.h" #include "brave/components/brave_shields/core/browser/ad_block_filters_provider_manager.h" @@ -37,8 +38,9 @@ void OnReadDATFileData( void(base::OnceCallback*)>)> cb, uint8_t permission_mask, std::optional buffer) { - std::move(cb).Run(base::BindOnce(&AddDATBufferToFilterSet, permission_mask, - buffer.value_or(DATFileDataBuffer()))); + std::move(cb).Run( + base::BindOnce(&AddDATBufferToFilterSet, permission_mask, + std::move(buffer).value_or(DATFileDataBuffer()))); } } // namespace @@ -114,11 +116,9 @@ base::FilePath AdBlockComponentFiltersProvider::GetFilterSetPath() { void AdBlockComponentFiltersProvider::LoadFilterSet( base::OnceCallback< void(base::OnceCallback*)>)> cb) { - base::FilePath list_file_path = GetFilterSetPath(); - if (!component_accessor_) { - // If the path is not ready yet, provide a no-op callback immediately. An - // update will be pushed later to notify about the newly available list. + // If the component is not ready yet, provide a no-op callback immediately. + // An update will be pushed later to notify about the newly available list. std::move(cb).Run(base::DoNothing()); return; } diff --git a/components/brave_shields/core/browser/ad_block_component_installer.cc b/components/brave_shields/core/browser/ad_block_component_installer.cc index 723358c0212a..c851329dbbe3 100644 --- a/components/brave_shields/core/browser/ad_block_component_installer.cc +++ b/components/brave_shields/core/browser/ad_block_component_installer.cc @@ -14,7 +14,7 @@ #include "base/files/file_path.h" #include "base/functional/bind.h" #include "brave/components/brave_component_updater/browser/brave_on_demand_updater.h" -#include "brave/components/brave_component_updater/browser/component_contents_verifier.h" +#include "brave/components/brave_component_updater/browser/component_contents_accessor.h" #include "brave/components/brave_shields/core/common/brave_shield_constants.h" #include "components/component_updater/component_installer.h" #include "components/component_updater/component_updater_service.h" @@ -22,17 +22,10 @@ using brave_component_updater::BraveOnDemandUpdater; -namespace brave_component_updater { -class ComponentContentsAccessor; -} - namespace brave_shields { namespace { -using OnComponentReadyCallback = base::RepeatingCallback)>; - constexpr size_t kHashSize = 32; class AdBlockComponentInstallerPolicy @@ -161,7 +154,7 @@ void RegisterAdBlockComponentInternal( const std::string& component_public_key, const std::string& component_id, const std::string& component_name, - OnSecureComponentReadyCallback callback) { + OnComponentReadyCallback callback) { // In test, |cus| could be nullptr. if (!cus) { return; @@ -177,7 +170,7 @@ void RegisterAdBlockComponentInternal( void RegisterAdBlockDefaultResourceComponent( component_updater::ComponentUpdateService* cus, - OnSecureComponentReadyCallback callback) { + OnComponentReadyCallback callback) { RegisterAdBlockComponentInternal( cus, kAdBlockResourceComponentBase64PublicKey, kAdBlockResourceComponentId, kAdBlockResourceComponentName, @@ -186,7 +179,7 @@ void RegisterAdBlockDefaultResourceComponent( void RegisterAdBlockFilterListCatalogComponent( component_updater::ComponentUpdateService* cus, - OnSecureComponentReadyCallback callback) { + OnComponentReadyCallback callback) { RegisterAdBlockComponentInternal( cus, kAdBlockFilterListCatalogComponentBase64PublicKey, kAdBlockFilterListCatalogComponentId, @@ -198,7 +191,7 @@ void RegisterAdBlockFiltersComponent( const std::string& component_public_key, const std::string& component_id, const std::string& component_name, - OnSecureComponentReadyCallback callback) { + OnComponentReadyCallback callback) { RegisterAdBlockComponentInternal(cus, component_public_key, component_id, component_name, std::move(callback)); } diff --git a/components/brave_shields/core/browser/ad_block_component_installer.h b/components/brave_shields/core/browser/ad_block_component_installer.h index bca6f5860ad7..3bf062036894 100644 --- a/components/brave_shields/core/browser/ad_block_component_installer.h +++ b/components/brave_shields/core/browser/ad_block_component_installer.h @@ -9,31 +9,31 @@ #include #include "base/functional/callback.h" -#include "brave/components/brave_component_updater/browser/component_contents_verifier.h" namespace component_updater { class ComponentUpdateService; +class ComponentContentsAccessor; } // namespace component_updater namespace brave_shields { -using OnSecureComponentReadyCallback = base::RepeatingCallback)>; void RegisterAdBlockDefaultResourceComponent( component_updater::ComponentUpdateService* cus, - OnSecureComponentReadyCallback callback); + OnComponentReadyCallback callback); void RegisterAdBlockFilterListCatalogComponent( component_updater::ComponentUpdateService* cus, - OnSecureComponentReadyCallback callback); + OnComponentReadyCallback callback); void RegisterAdBlockFiltersComponent( component_updater::ComponentUpdateService* cus, const std::string& component_public_key, const std::string& component_id, const std::string& component_name, - OnSecureComponentReadyCallback callback); + OnComponentReadyCallback callback); } // namespace brave_shields diff --git a/components/brave_shields/core/browser/ad_block_default_resource_provider.cc b/components/brave_shields/core/browser/ad_block_default_resource_provider.cc index b52313b83262..8c22f0085959 100644 --- a/components/brave_shields/core/browser/ad_block_default_resource_provider.cc +++ b/components/brave_shields/core/browser/ad_block_default_resource_provider.cc @@ -10,7 +10,7 @@ #include "base/files/file_path.h" #include "base/task/thread_pool.h" -#include "brave/components/brave_component_updater/browser/component_contents_verifier.h" +#include "brave/components/brave_component_updater/browser/component_contents_accessor.h" #include "brave/components/brave_shields/core/browser/ad_block_component_installer.h" namespace { @@ -58,8 +58,8 @@ void AdBlockDefaultResourceProvider::OnComponentReady( void AdBlockDefaultResourceProvider::LoadResources( base::OnceCallback cb) { if (!component_accessor_) { - // If the path is not ready yet, run the callback with empty resources to - // avoid blocking filter data loads. + // If the component is not ready yet, run the callback with empty resources + // to avoid blocking filter data loads. std::move(cb).Run("[]"); return; } @@ -67,7 +67,7 @@ void AdBlockDefaultResourceProvider::LoadResources( auto handle_file_content = base::BindOnce( [](base::OnceCallback cb, std::optional content) { - std::move(cb).Run(content.value_or("[]")); + std::move(cb).Run(std::move(content).value_or("[]")); }, std::move(cb)); diff --git a/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.cc b/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.cc index a1256d9843ba..a45e221b954c 100644 --- a/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.cc +++ b/components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.cc @@ -10,7 +10,7 @@ #include "base/files/file_path.h" #include "base/task/thread_pool.h" -#include "brave/components/brave_component_updater/browser/component_contents_verifier.h" +#include "brave/components/brave_component_updater/browser/component_contents_accessor.h" #include "brave/components/brave_shields/core/browser/ad_block_component_installer.h" constexpr char kListCatalogFile[] = "list_catalog.json"; @@ -59,15 +59,15 @@ void AdBlockFilterListCatalogProvider::OnComponentReady( void AdBlockFilterListCatalogProvider::LoadFilterListCatalog( base::OnceCallback cb) { if (!component_accessor_) { - // If the path is not ready yet, don't run the callback. An update should be - // pushed soon. + // If the component is not ready yet, don't run the callback. An update + // should be pushed soon. return; } auto on_load = base::BindOnce( [](base::OnceCallback cb, std::optional data) { - std::move(cb).Run(data.value_or("")); + std::move(cb).Run(std::move(data).value_or(std::string())); }, std::move(cb)); diff --git a/test/data/adblock-components/resources_corrupted/_metadata/verified_contents.json b/test/data/adblock-components/resources_corrupted/_metadata/verified_contents.json index aa9a40a0b038..3da16290fd95 100644 --- a/test/data/adblock-components/resources_corrupted/_metadata/verified_contents.json +++ b/test/data/adblock-components/resources_corrupted/_metadata/verified_contents.json @@ -1 +1 @@ -[{"description":"treehash per file","signed_content":{"payload":"eyJjb250ZW50X2hhc2hlcyI6W3siYmxvY2tfc2l6ZSI6NDA5NiwiZGlnZXN0Ijoic2hhMjU2IiwiZmlsZXMiOlt7InBhdGgiOiJyZXNvdXJjZXMuanNvbiIsInJvb3RfaGFzaCI6Ik9oM3pERVJjT3U0NWE2N21WQkFqeWcyNzEyUGxvek04UWtBeVNnZzVUMlkifV0sImZvcm1hdCI6InRyZWVoYXNoIiwiaGFzaF9ibG9ja19zaXplIjo0MDk2fV0sIml0ZW1faWQiOiJtZmRkaWJtYmxtYmNjcGFkZm5kZ2FraW9wbW1oZWJvcCIsIml0ZW1fdmVyc2lvbiI6IjEuMC4xMDMiLCJwcm90b2NvbF92ZXJzaW9uIjoxfQ","signatures":[{"protected":"eyJhbGciOiJSUzI1NiJ9","header":{"kid":"webstore"},"signature":"gxnzPORvPtzdFbXbaOD-MOa93ED7LFNA8bAswg7dCXBXe9CP5Loyjsj3ch1XxVSzCfyLQ-lai0LN7TlTjxTZocbXD7MRfwo8hR9Qn2iyTjc8QswSiWl_F0tuV7fg6b6oTiX7Cc7-EPoyngWAA9nX7_HqjfG8XspnoEWs_-lER-DPDCX7sbWdSZuZaPoEOgDaujILJgcYr264n0nZNzc213wVzKxwfQeZAPUZAnTofulO03QQquKuzjZURS8ZZdSW2vlc72duxylo3Xr40lERk60VGv35ZwYW1UZpt8PZH_XOame3vfyBrbrxylRySu7nAQ8GuyEuDP8ucIAHUpf5Hg"}]}}] \ No newline at end of file +[{"description":"treehash per file","signed_content":{"payload":"eyJjb250ZW50X2hhc2hlcyI6W3siYmxvY2tfc2l6ZSI6NDA5NiwiZGlnZXN0Ijoic2hhMjU2IiwiZmlsZXMiOlt7InBhdGgiOiJyZXNvdXJjZXMuanNvbiIsInJvb3RfaGFzaCI6Ink4UUlMemhranFaSGljeFlTd0FLRjdGQWplUTZGZFdNZ3M1aEtFNzdWemcifV0sImZvcm1hdCI6InRyZWVoYXNoIiwiaGFzaF9ibG9ja19zaXplIjo0MDk2fV0sIml0ZW1faWQiOiJtZmRkaWJtYmxtYmNjcGFkZm5kZ2FraW9wbW1oZWJvcCIsIml0ZW1fdmVyc2lvbiI6IjEuMC4xMDMiLCJwcm90b2NvbF92ZXJzaW9uIjoxfQ","signatures":[{"protected":"eyJhbGciOiJSUzI1NiJ9","header":{"kid":"webstore"},"signature":"dpdpkXJDynHykCchKb-AJmGsAl_A8AFnN9qwmiPn-rQkg3EywKuax_QgmTEvinue7tY5yLDlbhDGlPcl-YoMZqjwuo7gJBeKqHBhV7n3ImU2HIofYLJ71Gvn7tyhrHlSBQm0GdUGCGWB_dwcj9qAp08uqlc-Rz144uSO8uwYTlfQE3uX7AsETXpZ-PVjyFNXFYSGe0Yn34t2PD7kXCtvGt_sPJHEj9rFYdgibEjAk86f8qUAlo1cSE0H1VhzFCDM09x2gHB0cR0_p8C6BuZ-dpEqPH1iwiKf0mEaSdqMZC1pzDR4ibTpxrj-9XiM4RHzkUk6IvNJCP-VsAoLcBqWDQ"}]}}] \ No newline at end of file diff --git a/test/data/adblock-components/resources_corrupted/resources.json b/test/data/adblock-components/resources_corrupted/resources.json index 9f2c0f1717b1..9373704b06cd 100644 --- a/test/data/adblock-components/resources_corrupted/resources.json +++ b/test/data/adblock-components/resources_corrupted/resources.json @@ -1 +1 @@ -[{"name":"malware.js","aliases":[],"kind":{"mime":"application/javascript"}] \ No newline at end of file +[{"name":"edited.js","aliases":[],"kind":{"mime":"application/javascript"}}] \ No newline at end of file diff --git a/test/data/adblock-components/resources_ok/_metadata/verified_contents.json b/test/data/adblock-components/resources_ok/_metadata/verified_contents.json index aa9a40a0b038..3da16290fd95 100644 --- a/test/data/adblock-components/resources_ok/_metadata/verified_contents.json +++ b/test/data/adblock-components/resources_ok/_metadata/verified_contents.json @@ -1 +1 @@ -[{"description":"treehash per file","signed_content":{"payload":"eyJjb250ZW50X2hhc2hlcyI6W3siYmxvY2tfc2l6ZSI6NDA5NiwiZGlnZXN0Ijoic2hhMjU2IiwiZmlsZXMiOlt7InBhdGgiOiJyZXNvdXJjZXMuanNvbiIsInJvb3RfaGFzaCI6Ik9oM3pERVJjT3U0NWE2N21WQkFqeWcyNzEyUGxvek04UWtBeVNnZzVUMlkifV0sImZvcm1hdCI6InRyZWVoYXNoIiwiaGFzaF9ibG9ja19zaXplIjo0MDk2fV0sIml0ZW1faWQiOiJtZmRkaWJtYmxtYmNjcGFkZm5kZ2FraW9wbW1oZWJvcCIsIml0ZW1fdmVyc2lvbiI6IjEuMC4xMDMiLCJwcm90b2NvbF92ZXJzaW9uIjoxfQ","signatures":[{"protected":"eyJhbGciOiJSUzI1NiJ9","header":{"kid":"webstore"},"signature":"gxnzPORvPtzdFbXbaOD-MOa93ED7LFNA8bAswg7dCXBXe9CP5Loyjsj3ch1XxVSzCfyLQ-lai0LN7TlTjxTZocbXD7MRfwo8hR9Qn2iyTjc8QswSiWl_F0tuV7fg6b6oTiX7Cc7-EPoyngWAA9nX7_HqjfG8XspnoEWs_-lER-DPDCX7sbWdSZuZaPoEOgDaujILJgcYr264n0nZNzc213wVzKxwfQeZAPUZAnTofulO03QQquKuzjZURS8ZZdSW2vlc72duxylo3Xr40lERk60VGv35ZwYW1UZpt8PZH_XOame3vfyBrbrxylRySu7nAQ8GuyEuDP8ucIAHUpf5Hg"}]}}] \ No newline at end of file +[{"description":"treehash per file","signed_content":{"payload":"eyJjb250ZW50X2hhc2hlcyI6W3siYmxvY2tfc2l6ZSI6NDA5NiwiZGlnZXN0Ijoic2hhMjU2IiwiZmlsZXMiOlt7InBhdGgiOiJyZXNvdXJjZXMuanNvbiIsInJvb3RfaGFzaCI6Ink4UUlMemhranFaSGljeFlTd0FLRjdGQWplUTZGZFdNZ3M1aEtFNzdWemcifV0sImZvcm1hdCI6InRyZWVoYXNoIiwiaGFzaF9ibG9ja19zaXplIjo0MDk2fV0sIml0ZW1faWQiOiJtZmRkaWJtYmxtYmNjcGFkZm5kZ2FraW9wbW1oZWJvcCIsIml0ZW1fdmVyc2lvbiI6IjEuMC4xMDMiLCJwcm90b2NvbF92ZXJzaW9uIjoxfQ","signatures":[{"protected":"eyJhbGciOiJSUzI1NiJ9","header":{"kid":"webstore"},"signature":"dpdpkXJDynHykCchKb-AJmGsAl_A8AFnN9qwmiPn-rQkg3EywKuax_QgmTEvinue7tY5yLDlbhDGlPcl-YoMZqjwuo7gJBeKqHBhV7n3ImU2HIofYLJ71Gvn7tyhrHlSBQm0GdUGCGWB_dwcj9qAp08uqlc-Rz144uSO8uwYTlfQE3uX7AsETXpZ-PVjyFNXFYSGe0Yn34t2PD7kXCtvGt_sPJHEj9rFYdgibEjAk86f8qUAlo1cSE0H1VhzFCDM09x2gHB0cR0_p8C6BuZ-dpEqPH1iwiKf0mEaSdqMZC1pzDR4ibTpxrj-9XiM4RHzkUk6IvNJCP-VsAoLcBqWDQ"}]}}] \ No newline at end of file diff --git a/test/data/adblock-components/resources_ok/resources.json b/test/data/adblock-components/resources_ok/resources.json index eccd68bcad87..6cee6aa87479 100644 --- a/test/data/adblock-components/resources_ok/resources.json +++ b/test/data/adblock-components/resources_ok/resources.json @@ -1 +1 @@ -[{"name":"brave-fix.js","aliases":[],"kind":{"mime":"application/javascript"}] \ No newline at end of file +[{"name":"brave-fix.js","aliases":[],"kind":{"mime":"application/javascript"}}] \ No newline at end of file From 616de9bb6c76dc2d5feff62bb18b396573999eb2 Mon Sep 17 00:00:00 2001 From: boocmp Date: Mon, 16 Dec 2024 17:45:35 +0700 Subject: [PATCH 12/14] Review. --- .../brave_component_contents_verifier.cc | 24 +++++++++---------- .../browser/component_contents_accessor.h | 6 ++++- .../browser/component_contents_verifier.cc | 2 +- .../browser/component_contents_verifier.h | 2 +- 4 files changed, 19 insertions(+), 15 deletions(-) diff --git a/browser/component_updater/brave_component_contents_verifier.cc b/browser/component_updater/brave_component_contents_verifier.cc index b62fdc03653b..6e60a0a5d3db 100644 --- a/browser/component_updater/brave_component_contents_verifier.cc +++ b/browser/component_updater/brave_component_contents_verifier.cc @@ -123,17 +123,8 @@ bool ShouldBypassSignature() { return kBypass; } -} // namespace -#endif // BUILDFLAG(ENABLE_EXTENSIONS) - -namespace component_updater { - -BASE_FEATURE(kComponentContentsVerifier, - "ComponentContentsVerifier", - base::FEATURE_DISABLED_BY_DEFAULT); - -std::unique_ptr CreateExtensionsTreeHashContentsVerifier( - const base::FilePath& component_root) { +std::unique_ptr +CreateExtensionsTreeHashContentsVerifier(const base::FilePath& component_root) { #if BUILDFLAG(ENABLE_EXTENSIONS) if (!ShouldBypassSignature()) { return std::make_unique(component_root); @@ -144,9 +135,18 @@ std::unique_ptr CreateExtensionsTreeHashContentsVerifier( return nullptr; } +} // namespace +#endif // BUILDFLAG(ENABLE_EXTENSIONS) + +namespace component_updater { + +BASE_FEATURE(kComponentContentsVerifier, + "ComponentContentsVerifier", + base::FEATURE_DISABLED_BY_DEFAULT); + void SetupComponentContentsVerifier() { auto factory = base::BindRepeating(CreateExtensionsTreeHashContentsVerifier); - SetupContentsVerifierFactory(std::move(factory)); + SetContentsVerifierFactory(std::move(factory)); } } // namespace component_updater diff --git a/components/brave_component_updater/browser/component_contents_accessor.h b/components/brave_component_updater/browser/component_contents_accessor.h index 14c04878e58c..e1e11fdf52b5 100644 --- a/components/brave_component_updater/browser/component_contents_accessor.h +++ b/components/brave_component_updater/browser/component_contents_accessor.h @@ -16,7 +16,11 @@ namespace component_updater { -// Use on MAY_BLOCK sequence. +// This class provides secure access to the component's files. It requires +// 'verified_contents.json' that should be shipped with the component. If +// 'verified_contents.json' is missing or signature doesn't match the +// content of the files then accessor doesn't return data from GetFile* +// functions. Use on MAY_BLOCK sequence. class ComponentContentsAccessor : public base::RefCountedThreadSafe { public: diff --git a/components/brave_component_updater/browser/component_contents_verifier.cc b/components/brave_component_updater/browser/component_contents_verifier.cc index f466050541b7..69fd4de53f03 100644 --- a/components/brave_component_updater/browser/component_contents_verifier.cc +++ b/components/brave_component_updater/browser/component_contents_verifier.cc @@ -21,7 +21,7 @@ ContentsVerifierFactory& GetContentsVerifierFactory() { } // namespace -void SetupContentsVerifierFactory(ContentsVerifierFactory factory) { +void SetContentsVerifierFactory(ContentsVerifierFactory factory) { GetContentsVerifierFactory() = std::move(factory); } diff --git a/components/brave_component_updater/browser/component_contents_verifier.h b/components/brave_component_updater/browser/component_contents_verifier.h index e32f2ba7c210..b8c44b217c79 100644 --- a/components/brave_component_updater/browser/component_contents_verifier.h +++ b/components/brave_component_updater/browser/component_contents_verifier.h @@ -24,7 +24,7 @@ using ContentsVerifierFactory = base::RepeatingCallback( const base::FilePath& component_root)>; -void SetupContentsVerifierFactory(ContentsVerifierFactory factory); +void SetContentsVerifierFactory(ContentsVerifierFactory factory); // Uses factory set above to create the verifier. std::unique_ptr CreateContentsVerifier( From 83bafe10ef4a8c9564dc38ba8eb1fab0f3b9b8a3 Mon Sep 17 00:00:00 2001 From: boocmp Date: Wed, 25 Dec 2024 16:13:56 +0700 Subject: [PATCH 13/14] Feature moved to features.h. --- .../brave_shields/ad_block_service_browsertest.cc | 4 ++-- .../brave_component_contents_verifier.cc | 13 +++++-------- .../brave_component_contents_verifier.h | 4 ---- .../brave_component_updater/browser/features.cc | 4 ++++ .../brave_component_updater/browser/features.h | 1 + 5 files changed, 12 insertions(+), 14 deletions(-) diff --git a/browser/brave_shields/ad_block_service_browsertest.cc b/browser/brave_shields/ad_block_service_browsertest.cc index 35a2f1237365..25e030f82757 100644 --- a/browser/brave_shields/ad_block_service_browsertest.cc +++ b/browser/brave_shields/ad_block_service_browsertest.cc @@ -25,9 +25,9 @@ #include "base/threading/thread_restrictions.h" #include "brave/app/brave_command_ids.h" #include "brave/browser/brave_browser_process.h" -#include "brave/browser/component_updater/brave_component_contents_verifier.h" #include "brave/browser/net/brave_ad_block_tp_network_delegate_helper.h" #include "brave/components/brave_component_updater/browser/component_contents_accessor.h" +#include "brave/components/brave_component_updater/browser/features.h" #include "brave/components/brave_shields/content/browser/ad_block_custom_filters_provider.h" #include "brave/components/brave_shields/content/browser/ad_block_engine.h" #include "brave/components/brave_shields/content/browser/ad_block_service.h" @@ -1625,7 +1625,7 @@ class AdBlockServiceSignedComponentsTest : public AdBlockServiceTest { public: AdBlockServiceSignedComponentsTest() { feature_list_.InitAndEnableFeature( - component_updater::kComponentContentsVerifier); + brave_component_updater::kComponentContentsVerifier); } ~AdBlockServiceSignedComponentsTest() override = default; diff --git a/browser/component_updater/brave_component_contents_verifier.cc b/browser/component_updater/brave_component_contents_verifier.cc index 6e60a0a5d3db..e0df2fadb57f 100644 --- a/browser/component_updater/brave_component_contents_verifier.cc +++ b/browser/component_updater/brave_component_contents_verifier.cc @@ -11,17 +11,18 @@ #include #include +#include "base/command_line.h" +#include "base/containers/span_reader.h" #include "base/files/file_path.h" #include "base/files/file_util.h" #include "base/functional/bind.h" #include "brave/components/brave_component_updater/browser/component_contents_verifier.h" +#include "brave/components/brave_component_updater/browser/features.h" +#include "crypto/sha2.h" #include "extensions/buildflags/buildflags.h" #if BUILDFLAG(ENABLE_EXTENSIONS) -#include "base/command_line.h" -#include "base/containers/span_reader.h" -#include "crypto/sha2.h" #include "extensions/browser/content_hash_tree.h" #include "extensions/browser/verified_contents.h" @@ -117,7 +118,7 @@ class ExtensionsTreeHashContentsVerifier bool ShouldBypassSignature() { static const bool kBypass = !base::FeatureList::IsEnabled( - component_updater::kComponentContentsVerifier) || + brave_component_updater::kComponentContentsVerifier) || base::CommandLine::ForCurrentProcess()->HasSwitch( component_updater::kBypassComponentContentsVerifier); return kBypass; @@ -140,10 +141,6 @@ CreateExtensionsTreeHashContentsVerifier(const base::FilePath& component_root) { namespace component_updater { -BASE_FEATURE(kComponentContentsVerifier, - "ComponentContentsVerifier", - base::FEATURE_DISABLED_BY_DEFAULT); - void SetupComponentContentsVerifier() { auto factory = base::BindRepeating(CreateExtensionsTreeHashContentsVerifier); SetContentsVerifierFactory(std::move(factory)); diff --git a/browser/component_updater/brave_component_contents_verifier.h b/browser/component_updater/brave_component_contents_verifier.h index f0d42a8b9fb9..44d2cceb6905 100644 --- a/browser/component_updater/brave_component_contents_verifier.h +++ b/browser/component_updater/brave_component_contents_verifier.h @@ -6,12 +6,8 @@ #ifndef BRAVE_BROWSER_COMPONENT_UPDATER_BRAVE_COMPONENT_CONTENTS_VERIFIER_H_ #define BRAVE_BROWSER_COMPONENT_UPDATER_BRAVE_COMPONENT_CONTENTS_VERIFIER_H_ -#include "base/feature_list.h" - namespace component_updater { -BASE_DECLARE_FEATURE(kComponentContentsVerifier); - inline constexpr char kBypassComponentContentsVerifier[] = "bypass-component-contents-verifier"; diff --git a/components/brave_component_updater/browser/features.cc b/components/brave_component_updater/browser/features.cc index 220d3d68c175..f56264f9a74b 100644 --- a/components/brave_component_updater/browser/features.cc +++ b/components/brave_component_updater/browser/features.cc @@ -11,4 +11,8 @@ BASE_FEATURE(kUseDevUpdaterUrl, "UseDevUpdaterUrl", base::FEATURE_DISABLED_BY_DEFAULT); +BASE_FEATURE(kComponentContentsVerifier, + "ComponentContentsVerifier", + base::FEATURE_DISABLED_BY_DEFAULT); + } // namespace brave_component_updater diff --git a/components/brave_component_updater/browser/features.h b/components/brave_component_updater/browser/features.h index f399186cc520..3aa742e573a7 100644 --- a/components/brave_component_updater/browser/features.h +++ b/components/brave_component_updater/browser/features.h @@ -11,6 +11,7 @@ namespace brave_component_updater { BASE_DECLARE_FEATURE(kUseDevUpdaterUrl); +BASE_DECLARE_FEATURE(kComponentContentsVerifier); } // namespace brave_component_updater From b5a2e4a62b5d8dd549b657253ea8c5ed09ea83af Mon Sep 17 00:00:00 2001 From: boocmp Date: Wed, 25 Dec 2024 17:11:59 +0700 Subject: [PATCH 14/14] Added const modifier. --- .../brave_component_contents_verifier.cc | 12 +++++++----- .../browser/component_contents_accessor.h | 2 +- .../browser/component_contents_verifier.h | 2 +- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/browser/component_updater/brave_component_contents_verifier.cc b/browser/component_updater/brave_component_contents_verifier.cc index e0df2fadb57f..32b54d171a8f 100644 --- a/browser/component_updater/brave_component_contents_verifier.cc +++ b/browser/component_updater/brave_component_contents_verifier.cc @@ -100,7 +100,7 @@ class ExtensionsTreeHashContentsVerifier } bool VerifyContents(const base::FilePath& relative_path, - base::span contents) override { + base::span contents) const override { if (!verified_contents_ || !verified_contents_->HasTreeHashRoot(relative_path)) { return false; @@ -124,6 +124,12 @@ bool ShouldBypassSignature() { return kBypass; } +} // namespace +#endif // BUILDFLAG(ENABLE_EXTENSIONS) + +namespace component_updater { + +namespace { std::unique_ptr CreateExtensionsTreeHashContentsVerifier(const base::FilePath& component_root) { #if BUILDFLAG(ENABLE_EXTENSIONS) @@ -135,11 +141,7 @@ CreateExtensionsTreeHashContentsVerifier(const base::FilePath& component_root) { // the component files are protected by the OS. return nullptr; } - } // namespace -#endif // BUILDFLAG(ENABLE_EXTENSIONS) - -namespace component_updater { void SetupComponentContentsVerifier() { auto factory = base::BindRepeating(CreateExtensionsTreeHashContentsVerifier); diff --git a/components/brave_component_updater/browser/component_contents_accessor.h b/components/brave_component_updater/browser/component_contents_accessor.h index e1e11fdf52b5..576cfeabc01c 100644 --- a/components/brave_component_updater/browser/component_contents_accessor.h +++ b/components/brave_component_updater/browser/component_contents_accessor.h @@ -46,7 +46,7 @@ class ComponentContentsAccessor ~ComponentContentsAccessor(); const base::FilePath component_root_; - const std::unique_ptr verifier_; + const std::unique_ptr verifier_; }; } // namespace component_updater diff --git a/components/brave_component_updater/browser/component_contents_verifier.h b/components/brave_component_updater/browser/component_contents_verifier.h index b8c44b217c79..e59bd049c4b7 100644 --- a/components/brave_component_updater/browser/component_contents_verifier.h +++ b/components/brave_component_updater/browser/component_contents_verifier.h @@ -17,7 +17,7 @@ class ContentsVerifier { public: virtual ~ContentsVerifier() = default; virtual bool VerifyContents(const base::FilePath& relative_path, - base::span contents) = 0; + base::span contents) const = 0; }; using ContentsVerifierFactory =