From 8b8c9d9d760ae961c2381786a47dc5598e89c0e2 Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Wed, 28 Nov 2018 04:51:11 -0500 Subject: [PATCH] Disable shields for extensions The reasoning is they have APIs to do much worse tracking than our shields would protect them from. Users install them knowing they will have elevated permissions Fix https://github.com/brave/brave-browser/issues/1380 --- browser/brave_content_browser_client.cc | 8 +++-- ...ave_ad_block_tp_network_delegate_helper.cc | 8 +---- ...rave_site_hacks_network_delegate_helper.cc | 4 +++ ..._hacks_network_delegate_helper_unittest.cc | 28 +++++++++++++++ browser/net/url_context.cc | 5 +-- browser/net/url_context.h | 1 - common/BUILD.gn | 2 -- common/url_constants.cc | 1 + common/url_constants.h | 1 + common/url_util.cc | 28 --------------- common/url_util.h | 18 ---------- common/url_util_unittest.cc | 34 ------------------- .../core/browser/brave_cookie_settings.cc | 19 +++++++++++ test/BUILD.gn | 1 - 14 files changed, 62 insertions(+), 96 deletions(-) delete mode 100644 common/url_util.cc delete mode 100644 common/url_util.h delete mode 100644 common/url_util_unittest.cc diff --git a/browser/brave_content_browser_client.cc b/browser/brave_content_browser_client.cc index 0da3dabb1fa7..de5dde641da1 100644 --- a/browser/brave_content_browser_client.cc +++ b/browser/brave_content_browser_client.cc @@ -99,9 +99,11 @@ bool BraveContentBrowserClient::AllowAccessCookie(const GURL& url, const GURL& f BraveShieldsWebContentsObserver::GetTabURLFromRenderFrameInfo( render_process_id, render_frame_id).GetOrigin(); ProfileIOData* io_data = ProfileIOData::FromResourceContext(context); - bool allow_brave_shields = brave_shields::IsAllowContentSettingWithIOData( - io_data, tab_origin, tab_origin, CONTENT_SETTINGS_TYPE_PLUGINS, - brave_shields::kBraveShields); + bool allow_brave_shields = + brave_shields::IsAllowContentSettingWithIOData( + io_data, tab_origin, tab_origin, CONTENT_SETTINGS_TYPE_PLUGINS, + brave_shields::kBraveShields) && + !first_party.SchemeIs(kChromeExtensionScheme); bool allow_1p_cookies = brave_shields::IsAllowContentSettingWithIOData( io_data, tab_origin, GURL("https://firstParty/"), CONTENT_SETTINGS_TYPE_PLUGINS, brave_shields::kCookies); diff --git a/browser/net/brave_ad_block_tp_network_delegate_helper.cc b/browser/net/brave_ad_block_tp_network_delegate_helper.cc index ce509b7ad8eb..32376c014294 100644 --- a/browser/net/brave_ad_block_tp_network_delegate_helper.cc +++ b/browser/net/brave_ad_block_tp_network_delegate_helper.cc @@ -11,7 +11,6 @@ #include "brave/browser/brave_browser_process_impl.h" #include "brave/common/network_constants.h" #include "brave/common/shield_exceptions.h" -#include "brave/common/url_util.h" #include "brave/components/brave_shields/browser/ad_block_regional_service.h" #include "brave/components/brave_shields/browser/ad_block_service.h" #include "brave/components/brave_shields/browser/brave_shields_util.h" @@ -99,12 +98,7 @@ void OnBeforeURLRequestAdBlockTPOnTaskRunner(std::shared_ptr c } DCHECK(ctx->request_identifier != 0); - // For PDFJS we don't want to treat 2 different URLs as 3p for the main pdf - // load which is meant to be top level - // chrome-extension://oemmndcbldboiebfnladdacbdfmadadm/https://developer.att.com/.../file.pdf - // https://developer.att.com/.../file.pdf - // So if the tab origin is chrome-extension, set it to that of the PDF only for PDFJS - std::string tab_host = brave::GetURLOrPDFURL(ctx->tab_url).host(); + std::string tab_host = ctx->tab_origin.host(); if (!g_brave_browser_process->tracking_protection_service()-> ShouldStartRequest(ctx->request_url, ctx->resource_type, tab_host)) { ctx->new_url_spec = GetBlankDataURLForResourceType(ctx->resource_type).spec(); diff --git a/browser/net/brave_site_hacks_network_delegate_helper.cc b/browser/net/brave_site_hacks_network_delegate_helper.cc index 4f96970c495a..fb59bc399f8b 100644 --- a/browser/net/brave_site_hacks_network_delegate_helper.cc +++ b/browser/net/brave_site_hacks_network_delegate_helper.cc @@ -10,6 +10,7 @@ #include "base/strings/string_util.h" #include "brave/common/network_constants.h" #include "brave/common/shield_exceptions.h" +#include "brave/common/url_constants.h" #include "brave/components/brave_shields/browser/brave_shields_util.h" #include "brave/components/brave_shields/browser/brave_shields_web_contents_observer.h" #include "brave/components/brave_shields/common/brave_shield_constants.h" @@ -28,6 +29,9 @@ bool ApplyPotentialReferrerBlock(net::URLRequest* request) { DCHECK_CURRENTLY_ON(BrowserThread::IO); GURL target_origin = GURL(request->url()).GetOrigin(); GURL tab_origin = request->site_for_cookies().GetOrigin(); + if (tab_origin.SchemeIs(kChromeExtensionScheme)) { + return false; + } bool allow_referrers = brave_shields::IsAllowContentSettingFromIO( request, tab_origin, tab_origin, CONTENT_SETTINGS_TYPE_PLUGINS, brave_shields::kReferrers); diff --git a/browser/net/brave_site_hacks_network_delegate_helper_unittest.cc b/browser/net/brave_site_hacks_network_delegate_helper_unittest.cc index 5304fc6cda56..ceb9695faa98 100644 --- a/browser/net/brave_site_hacks_network_delegate_helper_unittest.cc +++ b/browser/net/brave_site_hacks_network_delegate_helper_unittest.cc @@ -271,4 +271,32 @@ TEST_F(BraveSiteHacksNetworkDelegateHelperTest, ReferrerCleared) { }); } +TEST_F(BraveSiteHacksNetworkDelegateHelperTest, ReferrerWouldBeClearedButExtensionSite) { + std::vector urls({ + GURL("https://digg.com/7"), + GURL("https://slashdot.org/5"), + GURL("https://bondy.brian.org") + }); + std::for_each(urls.begin(), urls.end(), + [this](GURL url){ + net::TestDelegate test_delegate; + std::unique_ptr request = + context()->CreateRequest(url, net::IDLE, &test_delegate, + TRAFFIC_ANNOTATION_FOR_TESTS); + request->set_site_for_cookies(GURL("chrome-extension://aemmndcbldboiebfnladdacbdfmadadm/test.html")); + std::string original_referrer = "https://hello.brianbondy.com/about"; + request->SetReferrer(original_referrer); + + std::shared_ptr + brave_request_info(new brave::BraveRequestInfo()); + brave::BraveRequestInfo::FillCTXFromRequest(request.get(), brave_request_info); + brave::ResponseCallback callback; + int ret = brave::OnBeforeURLRequest_SiteHacksWork(callback, brave_request_info); + EXPECT_EQ(ret, net::OK); + // new_url should not be set + EXPECT_TRUE(brave_request_info->new_url_spec.empty()); + EXPECT_STREQ(request->referrer().c_str(), original_referrer.c_str()); + }); +} + } // namespace diff --git a/browser/net/url_context.cc b/browser/net/url_context.cc index 79afcf8c0847..029c873fc8e9 100644 --- a/browser/net/url_context.cc +++ b/browser/net/url_context.cc @@ -7,6 +7,7 @@ #include +#include "brave/common/url_constants.h" #include "brave/components/brave_shields/browser/brave_shields_util.h" #include "brave/components/brave_shields/common/brave_shield_constants.h" #include "content/public/browser/resource_request_info.h" @@ -23,7 +24,6 @@ void BraveRequestInfo::FillCTXFromRequest(const net::URLRequest* request, std::shared_ptr ctx) { ctx->request_identifier = request->identifier(); ctx->request_url = request->url(); - ctx->tab_url = request->site_for_cookies(); ctx->tab_origin = request->site_for_cookies().GetOrigin(); auto* request_info = content::ResourceRequestInfo::ForRequest(request); if (request_info) { @@ -33,7 +33,8 @@ void BraveRequestInfo::FillCTXFromRequest(const net::URLRequest* request, &ctx->frame_tree_node_id); ctx->allow_brave_shields = brave_shields::IsAllowContentSettingFromIO( request, ctx->tab_origin, ctx->tab_origin, CONTENT_SETTINGS_TYPE_PLUGINS, - brave_shields::kBraveShields); + brave_shields::kBraveShields) && + !request->site_for_cookies().SchemeIs(kChromeExtensionScheme); ctx->allow_ads = brave_shields::IsAllowContentSettingFromIO( request, ctx->tab_origin, ctx->tab_origin, CONTENT_SETTINGS_TYPE_PLUGINS, brave_shields::kAds); diff --git a/browser/net/url_context.h b/browser/net/url_context.h index ebee381a407f..ec71bfd80d3a 100644 --- a/browser/net/url_context.h +++ b/browser/net/url_context.h @@ -51,7 +51,6 @@ struct BraveRequestInfo { ~BraveRequestInfo(); GURL request_url; GURL tab_origin; - GURL tab_url; std::string new_url_spec; bool allow_brave_shields = true; bool allow_ads = false; diff --git a/common/BUILD.gn b/common/BUILD.gn index e6087d5df90b..c194b5a5a8f0 100644 --- a/common/BUILD.gn +++ b/common/BUILD.gn @@ -49,8 +49,6 @@ source_set("common") { "shield_exceptions.h", "url_constants.cc", "url_constants.h", - "url_util.cc", - "url_util.h", ] public_deps = [ diff --git a/common/url_constants.cc b/common/url_constants.cc index 90018b6d32e8..cf5b7a0a0423 100644 --- a/common/url_constants.cc +++ b/common/url_constants.cc @@ -4,6 +4,7 @@ #include "brave/common/url_constants.h" +const char kChromeExtensionScheme[] = "chrome-extension"; const char kBraveUIScheme[] = "brave"; const char kMagnetScheme[] = "magnet"; const char kWidevineMoreInfoURL[] = "https://www.eff.org/issues/drm"; diff --git a/common/url_constants.h b/common/url_constants.h index fca2e0bd02a4..771a74b4cbfa 100644 --- a/common/url_constants.h +++ b/common/url_constants.h @@ -5,6 +5,7 @@ #ifndef BRAVE_COMMON_URL_CONSTANTS_H_ #define BRAVE_COMMON_URL_CONSTANTS_H_ +extern const char kChromeExtensionScheme[]; extern const char kBraveUIScheme[]; extern const char kMagnetScheme[]; extern const char kWidevineMoreInfoURL[]; diff --git a/common/url_util.cc b/common/url_util.cc deleted file mode 100644 index 9609f687998f..000000000000 --- a/common/url_util.cc +++ /dev/null @@ -1,28 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this file, - * You can obtain one at http://mozilla.org/MPL/2.0/. */ - -#include "brave/common/url_util.h" - -#include - -#include "brave/common/extensions/extension_constants.h" -#include "url/gurl.h" - -namespace brave { - -GURL GetURLOrPDFURL(const GURL& url) { - if (url.SchemeIs("chrome-extension") && - url.host() == pdfjs_extension_id) { - static size_t pdfjs_substring_len = std::string(pdfjs_extension_origin).length(); - size_t http_pos = url.spec().find(std::string(pdfjs_extension_origin) + "http://"); - size_t https_pos = url.spec().find(std::string(pdfjs_extension_origin) + "https://"); - if (http_pos != std::string::npos || https_pos != std::string::npos) { - return GURL(url.spec().substr(pdfjs_substring_len, - url.spec().length() - pdfjs_substring_len)); - } - } - return url; -} - -} // namespace brave diff --git a/common/url_util.h b/common/url_util.h deleted file mode 100644 index 2f8a15ab5eda..000000000000 --- a/common/url_util.h +++ /dev/null @@ -1,18 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this file, - * You can obtain one at http://mozilla.org/MPL/2.0/. */ - -#ifndef BRAVE_COMMON_URL_UTIL_H_ -#define BRAVE_COMMON_URL_UTIL_H_ - -class GURL; - -namespace brave { - -// Returns the location of the PDF if this URL is a PDFJS extension URL. -// Otherwise simply just returns the same URL as passed in. -GURL GetURLOrPDFURL(const GURL& url); - -} // namespace brave - -#endif // BRAVE_COMMON_URL_UTIL_H_ diff --git a/common/url_util_unittest.cc b/common/url_util_unittest.cc deleted file mode 100644 index 673ab9c24e3b..000000000000 --- a/common/url_util_unittest.cc +++ /dev/null @@ -1,34 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this file, - * You can obtain one at http://mozilla.org/MPL/2.0/. */ - -#include "brave/common/url_util.h" -#include "chrome/test/base/chrome_render_view_host_test_harness.h" -#include "url/gurl.h" - -typedef testing::Test BraveUrlUtilTest; - -namespace brave { - -TEST_F(BraveUrlUtilTest, GetURLOrPDFURL) { - std::vector unchanged_urls({ - // PDFJS URL but not to a PDF - GURL("chrome-extension://oemmndcbldboiebfnladdacbdfmadadm/test.html"), - // PDFJS ID but not chrome-extension scheme - GURL("chrome://oemmndcbldboiebfnladdacbdfmadadm/https://test.html"), - // Not PDFJS ID but format of a PDFJS PDF URL - GURL("chrome-extension://aaamndcbldboiebfnladdacbdfmadaaa/https://example.com/test.html"), - // Random other URL - GURL("https://example.com") - }); - std::for_each(unchanged_urls.begin(), unchanged_urls.end(), - [this](GURL url){ - EXPECT_EQ(brave::GetURLOrPDFURL(url), url); - }); - EXPECT_EQ(brave::GetURLOrPDFURL(GURL("chrome-extension://oemmndcbldboiebfnladdacbdfmadadm/http://example.com?test")), - GURL("http://example.com?test")); - EXPECT_EQ(brave::GetURLOrPDFURL(GURL("chrome-extension://oemmndcbldboiebfnladdacbdfmadadm/https://example.com?test")), - GURL("https://example.com?test")); -} - -} // namespace diff --git a/components/content_settings/core/browser/brave_cookie_settings.cc b/components/content_settings/core/browser/brave_cookie_settings.cc index 0b14113f4331..cbd330fb0a5e 100644 --- a/components/content_settings/core/browser/brave_cookie_settings.cc +++ b/components/content_settings/core/browser/brave_cookie_settings.cc @@ -6,6 +6,7 @@ #include "brave/components/brave_shields/common/brave_shield_constants.h" #include "brave/common/brave_cookie_blocking.h" +#include "extensions/buildflags/buildflags.h" #include "net/base/registry_controlled_domains/registry_controlled_domain.h" #include "url/gurl.h" @@ -36,6 +37,24 @@ void BraveCookieSettings::GetCookieSetting(const GURL& url, content_settings::SettingSource* source, ContentSetting* cookie_setting) const { DCHECK(cookie_setting); + + // Auto-allow in extensions or for WebUI embedded in a secure origin. + // This matches an early return case in CookieSettings::GetCookieSetting + if (first_party_url.SchemeIs(kChromeUIScheme) && + url.SchemeIsCryptographic()) { + *cookie_setting = CONTENT_SETTING_ALLOW; + return; + } + + // This matches an early return case in CookieSettings::GetCookieSetting +#if BUILDFLAG(ENABLE_EXTENSIONS) + if (url.SchemeIs(extension_scheme_) && + first_party_url.SchemeIs(extension_scheme_)) { + *cookie_setting = CONTENT_SETTING_ALLOW; + return; + } +#endif + CookieSettings::GetCookieSetting(url, first_party_url, source, cookie_setting); if (*cookie_setting == CONTENT_SETTING_BLOCK) { diff --git a/test/BUILD.gn b/test/BUILD.gn index 60fd688f9a26..20cd38be8f85 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -59,7 +59,6 @@ test("brave_unit_tests") { "//brave/common/shield_exceptions_unittest.cc", "//brave/common/tor/tor_test_constants.cc", "//brave/common/tor/tor_test_constants.h", - "//brave/common/url_util_unittest.cc", "//brave/components/assist_ranker/ranker_model_loader_impl_unittest.cc", "//brave/components/brave_shields/browser/ad_block_regional_service_unittest.cc", "//brave/components/brave_sync/bookmark_order_util_unittest.cc",