From 1dc4af7b5a6e56244f2755f7ece6039ec3e6cc41 Mon Sep 17 00:00:00 2001 From: lfg Date: Tue, 31 Mar 2015 16:04:26 -0700 Subject: [PATCH] Webview should only allow resources loading from the extension that embedded it. This fixes an issue where Webview would allow cross renderer resource loads without checking if the URL referenced the same App that embedded the Webview. BUG=470890 Review URL: https://codereview.chromium.org/1030323004 Cr-Commit-Position: refs/heads/master@{#323139} --- .../browser/plugins/plugin_info_message_filter.cc | 9 +- ...ifests_webview_accessible_resources_unittest.cc | 116 ++++++++++----------- .../webview_accessible_resources_1.json | 3 +- ..._1.json => webview_accessible_resources_2.json} | 5 +- extensions/browser/url_request_util.cc | 20 +++- .../common/manifest_handlers/webview_info.cc | 30 ++---- extensions/common/manifest_handlers/webview_info.h | 12 ++- 7 files changed, 98 insertions(+), 97 deletions(-) copy chrome/test/data/extensions/manifest_tests/{webview_accessible_resources_1.json => webview_accessible_resources_2.json} (89%) diff --git a/chrome/browser/plugins/plugin_info_message_filter.cc b/chrome/browser/plugins/plugin_info_message_filter.cc index 311cc5bc689e..c50d1c2627d9 100644 --- a/chrome/browser/plugins/plugin_info_message_filter.cc +++ b/chrome/browser/plugins/plugin_info_message_filter.cc @@ -38,6 +38,7 @@ #include "extensions/browser/guest_view/guest_view_base.h" #include "extensions/browser/guest_view/web_view/web_view_renderer_state.h" #include "extensions/common/extension.h" +#include "extensions/common/manifest_constants.h" #include "extensions/common/manifest_handlers/webview_info.h" #endif @@ -156,8 +157,12 @@ bool IsPluginLoadingAccessibleResourceInWebView( const extensions::Extension* extension = extension_registry->GetExtensionById(extension_id, extensions::ExtensionRegistry::ENABLED); - if (!extensions::WebviewInfo::IsResourceWebviewAccessible( - extension, partition_id, resource.path())) { + const extensions::WebviewInfo* webview_info = + static_cast(extension->GetManifestData( + extensions::manifest_keys::kWebviewAccessibleResources)); + if (!webview_info || + !webview_info->IsResourceWebviewAccessible(extension, partition_id, + resource.path())) { return false; } diff --git a/chrome/common/extensions/manifest_tests/extension_manifests_webview_accessible_resources_unittest.cc b/chrome/common/extensions/manifest_tests/extension_manifests_webview_accessible_resources_unittest.cc index 1f8578630172..86c20aa3bc6a 100644 --- a/chrome/common/extensions/manifest_tests/extension_manifests_webview_accessible_resources_unittest.cc +++ b/chrome/common/extensions/manifest_tests/extension_manifests_webview_accessible_resources_unittest.cc @@ -20,74 +20,64 @@ TEST_F(WebviewAccessibleResourcesManifestTest, WebviewAccessibleResources) { // Manifest version 2 with webview accessible resources specified. scoped_refptr extension1( LoadAndExpectSuccess("webview_accessible_resources_1.json")); + scoped_refptr extension2( + LoadAndExpectSuccess("webview_accessible_resources_2.json")); + DCHECK(extension1->id() != extension2->id()); + const WebviewInfo* webview_info1 = + static_cast(extension1->GetManifestData( + extensions::manifest_keys::kWebviewAccessibleResources)); + const WebviewInfo* webview_info2 = + static_cast(extension2->GetManifestData( + extensions::manifest_keys::kWebviewAccessibleResources)); - EXPECT_FALSE( - WebviewInfo::IsResourceWebviewAccessible(extension1.get(), - "fail", - "a.html")); - EXPECT_FALSE( - WebviewInfo::IsResourceWebviewAccessible(extension1.get(), - "fail", - "b.html")); - EXPECT_FALSE( - WebviewInfo::IsResourceWebviewAccessible(extension1.get(), - "fail", - "c.html")); - EXPECT_FALSE( - WebviewInfo::IsResourceWebviewAccessible(extension1.get(), - "fail", - "d.html")); + EXPECT_FALSE(webview_info1->IsResourceWebviewAccessible(extension1.get(), + "fail", "a.html")); + EXPECT_FALSE(webview_info1->IsResourceWebviewAccessible(extension1.get(), + "fail", "b.html")); + EXPECT_FALSE(webview_info1->IsResourceWebviewAccessible(extension1.get(), + "fail", "c.html")); + EXPECT_FALSE(webview_info1->IsResourceWebviewAccessible(extension1.get(), + "fail", "d.html")); - EXPECT_TRUE( - WebviewInfo::IsResourceWebviewAccessible(extension1.get(), - "foo", - "a.html")); - EXPECT_TRUE( - WebviewInfo::IsResourceWebviewAccessible(extension1.get(), - "foo", - "b.html")); - EXPECT_FALSE( - WebviewInfo::IsResourceWebviewAccessible(extension1.get(), - "foo", - "c.html")); - EXPECT_FALSE( - WebviewInfo::IsResourceWebviewAccessible(extension1.get(), - "foo", - "d.html")); + EXPECT_TRUE(webview_info1->IsResourceWebviewAccessible(extension1.get(), + "foo", "a.html")); + EXPECT_TRUE(webview_info1->IsResourceWebviewAccessible(extension1.get(), + "foo", "b.html")); + EXPECT_FALSE(webview_info1->IsResourceWebviewAccessible(extension1.get(), + "foo", "c.html")); + EXPECT_FALSE(webview_info1->IsResourceWebviewAccessible(extension1.get(), + "foo", "d.html")); - EXPECT_TRUE( - WebviewInfo::IsResourceWebviewAccessible(extension1.get(), - "bar", - "a.html")); - EXPECT_FALSE( - WebviewInfo::IsResourceWebviewAccessible(extension1.get(), - "bar", - "b.html")); - EXPECT_TRUE( - WebviewInfo::IsResourceWebviewAccessible(extension1.get(), - "bar", - "c.html")); - EXPECT_FALSE( - WebviewInfo::IsResourceWebviewAccessible(extension1.get(), - "bar", - "d.html")); + EXPECT_TRUE(webview_info1->IsResourceWebviewAccessible(extension1.get(), + "bar", "a.html")); + EXPECT_FALSE(webview_info1->IsResourceWebviewAccessible(extension1.get(), + "bar", "b.html")); + EXPECT_TRUE(webview_info1->IsResourceWebviewAccessible(extension1.get(), + "bar", "c.html")); + EXPECT_FALSE(webview_info1->IsResourceWebviewAccessible(extension1.get(), + "bar", "d.html")); - EXPECT_TRUE( - WebviewInfo::IsResourceWebviewAccessible(extension1.get(), - "foobar", - "a.html")); - EXPECT_TRUE( - WebviewInfo::IsResourceWebviewAccessible(extension1.get(), - "foobar", - "b.html")); - EXPECT_TRUE( - WebviewInfo::IsResourceWebviewAccessible(extension1.get(), - "foobar", - "c.html")); + EXPECT_TRUE(webview_info1->IsResourceWebviewAccessible(extension1.get(), + "foobar", "a.html")); + EXPECT_TRUE(webview_info1->IsResourceWebviewAccessible(extension1.get(), + "foobar", "b.html")); + EXPECT_TRUE(webview_info1->IsResourceWebviewAccessible(extension1.get(), + "foobar", "c.html")); + EXPECT_FALSE(webview_info1->IsResourceWebviewAccessible(extension1.get(), + "foobar", "d.html")); + + // Check that if the owner extension doesn't match the request extension + // the resource will not be accessible. + EXPECT_FALSE(webview_info2->IsResourceWebviewAccessible(extension1.get(), + "foobar", "a.html")); + EXPECT_FALSE(webview_info1->IsResourceWebviewAccessible(extension2.get(), + "foobar", "a.html")); + EXPECT_TRUE(webview_info2->IsResourceWebviewAccessible(extension2.get(), + "foobar", "a.html")); + EXPECT_FALSE( + webview_info1->IsResourceWebviewAccessible(nullptr, "foobar", "a.html")); EXPECT_FALSE( - WebviewInfo::IsResourceWebviewAccessible(extension1.get(), - "foobar", - "d.html")); + webview_info2->IsResourceWebviewAccessible(nullptr, "foobar", "a.html")); } TEST_F(WebviewAccessibleResourcesManifestTest, InvalidManifest) { diff --git a/chrome/test/data/extensions/manifest_tests/webview_accessible_resources_1.json b/chrome/test/data/extensions/manifest_tests/webview_accessible_resources_1.json index f1ded650b9a5..275ccaf3cae6 100644 --- a/chrome/test/data/extensions/manifest_tests/webview_accessible_resources_1.json +++ b/chrome/test/data/extensions/manifest_tests/webview_accessible_resources_1.json @@ -21,5 +21,6 @@ "accessible_resources": ["a.html", "c.html"] } ] - } + }, + "key": "dGVzdDEK" } diff --git a/chrome/test/data/extensions/manifest_tests/webview_accessible_resources_1.json b/chrome/test/data/extensions/manifest_tests/webview_accessible_resources_2.json similarity index 89% copy from chrome/test/data/extensions/manifest_tests/webview_accessible_resources_1.json copy to chrome/test/data/extensions/manifest_tests/webview_accessible_resources_2.json index f1ded650b9a5..b61364199712 100644 --- a/chrome/test/data/extensions/manifest_tests/webview_accessible_resources_1.json +++ b/chrome/test/data/extensions/manifest_tests/webview_accessible_resources_2.json @@ -1,5 +1,5 @@ { - "name": "test", + "name": "test2", "version": "2", "manifest_version": 2, "permissions": [ @@ -21,5 +21,6 @@ "accessible_resources": ["a.html", "c.html"] } ] - } + }, + "key": "dGVzdDIK" } diff --git a/extensions/browser/url_request_util.cc b/extensions/browser/url_request_util.cc index f82b9739761c..467e637c274d 100644 --- a/extensions/browser/url_request_util.cc +++ b/extensions/browser/url_request_util.cc @@ -10,6 +10,7 @@ #include "extensions/browser/guest_view/web_view/web_view_renderer_state.h" #include "extensions/browser/info_map.h" #include "extensions/common/extension.h" +#include "extensions/common/manifest_constants.h" #include "extensions/common/manifest_handlers/icons_handler.h" #include "extensions/common/manifest_handlers/web_accessible_resources_info.h" #include "extensions/common/manifest_handlers/webview_info.h" @@ -27,13 +28,26 @@ bool AllowCrossRendererResourceLoad(net::URLRequest* request, content::ResourceRequestInfo::ForRequest(request); // Extensions with webview: allow loading certain resources by guest renderers - // with privileged partition IDs as specified in the manifest file. + // with privileged partition IDs as specified in owner's extension the + // manifest file. + std::string owner_extension_id; + int owner_process_id; + WebViewRendererState::GetInstance()->GetOwnerInfo( + info->GetChildID(), &owner_process_id, &owner_extension_id); + const Extension* owner_extension = + extension_info_map->extensions().GetByID(owner_extension_id); + const WebviewInfo* webview_info = + owner_extension + ? static_cast(owner_extension->GetManifestData( + manifest_keys::kWebviewAccessibleResources)) + : nullptr; std::string partition_id; bool is_guest = WebViewRendererState::GetInstance()->GetPartitionID( info->GetChildID(), &partition_id); std::string resource_path = request->url().path(); - if (is_guest && WebviewInfo::IsResourceWebviewAccessible( - extension, partition_id, resource_path)) { + if (is_guest && webview_info && + webview_info->IsResourceWebviewAccessible(extension, partition_id, + resource_path)) { *allowed = true; return true; } diff --git a/extensions/common/manifest_handlers/webview_info.cc b/extensions/common/manifest_handlers/webview_info.cc index 3b30bbe42158..99e9d40c5de9 100644 --- a/extensions/common/manifest_handlers/webview_info.cc +++ b/extensions/common/manifest_handlers/webview_info.cc @@ -18,16 +18,6 @@ namespace extensions { namespace keys = extensions::manifest_keys; namespace errors = extensions::manifest_errors; -namespace { - -const WebviewInfo* GetResourcesInfo( - const Extension& extension) { - return static_cast( - extension.GetManifestData(keys::kWebviewAccessibleResources)); -} - -} // namespace - // A PartitionItem represents a set of accessible resources given a partition // ID pattern. class PartitionItem { @@ -60,27 +50,25 @@ class PartitionItem { URLPatternSet accessible_resources_; }; - -WebviewInfo::WebviewInfo() { +WebviewInfo::WebviewInfo(const std::string& extension_id) + : extension_id_(extension_id) { } WebviewInfo::~WebviewInfo() { } -// static bool WebviewInfo::IsResourceWebviewAccessible( const Extension* extension, const std::string& partition_id, - const std::string& relative_path) { - if (!extension) + const std::string& relative_path) const { + if (!extension || extension->id() != extension_id_) return false; - const WebviewInfo* info = GetResourcesInfo(*extension); - if (!info) - return false; + DCHECK_EQ(this, + extension->GetManifestData(keys::kWebviewAccessibleResources)); - for (size_t i = 0; i < info->partition_items_.size(); ++i) { - const PartitionItem* const item = info->partition_items_[i]; + for (size_t i = 0; i < partition_items_.size(); ++i) { + const PartitionItem* const item = partition_items_[i]; if (item->Matches(partition_id) && extension->ResourceMatches(item->accessible_resources(), relative_path)) { @@ -102,7 +90,7 @@ WebviewHandler::~WebviewHandler() { } bool WebviewHandler::Parse(Extension* extension, base::string16* error) { - scoped_ptr info(new WebviewInfo()); + scoped_ptr info(new WebviewInfo(extension->id())); const base::DictionaryValue* dict_value = NULL; if (!extension->manifest()->GetDictionary(keys::kWebview, diff --git a/extensions/common/manifest_handlers/webview_info.h b/extensions/common/manifest_handlers/webview_info.h index 2cd501efb657..968e5fde9609 100644 --- a/extensions/common/manifest_handlers/webview_info.h +++ b/extensions/common/manifest_handlers/webview_info.h @@ -21,17 +21,19 @@ class PartitionItem; class WebviewInfo : public Extension::ManifestData { public: // Define out of line constructor/destructor to please Clang. - WebviewInfo(); + WebviewInfo(const std::string& extension_id); ~WebviewInfo() override; - // Returns true if the specified resource is web accessible. - static bool IsResourceWebviewAccessible(const Extension* extension, - const std::string& partition_id, - const std::string& relative_path); + // Returns true if the specified resource is web accessible and the extension + // matches the manifest's extension. + bool IsResourceWebviewAccessible(const Extension* extension, + const std::string& partition_id, + const std::string& relative_path) const; void AddPartitionItem(scoped_ptr item); private: + std::string extension_id_; ScopedVector partition_items_; }; -- 2.11.4.GIT