Add support for blob partitioning. #12686

goodov · 2022-03-21T16:22:17Z

Added blob partitioning.

The idea is to use our custom ephemeral opaque origins which are safely passed via renderer<->browser boundary, but additionally carry a nonce that we can use on browser side to effectively partition the blob URLs by crafting a custom URL path that is mapped to a blob.

Resolves brave/brave-browser#21746

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
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, npm run lint, 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:

Test Plan:

goodov · 2022-03-28T14:23:08Z

The PR is ready for review.

goodov · 2022-04-01T13:36:26Z

@iefremov @fmarier @bridiver PTAL.

fmarier

I didn't look at the code in details, but the overall approach looks good to me.

bridiver · 2022-04-05T19:53:08Z

chromium_src/chrome/renderer/worker_content_settings_client.h

-#define BRAVE_WORKER_CONTENT_SETTINGS_CLIENT_H         \
-  BraveFarblingLevel GetBraveFarblingLevel() override; \
-  bool AllowFingerprinting(bool enabled_per_settings) override;
+#define BRAVE_WORKER_CONTENT_SETTINGS_CLIENT_H                  \


unrelated to this PR - why are we doing a chromium src override here instead of BraveContentRendererClient::CreateWorkerContentSettingsClient ?

bridiver · 2022-04-05T19:55:44Z

patches/storage-browser-blob-blob_url_store_impl.cc.patch

+   } else {
+     valid_origin = origin_ == url_origin;
+   }
+  valid_origin |= IsBlobUrlValidForPartitionedOrigin(origin_, url_origin);


these patches don't follow patching standards, please replace with a define from a chromium src override

bridiver · 2022-04-05T20:09:46Z

chromium_src/url/origin.h

+#define GetTupleOrPrecursorTupleIfOpaque                                    \
+  NotUsed() const;                                                          \
+  const base::UnguessableToken& GetNonceForEphemeralStorageKeying() const { \
+    return nonce_->raw_token();                                             \


why are we getting the raw token here?

// Do not use in cases where lazy initialization is expected! This // accessor does not initialize the |token_| member.

the token must be initialized before. If an origin is opaque, but token is not initialized, then it's an error and this whole thing shouldn't work. I've added some checks to ensure this is used properly.

bridiver · 2022-04-05T20:25:23Z

patches/third_party-blink-renderer-core-fileapi-public_url_manager.cc.patch

+       is_stopped_(false),
+       url_store_(context) {
+   BlobDataHandle::GetBlobRegistry()->URLStoreForOrigin(
+-      context->GetSecurityOrigin(),


I think you can get rid of this patch with something like

#define URLStoreForOrigin(ORIGIN, URL_STORE) URLStoreForOrigin(!GetEphemeralOrOriginalSecurityOrigin(context).is_null() ? GetEphemeralOrOriginalSecurityOrigin(context) : ORIGIN, URL_STORE)

actually why do we need this patch at all? This ends up calling BlobURLStoreImpl which we are overriding anyway

I guess we don't have access to the content settings there, but according to @pes10k this doesn't even need to be dependent on content settings which could potentially simplify a lot of this.

I think you can get rid of this patch with something like #define URLStoreForOrigin ....

Thanks, this is much better.

actually why do we need this patch at all? This ends up calling BlobURLStoreImpl which we are overriding anyway
I guess we don't have access to the content settings there

yes, we need to pass ephemeral storage origin which is available only on the renderer side. We can't do this inside BlobURLStoreImpl which is located in Storage process.

but according to @pes10k this doesn't even need to be dependent on content settings which could potentially simplify a lot of this.

Our ephemeral storage approach relies on content settings to create and get an ephemeral storage key.

bridiver · 2022-04-05T20:37:15Z

browser/ephemeral_storage/blob_url_browsertest.cc

+  }
+}
+
+// Tests for the blob: URL scheme, originally implemented in


We're copying upstream tests here? That seems problematic to me because they will get out of sync

We need to have Blob tests now. These particular tests are pretty standard, there's no some weird upstream-specific test helpers which were made exactly for Blob tests, that's why I pulled those.
I did drop Chromium relation from the name and we can just think of it as our tests.

bridiver · 2022-04-05T20:48:37Z

browser/ephemeral_storage/blob_url_browsertest.cc

+  }
+};
+
+IN_PROC_BROWSER_TEST_F(BlobUrlBrowserTest, BlobsArePartitioned) {


we need to test that they are not partitioned when the feature is off and also test based on content settings since we're checking those here https://github.com/brave/brave-core/pull/12686/files#diff-430f56739d7df3701057dcb1c3ea5412094160cc91008cfd596d42d889e4d8d4R72

we also need to check that it's disabled when the main ephemeral storage is disabled and check that only 1p is disabled when the 1p flag is disabled

added more test, including 1pes check, shields on/off states and feature on/off states.

chromium_src/chrome/renderer/worker_content_settings_client.cc

bridiver · 2022-04-07T00:14:26Z

chromium_src/url/origin.h

+#define GetTupleOrPrecursorTupleIfOpaque                                   \
+  NotUsed() const;                                                         \
+  /* Checks if origin is opaque and nonce is initialized. */               \
+  bool CanUseNonceForEphemeralStorageKeying() const;                       \


I don't think this method needs to be on Origin, does it? It doesn't seem right to me to attach ephemeral storage specific logic to Origin and we have access to both opaque() and now the raw_token through GetNonceForEphemeralStorageKeying. Related I think we should change GetNonceForEphemeralStorageKeying -> raw_token() or something. Maybe both methods can become utility functions that take Origin as a param?

agree, it doesn't have to be here. Moved it into a dedicated helper to access Origin internals related to Ephemeral Storage more explicitly.

goodov requested review from a team as code owners March 21, 2022 16:22

github-actions bot assigned goodov Mar 21, 2022

github-actions bot added the CI/run-network-audit Run network-audit label Mar 21, 2022

goodov force-pushed the issues/21746 branch 2 times, most recently from d5cf988 to a3f64d3 Compare March 23, 2022 13:51

goodov added the CI/run-upstream-tests label Mar 24, 2022

goodov force-pushed the issues/21746 branch 2 times, most recently from 9eddc22 to f719542 Compare March 24, 2022 16:01

goodov removed the CI/run-upstream-tests label Mar 25, 2022

goodov force-pushed the issues/21746 branch 2 times, most recently from 7fbf1a6 to b7b07ba Compare March 25, 2022 08:38

goodov requested review from fmarier, iefremov and bridiver March 25, 2022 08:40

fmarier approved these changes Apr 4, 2022

View reviewed changes

bridiver reviewed Apr 5, 2022

View reviewed changes

chromium_src/chrome/renderer/worker_content_settings_client.cc Show resolved Hide resolved

goodov added 4 commits April 6, 2022 18:12

Add support for blob partitioning.

cb183d2

Improve chromium overrides.

daf7727

Add more tests, including feature on/off and 1PES mode.

fe7fad6

Make Origin nonce usage for ephemeral storage explicit and validated.

caa9ec4

goodov force-pushed the issues/21746 branch from b7b07ba to caa9ec4 Compare April 6, 2022 12:33

iefremov approved these changes Apr 6, 2022

View reviewed changes

bridiver reviewed Apr 7, 2022

View reviewed changes

Move Origin nonce accessors to a dedicated helper.

d52c75a

bridiver approved these changes Apr 7, 2022

View reviewed changes

goodov added this to the 1.39.x - Nightly milestone Apr 8, 2022

goodov merged commit fabd862 into master Apr 8, 2022

goodov deleted the issues/21746 branch April 8, 2022 07:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for blob partitioning. #12686

Add support for blob partitioning. #12686

goodov commented Mar 21, 2022 •

edited

goodov commented Mar 28, 2022

goodov commented Apr 1, 2022

fmarier left a comment

bridiver Apr 5, 2022

bridiver Apr 5, 2022

goodov Apr 6, 2022

bridiver Apr 5, 2022

goodov Apr 6, 2022

bridiver Apr 5, 2022

bridiver Apr 5, 2022

bridiver Apr 5, 2022

goodov Apr 6, 2022

bridiver Apr 5, 2022

goodov Apr 6, 2022 •

edited

bridiver Apr 5, 2022

bridiver Apr 5, 2022

goodov Apr 6, 2022

bridiver Apr 7, 2022

goodov Apr 7, 2022

Add support for blob partitioning. #12686

Add support for blob partitioning. #12686

Conversation

goodov commented Mar 21, 2022 • edited

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

goodov commented Mar 28, 2022

goodov commented Apr 1, 2022

fmarier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goodov Apr 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goodov commented Mar 21, 2022 •

edited

goodov Apr 6, 2022 •

edited