diff --git a/patches/chromium/.patches b/patches/chromium/.patches index 8a7b8f0648ff4..a09f061839c3b 100644 --- a/patches/chromium/.patches +++ b/patches/chromium/.patches @@ -169,3 +169,4 @@ allow_null_skbitmap_to_be_transferred_across_threads.patch use_weakptrs_for_the_threadediconloader_s_background_tasks.patch cachestorage_store_partial_opaque_responses.patch cherry-pick-a5f54612590d.patch +m96_fileapi_move_origin_checks_in_bloburlstore_sooner.patch diff --git a/patches/chromium/m96_fileapi_move_origin_checks_in_bloburlstore_sooner.patch b/patches/chromium/m96_fileapi_move_origin_checks_in_bloburlstore_sooner.patch new file mode 100644 index 0000000000000..bd5f61a96a2ed --- /dev/null +++ b/patches/chromium/m96_fileapi_move_origin_checks_in_bloburlstore_sooner.patch @@ -0,0 +1,420 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Marijn Kruisselbrink +Date: Tue, 16 Nov 2021 22:16:15 +0000 +Subject: M96: [FileAPI] Move origin checks in BlobURLStore sooner. + +Rather than waiting to verify if a valid origin was passed in until +register/revoke time, check if the origin is valid at the time the +mojo interface is requested. This avoids the need to pass the +delegate on to BlobURLStoreImpl, further decoupling this from +BlobRegistryImpl. + +(cherry picked from commit 15cfa2bed3ce9dcdd0a06d3cd3b8330e3591acdc) + +Bug: 1262183 +Change-Id: I4889685d03501158abfe4f87c647dc468d005f78 +Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3264353 +Commit-Queue: Marijn Kruisselbrink +Reviewed-by: Alex Moshchuk +Cr-Original-Commit-Position: refs/heads/main@{#940015} +Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3285385 +Bot-Commit: Rubber Stamper +Cr-Commit-Position: refs/branch-heads/4664@{#1078} +Cr-Branched-From: 24dc4ee75e01a29d390d43c9c264372a169273a7-refs/heads/main@{#929512} + +diff --git a/chrome/browser/chrome_security_exploit_browsertest.cc b/chrome/browser/chrome_security_exploit_browsertest.cc +index fd8ec071c4aa52ed59e9c9dbad5b813840ee2510..2ffc8f4b29e2573ed862468ff34b26190dac1372 100644 +--- a/chrome/browser/chrome_security_exploit_browsertest.cc ++++ b/chrome/browser/chrome_security_exploit_browsertest.cc +@@ -482,8 +482,8 @@ IN_PROC_BROWSER_TEST_F(ChromeSecurityExploitBrowserTestMojoBlobURLs, + + // If the process is killed, this test passes. + EXPECT_EQ( +- "Received bad user message: Non committable URL passed to " +- "BlobURLStore::Register", ++ "Received bad user message: " ++ "URL with invalid origin passed to BlobURLStore::Register", + crash_observer.Wait()); + } + +@@ -518,7 +518,7 @@ IN_PROC_BROWSER_TEST_F(ChromeSecurityExploitBrowserTestMojoBlobURLs, + + // If the process is killed, this test passes. + EXPECT_EQ( +- "Received bad user message: Non committable URL passed to " +- "BlobURLStore::Register", ++ "Received bad user message: " ++ "URL with invalid origin passed to BlobURLStore::Register", + crash_observer.Wait()); + } +diff --git a/content/browser/blob_storage/blob_url_unittest.cc b/content/browser/blob_storage/blob_url_unittest.cc +index 0a7e8fb9285f0efcfdedfab1ecb70eb7fa61cfdf..9c2bac10bbb76c44adfa52d27d00151538ae336a 100644 +--- a/content/browser/blob_storage/blob_url_unittest.cc ++++ b/content/browser/blob_storage/blob_url_unittest.cc +@@ -182,15 +182,14 @@ class BlobURLTest : public testing::Test { + + void TestRequest(const std::string& method, + const net::HttpRequestHeaders& extra_headers) { +- GURL url("blob:blah"); ++ auto origin = url::Origin::Create(GURL("https://example.com")); ++ auto url = GURL("blob:" + origin.Serialize() + "/id1"); + network::ResourceRequest request; + request.url = url; + request.method = method; + request.headers = extra_headers; + +- storage::MockBlobRegistryDelegate delegate; +- storage::BlobURLStoreImpl url_store(blob_url_registry_.AsWeakPtr(), +- &delegate); ++ storage::BlobURLStoreImpl url_store(origin, blob_url_registry_.AsWeakPtr()); + + mojo::PendingRemote blob_remote; + storage::BlobImpl::Create( +diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc +index 1fd9f1ba1b07e4c890146f5b29bdff584e0fc95b..a436c9d5533800adf0551e97ba874cbb4498209c 100644 +--- a/content/browser/security_exploit_browsertest.cc ++++ b/content/browser/security_exploit_browsertest.cc +@@ -679,7 +679,7 @@ IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTestMojoBlobURLs, + // If the process is killed, this test passes. + EXPECT_EQ( + "Received bad user message: " +- "Non committable URL passed to BlobURLStore::Register", ++ "URL with invalid origin passed to BlobURLStore::Register", + crash_observer.Wait()); + } + +diff --git a/storage/browser/blob/blob_registry_impl.cc b/storage/browser/blob/blob_registry_impl.cc +index 34e736e80de6093dd400e4dae21d8ff4ff3180d6..2479803044e63b20964977eda75f89da31fb9529 100644 +--- a/storage/browser/blob/blob_registry_impl.cc ++++ b/storage/browser/blob/blob_registry_impl.cc +@@ -629,13 +629,14 @@ void BlobRegistryImpl::GetBlobFromUUID( + void BlobRegistryImpl::URLStoreForOrigin( + const url::Origin& origin, + mojo::PendingAssociatedReceiver receiver) { +- // TODO(mek): Pass origin on to BlobURLStoreImpl so it can use it to generate +- // Blob URLs, and verify at this point that the renderer can create URLs for +- // that origin. + Delegate* delegate = receivers_.current_context().get(); + DCHECK(delegate); ++ if (!origin.opaque() && !delegate->CanCommitURL(origin.GetURL())) { ++ mojo::ReportBadMessage( ++ "Non committable origin passed to BlobRegistryImpl::URLStoreForOrigin"); ++ } + auto self_owned_associated_receiver = mojo::MakeSelfOwnedAssociatedReceiver( +- std::make_unique(url_registry_, delegate), ++ std::make_unique(origin, url_registry_), + std::move(receiver)); + if (g_url_store_creation_hook) + g_url_store_creation_hook->Run(self_owned_associated_receiver); +diff --git a/storage/browser/blob/blob_url_store_impl.cc b/storage/browser/blob/blob_url_store_impl.cc +index b46ee60849c9b758e022d7e576de61b080fd59f5..d4edd988f5982788f5157d27fbfa31abcb1eede7 100644 +--- a/storage/browser/blob/blob_url_store_impl.cc ++++ b/storage/browser/blob/blob_url_store_impl.cc +@@ -5,6 +5,7 @@ + #include "storage/browser/blob/blob_url_store_impl.h" + + #include "base/bind.h" ++#include "base/strings/strcat.h" + #include "mojo/public/cpp/bindings/receiver_set.h" + #include "storage/browser/blob/blob_impl.h" + #include "storage/browser/blob/blob_url_loader_factory.h" +@@ -58,9 +59,9 @@ class BlobURLTokenImpl : public blink::mojom::BlobURLToken { + const base::UnguessableToken token_; + }; + +-BlobURLStoreImpl::BlobURLStoreImpl(base::WeakPtr registry, +- BlobRegistryImpl::Delegate* delegate) +- : registry_(std::move(registry)), delegate_(delegate) {} ++BlobURLStoreImpl::BlobURLStoreImpl(const url::Origin& origin, ++ base::WeakPtr registry) ++ : origin_(origin), registry_(std::move(registry)) {} + + BlobURLStoreImpl::~BlobURLStoreImpl() { + if (registry_) { +@@ -72,20 +73,9 @@ BlobURLStoreImpl::~BlobURLStoreImpl() { + void BlobURLStoreImpl::Register(mojo::PendingRemote blob, + const GURL& url, + RegisterCallback callback) { +- if (!url.SchemeIsBlob()) { +- mojo::ReportBadMessage("Invalid scheme passed to BlobURLStore::Register"); +- std::move(callback).Run(); +- return; +- } +- if (!delegate_->CanCommitURL(url)) { +- mojo::ReportBadMessage( +- "Non committable URL passed to BlobURLStore::Register"); +- std::move(callback).Run(); +- return; +- } +- if (BlobUrlUtils::UrlHasFragment(url)) { +- mojo::ReportBadMessage( +- "URL with fragment passed to BlobURLStore::Register"); ++ // TODO(mek): Generate blob URLs here, rather than validating the URLs the ++ // renderer process generated. ++ if (!BlobUrlIsValid(url, "Register")) { + std::move(callback).Run(); + return; + } +@@ -97,19 +87,8 @@ void BlobURLStoreImpl::Register(mojo::PendingRemote blob, + } + + void BlobURLStoreImpl::Revoke(const GURL& url) { +- if (!url.SchemeIsBlob()) { +- mojo::ReportBadMessage("Invalid scheme passed to BlobURLStore::Revoke"); +- return; +- } +- if (!delegate_->CanCommitURL(url)) { +- mojo::ReportBadMessage( +- "Non committable URL passed to BlobURLStore::Revoke"); ++ if (!BlobUrlIsValid(url, "Revoke")) + return; +- } +- if (BlobUrlUtils::UrlHasFragment(url)) { +- mojo::ReportBadMessage("URL with fragment passed to BlobURLStore::Revoke"); +- return; +- } + + if (registry_) + registry_->RemoveUrlMapping(url); +@@ -144,4 +123,38 @@ void BlobURLStoreImpl::ResolveForNavigation( + new BlobURLTokenImpl(registry_, url, std::move(blob), std::move(token)); + } + ++bool BlobURLStoreImpl::BlobUrlIsValid(const GURL& url, ++ const char* method) const { ++ if (!url.SchemeIsBlob()) { ++ mojo::ReportBadMessage( ++ base::StrCat({"Invalid scheme passed to BlobURLStore::", method})); ++ return false; ++ } ++ url::Origin url_origin = url::Origin::Create(url); ++ // For file:// origins blink sometimes creates blob URLs with "null" as origin ++ // and other times "file://" (based on a runtime setting). On the other hand, ++ // `origin_` will always be a non-opaque file: origin for pages loaded from ++ // file:// URLs. To deal with this, we treat file:// origins and ++ // opaque origins separately from non-opaque origins. ++ bool valid_origin = true; ++ if (url_origin.scheme() == url::kFileScheme) { ++ valid_origin = origin_.scheme() == url::kFileScheme; ++ } else if (url_origin.opaque()) { ++ valid_origin = origin_.opaque() || origin_.scheme() == url::kFileScheme; ++ } else { ++ valid_origin = origin_ == url_origin; ++ } ++ if (!valid_origin) { ++ mojo::ReportBadMessage(base::StrCat( ++ {"URL with invalid origin passed to BlobURLStore::", method})); ++ return false; ++ } ++ if (BlobUrlUtils::UrlHasFragment(url)) { ++ mojo::ReportBadMessage( ++ base::StrCat({"URL with fragment passed to BlobURLStore::", method})); ++ return false; ++ } ++ return true; ++} ++ + } // namespace storage +diff --git a/storage/browser/blob/blob_url_store_impl.h b/storage/browser/blob/blob_url_store_impl.h +index 6b4a738a4646f8d8e35e8bb1be5aba391e612917..5db371d7c6d50520d8b40a3b4cb6d6bbf920b570 100644 +--- a/storage/browser/blob/blob_url_store_impl.h ++++ b/storage/browser/blob/blob_url_store_impl.h +@@ -11,8 +11,9 @@ + #include "mojo/public/cpp/bindings/pending_receiver.h" + #include "mojo/public/cpp/bindings/pending_remote.h" + #include "mojo/public/cpp/bindings/remote.h" +-#include "storage/browser/blob/blob_registry_impl.h" ++#include "storage/browser/blob/blob_url_registry.h" + #include "third_party/blink/public/mojom/blob/blob_url_store.mojom.h" ++#include "url/origin.h" + + namespace storage { + +@@ -21,8 +22,9 @@ class BlobUrlRegistry; + class COMPONENT_EXPORT(STORAGE_BROWSER) BlobURLStoreImpl + : public blink::mojom::BlobURLStore { + public: +- BlobURLStoreImpl(base::WeakPtr registry, +- BlobRegistryImpl::Delegate* delegate); ++ BlobURLStoreImpl(const url::Origin& origin, ++ base::WeakPtr registry); ++ + ~BlobURLStoreImpl() override; + + void Register(mojo::PendingRemote blob, +@@ -39,8 +41,12 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) BlobURLStoreImpl + mojo::PendingReceiver token) override; + + private: ++ // Checks if the passed in url is a valid blob url for this blob url store. ++ // Returns false and reports a bad mojo message if not. ++ bool BlobUrlIsValid(const GURL& url, const char* method) const; ++ ++ const url::Origin origin_; + base::WeakPtr registry_; +- BlobRegistryImpl::Delegate* delegate_; + + std::set urls_; + +diff --git a/storage/browser/blob/blob_url_store_impl_unittest.cc b/storage/browser/blob/blob_url_store_impl_unittest.cc +index 69428d0adebbc615fbeedb6b097d43ec6d513e2c..52d17152b3841ea655efd22d3c0a277762994d03 100644 +--- a/storage/browser/blob/blob_url_store_impl_unittest.cc ++++ b/storage/browser/blob/blob_url_store_impl_unittest.cc +@@ -17,7 +17,6 @@ + #include "storage/browser/blob/blob_impl.h" + #include "storage/browser/blob/blob_storage_context.h" + #include "storage/browser/blob/blob_url_registry.h" +-#include "storage/browser/test/mock_blob_registry_delegate.h" + #include "testing/gtest/include/gtest/gtest.h" + + using blink::mojom::BlobURLStore; +@@ -69,9 +68,9 @@ class BlobURLStoreImplTest : public testing::Test { + + mojo::PendingRemote CreateURLStore() { + mojo::PendingRemote result; +- mojo::MakeSelfOwnedReceiver(std::make_unique( +- url_registry_.AsWeakPtr(), &delegate_), +- result.InitWithNewPipeAndPassReceiver()); ++ mojo::MakeSelfOwnedReceiver( ++ std::make_unique(kOrigin, url_registry_.AsWeakPtr()), ++ result.InitWithNewPipeAndPassReceiver()); + return result; + } + +@@ -101,15 +100,19 @@ class BlobURLStoreImplTest : public testing::Test { + } + + const std::string kId = "id"; +- const GURL kValidUrl = GURL("blob:id"); ++ const url::Origin kOrigin = url::Origin::Create(GURL("https://example.com")); ++ const GURL kValidUrl = GURL("blob:" + kOrigin.Serialize() + "/id1"); ++ const GURL kValidUrl2 = GURL("blob:" + kOrigin.Serialize() + "/id2"); + const GURL kInvalidUrl = GURL("bolb:id"); +- const GURL kFragmentUrl = GURL("blob:id#fragment"); ++ const GURL kFragmentUrl = GURL(kValidUrl.spec() + "#fragment"); ++ const url::Origin kWrongOrigin = ++ url::Origin::Create(GURL("https://test.com")); ++ const GURL kWrongOriginUrl = GURL("blob:" + kWrongOrigin.Serialize() + "/id"); + + protected: + base::test::TaskEnvironment task_environment_; + std::unique_ptr context_; + BlobUrlRegistry url_registry_; +- MockBlobRegistryDelegate delegate_; + std::vector bad_messages_; + }; + +@@ -118,7 +121,7 @@ TEST_F(BlobURLStoreImplTest, BasicRegisterRevoke) { + CreateBlobFromString(kId, "hello world"); + + // Register a URL and make sure the URL keeps the blob alive. +- BlobURLStoreImpl url_store(url_registry_.AsWeakPtr(), &delegate_); ++ BlobURLStoreImpl url_store(kOrigin, url_registry_.AsWeakPtr()); + RegisterURL(&url_store, std::move(blob), kValidUrl); + + blob = url_registry_.GetBlobFromUrl(kValidUrl); +@@ -146,15 +149,13 @@ TEST_F(BlobURLStoreImplTest, RegisterInvalidScheme) { + EXPECT_EQ(1u, bad_messages_.size()); + } + +-TEST_F(BlobURLStoreImplTest, RegisterCantCommit) { ++TEST_F(BlobURLStoreImplTest, RegisterWrongOrigin) { + mojo::PendingRemote blob = + CreateBlobFromString(kId, "hello world"); + +- delegate_.can_commit_url_result = false; +- + mojo::Remote url_store(CreateURLStore()); +- RegisterURL(url_store.get(), std::move(blob), kValidUrl); +- EXPECT_FALSE(url_registry_.GetBlobFromUrl(kValidUrl)); ++ RegisterURL(url_store.get(), std::move(blob), kWrongOriginUrl); ++ EXPECT_FALSE(url_registry_.GetBlobFromUrl(kWrongOriginUrl)); + EXPECT_EQ(1u, bad_messages_.size()); + } + +@@ -169,14 +170,13 @@ TEST_F(BlobURLStoreImplTest, RegisterUrlFragment) { + } + + TEST_F(BlobURLStoreImplTest, ImplicitRevoke) { +- const GURL kValidUrl2("blob:id2"); + mojo::Remote blob( + CreateBlobFromString(kId, "hello world")); + mojo::PendingRemote blob2; + blob->Clone(blob2.InitWithNewPipeAndPassReceiver()); + + auto url_store = +- std::make_unique(url_registry_.AsWeakPtr(), &delegate_); ++ std::make_unique(kOrigin, url_registry_.AsWeakPtr()); + RegisterURL(url_store.get(), blob.Unbind(), kValidUrl); + EXPECT_TRUE(url_registry_.GetBlobFromUrl(kValidUrl)); + RegisterURL(url_store.get(), std::move(blob2), kValidUrl2); +@@ -192,8 +192,8 @@ TEST_F(BlobURLStoreImplTest, RevokeThroughDifferentURLStore) { + mojo::PendingRemote blob = + CreateBlobFromString(kId, "hello world"); + +- BlobURLStoreImpl url_store1(url_registry_.AsWeakPtr(), &delegate_); +- BlobURLStoreImpl url_store2(url_registry_.AsWeakPtr(), &delegate_); ++ BlobURLStoreImpl url_store1(kOrigin, url_registry_.AsWeakPtr()); ++ BlobURLStoreImpl url_store2(kOrigin, url_registry_.AsWeakPtr()); + + RegisterURL(&url_store1, std::move(blob), kValidUrl); + EXPECT_TRUE(url_registry_.GetBlobFromUrl(kValidUrl)); +@@ -209,11 +209,9 @@ TEST_F(BlobURLStoreImplTest, RevokeInvalidScheme) { + EXPECT_EQ(1u, bad_messages_.size()); + } + +-TEST_F(BlobURLStoreImplTest, RevokeCantCommit) { +- delegate_.can_commit_url_result = false; +- ++TEST_F(BlobURLStoreImplTest, RevokeWrongOrigin) { + mojo::Remote url_store(CreateURLStore()); +- url_store->Revoke(kValidUrl); ++ url_store->Revoke(kWrongOriginUrl); + url_store.FlushForTesting(); + EXPECT_EQ(1u, bad_messages_.size()); + } +@@ -229,7 +227,7 @@ TEST_F(BlobURLStoreImplTest, Resolve) { + mojo::PendingRemote blob = + CreateBlobFromString(kId, "hello world"); + +- BlobURLStoreImpl url_store(url_registry_.AsWeakPtr(), &delegate_); ++ BlobURLStoreImpl url_store(kOrigin, url_registry_.AsWeakPtr()); + RegisterURL(&url_store, std::move(blob), kValidUrl); + + blob = ResolveURL(&url_store, kValidUrl); +@@ -244,7 +242,7 @@ TEST_F(BlobURLStoreImplTest, Resolve) { + } + + TEST_F(BlobURLStoreImplTest, ResolveNonExistentURL) { +- BlobURLStoreImpl url_store(url_registry_.AsWeakPtr(), &delegate_); ++ BlobURLStoreImpl url_store(kOrigin, url_registry_.AsWeakPtr()); + + mojo::PendingRemote blob = + ResolveURL(&url_store, kValidUrl); +@@ -254,7 +252,7 @@ TEST_F(BlobURLStoreImplTest, ResolveNonExistentURL) { + } + + TEST_F(BlobURLStoreImplTest, ResolveInvalidURL) { +- BlobURLStoreImpl url_store(url_registry_.AsWeakPtr(), &delegate_); ++ BlobURLStoreImpl url_store(kOrigin, url_registry_.AsWeakPtr()); + + mojo::PendingRemote blob = + ResolveURL(&url_store, kInvalidUrl); +@@ -265,7 +263,7 @@ TEST_F(BlobURLStoreImplTest, ResolveAsURLLoaderFactory) { + mojo::PendingRemote blob = + CreateBlobFromString(kId, "hello world"); + +- BlobURLStoreImpl url_store(url_registry_.AsWeakPtr(), &delegate_); ++ BlobURLStoreImpl url_store(kOrigin, url_registry_.AsWeakPtr()); + RegisterURL(&url_store, std::move(blob), kValidUrl); + + mojo::Remote factory; +@@ -291,7 +289,7 @@ TEST_F(BlobURLStoreImplTest, ResolveForNavigation) { + mojo::PendingRemote blob = + CreateBlobFromString(kId, "hello world"); + +- BlobURLStoreImpl url_store(url_registry_.AsWeakPtr(), &delegate_); ++ BlobURLStoreImpl url_store(kOrigin, url_registry_.AsWeakPtr()); + RegisterURL(&url_store, std::move(blob), kValidUrl); + + mojo::Remote token_remote;