Better deal with invalid blob URLs in future mojo code path.

BlobEntryRegistry will DCHECK when presented with invalid blob URLs,
so both make sure the renderer doesn't send such URLs, and kill the
renderer if it does happen to do so.

Bug: 756743

Change-Id: Ife653cc4d8687fc30c8a03336caa66905951556b
Reviewed-on: https://chromium-review.googlesource.com/884522Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532036}

storage/browser/BUILD.gn

@@ -36,6 +36,8 @@ component("browser") {
     "blob/blob_url_request_job_factory.h",
     "blob/blob_url_store_impl.cc",
     "blob/blob_url_store_impl.h",
+    "blob/blob_url_utils.cc",
+    "blob/blob_url_utils.h",
     "blob/mojo_blob_reader.cc",
     "blob/mojo_blob_reader.h",
     "blob/scoped_file.cc",

storage/browser/blob/blob_storage_registry.cc

@@ -12,29 +12,11 @@
 #include "base/location.h"
 #include "base/logging.h"
 #include "storage/browser/blob/blob_entry.h"
+#include "storage/browser/blob/blob_url_utils.h"
 #include "url/gurl.h"
 
 namespace storage {
 
-namespace {
-// We can't use GURL directly for these hash fragment manipulations
-// since it doesn't have specific knowlege of the BlobURL format. GURL
-// treats BlobURLs as if they were PathURLs which don't support hash
-// fragments.
-
-bool BlobUrlHasRef(const GURL& url) {
-  return url.spec().find('#') != std::string::npos;
-}
-
-GURL ClearBlobUrlRef(const GURL& url) {
-  size_t hash_pos = url.spec().find('#');
-  if (hash_pos == std::string::npos)
-    return url;
-  return GURL(url.spec().substr(0, hash_pos));
-}
-
-}  // namespace
-
 BlobStorageRegistry::BlobStorageRegistry() = default;
 
 BlobStorageRegistry::~BlobStorageRegistry() {
@@ -76,7 +58,7 @@ const BlobEntry* BlobStorageRegistry::GetEntry(const std::string& uuid) const {
 
 bool BlobStorageRegistry::CreateUrlMapping(const GURL& blob_url,
                                            const std::string& uuid) {
-  DCHECK(!BlobUrlHasRef(blob_url));
+  DCHECK(!BlobUrlUtils::UrlHasFragment(blob_url));
   if (blob_map_.find(uuid) == blob_map_.end() || IsURLMapped(blob_url))
     return false;
   url_to_uuid_[blob_url] = uuid;
@@ -85,7 +67,7 @@ bool BlobStorageRegistry::CreateUrlMapping(const GURL& blob_url,
 
 bool BlobStorageRegistry::DeleteURLMapping(const GURL& blob_url,
                                            std::string* uuid) {
-  DCHECK(!BlobUrlHasRef(blob_url));
+  DCHECK(!BlobUrlUtils::UrlHasFragment(blob_url));
   auto found = url_to_uuid_.find(blob_url);
   if (found == url_to_uuid_.end())
     return false;
@@ -101,8 +83,7 @@ bool BlobStorageRegistry::IsURLMapped(const GURL& blob_url) const {
 
 BlobEntry* BlobStorageRegistry::GetEntryFromURL(const GURL& url,
                                                 std::string* uuid) {
-  auto found =
-      url_to_uuid_.find(BlobUrlHasRef(url) ? ClearBlobUrlRef(url) : url);
+  auto found = url_to_uuid_.find(BlobUrlUtils::ClearUrlFragment(url));
   if (found == url_to_uuid_.end())
     return nullptr;
   BlobEntry* entry = GetEntry(found->second);

storage/browser/blob/blob_url_store_impl.cc

@@ -6,6 +6,7 @@
 
 #include "storage/browser/blob/blob_impl.h"
 #include "storage/browser/blob/blob_storage_context.h"
+#include "storage/browser/blob/blob_url_utils.h"
 
 namespace storage {
 
@@ -25,7 +26,8 @@ BlobURLStoreImpl::~BlobURLStoreImpl() {
 void BlobURLStoreImpl::Register(blink::mojom::BlobPtr blob,
                                 const GURL& url,
                                 RegisterCallback callback) {
-  if (!url.SchemeIsBlob() || !delegate_->CanCommitURL(url)) {
+  if (!url.SchemeIsBlob() || !delegate_->CanCommitURL(url) ||
+      BlobUrlUtils::UrlHasFragment(url)) {
     mojo::ReportBadMessage("Invalid Blob URL passed to BlobURLStore::Register");
     std::move(callback).Run();
     return;
@@ -38,6 +40,11 @@ void BlobURLStoreImpl::Register(blink::mojom::BlobPtr blob,
 }
 
 void BlobURLStoreImpl::Revoke(const GURL& url) {
+  if (!url.SchemeIsBlob() || !delegate_->CanCommitURL(url) ||
+      BlobUrlUtils::UrlHasFragment(url)) {
+    mojo::ReportBadMessage("Invalid Blob URL passed to BlobURLStore::Revoke");
+    return;
+  }
   if (context_)
     context_->RevokePublicBlobURL(url);
   urls_.erase(url);

storage/browser/blob/blob_url_store_impl_unittest.cc

@@ -93,6 +93,7 @@ class BlobURLStoreImplTest : public testing::Test {
   const std::string kId = "id";
   const GURL kValidUrl = GURL("blob:id");
   const GURL kInvalidUrl = GURL("bolb:id");
+  const GURL kFragmentUrl = GURL("blob:id#fragment");
 
  protected:
   base::test::ScopedTaskEnvironment scoped_task_environment_;
@@ -143,6 +144,15 @@ TEST_F(BlobURLStoreImplTest, RegisterCantCommit) {
   EXPECT_EQ(1u, bad_messages_.size());
 }
 
+TEST_F(BlobURLStoreImplTest, RegisterUrlFragment) {
+  BlobPtr blob = CreateBlobFromString(kId, "hello world");
+
+  BlobURLStorePtr url_store = CreateURLStore();
+  RegisterURL(url_store.get(), std::move(blob), kFragmentUrl);
+  EXPECT_FALSE(context_->GetBlobDataFromPublicURL(kFragmentUrl));
+  EXPECT_EQ(1u, bad_messages_.size());
+}
+
 TEST_F(BlobURLStoreImplTest, ImplicitRevoke) {
   const GURL kValidUrl2("blob:id2");
   BlobPtr blob = CreateBlobFromString(kId, "hello world");
@@ -175,6 +185,29 @@ TEST_F(BlobURLStoreImplTest, RevokeThroughDifferentURLStore) {
   EXPECT_FALSE(context_->GetBlobDataFromPublicURL(kValidUrl));
 }
 
+TEST_F(BlobURLStoreImplTest, RevokeInvalidScheme) {
+  BlobURLStorePtr url_store = CreateURLStore();
+  url_store->Revoke(kInvalidUrl);
+  url_store.FlushForTesting();
+  EXPECT_EQ(1u, bad_messages_.size());
+}
+
+TEST_F(BlobURLStoreImplTest, RevokeCantCommit) {
+  delegate_.can_commit_url_result = false;
+
+  BlobURLStorePtr url_store = CreateURLStore();
+  url_store->Revoke(kValidUrl);
+  url_store.FlushForTesting();
+  EXPECT_EQ(1u, bad_messages_.size());
+}
+
+TEST_F(BlobURLStoreImplTest, RevokeURLWithFragment) {
+  BlobURLStorePtr url_store = CreateURLStore();
+  url_store->Revoke(kFragmentUrl);
+  url_store.FlushForTesting();
+  EXPECT_EQ(1u, bad_messages_.size());
+}
+
 TEST_F(BlobURLStoreImplTest, Resolve) {
   BlobPtr blob = CreateBlobFromString(kId, "hello world");
 
@@ -184,6 +217,9 @@ TEST_F(BlobURLStoreImplTest, Resolve) {
   blob = ResolveURL(&url_store, kValidUrl);
   ASSERT_TRUE(blob);
   EXPECT_EQ(kId, UUIDFromBlob(blob.get()));
+  blob = ResolveURL(&url_store, kFragmentUrl);
+  ASSERT_TRUE(blob);
+  EXPECT_EQ(kId, UUIDFromBlob(blob.get()));
 }
 
 TEST_F(BlobURLStoreImplTest, ResolveNonExistentURL) {
@@ -191,6 +227,15 @@ TEST_F(BlobURLStoreImplTest, ResolveNonExistentURL) {
 
   BlobPtr blob = ResolveURL(&url_store, kValidUrl);
   EXPECT_FALSE(blob);
+  blob = ResolveURL(&url_store, kFragmentUrl);
+  EXPECT_FALSE(blob);
+}
+
+TEST_F(BlobURLStoreImplTest, ResolveInvalidURL) {
+  BlobURLStoreImpl url_store(context_->AsWeakPtr(), &delegate_);
+
+  BlobPtr blob = ResolveURL(&url_store, kInvalidUrl);
+  EXPECT_FALSE(blob);
 }
 
 }  // namespace storage

storage/browser/blob/blob_url_utils.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "storage/browser/blob/blob_url_utils.h"
+
+namespace storage {
+
+namespace BlobUrlUtils {
+
+bool UrlHasFragment(const GURL& url) {
+  return url.spec().find('#') != std::string::npos;
+}
+
+GURL ClearUrlFragment(const GURL& url) {
+  size_t hash_pos = url.spec().find('#');
+  if (hash_pos == std::string::npos)
+    return url;
+  return GURL(url.spec().substr(0, hash_pos));
+}
+
+}  // namespace BlobUrlUtils
+}  // namespace storage

storage/browser/blob/blob_url_utils.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "url/gurl.h"
+
+namespace storage {
+
+namespace BlobUrlUtils {
+// We can't use GURL directly for these hash fragment manipulations
+// since it doesn't have specific knowlege of the BlobURL format. GURL
+// treats BlobURLs as if they were PathURLs which don't support hash
+// fragments.
+
+// Returns true iff |url| contains a hash fragment.
+bool UrlHasFragment(const GURL& url);
+
+// Returns |url| with any potential hash fragment stripped. If |url| didn't
+// contain such a fragment this returns |url| unchanged.
+GURL ClearUrlFragment(const GURL& url);
+
+}  // namespace BlobUrlUtils
+}  // namespace storage

third_party/WebKit/Source/core/fileapi/PublicURLManager.cpp

@@ -129,10 +129,17 @@ String PublicURLManager::RegisterURL(URLRegistrable* registrable) {
 }
 
 void PublicURLManager::Revoke(const KURL& url) {
-  if (!url.ProtocolIs("blob"))
+  if (is_stopped_)
+    return;
+  // Don't bother trying to revoke URLs that can't have been registered anyway.
+  if (!url.ProtocolIs("blob") || url.HasFragmentIdentifier())
+    return;
+  // Don't support revoking cross-origin blob URLs.
+  if (!SecurityOrigin::Create(url)->IsSameSchemeHostPort(
+          GetExecutionContext()->GetSecurityOrigin()))
     return;
 
-  if (RuntimeEnabledFeatures::MojoBlobURLsEnabled() && !is_stopped_) {
+  if (RuntimeEnabledFeatures::MojoBlobURLsEnabled()) {
     if (!url_store_) {
       BlobDataHandle::GetBlobRegistry()->URLStoreForOrigin(
           GetExecutionContext()->GetSecurityOrigin(), MakeRequest(&url_store_));

third_party/WebKit/Source/core/fileapi/PublicURLManagerTest.cpp

@@ -116,6 +116,11 @@ TEST_F(PublicURLManagerTest, RegisterNonMojoBlob) {
   url_manager().Revoke(KURL(url));
   EXPECT_FALSE(SecurityOrigin::CreateFromString(url)->IsSameSchemeHostPort(
       execution_context_->GetSecurityOrigin()));
+  url_store_binding_.FlushForTesting();
+  // Even though this was not a mojo blob, the PublicURLManager might not know
+  // that, so still expect a revocation on the mojo interface.
+  ASSERT_EQ(1u, url_store_.revocations.size());
+  EXPECT_EQ(url, url_store_.revocations[0]);
 }
 
 TEST_F(PublicURLManagerTest, RegisterMojoBlob) {
@@ -135,6 +140,35 @@ TEST_F(PublicURLManagerTest, RegisterMojoBlob) {
   url_manager().Revoke(KURL(url));
   EXPECT_FALSE(SecurityOrigin::CreateFromString(url)->IsSameSchemeHostPort(
       execution_context_->GetSecurityOrigin()));
+  url_store_binding_.FlushForTesting();
+  ASSERT_EQ(1u, url_store_.revocations.size());
+  EXPECT_EQ(url, url_store_.revocations[0]);
+}
+
+TEST_F(PublicURLManagerTest, RevokeValidNonRegisteredURL) {
+  execution_context_->SetURL(KURL("http://example.com/foo/bar"));
+  execution_context_->SetUpSecurityContext();
+
+  KURL url = KURL("blob:http://example.com/id");
+  url_manager().Revoke(url);
+  url_store_binding_.FlushForTesting();
+  ASSERT_EQ(1u, url_store_.revocations.size());
+  EXPECT_EQ(url, url_store_.revocations[0]);
+}
+
+TEST_F(PublicURLManagerTest, RevokeInvalidURL) {
+  execution_context_->SetURL(KURL("http://example.com/foo/bar"));
+  execution_context_->SetUpSecurityContext();
+
+  KURL invalid_scheme_url = KURL("blb:http://example.com/id");
+  KURL fragment_url = KURL("blob:http://example.com/id#fragment");
+  KURL invalid_origin_url = KURL("blob:http://foobar.com/id");
+  url_manager().Revoke(invalid_scheme_url);
+  url_manager().Revoke(fragment_url);
+  url_manager().Revoke(invalid_origin_url);
+  url_store_binding_.FlushForTesting();
+  // Both should have been silently ignored.
+  EXPECT_TRUE(url_store_.revocations.IsEmpty());
 }
 
 }  // namespace blink

third_party/WebKit/Source/platform/blob/testing/FakeBlobURLStore.cpp

@@ -15,6 +15,7 @@ void FakeBlobURLStore::Register(mojom::blink::BlobPtr blob,
 
 void FakeBlobURLStore::Revoke(const KURL& url) {
   registrations.erase(url);
+  revocations.push_back(url);
 }
 
 void FakeBlobURLStore::Resolve(const KURL& url, ResolveCallback callback) {

third_party/WebKit/Source/platform/blob/testing/FakeBlobURLStore.h

@@ -10,6 +10,7 @@
 #include "mojo/public/cpp/bindings/binding_set.h"
 #include "platform/weborigin/KURLHash.h"
 #include "platform/wtf/HashMap.h"
+#include "platform/wtf/Vector.h"
 
 namespace blink {
 
@@ -21,6 +22,7 @@ class FakeBlobURLStore : public mojom::blink::BlobURLStore {
   void Resolve(const KURL&, ResolveCallback) override;
 
   HashMap<KURL, mojom::blink::BlobPtr> registrations;
+  Vector<KURL> revocations;
 };
 
 }  // namespace blink