Added ComponentContentsVerifier that checks verified_contents.json shipped with a component. #26660

boocmp · 2024-11-20T16:01:23Z

Resolves brave/brave-browser#42274

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

github-actions · 2024-11-26T02:25:35Z

components/brave_component_updater/browser/component_contents_verifier.h

+
+// Use on MAY_BLOCK sequence.
+class ComponentContentsAccessor
+    : public base::RefCountedThreadSafe<ComponentContentsAccessor> {


_{reported by reviewdog 🐶}
[semgrep] Reference counting is occasionally useful but is more often a sign that someone isn't thinking carefully about ownership. Use it when ownership is truly shared (for example, multiple tabs sharing the same renderer process), not for when lifetime management is difficult to reason about.

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/refcounted-usage.yaml

Cc @stoletheminerals @thypon @cdesouza-chromium @bridiver

I'm not sure about this wording:

Reference counting is occasionally useful but is more often a sign that someone isn't thinking carefully about ownership.

@stoletheminerals I think we need some tuning of the wording here.

browser/component_updater/brave_component_contents_verifier.cc

components/brave_component_updater/browser/component_contents_verifier.cc

components/brave_component_updater/browser/component_contents_verifier.h

components/brave_shields/core/browser/ad_block_default_resource_provider.cc

components/brave_component_updater/browser/component_contents_verifier.h

cdesouza-chromium · 2024-11-26T11:59:16Z

browser/component_updater/brave_component_contents_verifier.cc

+scoped_refptr<brave_component_updater::ComponentContentsAccessor>
+CreateComponentContentsAccessor(bool with_verifier,
+                                const base::FilePath& component_root) {
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+  if (with_verifier) {
+    return base::MakeRefCounted<ComponentContentsAccessorImpl>(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<ComponentNoChecksContentsAccessorImpl>(
+      component_root);
+}


We have ComponentContentsAccessor -> ComponentNoChecksContentsAccessorImpl -> ComponentContentsAccessorImpl, and then we wrap this all behind a repeating callback to CreateComponentContentsAccessor.

ComponentContentsAccessorImpl with ignore_invalid_signature_=true is close enough to ComponentNoChecksContentsAccessorImpl.

I'm wondering if subclassing things like this is really a gain. The main difference here is that certain build configurations don't have BUILDFLAG(ENABLE_EXTENSIONS). I feel this should all be one single class, namely, ComponentContentsAccessor, and then a different .cc for build configs that don't support extension. This would avoid the whole use of inheritance here.

Without inheritance here we don't need a factory function, and without a factory function, we wouldn't need the NoDestructor class.

We need this factory because we can't include (and use) code from extensions on the components level. But a lot of downloadable components live in components dir.

Right, that makes sense. Thanks for clarifying. It seems to me then that this very specific code should be part of something like a delegate, rather than just be subclassing ComponentContentsAccessor, and that delegate should be passed into ComponentContentsAccessor however way it gets constructed.

To clarify maybe this should be something like this:

std::unique_ptr<brave_component_updater::ComponentContentsAccessor::Delegate> CreateComponentContentsAccessor(bool with_verifier, const base::FilePath& component_root) {

and

void SetupComponentContentsVerifier() { auto factory = base::BindRepeating(CreateComponentContentsAccessor, true); brave_component_updater::ComponentContentsAccessor::Setup(std::move(factory)); }

What are your thoughts?

cdesouza-chromium · 2024-11-26T12:02:13Z

Thanks @boocmp for this change. Good work. I think we are nearly there. After going over it, I think the general gist is that we can simplify this quite a bit, by removing the need of inheritance/factory/global-no-destructor.

components/brave_component_updater/browser/component_contents_verifier.h

browser/component_updater/brave_component_contents_verifier.cc

components/brave_component_updater/browser/component_contents_verifier.h

components/brave_component_updater/browser/component_contents_verifier.cc

components/brave_shields/core/browser/ad_block_component_filters_provider.cc

components/brave_shields/core/browser/ad_block_default_resource_provider.cc

components/brave_shields/core/browser/ad_block_filter_list_catalog_provider.cc

components/brave_component_updater/browser/component_contents_verifier.cc

components/brave_component_updater/browser/component_contents_verifier.h

components/brave_shields/core/browser/ad_block_component_installer.cc

components/brave_component_updater/browser/component_contents_verifier.h

browser/brave_shields/ad_block_service_browsertest.cc

github-actions · 2024-12-13T15:30:24Z

components/brave_component_updater/browser/component_contents_accessor.h

+
+// Use on MAY_BLOCK sequence.
+class ComponentContentsAccessor
+    : public base::RefCountedThreadSafe<ComponentContentsAccessor> {


_{reported by reviewdog 🐶}
[semgrep] Reference counting is occasionally useful but is more often a sign that someone isn't thinking carefully about ownership. Use it when ownership is truly shared (for example, multiple tabs sharing the same renderer process), not for when lifetime management is difficult to reason about.

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/refcounted-usage.yaml

Cc @stoletheminerals @thypon @cdesouza-chromium @bridiver

Commented in several places that this does not appear to be an appropriate use of ref counted

github-actions · 2024-12-13T15:30:24Z

components/brave_component_updater/browser/component_contents_accessor.h

+      const base::FilePath& relative_path);
+
+ private:
+  friend class base::RefCountedThreadSafe<ComponentContentsAccessor>;


_{reported by reviewdog 🐶}
[semgrep] Reference counting is occasionally useful but is more often a sign that someone isn't thinking carefully about ownership. Use it when ownership is truly shared (for example, multiple tabs sharing the same renderer process), not for when lifetime management is difficult to reason about.

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/refcounted-usage.yaml

Cc @stoletheminerals @thypon @cdesouza-chromium @bridiver

This seems to be misplaced

goodov

lgtm with few nits

browser/component_updater/brave_component_contents_verifier.cc

components/brave_component_updater/browser/component_contents_verifier.h

components/brave_component_updater/browser/component_contents_accessor.h

github-actions · 2024-12-16T10:53:29Z

components/brave_component_updater/browser/component_contents_accessor.h

+// content of the files then accessor doesn't return data from GetFile*
+// functions. Use on MAY_BLOCK sequence.
+class ComponentContentsAccessor
+    : public base::RefCountedThreadSafe<ComponentContentsAccessor> {


_{reported by reviewdog 🐶}
[semgrep] Reference counting is occasionally useful but is more often a sign that someone isn't thinking carefully about ownership. Use it when ownership is truly shared (for example, multiple tabs sharing the same renderer process), not for when lifetime management is difficult to reason about.

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/refcounted-usage.yaml

Cc @stoletheminerals @thypon @cdesouza-chromium @bridiver

github-actions · 2024-12-16T10:53:29Z

components/brave_component_updater/browser/component_contents_accessor.h

+      const base::FilePath& relative_path);
+
+ private:
+  friend class base::RefCountedThreadSafe<ComponentContentsAccessor>;


_{reported by reviewdog 🐶}
[semgrep] Reference counting is occasionally useful but is more often a sign that someone isn't thinking carefully about ownership. Use it when ownership is truly shared (for example, multiple tabs sharing the same renderer process), not for when lifetime management is difficult to reason about.

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/refcounted-usage.yaml

Cc @stoletheminerals @thypon @cdesouza-chromium @bridiver

If possible we should avoid flagging usage as a friend

bridiver · 2024-12-24T21:27:29Z

components/brave_component_updater/browser/component_contents_accessor.h

+// '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


nit this name is a bit weird imo, maybe something like VerifiedContentsLoader or VerifiedContentsReader?

bridiver · 2024-12-24T21:28:54Z

components/brave_component_updater/browser/component_contents_accessor.h

+// content of the files then accessor doesn't return data from GetFile*
+// functions. Use on MAY_BLOCK sequence.
+class ComponentContentsAccessor
+    : public base::RefCountedThreadSafe<ComponentContentsAccessor> {


why is this ref counted?

I think this is working around a design issue and there appear to be some thread safety concerns. I think we're adding unnecessary complexity here by wrapping the reading of the data in this class, can't we just use it to verify the contents and only access it on the task runner?

If we wanted to continue wrapping the read it could be done without any need for ref counted anyway

bridiver · 2024-12-24T21:34:05Z

components/brave_component_updater/browser/component_contents_verifier.h

+
+void SetContentsVerifierFactory(ContentsVerifierFactory factory);
+
+// Uses factory set above to create the verifier.


I don't understand why the factory is needed, why can't we just directly create an instance of ContentsVerifier?

Because we want to use it in components, but the implementation requires code from extensions.

afaik it's fine to use extensions in components as long as it's not from chrome/browser

bridiver · 2024-12-24T21:38:36Z

browser/component_updater/brave_component_contents_verifier.h

+
+#include "base/feature_list.h"
+
+namespace component_updater {


this filename doesn't make sense to me with the contents. The feature should be in features.h and this also seems like a strange filename for SetupComponentContentsVerifier

bridiver · 2024-12-24T21:42:14Z

browser/brave_shields/ad_block_service_browsertest.cc

+    base::RunLoop run_loop;
+    resource_provider->LoadResources(
+        base::BindLambdaForTesting([&run_loop](const std::string& resources) {
+#if BUILDFLAG(ENABLE_EXTENSIONS)


why does it matter if extensions are enabled for adblock? It's not an extension

Because it uses the chromium code which placed under this flag.

shouldn't this have its own buildflag then and use that here?

bridiver · 2024-12-24T21:43:30Z

browser/component_updater/brave_component_contents_verifier.cc

+
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+
+#include "base/command_line.h"


These headers are not all guarded by enable_extension in gn. Don't match headers with guards based on how they are used in the file, they should always match the guards in gn. If you don't match them with gn guards then you effectively break gn check because it doesn't understand the preprocessor guards. It's also confusing imo.

bridiver · 2024-12-24T21:46:48Z

components/brave_component_updater/browser/component_contents_accessor.cc

+ComponentContentsAccessor::ComponentContentsAccessor(
+    const base::FilePath& component_root)
+    : component_root_(component_root),
+      verifier_(CreateContentsVerifier(component_root)) {}


we're using this class across multiple threads, but extensions::VerifiedContents is not thread safe

verifier_ is a part of ref counted object and stored as const std::unique_ptr<...>. It's a readonly object.

We shouldn't be using ref counted here because this class does not have shared ownership.

"Use it when ownership is truly shared (for example, multiple tabs sharing the same renderer process), not for when lifetime management is difficult to reason about."

And also https://www.chromium.org/developers/smart-pointer-guidelines/#when-do-we-use-each-smart-pointer

Re thread safety I think I was looking at the wrong method call somewhere, but that's part of the problem here because calling this class across multiple threads and use of ref counting for lifetime management both contribute to the difficulty of understanding this code.

…ipped with a component.

github-actions · 2024-12-25T09:17:54Z

[puLL-Merge] - brave/brave-core@26660

Description

This PR introduces a new feature for verifying the contents of component updates in Brave's ad-blocking system. It adds a new ComponentContentsAccessor class and associated functionality to securely access and verify component files using a signature-based approach.

Possible Issues

The new verification system might introduce performance overhead when accessing component files.
If the signature verification fails, it could potentially break ad-blocking functionality for users.

Security Hotspots

The kComponentContentsVerifierPublicKey is hardcoded in the source code. Ensure this key is properly secured and rotated if necessary.
The ShouldBypassSignature() function allows bypassing the signature verification, which could be a security risk if misused.

Changes

Changes

browser/brave_browser_main_parts.cc:
- Added setup for component contents verifier
browser/brave_shields/ad_block_service_browsertest.cc:
- Updated tests to use the new ComponentContentsAccessor
- Added new test case for signed components
browser/component_updater/:
- Added new files for component contents verifier implementation
components/brave_component_updater/browser/:
- Added new ComponentContentsAccessor and ContentsVerifier classes
components/brave_shields/core/browser/:
- Updated various ad-block related classes to use the new ComponentContentsAccessor
test/data/adblock-components/:
- Added test data for verifying signed components

sequenceDiagram
    participant BraveBrowserMainParts
    participant ComponentUpdater
    participant AdBlockService
    participant ComponentContentsAccessor
    participant ContentsVerifier

    BraveBrowserMainParts->>ComponentUpdater: SetupComponentContentsVerifier()
    ComponentUpdater->>ContentsVerifier: Create
    AdBlockService->>ComponentUpdater: Request component update
    ComponentUpdater->>ComponentContentsAccessor: Create
    ComponentContentsAccessor->>ContentsVerifier: Verify contents
    ContentsVerifier-->>ComponentContentsAccessor: Verification result
    ComponentContentsAccessor-->>AdBlockService: Provide verified content
    AdBlockService->>AdBlockService: Update ad-blocking rules

boocmp self-assigned this Nov 20, 2024

boocmp force-pushed the components_verify branch 4 times, most recently from e876860 to cb708f8 Compare November 25, 2024 07:22

boocmp added the needs-security-review label Nov 25, 2024

boocmp marked this pull request as ready for review November 26, 2024 02:18

boocmp requested a review from a team as a code owner November 26, 2024 02:18

boocmp force-pushed the components_verify branch from c3df34c to 030307f Compare November 26, 2024 02:20

github-actions bot reviewed Nov 26, 2024

View reviewed changes

github-actions bot assigned bridiver, cdesouza-chromium, stoletheminerals and thypon Nov 26, 2024