From a1d50a38b2621eb65e6472d32a73a868d6a9b3d5 Mon Sep 17 00:00:00 2001 From: Francois Marier Date: Thu, 24 Sep 2020 18:36:42 -0700 Subject: [PATCH 1/4] Enable proxied network connections to the download reputation endpoint. This partially reverts 83fc520c06c921af29191b4817370330a2d05a47. --- browser/net/brave_block_safebrowsing_urls.cc | 11 +++++++++++ browser/net/brave_block_safebrowsing_urls_unittest.cc | 1 - .../brave_static_redirect_network_delegate_helper.cc | 8 ++------ ...tatic_redirect_network_delegate_helper_unittest.cc | 5 ----- common/network_constants.cc | 3 ++- 5 files changed, 15 insertions(+), 13 deletions(-) diff --git a/browser/net/brave_block_safebrowsing_urls.cc b/browser/net/brave_block_safebrowsing_urls.cc index a95e36158aaa..86927592160d 100644 --- a/browser/net/brave_block_safebrowsing_urls.cc +++ b/browser/net/brave_block_safebrowsing_urls.cc @@ -16,6 +16,11 @@ namespace brave { const char kDummyUrl[] = "https://no-thanks.invalid"; bool IsSafeBrowsingReportingURL(const GURL& gurl) { + static const std::vector allowed_patterns({ + URLPattern( + URLPattern::SCHEME_HTTPS, + "https://sb-ssl.google.com/safebrowsing/clientreport/download*"), + }); static const std::vector reporting_patterns({ URLPattern(URLPattern::SCHEME_HTTPS, "https://sb-ssl.google.com/safebrowsing/clientreport/*"), @@ -26,6 +31,12 @@ bool IsSafeBrowsingReportingURL(const GURL& gurl) { URLPattern(URLPattern::SCHEME_HTTPS, "https://safebrowsing.google.com/safebrowsing/uploads/*"), }); + + if (std::any_of( + allowed_patterns.begin(), allowed_patterns.end(), + [&gurl](URLPattern pattern) { return pattern.MatchesURL(gurl); })) { + return false; + } return std::any_of( reporting_patterns.begin(), reporting_patterns.end(), [&gurl](URLPattern pattern) { return pattern.MatchesURL(gurl); }); diff --git a/browser/net/brave_block_safebrowsing_urls_unittest.cc b/browser/net/brave_block_safebrowsing_urls_unittest.cc index 880d1ae52c50..f0fdd037c2fe 100644 --- a/browser/net/brave_block_safebrowsing_urls_unittest.cc +++ b/browser/net/brave_block_safebrowsing_urls_unittest.cc @@ -43,7 +43,6 @@ TEST(BraveBlockReportingUrlsHelperTest, PreserveNormalUrls) { TEST(BraveBlockReportingUrlsHelperTest, CancelReportingUrl) { const std::vector reportingUrls({ - "https://sb-ssl.google.com/safebrowsing/clientreport/download", "https://sb-ssl.google.com/safebrowsing/clientreport/chrome-reset", "https://sb-ssl.google.com/safebrowsing/clientreport/incident", "https://sb-ssl.google.com/safebrowsing/clientreport/login", diff --git a/browser/net/brave_static_redirect_network_delegate_helper.cc b/browser/net/brave_static_redirect_network_delegate_helper.cc index f9aac66c84bd..6bbfeb85a58f 100644 --- a/browser/net/brave_static_redirect_network_delegate_helper.cc +++ b/browser/net/brave_static_redirect_network_delegate_helper.cc @@ -106,12 +106,8 @@ int OnBeforeURLRequest_StaticRedirectWorkForGURL( } if (safebrowsingfilecheck_pattern.MatchesHost(request_url)) { - // TODO(@fmarier): Re-enable download protection once we have - // truncated the list of metadata that it sends to the server - // (brave/brave-browser#6267). - // - // replacements.SetHostStr(kBraveSafeBrowsingFileCheckProxy); - // *new_url = request_url.ReplaceComponents(replacements); + replacements.SetHostStr(kBraveSafeBrowsingFileCheckProxy); + *new_url = request_url.ReplaceComponents(replacements); return net::OK; } diff --git a/browser/net/brave_static_redirect_network_delegate_helper_unittest.cc b/browser/net/brave_static_redirect_network_delegate_helper_unittest.cc index 9371635ae2ae..bd4878260c47 100644 --- a/browser/net/brave_static_redirect_network_delegate_helper_unittest.cc +++ b/browser/net/brave_static_redirect_network_delegate_helper_unittest.cc @@ -269,10 +269,6 @@ TEST(BraveStaticRedirectNetworkDelegateHelperTest, EXPECT_EQ(rc, net::OK); } -// TODO(@fmarier): Re-enable download protection once we have -// truncated the list of metadata that it sends to the server -// (brave/brave-browser#6267). -#if 0 TEST(BraveStaticRedirectNetworkDelegateHelperTest, ModifySafeBrowsingFileCheckURL) { const GURL url( @@ -288,7 +284,6 @@ TEST(BraveStaticRedirectNetworkDelegateHelperTest, EXPECT_EQ(request_info->new_url_spec, expected_url); EXPECT_EQ(rc, net::OK); } -#endif // 0 #if BUILDFLAG(ENABLE_BRAVE_TRANSLATE_GO) TEST(BraveStaticRedirectNetworkDelegateHelperTest, RedirectTranslate) { diff --git a/common/network_constants.cc b/common/network_constants.cc index 6dc0f9b42d36..288a9293adbd 100644 --- a/common/network_constants.cc +++ b/common/network_constants.cc @@ -33,7 +33,8 @@ const char kJSDataURLPrefix[] = "data:application/javascript;base64,"; const char kGeoLocationsPattern[] = "https://www.googleapis.com/geolocation/v1/geolocate?key=*"; const char kSafeBrowsingPrefix[] = "https://safebrowsing.googleapis.com/"; -const char kSafeBrowsingFileCheckPrefix[] = "https://sb-ssl.google.com/"; +const char kSafeBrowsingFileCheckPrefix[] = + "https://sb-ssl.google.com/safebrowsing/clientreport/download"; const char kCRLSetPrefix1[] = "*://dl.google.com/release2/chrome_component/*crl-set*"; const char kCRLSetPrefix2[] = From 5df0be0f6ef31e4ec2286f281b37a64eeb96cd4f Mon Sep 17 00:00:00 2001 From: Francois Marier Date: Thu, 24 Sep 2020 18:38:12 -0700 Subject: [PATCH 2/4] Add new URLs present in the Chromium codebase. --- browser/net/brave_block_safebrowsing_urls_unittest.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/browser/net/brave_block_safebrowsing_urls_unittest.cc b/browser/net/brave_block_safebrowsing_urls_unittest.cc index f0fdd037c2fe..ce847ca8e290 100644 --- a/browser/net/brave_block_safebrowsing_urls_unittest.cc +++ b/browser/net/brave_block_safebrowsing_urls_unittest.cc @@ -43,16 +43,22 @@ TEST(BraveBlockReportingUrlsHelperTest, PreserveNormalUrls) { TEST(BraveBlockReportingUrlsHelperTest, CancelReportingUrl) { const std::vector reportingUrls({ + "https://sb-ssl.google.com/safebrowsing/clientreport/chrome-cct", "https://sb-ssl.google.com/safebrowsing/clientreport/chrome-reset", + "https://sb-ssl.google.com/safebrowsing/clientreport/chrome-sw-reporter", "https://sb-ssl.google.com/safebrowsing/clientreport/incident", "https://sb-ssl.google.com/safebrowsing/clientreport/login", "https://sb-ssl.google.com/safebrowsing/clientreport/phishing", "https://sb-ssl.google.com/safebrowsing/clientreport/malware-check", + "https://safebrowsing.google.com/safebrowsing/uploads/app", + "https://safebrowsing.google.com/safebrowsing/uploads/chrome", + "https://safebrowsing.google.com/safebrowsing/uploads/scan", "https://safebrowsing.google.com/safebrowsing/uploads/webprotect", "https://safebrowsing.google.com/safebrowsing/report", "https://safebrowsing.google.com/safebrowsing/clientreport/malware", "https://safebrowsing.google.com/safebrowsing/uploads/chrome", "https://safebrowsing.google.com/safebrowsing/clientreport/crx-list-info", + "https://safebrowsing.google.com/safebrowsing/clientreport/realtime", }); for (const auto& url : reportingUrls) { From e4c553019fb7850f616d61c4754013450042cbee Mon Sep 17 00:00:00 2001 From: Francois Marier Date: Mon, 28 Sep 2020 16:48:47 -0700 Subject: [PATCH 3/4] Enable the FileTypePolicies Chromium component. This is needed for the download protection component of Safe Browsing. --- .../file_type_policies_component_installer.cc | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 chromium_src/chrome/browser/component_updater/file_type_policies_component_installer.cc diff --git a/chromium_src/chrome/browser/component_updater/file_type_policies_component_installer.cc b/chromium_src/chrome/browser/component_updater/file_type_policies_component_installer.cc new file mode 100644 index 000000000000..9764e1666376 --- /dev/null +++ b/chromium_src/chrome/browser/component_updater/file_type_policies_component_installer.cc @@ -0,0 +1,29 @@ +/* Copyright (c) 2020 The Brave Authors. All rights reserved. + * 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 https://mozilla.org/MPL/2.0/. */ + +#define RegisterFileTypePoliciesComponent \ + RegisterFileTypePoliciesComponent_ChromiumImpl +#include "../../../../../chrome/browser/component_updater/file_type_policies_component_installer.cc" // NOLINT +#undef RegisterFileTypePoliciesComponent + +#include "chrome/browser/component_updater/component_updater_utils.h" +#include "components/component_updater/component_updater_service.h" + +namespace component_updater { + +const char kFileTypePoliciesComponentId[] = "khaoiebndkojlmppeemjhbpbandiljpe"; + +void OnFileTypePoliciesRegistered() { + component_updater::BraveOnDemandUpdate(kFileTypePoliciesComponentId); +} + +void RegisterFileTypePoliciesComponent(ComponentUpdateService* cus, + const base::FilePath& user_data_dir) { + auto installer = base::MakeRefCounted( + std::make_unique()); + installer->Register(cus, base::Bind(&OnFileTypePoliciesRegistered)); +} + +} // namespace component_updater From fd247c8f4ffdb591192901654a68750c354dd952 Mon Sep 17 00:00:00 2001 From: Francois Marier Date: Tue, 29 Sep 2020 17:34:51 -0700 Subject: [PATCH 4/4] Filter personal data out of download protection pings (fixes brave/brave-browser#6267) --- .../check_client_download_request_base.cc | 29 ++++++ ...lient_download_request_base_browsertest.cc | 86 ++++++++++++++++++ ...heck_client_download_request_base.cc.patch | 12 +++ test/BUILD.gn | 1 + test/data/test.exe | Bin 0 -> 120 bytes 5 files changed, 128 insertions(+) create mode 100644 chromium_src/chrome/browser/safe_browsing/download_protection/check_client_download_request_base.cc create mode 100644 chromium_src/chrome/browser/safe_browsing/download_protection/check_client_download_request_base_browsertest.cc create mode 100644 patches/chrome-browser-safe_browsing-download_protection-check_client_download_request_base.cc.patch create mode 100644 test/data/test.exe diff --git a/chromium_src/chrome/browser/safe_browsing/download_protection/check_client_download_request_base.cc b/chromium_src/chrome/browser/safe_browsing/download_protection/check_client_download_request_base.cc new file mode 100644 index 000000000000..9a19cd4bb557 --- /dev/null +++ b/chromium_src/chrome/browser/safe_browsing/download_protection/check_client_download_request_base.cc @@ -0,0 +1,29 @@ +/* Copyright (c) 2020 The Brave Authors. All rights reserved. + * 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 https://mozilla.org/MPL/2.0/. */ + +#include "components/safe_browsing/core/proto/csd.pb.h" + +#define BRAVE_SEND_REQUEST_FILTER BraveFilterRequest(request.get()); + +namespace safe_browsing { + +void BraveFilterRequest(ClientDownloadRequest* request) { + request->set_url(""); // URL must be present or we get a 400. + request->clear_file_basename(); + request->clear_locale(); + request->clear_resources(); // Contains URLs and referrers + request->clear_referrer_chain(); + + // Filter binaries within archives. + for (int i = 0; i < request->archived_binary_size(); i++) { + ClientDownloadRequest_ArchivedBinary* archived_binary = + request->mutable_archived_binary(i); + archived_binary->clear_file_basename(); + } +} + +} // namespace safe_browsing + +#include "../../../../../../chrome/browser/safe_browsing/download_protection/check_client_download_request_base.cc" // NOLINT diff --git a/chromium_src/chrome/browser/safe_browsing/download_protection/check_client_download_request_base_browsertest.cc b/chromium_src/chrome/browser/safe_browsing/download_protection/check_client_download_request_base_browsertest.cc new file mode 100644 index 000000000000..920d06d8d08a --- /dev/null +++ b/chromium_src/chrome/browser/safe_browsing/download_protection/check_client_download_request_base_browsertest.cc @@ -0,0 +1,86 @@ +/* Copyright (c) 2020 The Brave Authors. All rights reserved. + * 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 https://mozilla.org/MPL/2.0/. */ + +#include "base/path_service.h" +#include "base/strings/stringprintf.h" +#include "brave/common/brave_paths.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/browser/ui/browser.h" +#include "chrome/common/pref_names.h" +#include "chrome/test/base/in_process_browser_test.h" +#include "chrome/test/base/ui_test_utils.h" +#include "components/network_session_configurator/common/network_switches.h" +#include "components/prefs/pref_service.h" +#include "components/safe_browsing/content/web_ui/safe_browsing_ui.h" +#include "components/safe_browsing/core/proto/csd.pb.h" +#include "content/public/browser/browser_context.h" +#include "content/public/browser/download_manager.h" +#include "content/public/test/browser_test.h" +#include "content/public/test/browser_test_utils.h" +#include "content/public/test/download_test_observer.h" +#include "content/public/test/test_utils.h" +#include "net/dns/mock_host_resolver.h" +#include "net/test/embedded_test_server/default_handlers.h" +#include "net/test/embedded_test_server/http_request.h" +#include "ui/base/window_open_disposition.h" + +class BraveCheckClientDownloadRequestBaseBrowserTest + : public InProcessBrowserTest { + public: + BraveCheckClientDownloadRequestBaseBrowserTest() + : https_server_(net::EmbeddedTestServer::TYPE_HTTPS) {} + + void SetUpOnMainThread() override { + InProcessBrowserTest::SetUpOnMainThread(); + + host_resolver()->AddRule("*", "127.0.0.1"); + + browser()->profile()->GetPrefs()->SetBoolean(prefs::kPromptForDownload, + false); + + brave::RegisterPathProvider(); + base::PathService::Get(brave::DIR_TEST_DATA, &test_data_dir_); + https_server_.ServeFilesFromDirectory(test_data_dir_); + https_server_.AddDefaultHandlers(GetChromeTestDataDir()); + safe_browsing::WebUIInfoSingleton::GetInstance()->AddListenerForTesting(); + + ASSERT_TRUE(https_server_.Start()); + + download_url_ = https_server_.GetURL("a.com", "/test.exe"); + } + + void SetUpCommandLine(base::CommandLine* command_line) override { + InProcessBrowserTest::SetUpCommandLine(command_line); + // This is needed to load pages from "domain.com" without an interstitial. + command_line->AppendSwitch(switches::kIgnoreCertificateErrors); + } + + const net::EmbeddedTestServer& https_server() { return https_server_; } + const GURL& download_url() { return download_url_; } + + private: + GURL download_url_; + base::FilePath test_data_dir_; + + net::test_server::EmbeddedTestServer https_server_; +}; + +IN_PROC_BROWSER_TEST_F(BraveCheckClientDownloadRequestBaseBrowserTest, + FilterRequest) { + ui_test_utils::DownloadURL(browser(), download_url()); + + const std::vector>& + requests = safe_browsing::WebUIInfoSingleton::GetInstance() + ->client_download_requests_sent(); + + ASSERT_EQ(requests.size(), 1u); + + EXPECT_TRUE(requests[0]->has_url()); + EXPECT_EQ(requests[0]->url(), ""); + EXPECT_FALSE(requests[0]->has_locale()); + EXPECT_FALSE(requests[0]->has_file_basename()); + EXPECT_EQ(requests[0]->referrer_chain_size(), 0); + EXPECT_EQ(requests[0]->resources_size(), 0); +} diff --git a/patches/chrome-browser-safe_browsing-download_protection-check_client_download_request_base.cc.patch b/patches/chrome-browser-safe_browsing-download_protection-check_client_download_request_base.cc.patch new file mode 100644 index 000000000000..ffe4850fe2fc --- /dev/null +++ b/patches/chrome-browser-safe_browsing-download_protection-check_client_download_request_base.cc.patch @@ -0,0 +1,12 @@ +diff --git a/chrome/browser/safe_browsing/download_protection/check_client_download_request_base.cc b/chrome/browser/safe_browsing/download_protection/check_client_download_request_base.cc +index 4f7fd86c5e6edd387254d0eab6aa399db5e42105..1edf51b8e80c967e275e327d8143973ec4881871 100644 +--- a/chrome/browser/safe_browsing/download_protection/check_client_download_request_base.cc ++++ b/chrome/browser/safe_browsing/download_protection/check_client_download_request_base.cc +@@ -554,6 +554,7 @@ void CheckClientDownloadRequestBase::SendRequest() { + request->set_archive_directory_count(directory_count_); + request->set_request_ap_verdicts(is_under_advanced_protection_); + ++ BRAVE_SEND_REQUEST_FILTER + if (!request->SerializeToString(&client_download_request_data_)) { + FinishRequest(DownloadCheckResult::UNKNOWN, REASON_INVALID_REQUEST_PROTO); + return; diff --git a/test/BUILD.gn b/test/BUILD.gn index e959033765bb..8049aca1e06d 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -552,6 +552,7 @@ if (!is_android) { "//brave/browser/ui/webui/brave_welcome_ui_browsertest.cc", "//brave/chromium_src/chrome/browser/media/router/media_router_feature_browsertest.cc", "//brave/chromium_src/chrome/browser/profiles/profile_window_browsertest.cc", + "//brave/chromium_src/chrome/browser/safe_browsing/download_protection/check_client_download_request_base_browsertest.cc", "//brave/chromium_src/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc", "//brave/chromium_src/chrome/browser/ui/views/tabs/tab_hover_card_bubble_view_browsertest.cc", "//brave/chromium_src/components/content_settings/core/browser/brave_content_settings_registry_browsertest.cc", diff --git a/test/data/test.exe b/test/data/test.exe new file mode 100644 index 0000000000000000000000000000000000000000..f35beb4f2552c3bf7afe6040b3b1e15f90d3ad28 GIT binary patch literal 120 zcmeZ`n!v!!z`(!)#Q*;@Fzf)*Am9Kd@e>U|X+HT~d<