diff options
Diffstat (limited to 'chromium/components/safe_browsing')
128 files changed, 1781 insertions, 1131 deletions
diff --git a/chromium/components/safe_browsing/BUILD.gn b/chromium/components/safe_browsing/BUILD.gn index 71f40a9d5da..f076662b8da 100644 --- a/chromium/components/safe_browsing/BUILD.gn +++ b/chromium/components/safe_browsing/BUILD.gn @@ -9,6 +9,19 @@ buildflag_header("buildflags") { header = "buildflags.h" flags = [] + + # FULL_SAFE_BROWSING means "are all Safe Browsing features available?" + # This is true only for desktop OSes. + # + # SAFE_BROWSING_AVAILABLE means "are any Safe Browsing features available?" + # This is true only for desktop OSes or Android. + # + # SAFE_BROWSING_DB_LOCAL means "are SB databases available locally?" + # This is true only for desktop OSes. + # + # SAFE_BROWSING_DB_REMOTE means "are SB databases available via GMS Core?" + # This is true only for Android. + # if (safe_browsing_mode == 0) { flags += [ "FULL_SAFE_BROWSING=0" ] flags += [ "SAFE_BROWSING_AVAILABLE=0" ] diff --git a/chromium/components/safe_browsing/DEPS b/chromium/components/safe_browsing/DEPS index f4d1cf27330..e7a9e52494c 100644 --- a/chromium/components/safe_browsing/DEPS +++ b/chromium/components/safe_browsing/DEPS @@ -30,6 +30,7 @@ include_rules = [ "+third_party/tflite_support", "+third_party/tflite", "+third_party/protobuf", + "+third_party/blink/public/common/associated_interfaces", "+ui/base/resource/resource_bundle.h", "+ui/android/view_android.h", diff --git a/chromium/components/safe_browsing/android/BUILD.gn b/chromium/components/safe_browsing/android/BUILD.gn index 32b6836d9cf..7c783081624 100644 --- a/chromium/components/safe_browsing/android/BUILD.gn +++ b/chromium/components/safe_browsing/android/BUILD.gn @@ -8,6 +8,8 @@ import("//build/config/android/rules.gni") android_library("safe_browsing_java") { deps = [ "//base:base_java", + "//base:jni_java", + "//build/android:build_java", "//third_party/androidx:androidx_annotation_annotation_java", ] annotation_processor_deps = [ "//base/android/jni_generator:jni_processor" ] @@ -66,25 +68,19 @@ static_library("safe_browsing_api_handler_util") { source_set("safe_browsing_api_handler") { sources = [ - "safe_browsing_api_handler.cc", - "safe_browsing_api_handler.h", + "safe_browsing_api_handler_bridge.cc", + "safe_browsing_api_handler_bridge.h", ] deps = [ + ":jni_headers", ":safe_browsing_api_handler_util", "//base", + "//components/safe_browsing:buildflags", "//components/safe_browsing/core/browser/db:util", "//components/safe_browsing/core/common", "//content/public/browser:browser", "//url", ] - - if (is_android) { - deps += [ ":jni_headers" ] - sources += [ - "safe_browsing_api_handler_bridge.cc", - "safe_browsing_api_handler_bridge.h", - ] - } } source_set("unit_tests_mobile") { diff --git a/chromium/components/safe_browsing/android/remote_database_manager.cc b/chromium/components/safe_browsing/android/remote_database_manager.cc index c7e4c500abb..e86a3dd3cf7 100644 --- a/chromium/components/safe_browsing/android/remote_database_manager.cc +++ b/chromium/components/safe_browsing/android/remote_database_manager.cc @@ -14,7 +14,7 @@ #include "base/strings/string_number_conversions.h" #include "base/strings/string_split.h" #include "base/timer/elapsed_timer.h" -#include "components/safe_browsing/android/safe_browsing_api_handler.h" +#include "components/safe_browsing/android/safe_browsing_api_handler_bridge.h" #include "components/safe_browsing/core/browser/db/v4_get_hash_protocol_manager.h" #include "components/safe_browsing/core/browser/db/v4_protocol_manager_util.h" #include "components/variations/variations_associated_data.h" @@ -206,16 +206,11 @@ bool RemoteSafeBrowsingDatabaseManager::CheckBrowseUrl( std::unique_ptr<ClientRequest> req(new ClientRequest(client, this, url)); DVLOG(1) << "Checking for client " << client << " and URL " << url; - SafeBrowsingApiHandler* api_handler = SafeBrowsingApiHandler::GetInstance(); - // This shouldn't happen since SafeBrowsingResourceThrottle and - // SubresourceFilterSafeBrowsingActivationThrottle check IsSupported() - // earlier. - DCHECK(api_handler) << "SafeBrowsingApiHandler was never constructed"; - auto callback = - std::make_unique<SafeBrowsingApiHandler::URLCheckCallbackMeta>( + std::make_unique<SafeBrowsingApiHandlerBridge::ResponseCallback>( base::BindOnce(&ClientRequest::OnRequestDoneWeak, req->GetWeakPtr())); - api_handler->StartURLCheck(std::move(callback), url, threat_types); + SafeBrowsingApiHandlerBridge::GetInstance().StartURLCheck(std::move(callback), + url, threat_types); current_requests_.push_back(req.release()); @@ -253,8 +248,8 @@ RemoteSafeBrowsingDatabaseManager::CheckUrlForHighConfidenceAllowlist( return AsyncMatch::NO_MATCH; // TODO(crbug.com/1014202): Make this call async. - SafeBrowsingApiHandler* api_handler = SafeBrowsingApiHandler::GetInstance(); - bool is_match = api_handler->StartHighConfidenceAllowlistCheck(url); + bool is_match = SafeBrowsingApiHandlerBridge::GetInstance() + .StartHighConfidenceAllowlistCheck(url); return is_match ? AsyncMatch::MATCH : AsyncMatch::NO_MATCH; } @@ -276,15 +271,10 @@ bool RemoteSafeBrowsingDatabaseManager::CheckUrlForSubresourceFilter( std::unique_ptr<ClientRequest> req(new ClientRequest(client, this, url)); DVLOG(1) << "Checking for client " << client << " and URL " << url; - SafeBrowsingApiHandler* api_handler = SafeBrowsingApiHandler::GetInstance(); - // This shouldn't happen since SafeBrowsingResourceThrottle and - // SubresourceFilterSafeBrowsingActivationThrottle check IsSupported() - // earlier. - DCHECK(api_handler) << "SafeBrowsingApiHandler was never constructed"; auto callback = - std::make_unique<SafeBrowsingApiHandler::URLCheckCallbackMeta>( + std::make_unique<SafeBrowsingApiHandlerBridge::ResponseCallback>( base::BindOnce(&ClientRequest::OnRequestDoneWeak, req->GetWeakPtr())); - api_handler->StartURLCheck( + SafeBrowsingApiHandlerBridge::GetInstance().StartURLCheck( std::move(callback), url, CreateSBThreatTypeSet( {SB_THREAT_TYPE_SUBRESOURCE_FILTER, SB_THREAT_TYPE_URL_PHISHING})); @@ -305,9 +295,9 @@ AsyncMatch RemoteSafeBrowsingDatabaseManager::CheckCsdAllowlistUrl( return AsyncMatch::MATCH; } - // TODO(crbug.com/995926): Make this call async - SafeBrowsingApiHandler* api_handler = SafeBrowsingApiHandler::GetInstance(); - bool is_match = api_handler->StartCSDAllowlistCheck(url); + // TODO(crbug.com/995926): Make this call async. + bool is_match = + SafeBrowsingApiHandlerBridge::GetInstance().StartCSDAllowlistCheck(url); return is_match ? AsyncMatch::MATCH : AsyncMatch::NO_MATCH; } @@ -332,10 +322,6 @@ bool RemoteSafeBrowsingDatabaseManager::IsDownloadProtectionEnabled() const { return false; } -bool RemoteSafeBrowsingDatabaseManager::IsSupported() const { - return SafeBrowsingApiHandler::GetInstance() != nullptr; -} - void RemoteSafeBrowsingDatabaseManager::StartOnIOThread( scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, const V4ProtocolConfig& config) { diff --git a/chromium/components/safe_browsing/android/remote_database_manager.h b/chromium/components/safe_browsing/android/remote_database_manager.h index d8a559fdfee..af709615304 100644 --- a/chromium/components/safe_browsing/android/remote_database_manager.h +++ b/chromium/components/safe_browsing/android/remote_database_manager.h @@ -62,7 +62,6 @@ class RemoteSafeBrowsingDatabaseManager : public SafeBrowsingDatabaseManager { bool MatchMalwareIP(const std::string& ip_address) override; safe_browsing::ThreatSource GetThreatSource() const override; bool IsDownloadProtectionEnabled() const override; - bool IsSupported() const override; void StartOnIOThread( scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, const V4ProtocolConfig& config) override; diff --git a/chromium/components/safe_browsing/android/remote_database_manager_unittest.cc b/chromium/components/safe_browsing/android/remote_database_manager_unittest.cc index 1f54c5c3143..1f3e32496c7 100644 --- a/chromium/components/safe_browsing/android/remote_database_manager_unittest.cc +++ b/chromium/components/safe_browsing/android/remote_database_manager_unittest.cc @@ -10,7 +10,7 @@ #include "base/metrics/field_trial.h" #include "base/run_loop.h" #include "base/time/time.h" -#include "components/safe_browsing/android/safe_browsing_api_handler.h" +#include "components/safe_browsing/android/safe_browsing_api_handler_bridge.h" #include "components/variations/variations_associated_data.h" #include "content/public/test/browser_task_environment.h" #include "services/network/public/mojom/fetch_api.mojom.h" @@ -20,15 +20,12 @@ namespace safe_browsing { namespace { -class TestSafeBrowsingApiHandler : public SafeBrowsingApiHandler { +class BlackHoleInterceptor : public safe_browsing::UrlCheckInterceptor { public: - void StartURLCheck(std::unique_ptr<URLCheckCallbackMeta> callback, - const GURL& url, - const SBThreatTypeSet& threat_types) override {} - bool StartCSDAllowlistCheck(const GURL& url) override { return false; } - bool StartHighConfidenceAllowlistCheck(const GURL& url) override { - return false; - } + void Check( + std::unique_ptr<SafeBrowsingApiHandlerBridge::ResponseCallback> callback, + const GURL& url) const override{}; + ~BlackHoleInterceptor() override{}; }; } // namespace @@ -38,7 +35,8 @@ class RemoteDatabaseManagerTest : public testing::Test { RemoteDatabaseManagerTest() {} void SetUp() override { - SafeBrowsingApiHandler::SetInstance(&api_handler_); + SafeBrowsingApiHandlerBridge::GetInstance().SetInterceptorForTesting( + &url_interceptor_); db_ = new RemoteSafeBrowsingDatabaseManager(); } @@ -66,17 +64,10 @@ class RemoteDatabaseManagerTest : public testing::Test { } content::BrowserTaskEnvironment task_environment_; - TestSafeBrowsingApiHandler api_handler_; + BlackHoleInterceptor url_interceptor_; scoped_refptr<RemoteSafeBrowsingDatabaseManager> db_; }; -TEST_F(RemoteDatabaseManagerTest, DisabledViaNull) { - EXPECT_TRUE(db_->IsSupported()); - - SafeBrowsingApiHandler::SetInstance(nullptr); - EXPECT_FALSE(db_->IsSupported()); -} - TEST_F(RemoteDatabaseManagerTest, DestinationsToCheckDefault) { // Most are true, a few are false. for (int t_int = 0; diff --git a/chromium/components/safe_browsing/android/safe_browsing_api_handler.cc b/chromium/components/safe_browsing/android/safe_browsing_api_handler.cc deleted file mode 100644 index b8d15323ffa..00000000000 --- a/chromium/components/safe_browsing/android/safe_browsing_api_handler.cc +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright 2015 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 "components/safe_browsing/android/safe_browsing_api_handler.h" -#include "base/bind.h" - -namespace safe_browsing { - -SafeBrowsingApiHandler* SafeBrowsingApiHandler::instance_ = nullptr; - -// static -void SafeBrowsingApiHandler::SetInstance(SafeBrowsingApiHandler* instance) { - instance_ = instance; -} - -// static -SafeBrowsingApiHandler* SafeBrowsingApiHandler::GetInstance() { - return instance_; -} - -} // namespace safe_browsing diff --git a/chromium/components/safe_browsing/android/safe_browsing_api_handler.h b/chromium/components/safe_browsing/android/safe_browsing_api_handler.h deleted file mode 100644 index 51fcb763cd3..00000000000 --- a/chromium/components/safe_browsing/android/safe_browsing_api_handler.h +++ /dev/null @@ -1,48 +0,0 @@ -// Copyright 2015 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. -// -// Glue to pass Safe Browsing API requests between -// RemoteSafeBrowsingDatabaseManager and Java-based API to check URLs. - -#ifndef COMPONENTS_SAFE_BROWSING_ANDROID_SAFE_BROWSING_API_HANDLER_H_ -#define COMPONENTS_SAFE_BROWSING_ANDROID_SAFE_BROWSING_API_HANDLER_H_ - -#include <memory> - -#include "base/callback.h" -#include "components/safe_browsing/core/browser/db/util.h" -#include "components/safe_browsing/core/browser/db/v4_protocol_manager_util.h" -#include "url/gurl.h" - -namespace safe_browsing { - -class SafeBrowsingApiHandler { - public: - // Singleton interface. - static void SetInstance(SafeBrowsingApiHandler* instance); - static SafeBrowsingApiHandler* GetInstance(); - - typedef base::OnceCallback<void(SBThreatType sb_threat_type, - const ThreatMetadata& metadata)> - URLCheckCallbackMeta; - - // Makes Native->Java call and invokes callback when check is done. - virtual void StartURLCheck(std::unique_ptr<URLCheckCallbackMeta> callback, - const GURL& url, - const SBThreatTypeSet& threat_types) = 0; - - virtual bool StartCSDAllowlistCheck(const GURL& url) = 0; - - virtual bool StartHighConfidenceAllowlistCheck(const GURL& url) = 0; - - virtual ~SafeBrowsingApiHandler() {} - - private: - // Pointer not owned. - static SafeBrowsingApiHandler* instance_; -}; - -} // namespace safe_browsing - -#endif // COMPONENTS_SAFE_BROWSING_ANDROID_SAFE_BROWSING_API_HANDLER_H_ diff --git a/chromium/components/safe_browsing/android/safe_browsing_api_handler_bridge.cc b/chromium/components/safe_browsing/android/safe_browsing_api_handler_bridge.cc index d7b3d9636f7..47ce6729fac 100644 --- a/chromium/components/safe_browsing/android/safe_browsing_api_handler_bridge.cc +++ b/chromium/components/safe_browsing/android/safe_browsing_api_handler_bridge.cc @@ -36,11 +36,9 @@ namespace safe_browsing { namespace { void RunCallbackOnIOThread( - std::unique_ptr<SafeBrowsingApiHandler::URLCheckCallbackMeta> callback, + std::unique_ptr<SafeBrowsingApiHandlerBridge::ResponseCallback> callback, SBThreatType threat_type, const ThreatMetadata& metadata) { - CHECK(callback); // Remove after fixing crbug.com/889972 - CHECK(!callback->is_null()); // Remove after fixing crbug.com/889972 content::GetIOThreadTaskRunner({})->PostTask( FROM_HERE, base::BindOnce(std::move(*callback), threat_type, metadata)); } @@ -91,7 +89,7 @@ ScopedJavaLocalRef<jintArray> SBThreatTypeSetToJavaArray( // response. typedef std::unordered_map< jlong, - std::unique_ptr<SafeBrowsingApiHandler::URLCheckCallbackMeta>> + std::unique_ptr<SafeBrowsingApiHandlerBridge::ResponseCallback>> PendingCallbacksMap; static PendingCallbacksMap* GetPendingCallbacksMapOnIOThread() { @@ -104,14 +102,32 @@ static PendingCallbacksMap* GetPendingCallbacksMapOnIOThread() { return &pending_callbacks; } +bool StartAllowlistCheck(const GURL& url, const SBThreatType& sb_threat_type) { + DCHECK_CURRENTLY_ON(BrowserThread::IO); + JNIEnv* env = AttachCurrentThread(); + if (!Java_SafeBrowsingApiBridge_ensureCreated(env)) { + return false; + } + + ScopedJavaLocalRef<jstring> j_url = ConvertUTF8ToJavaString(env, url.spec()); + int j_threat_type = SBThreatTypeToJavaThreatType(sb_threat_type); + return Java_SafeBrowsingApiBridge_startAllowlistLookup(env, j_url, + j_threat_type); +} + } // namespace +// static +SafeBrowsingApiHandlerBridge& SafeBrowsingApiHandlerBridge::GetInstance() { + static base::NoDestructor<SafeBrowsingApiHandlerBridge> instance; + return *instance.get(); +} // Respond to the URL reputation request by looking up the callback information // stored in |pending_callbacks|. -// |callback_id| is an int form of pointer to a URLCheckCallbackMeta +// |callback_id| is an int form of pointer to a ::ResponseCallback // that will be called and then deleted here. -// |result_status| is one of those from SafeBrowsingApiHandler.java +// |result_status| is one of those from SafeBrowsingApiHandlerBridge.java // |metadata| is a JSON string classifying the threat if there is one. void OnUrlCheckDoneOnIOThread(jlong callback_id, jint result_status, @@ -124,19 +140,14 @@ void OnUrlCheckDoneOnIOThread(jlong callback_id, if (!found) return; - std::unique_ptr<SafeBrowsingApiHandler::URLCheckCallbackMeta> callback = + std::unique_ptr<SafeBrowsingApiHandlerBridge::ResponseCallback> callback = std::move((*pending_callbacks)[callback_id]); - CHECK(callback); // Remove after fixing crbug.com/889972 pending_callbacks->erase(callback_id); if (result_status != RESULT_STATUS_SUCCESS) { if (result_status == RESULT_STATUS_TIMEOUT) { - CHECK(!callback->is_null()); // Remove after fixing crbug.com/889972 - ReportUmaResult(UMA_STATUS_TIMEOUT); } else { - CHECK(!callback->is_null()); // Remove after fixing crbug.com/889972 - DCHECK_EQ(result_status, RESULT_STATUS_INTERNAL_ERROR); ReportUmaResult(UMA_STATUS_INTERNAL_ERROR); } @@ -146,13 +157,9 @@ void OnUrlCheckDoneOnIOThread(jlong callback_id, // Shortcut for safe, so we don't have to parse JSON. if (metadata == "{}") { - CHECK(!callback->is_null()); // Remove after fixing crbug.com/889972 - ReportUmaResult(UMA_STATUS_SAFE); std::move(*callback).Run(SB_THREAT_TYPE_SAFE, ThreatMetadata()); } else { - CHECK(!callback->is_null()); // Remove after fixing crbug.com/889972 - // Unsafe, assuming we can parse the JSON. SBThreatType worst_threat; ThreatMetadata threat_metadata; @@ -165,9 +172,9 @@ void OnUrlCheckDoneOnIOThread(jlong callback_id, // Java->Native call, invoked when a check is done. // |callback_id| is a key into the |pending_callbacks_| map, whose value is a -// URLCheckCallbackMeta that will be called and then deleted on +// ::ResponseCallback that will be called and then deleted on // the IO thread. -// |result_status| is one of those from SafeBrowsingApiHandler.java +// |result_status| is a @SafeBrowsingResult from SafeBrowsingApiHandler.java // |metadata| is a JSON string classifying the threat if there is one. // |check_delta| is the number of microseconds it took to look up the URL // reputation from GmsCore. @@ -196,42 +203,20 @@ void JNI_SafeBrowsingApiBridge_OnUrlCheckDone( // // SafeBrowsingApiHandlerBridge // -SafeBrowsingApiHandlerBridge::SafeBrowsingApiHandlerBridge() - : checked_api_support_(false) {} - SafeBrowsingApiHandlerBridge::~SafeBrowsingApiHandlerBridge() {} -bool SafeBrowsingApiHandlerBridge::CheckApiIsSupported() { - DCHECK_CURRENTLY_ON(BrowserThread::IO); - if (!checked_api_support_) { - j_api_handler_ = base::android::ScopedJavaGlobalRef<jobject>( - Java_SafeBrowsingApiBridge_create(AttachCurrentThread())); - checked_api_support_ = true; - } - return j_api_handler_.obj() != nullptr; -} - -bool SafeBrowsingApiHandlerBridge::StartAllowlistCheck( - const GURL& url, - const SBThreatType& sb_threat_type) { - DCHECK_CURRENTLY_ON(BrowserThread::IO); - if (!CheckApiIsSupported()) { - return false; - } - - JNIEnv* env = AttachCurrentThread(); - ScopedJavaLocalRef<jstring> j_url = ConvertUTF8ToJavaString(env, url.spec()); - int j_threat_type = SBThreatTypeToJavaThreatType(sb_threat_type); - return Java_SafeBrowsingApiBridge_startAllowlistLookup(env, j_api_handler_, - j_url, j_threat_type); -} - void SafeBrowsingApiHandlerBridge::StartURLCheck( - std::unique_ptr<SafeBrowsingApiHandler::URLCheckCallbackMeta> callback, + std::unique_ptr<ResponseCallback> callback, const GURL& url, const SBThreatTypeSet& threat_types) { + if (interceptor_for_testing_) { + // For testing, only check the interceptor. + interceptor_for_testing_->Check(std::move(callback), url); + return; + } DCHECK_CURRENTLY_ON(BrowserThread::IO); - if (!CheckApiIsSupported()) { + JNIEnv* env = AttachCurrentThread(); + if (!Java_SafeBrowsingApiBridge_ensureCreated(env)) { // Mark all requests as safe. Only users who have an old, broken GMSCore or // have sideloaded Chrome w/o PlayStore should land here. RunCallbackOnIOThread(std::move(callback), SB_THREAT_TYPE_SAFE, @@ -246,21 +231,24 @@ void SafeBrowsingApiHandlerBridge::StartURLCheck( DCHECK(!threat_types.empty()); - JNIEnv* env = AttachCurrentThread(); ScopedJavaLocalRef<jstring> j_url = ConvertUTF8ToJavaString(env, url.spec()); ScopedJavaLocalRef<jintArray> j_threat_types = SBThreatTypeSetToJavaArray(env, threat_types); - Java_SafeBrowsingApiBridge_startUriLookup(env, j_api_handler_, callback_id, - j_url, j_threat_types); + Java_SafeBrowsingApiBridge_startUriLookup(env, callback_id, j_url, + j_threat_types); } bool SafeBrowsingApiHandlerBridge::StartCSDAllowlistCheck(const GURL& url) { + if (interceptor_for_testing_) + return false; return StartAllowlistCheck(url, safe_browsing::SB_THREAT_TYPE_CSD_ALLOWLIST); } bool SafeBrowsingApiHandlerBridge::StartHighConfidenceAllowlistCheck( const GURL& url) { + if (interceptor_for_testing_) + return false; return StartAllowlistCheck( url, safe_browsing::SB_THREAT_TYPE_HIGH_CONFIDENCE_ALLOWLIST); } diff --git a/chromium/components/safe_browsing/android/safe_browsing_api_handler_bridge.h b/chromium/components/safe_browsing/android/safe_browsing_api_handler_bridge.h index ca63d2cb785..298617c7f9a 100644 --- a/chromium/components/safe_browsing/android/safe_browsing_api_handler_bridge.h +++ b/chromium/components/safe_browsing/android/safe_browsing_api_handler_bridge.h @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. // -// Glue to pass Safe Browsing API requests between Chrome and GMSCore +// Glue to pass Safe Browsing API requests between Chrome and GMSCore. #ifndef COMPONENTS_SAFE_BROWSING_ANDROID_SAFE_BROWSING_API_HANDLER_BRIDGE_H_ #define COMPONENTS_SAFE_BROWSING_ANDROID_SAFE_BROWSING_API_HANDLER_BRIDGE_H_ @@ -10,48 +10,63 @@ #include <jni.h> #include "base/android/jni_android.h" -#include "components/safe_browsing/android/safe_browsing_api_handler.h" +#include "base/callback.h" #include "components/safe_browsing/core/browser/db/v4_protocol_manager_util.h" -#include "url/gurl.h" + +class GURL; namespace safe_browsing { -class SafeBrowsingApiHandlerBridge : public SafeBrowsingApiHandler { +class UrlCheckInterceptor; +struct ThreatMetadata; + +class SafeBrowsingApiHandlerBridge { public: - SafeBrowsingApiHandlerBridge(); + using ResponseCallback = + base::OnceCallback<void(SBThreatType, const ThreatMetadata&)>; + + SafeBrowsingApiHandlerBridge() = default; + + ~SafeBrowsingApiHandlerBridge(); SafeBrowsingApiHandlerBridge(const SafeBrowsingApiHandlerBridge&) = delete; SafeBrowsingApiHandlerBridge& operator=(const SafeBrowsingApiHandlerBridge&) = delete; - ~SafeBrowsingApiHandlerBridge() override; + // Returns a reference to the singleton. + static SafeBrowsingApiHandlerBridge& GetInstance(); - // Makes Native->Java call to check the URL against Safe Browsing lists. - void StartURLCheck(std::unique_ptr<URLCheckCallbackMeta> callback, + // Makes Native-to-Java call to check the URL against Safe Browsing lists. + void StartURLCheck(std::unique_ptr<ResponseCallback> callback, const GURL& url, - const SBThreatTypeSet& threat_types) override; - - bool StartCSDAllowlistCheck(const GURL& url) override; - - bool StartHighConfidenceAllowlistCheck(const GURL& url) override; - - private: - // Creates the |j_api_handler_| if it hasn't been already. If the API is not - // supported, this will return false and j_api_handler_ will remain nullptr. - bool CheckApiIsSupported(); + const SBThreatTypeSet& threat_types); - bool StartAllowlistCheck(const GURL& url, const SBThreatType& sb_threat_type); + bool StartCSDAllowlistCheck(const GURL& url); - // The Java-side SafeBrowsingApiHandler. Must call CheckApiIsSupported first. - base::android::ScopedJavaGlobalRef<jobject> j_api_handler_; + bool StartHighConfidenceAllowlistCheck(const GURL& url); - // True if we've once tried to create the above object. - bool checked_api_support_; + void SetInterceptorForTesting(UrlCheckInterceptor* interceptor) { + interceptor_for_testing_ = interceptor; + } + private: // Used as a key to identify unique requests sent to Java to get Safe Browsing // reputation from GmsCore. jlong next_callback_id_ = 0; + + UrlCheckInterceptor* interceptor_for_testing_ = nullptr; +}; + +// Interface allowing simplified interception of calls to +// SafeBrowsingApiHandlerBridge. Intended for use only in tests. +class UrlCheckInterceptor { + public: + virtual ~UrlCheckInterceptor(){}; + virtual void Check( + std::unique_ptr<SafeBrowsingApiHandlerBridge::ResponseCallback> callback, + const GURL& url) const = 0; }; } // namespace safe_browsing + #endif // COMPONENTS_SAFE_BROWSING_ANDROID_SAFE_BROWSING_API_HANDLER_BRIDGE_H_ diff --git a/chromium/components/safe_browsing/android/safe_browsing_api_handler_util.h b/chromium/components/safe_browsing/android/safe_browsing_api_handler_util.h index 884c8eecab5..48eb41b9219 100644 --- a/chromium/components/safe_browsing/android/safe_browsing_api_handler_util.h +++ b/chromium/components/safe_browsing/android/safe_browsing_api_handler_util.h @@ -59,10 +59,6 @@ UmaRemoteCallResult ParseJsonFromGMSCore(const std::string& metadata_str, SBThreatType* worst_threat, ThreatMetadata* metadata); -// DEPRECATED. Will be removed. -UmaRemoteCallResult ParseJsonToThreatAndPB(const std::string& metadata_str, - SBThreatType* worst_threat, - std::string* metadata_pb_str); } // namespace safe_browsing #endif // COMPONENTS_SAFE_BROWSING_ANDROID_SAFE_BROWSING_API_HANDLER_UTIL_H_ diff --git a/chromium/components/safe_browsing/buildflags.gni b/chromium/components/safe_browsing/buildflags.gni index 48e74db6c18..ebdc4d3e942 100644 --- a/chromium/components/safe_browsing/buildflags.gni +++ b/chromium/components/safe_browsing/buildflags.gni @@ -12,7 +12,7 @@ declare_args() { # safe browsing feature. Safe browsing can be compiled in 3 different levels: # 0 disables it, 1 enables it fully, and 2 enables mobile protection via an # external API. - if (is_ios || is_chromecast) { + if (is_ios || is_castos || is_cast_android) { safe_browsing_mode = 0 } else if (is_android) { safe_browsing_mode = 2 diff --git a/chromium/components/safe_browsing/content/browser/BUILD.gn b/chromium/components/safe_browsing/content/browser/BUILD.gn index 0ff7a602b31..db86826f858 100644 --- a/chromium/components/safe_browsing/content/browser/BUILD.gn +++ b/chromium/components/safe_browsing/content/browser/BUILD.gn @@ -48,11 +48,11 @@ if (safe_browsing_mode > 0) { "//components/no_state_prefetch/browser", "//components/prefs", "//components/safe_browsing/content/browser/triggers", + "//components/safe_browsing/content/browser/web_ui", "//components/safe_browsing/core/browser", "//components/safe_browsing/core/browser:safe_browsing_metrics_collector", "//components/safe_browsing/core/common", "//components/safe_browsing/core/common:safe_browsing_prefs", - "//components/safe_browsing/core/common/proto:csd_proto", "//components/security_interstitials/content:security_interstitial_page", "//components/security_interstitials/core", "//components/security_interstitials/core:unsafe_resource", diff --git a/chromium/components/safe_browsing/content/browser/DEPS b/chromium/components/safe_browsing/content/browser/DEPS index 5e1c18dd8dd..1a19a80b0b2 100644 --- a/chromium/components/safe_browsing/content/browser/DEPS +++ b/chromium/components/safe_browsing/content/browser/DEPS @@ -16,6 +16,7 @@ include_rules = [ "+content/public/test", "+crypto/sha2.h", "+ipc/ipc_message.h", + "+ipc/ipc_channel_proxy.h", "+net/cookies", "+net/extras", "+net/http", diff --git a/chromium/components/safe_browsing/content/browser/base_ui_manager.cc b/chromium/components/safe_browsing/content/browser/base_ui_manager.cc index c81f75938d6..bee05840ec0 100644 --- a/chromium/components/safe_browsing/content/browser/base_ui_manager.cc +++ b/chromium/components/safe_browsing/content/browser/base_ui_manager.cc @@ -315,7 +315,7 @@ void BaseUIManager::DisplayBlockingPage(const UnsafeResource& resource) { outermost_contents, unsafe_url, resource)); base::WeakPtr<content::NavigationHandle> error_page_navigation_handle = outermost_contents->GetController().LoadPostCommitErrorPage( - outermost_contents->GetMainFrame(), unsafe_url, + outermost_contents->GetPrimaryMainFrame(), unsafe_url, blocking_page->GetHTMLContents(), net::ERR_BLOCKED_BY_CLIENT); if (error_page_navigation_handle) { blocking_page->CreatedPostCommitErrorPageNavigation( @@ -357,9 +357,9 @@ void BaseUIManager::MaybeReportSafeBrowsingHit( // If the user had opted-in to send ThreatDetails, this gets called // when the report is ready. -void BaseUIManager::SendSerializedThreatDetails( +void BaseUIManager::SendThreatDetails( content::BrowserContext* browser_context, - const std::string& serialized) { + std::unique_ptr<ClientSafeBrowsingReportRequest> report) { DCHECK_CURRENTLY_ON(BrowserThread::UI); return; } diff --git a/chromium/components/safe_browsing/content/browser/base_ui_manager.h b/chromium/components/safe_browsing/content/browser/base_ui_manager.h index 0cc29218bb9..17b1f4a3070 100644 --- a/chromium/components/safe_browsing/content/browser/base_ui_manager.h +++ b/chromium/components/safe_browsing/content/browser/base_ui_manager.h @@ -11,6 +11,7 @@ #include "base/callback_helpers.h" #include "base/memory/ref_counted.h" +#include "components/safe_browsing/core/common/proto/csd.pb.h" #include "components/security_interstitials/core/unsafe_resource.h" class GURL; @@ -49,11 +50,10 @@ class BaseUIManager : public base::RefCountedThreadSafe<BaseUIManager> { virtual void DisplayBlockingPage(const UnsafeResource& resource); // This is a no-op in the base class, but should be overridden to send threat - // details. Called on the UI thread by the ThreatDetails with the serialized - // protocol buffer. - virtual void SendSerializedThreatDetails( + // details. Called on the UI thread by the ThreatDetails with the report. + virtual void SendThreatDetails( content::BrowserContext* browser_context, - const std::string& serialized); + std::unique_ptr<ClientSafeBrowsingReportRequest> report); // Updates the allowlist URL set for |web_contents|. Called on the UI thread. void AddToAllowlistUrlSet(const GURL& allowlist_url, diff --git a/chromium/components/safe_browsing/content/browser/browser_url_loader_throttle.cc b/chromium/components/safe_browsing/content/browser/browser_url_loader_throttle.cc index c3d058a9314..144f3c794f6 100644 --- a/chromium/components/safe_browsing/content/browser/browser_url_loader_throttle.cc +++ b/chromium/components/safe_browsing/content/browser/browser_url_loader_throttle.cc @@ -68,7 +68,6 @@ class BrowserURLLoaderThrottle::CheckerOnIO std::move(delegate_getter_).Run(); skip_checks_ = !url_checker_delegate || - !url_checker_delegate->GetDatabaseManager()->IsSupported() || url_checker_delegate->ShouldSkipRequestCheck( url, frame_tree_node_id_, content::ChildProcessHost::kInvalidUniqueID /* render_process_id */, diff --git a/chromium/components/safe_browsing/content/browser/client_side_detection_host.cc b/chromium/components/safe_browsing/content/browser/client_side_detection_host.cc index 68ea7b87504..8eaa56e25a0 100644 --- a/chromium/components/safe_browsing/content/browser/client_side_detection_host.cc +++ b/chromium/components/safe_browsing/content/browser/client_side_detection_host.cc @@ -45,6 +45,7 @@ #include "net/base/ip_endpoint.h" #include "net/http/http_response_headers.h" #include "services/service_manager/public/cpp/interface_provider.h" +#include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h" #include "third_party/blink/public/mojom/loader/referrer.mojom.h" #include "url/gurl.h" @@ -384,27 +385,12 @@ ClientSideDetectionHost::ClientSideDetectionHost( // be null if safe browsing service is not available in the embedder. ui_manager_ = delegate_->GetSafeBrowsingUIManager(); database_manager_ = delegate_->GetSafeBrowsingDBManager(); - - // We want to accurately track all active RenderFrameHosts, so make sure we - // know about any pre-existings ones. - web_contents()->ForEachRenderFrameHost(base::BindRepeating( - [](ClientSideDetectionHost* csdh, - const content::WebContents* web_contents, - content::RenderFrameHost* rfh) { - // Don't cross into inner WebContents since we wouldn't be notified of - // its changes. See https://crbug.com/1308829 - if (content::WebContents::FromRenderFrameHost(rfh) != web_contents) { - return content::RenderFrameHost::FrameIterationAction::kSkipChildren; - } - csdh->InitializePhishingDetector(rfh); - return content::RenderFrameHost::FrameIterationAction::kContinue; - }, - base::Unretained(this), web_contents())); } ClientSideDetectionHost::~ClientSideDetectionHost() { - if (csd_service_) - csd_service_->RemoveClientSideDetectionHost(this); + if (classification_request_.get()) { + classification_request_->Cancel(); + } } void ClientSideDetectionHost::DidFinishNavigation( @@ -414,6 +400,9 @@ void ClientSideDetectionHost::DidFinishNavigation( return; } + if (base::FeatureList::IsEnabled(kClientSideDetectionKillswitch)) + return; + // TODO(noelutz): move this DCHECK to WebContents and fix all the unit tests // that don't call this method on the UI thread. // DCHECK_CURRENTLY_ON(BrowserThread::UI); @@ -452,71 +441,20 @@ void ClientSideDetectionHost::DidFinishNavigation( classification_request_->Start(); } -void ClientSideDetectionHost::SetPhishingModel( - const mojo::Remote<mojom::PhishingDetector>& phishing_detector) { - switch (csd_service_->GetModelType()) { - case CSDModelType::kNone: - case CSDModelType::kProtobuf: - phishing_detector->SetPhishingModel( - csd_service_->GetModelStr(), - csd_service_->GetVisualTfLiteModel().Duplicate()); - return; - case CSDModelType::kFlatbuffer: - phishing_detector->SetPhishingFlatBufferModel( - csd_service_->GetModelSharedMemoryRegion(), - csd_service_->GetVisualTfLiteModel().Duplicate()); - return; - } -} - -void ClientSideDetectionHost::SendModelToRenderFrame() { - DCHECK_CURRENTLY_ON(BrowserThread::UI); - if (!web_contents() || web_contents() != tab_ || !csd_service_) - return; - - for (const auto& frame_and_remote : phishing_detectors_) { - SetPhishingModel(frame_and_remote.second); - } -} - -void ClientSideDetectionHost::WebContentsDestroyed() { - // Tell any pending classification request that it is being canceled. - if (classification_request_.get()) { - classification_request_->Cancel(); - } - if (csd_service_) - csd_service_->RemoveClientSideDetectionHost(this); -} - -void ClientSideDetectionHost::RenderFrameCreated( - content::RenderFrameHost* render_frame_host) { - InitializePhishingDetector(render_frame_host); -} - -void ClientSideDetectionHost::RenderFrameDeleted( - content::RenderFrameHost* render_frame_host) { - ClearPhishingDetector(render_frame_host->GetGlobalId()); -} - void ClientSideDetectionHost::OnPhishingPreClassificationDone( bool should_classify) { DCHECK_CURRENTLY_ON(BrowserThread::UI); if (should_classify) { - content::RenderFrameHost* rfh = web_contents()->GetMainFrame(); - auto it = phishing_detectors_.find(rfh->GetGlobalId()); - bool remote_valid = - (it != phishing_detectors_.end() && it->second.is_connected()); - - base::UmaHistogramBoolean("SBClientPhishing.MainFrameRemoteExists", - it != phishing_detectors_.end()); - if (it != phishing_detectors_.end()) { - base::UmaHistogramBoolean("SBClientPhishing.MainFrameRemoteConnected", - it->second.is_connected()); - } + content::RenderFrameHost* rfh = web_contents()->GetPrimaryMainFrame(); + + phishing_detector_.reset(); + rfh->GetRemoteAssociatedInterfaces()->GetInterface(&phishing_detector_); + base::UmaHistogramBoolean("SBClientPhishing.MainFrameRemoteConnected", + phishing_detector_.is_bound()); - if (remote_valid) { + if (phishing_detector_.is_bound()) { phishing_detection_start_time_ = tick_clock_->NowTicks(); - it->second->StartPhishingDetection( + phishing_detector_->StartPhishingDetection( current_url_, base::BindOnce(&ClientSideDetectionHost::PhishingDetectionDone, weak_factory_.GetWeakPtr())); @@ -533,6 +471,8 @@ void ClientSideDetectionHost::PhishingDetectionDone( // if there isn't any service class in the browser. DCHECK(csd_service_); + phishing_detector_.reset(); + UmaHistogramMediumTimes( "SBClientPhishing.PhishingDetectionDuration", base::TimeTicks::Now() - phishing_detection_start_time_); @@ -632,7 +572,7 @@ void ClientSideDetectionHost::MaybeShowPhishingWarning(bool is_from_cache, DCHECK(web_contents()); if (ui_manager_.get()) { const content::GlobalRenderFrameHostId primary_main_frame_id = - web_contents()->GetMainFrame()->GetGlobalId(); + web_contents()->GetPrimaryMainFrame()->GetGlobalId(); security_interstitials::UnsafeResource resource; resource.url = phishing_url; @@ -676,27 +616,6 @@ void ClientSideDetectionHost::OnGotAccessToken( ClientSideDetectionHost::SendRequest(std::move(verdict), access_token); } -void ClientSideDetectionHost::InitializePhishingDetector( - content::RenderFrameHost* render_frame_host) { - if (render_frame_host->IsRenderFrameCreated()) { - const content::GlobalRenderFrameHostId rfh_id = - render_frame_host->GetGlobalId(); - mojo::Remote<mojom::PhishingDetector> new_detector; - render_frame_host->GetRemoteInterfaces()->GetInterface( - new_detector.BindNewPipeAndPassReceiver()); - new_detector.set_disconnect_handler( - base::BindOnce(&ClientSideDetectionHost::ClearPhishingDetector, - weak_factory_.GetWeakPtr(), rfh_id)); - phishing_detectors_[rfh_id] = std::move(new_detector); - SetPhishingModel(phishing_detectors_[rfh_id]); - } -} - -void ClientSideDetectionHost::ClearPhishingDetector( - content::GlobalRenderFrameHostId rfh_id) { - phishing_detectors_.erase(rfh_id); -} - bool ClientSideDetectionHost::CanGetAccessToken() { if (is_off_the_record_) return false; diff --git a/chromium/components/safe_browsing/content/browser/client_side_detection_host.h b/chromium/components/safe_browsing/content/browser/client_side_detection_host.h index 30c50c3b162..b91d1f3350c 100644 --- a/chromium/components/safe_browsing/content/browser/client_side_detection_host.h +++ b/chromium/components/safe_browsing/content/browser/client_side_detection_host.h @@ -23,6 +23,7 @@ #include "components/safe_browsing/core/common/safe_browsing_prefs.h" #include "content/public/browser/global_routing_id.h" #include "content/public/browser/web_contents_observer.h" +#include "mojo/public/cpp/bindings/associated_remote.h" #include "mojo/public/cpp/bindings/remote.h" #include "services/service_manager/public/cpp/binder_registry.h" #include "url/gurl.h" @@ -39,7 +40,6 @@ class ClientSideDetectionService; // notifies the browser that a URL was classified as phishing. This // class relays this information to the client-side detection service // class which sends a ping to a server to validate the verdict. -// TODO(noelutz): move all client-side detection IPCs to this class. class ClientSideDetectionHost : public content::WebContentsObserver { public: // A callback via which the client of this component indicates whether the @@ -82,8 +82,6 @@ class ClientSideDetectionHost : public content::WebContentsObserver { ClientSideDetectionHost(const ClientSideDetectionHost&) = delete; ClientSideDetectionHost& operator=(const ClientSideDetectionHost&) = delete; - // The caller keeps ownership of the tab object and is responsible for - // ensuring that it stays valid until WebContentsDestroyed is called. ~ClientSideDetectionHost() override; // From content::WebContentsObserver. If we navigate away we cancel all @@ -92,9 +90,6 @@ class ClientSideDetectionHost : public content::WebContentsObserver { void DidFinishNavigation( content::NavigationHandle* navigation_handle) override; - // Send the model to all the render frame hosts in this WebContents. - void SendModelToRenderFrame(); - protected: explicit ClientSideDetectionHost( content::WebContents* tab, @@ -104,11 +99,6 @@ class ClientSideDetectionHost : public content::WebContentsObserver { bool is_off_the_record, const PrimaryAccountSignedIn& account_signed_in_callback); - // From content::WebContentsObserver. - void WebContentsDestroyed() override; - void RenderFrameCreated(content::RenderFrameHost* render_frame_host) override; - void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override; - // Used for testing. void set_ui_manager(BaseUIManager* ui_manager); void set_database_manager(SafeBrowsingDatabaseManager* database_manager); @@ -172,10 +162,6 @@ class ClientSideDetectionHost : public content::WebContentsObserver { // users, who are signed in and not in incognito mode. bool CanGetAccessToken(); - // Set phishing model in PhishingDetector in renderers. - void SetPhishingModel( - const mojo::Remote<mojom::PhishingDetector>& phishing_detector); - // Send the client report to CSD server. void SendRequest(std::unique_ptr<ClientPhishingRequest> verdict, const std::string& access_token); @@ -184,11 +170,6 @@ class ClientSideDetectionHost : public content::WebContentsObserver { void OnGotAccessToken(std::unique_ptr<ClientPhishingRequest> verdict, const std::string& access_token); - // Setup a PhishingDetector Mojo connection for the given render frame. - void InitializePhishingDetector(content::RenderFrameHost* render_frame_host); - - void ClearPhishingDetector(content::GlobalRenderFrameHostId rfh_id); - // This pointer may be nullptr if client-side phishing detection is // disabled. raw_ptr<ClientSideDetectionService> csd_service_; @@ -199,17 +180,12 @@ class ClientSideDetectionHost : public content::WebContentsObserver { scoped_refptr<BaseUIManager> ui_manager_; // Keep a handle to the latest classification request so that we can cancel // it if necessary. + // TODO(drubery): Make this a std::unique_ptr, for clearer lifetimes. scoped_refptr<ShouldClassifyUrlRequest> classification_request_; // The current URL GURL current_url_; // The current outermost main frame's id. content::GlobalRenderFrameHostId current_outermost_main_frame_id_; - // A map from the live RenderFrameHosts to their PhishingDetector. These - // correspond to the `phishing_detector_receiver_` in the - // PhishingClassifierDelegate. - base::flat_map<content::GlobalRenderFrameHostId, - mojo::Remote<mojom::PhishingDetector>> - phishing_detectors_; // Records the start time of when phishing detection started. base::TimeTicks phishing_detection_start_time_; @@ -231,6 +207,9 @@ class ClientSideDetectionHost : public content::WebContentsObserver { // acces_token. PrimaryAccountSignedIn account_signed_in_callback_; + // The remote for the currently active phishing classification. + mojo::AssociatedRemote<mojom::PhishingDetector> phishing_detector_; + base::WeakPtrFactory<ClientSideDetectionHost> weak_factory_{this}; }; diff --git a/chromium/components/safe_browsing/content/browser/client_side_detection_service.cc b/chromium/components/safe_browsing/content/browser/client_side_detection_service.cc index 606feb3a25a..2ae8d3fd695 100644 --- a/chromium/components/safe_browsing/content/browser/client_side_detection_service.cc +++ b/chromium/components/safe_browsing/content/browser/client_side_detection_service.cc @@ -15,6 +15,7 @@ #include "base/memory/scoped_refptr.h" #include "base/metrics/histogram_functions.h" #include "base/metrics/histogram_macros.h" +#include "base/strings/escape.h" #include "base/strings/strcat.h" #include "base/task/single_thread_task_runner.h" #include "base/threading/thread_task_runner_handle.h" @@ -37,8 +38,9 @@ #include "content/public/browser/render_process_host.h" #include "crypto/sha2.h" #include "google_apis/google_api_keys.h" +#include "ipc/ipc_channel_proxy.h" +#include "mojo/public/cpp/bindings/associated_remote.h" #include "mojo/public/cpp/bindings/remote.h" -#include "net/base/escape.h" #include "net/base/ip_address.h" #include "net/base/load_flags.h" #include "net/http/http_request_headers.h" @@ -164,19 +166,6 @@ bool ClientSideDetectionService::IsLocalResource( return !address.IsValid(); } -void ClientSideDetectionService::AddClientSideDetectionHost( - ClientSideDetectionHost* host) { - csd_hosts_.push_back(host); -} - -void ClientSideDetectionService::RemoveClientSideDetectionHost( - ClientSideDetectionHost* host) { - std::vector<ClientSideDetectionHost*>::iterator position = - std::find(csd_hosts_.begin(), csd_hosts_.end(), host); - if (position != csd_hosts_.end()) - csd_hosts_.erase(position); -} - void ClientSideDetectionService::OnURLLoaderComplete( network::SimpleURLLoader* url_loader, base::Time start_time, @@ -199,8 +188,10 @@ void ClientSideDetectionService::OnURLLoaderComplete( } void ClientSideDetectionService::SendModelToRenderers() { - for (ClientSideDetectionHost* host : csd_hosts_) { - host->SendModelToRenderFrame(); + for (content::RenderProcessHost::iterator it( + content::RenderProcessHost::AllHostsIterator()); + !it.IsAtEnd(); it.Advance()) { + SetPhishingModel(it.GetCurrentValue()); } } @@ -429,7 +420,7 @@ GURL ClientSideDetectionService::GetClientReportUrl( GURL url(report_url); std::string api_key = google_apis::GetAPIKey(); if (!api_key.empty()) - url = url.Resolve("?key=" + net::EscapeQueryParamValue(api_key, true)); + url = url.Resolve("?key=" + base::EscapeQueryParamValue(api_key, true)); return url; } @@ -456,4 +447,29 @@ void ClientSideDetectionService::SetURLLoaderFactoryForTesting( url_loader_factory_ = url_loader_factory; } +void ClientSideDetectionService::OnRenderProcessHostCreated( + content::RenderProcessHost* rph) { + SetPhishingModel(rph); +} + +void ClientSideDetectionService::SetPhishingModel( + content::RenderProcessHost* rph) { + if (!rph->GetChannel()) + return; + mojo::AssociatedRemote<mojom::PhishingModelSetter> model_setter; + rph->GetChannel()->GetRemoteAssociatedInterface(&model_setter); + switch (GetModelType()) { + case CSDModelType::kNone: + return; + case CSDModelType::kProtobuf: + model_setter->SetPhishingModel(GetModelStr(), + GetVisualTfLiteModel().Duplicate()); + return; + case CSDModelType::kFlatbuffer: + model_setter->SetPhishingFlatBufferModel( + GetModelSharedMemoryRegion(), GetVisualTfLiteModel().Duplicate()); + return; + } +} + } // namespace safe_browsing diff --git a/chromium/components/safe_browsing/content/browser/client_side_detection_service.h b/chromium/components/safe_browsing/content/browser/client_side_detection_service.h index d08877f2f7b..3d7250ca3af 100644 --- a/chromium/components/safe_browsing/content/browser/client_side_detection_service.h +++ b/chromium/components/safe_browsing/content/browser/client_side_detection_service.h @@ -34,6 +34,7 @@ #include "content/public/browser/browser_thread.h" #include "content/public/browser/notification_observer.h" #include "content/public/browser/notification_registrar.h" +#include "content/public/browser/render_process_host_creation_observer.h" #include "net/base/ip_address.h" #include "services/network/public/cpp/shared_url_loader_factory.h" #include "url/gurl.h" @@ -49,7 +50,9 @@ class ClientSideDetectionHost; // Main service which pushes models to the renderers, responds to classification // requests. This owns two ModelLoader objects. -class ClientSideDetectionService : public KeyedService { +class ClientSideDetectionService + : public KeyedService, + public content::RenderProcessHostCreationObserver { public: // void(GURL phishing_url, bool is_phishing). typedef base::OnceCallback<void(GURL, bool)> @@ -86,9 +89,6 @@ class ClientSideDetectionService : public KeyedService { return enabled_; } - void AddClientSideDetectionHost(ClientSideDetectionHost* host); - void RemoveClientSideDetectionHost(ClientSideDetectionHost* host); - void OnURLLoaderComplete(network::SimpleURLLoader* url_loader, base::Time start_time, std::unique_ptr<std::string> response_body); @@ -153,6 +153,9 @@ class ClientSideDetectionService : public KeyedService { void SetURLLoaderFactoryForTesting( scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory); + // Sends a model to each renderer. + void SetPhishingModel(content::RenderProcessHost* rph); + private: friend class ClientSideDetectionServiceTest; FRIEND_TEST_ALL_PREFIXES(ClientSideDetectionServiceTest, @@ -219,6 +222,9 @@ class ClientSideDetectionService : public KeyedService { // Returns the URL that will be used for phishing requests. static GURL GetClientReportUrl(const std::string& report_url); + // content::RenderProcessHostCreationObserver: + void OnRenderProcessHostCreated(content::RenderProcessHost* rph) override; + // Whether the service is running or not. When the service is not running, // it won't download the model nor report detected phishing URLs. bool enabled_ = false; @@ -252,8 +258,6 @@ class ClientSideDetectionService : public KeyedService { // PrefChangeRegistrar used to track when the Safe Browsing pref changes. PrefChangeRegistrar pref_change_registrar_; - std::vector<ClientSideDetectionHost*> csd_hosts_; - std::unique_ptr<Delegate> delegate_; base::CallbackListSubscription update_model_subscription_; diff --git a/chromium/components/safe_browsing/content/browser/client_side_phishing_model.cc b/chromium/components/safe_browsing/content/browser/client_side_phishing_model.cc index a911a32fedc..3a2e1b890f1 100644 --- a/chromium/components/safe_browsing/content/browser/client_side_phishing_model.cc +++ b/chromium/components/safe_browsing/content/browser/client_side_phishing_model.cc @@ -31,19 +31,46 @@ namespace { // provided with an absolute path. const char kOverrideCsdModelFlag[] = "csd-model-override-path"; -std::string ReadFileIntoString(base::FilePath path) { - if (path.empty()) - return std::string(); +void ReturnModelOverrideFailure( + base::OnceCallback<void(std::pair<std::string, base::File>)> callback) { + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(std::move(callback), + std::make_pair(std::string(), base::File()))); +} - base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ); - if (!file.IsValid()) - return std::string(); +void ReadOverridenModel( + base::FilePath path, + base::OnceCallback<void(std::pair<std::string, base::File>)> callback) { + if (path.empty()) { + VLOG(2) << "Failed to override model. Path is empty."; + ReturnModelOverrideFailure(std::move(callback)); + return; + } - std::vector<char> model_data(file.GetLength()); - if (file.ReadAtCurrentPos(model_data.data(), model_data.size()) == -1) - return std::string(); + base::File model(path.AppendASCII("client_model.pb"), + base::File::FLAG_OPEN | base::File::FLAG_READ); + base::File tflite_model(path.AppendASCII("visual_model.tflite"), + base::File::FLAG_OPEN | base::File::FLAG_READ); + // `tflite_model` is allowed to be invalid, when testing a DOM-only model. + if (!model.IsValid()) { + VLOG(2) << "Failed to override model. Could not open: " + << path.AppendASCII("client_model.pb"); + ReturnModelOverrideFailure(std::move(callback)); + return; + } + + std::vector<char> model_data(model.GetLength()); + if (model.ReadAtCurrentPos(model_data.data(), model_data.size()) == -1) { + VLOG(2) << "Failed to override model. Could not read model data."; + ReturnModelOverrideFailure(std::move(callback)); + return; + } - return std::string(model_data.begin(), model_data.end()); + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(std::move(callback), + std::make_pair(std::string(model_data.begin(), + model_data.end()), + std::move(tflite_model)))); } } // namespace @@ -205,28 +232,38 @@ void* ClientSidePhishingModel::GetFlatBufferMemoryAddressForTesting() { return mapped_region_.mapping.memory(); } +void ClientSidePhishingModel::NotifyCallbacksOfUpdateForTesting() { + // base::Unretained is safe because this is a singleton. + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&ClientSidePhishingModel::NotifyCallbacksOnUI, + base::Unretained(this))); +} + void ClientSidePhishingModel::MaybeOverrideModel() { if (base::CommandLine::ForCurrentProcess()->HasSwitch( kOverrideCsdModelFlag)) { - base::FilePath overriden_model_path = + base::FilePath overriden_model_directory = base::CommandLine::ForCurrentProcess()->GetSwitchValuePath( kOverrideCsdModelFlag); CSDModelType model_type = base::FeatureList::IsEnabled(kClientSideDetectionModelIsFlatBuffer) ? CSDModelType::kFlatbuffer : CSDModelType::kProtobuf; - base::ThreadPool::PostTaskAndReplyWithResult( + base::ThreadPool::PostTask( FROM_HERE, {base::MayBlock()}, - base::BindOnce(&ReadFileIntoString, overriden_model_path), - // base::Unretained is safe because this is a singleton. - base::BindOnce(&ClientSidePhishingModel::OnGetOverridenModelData, - base::Unretained(this), model_type)); + base::BindOnce( + &ReadOverridenModel, overriden_model_directory, + // base::Unretained is safe because this is a singleton. + base::BindOnce(&ClientSidePhishingModel::OnGetOverridenModelData, + base::Unretained(this), model_type))); } } void ClientSidePhishingModel::OnGetOverridenModelData( CSDModelType model_type, - const std::string& model_data) { + std::pair<std::string, base::File> model_and_tflite) { + const std::string& model_data = model_and_tflite.first; + base::File tflite_model = std::move(model_and_tflite.second); if (model_data.empty()) { VLOG(2) << "Overriden model data is empty"; return; @@ -269,6 +306,10 @@ void ClientSidePhishingModel::OnGetOverridenModelData( return; } + if (tflite_model.IsValid()) { + visual_tflite_model_ = std::move(tflite_model); + } + VLOG(2) << "Model overriden successfully"; // Unretained is safe because this is a singleton. diff --git a/chromium/components/safe_browsing/content/browser/client_side_phishing_model.h b/chromium/components/safe_browsing/content/browser/client_side_phishing_model.h index cd38d5296cb..747a05f1f38 100644 --- a/chromium/components/safe_browsing/content/browser/client_side_phishing_model.h +++ b/chromium/components/safe_browsing/content/browser/client_side_phishing_model.h @@ -68,6 +68,8 @@ class ClientSidePhishingModel { void ClearMappedRegionForTesting(); // Get flatbuffer memory address. void* GetFlatBufferMemoryAddressForTesting(); + // Notifies all the callbacks of a change in model. + void NotifyCallbacksOfUpdateForTesting(); // Called to check the command line and maybe override the current model. void MaybeOverrideModel(); @@ -80,8 +82,9 @@ class ClientSidePhishingModel { void NotifyCallbacksOnUI(); // Callback when the local file overriding the model has been read. - void OnGetOverridenModelData(CSDModelType model_type, - const std::string& model_data); + void OnGetOverridenModelData( + CSDModelType model_type, + std::pair<std::string, base::File> model_and_tflite); // The list of callbacks to notify when a new model is ready. Protected by // lock_. Will always be notified on the UI thread. diff --git a/chromium/components/safe_browsing/content/browser/client_side_phishing_model_unittest.cc b/chromium/components/safe_browsing/content/browser/client_side_phishing_model_unittest.cc index e91f3f7b991..783935c09ef 100644 --- a/chromium/components/safe_browsing/content/browser/client_side_phishing_model_unittest.cc +++ b/chromium/components/safe_browsing/content/browser/client_side_phishing_model_unittest.cc @@ -214,11 +214,10 @@ TEST(ClientSidePhishingModelTest, CanOverrideProtoWithFlag) { /*disabled_features=*/{kClientSideDetectionModelIsFlatBuffer}); base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - const base::FilePath file_path = - temp_dir.GetPath().AppendASCII("overridden_model.proto"); - base::File file(file_path, base::File::FLAG_OPEN_ALWAYS | - base::File::FLAG_READ | - base::File::FLAG_WRITE); + const base::FilePath file_path = temp_dir.GetPath(); + base::File file(file_path.AppendASCII("client_model.pb"), + base::File::FLAG_OPEN_ALWAYS | base::File::FLAG_READ | + base::File::FLAG_WRITE); ClientSideModel model_proto; model_proto.set_version(123); model_proto.set_max_words_per_term(0); // Required field @@ -260,11 +259,10 @@ TEST(ClientSidePhishingModelTest, CanOverrideFlatBufferWithFlag) { /*disabled_features=*/{}); base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - const base::FilePath file_path = - temp_dir.GetPath().AppendASCII("overridden_model.fb"); - base::File file(file_path, base::File::FLAG_OPEN_ALWAYS | - base::File::FLAG_READ | - base::File::FLAG_WRITE); + const base::FilePath file_path = temp_dir.GetPath(); + base::File file(file_path.AppendASCII("client_model.pb"), + base::File::FLAG_OPEN_ALWAYS | base::File::FLAG_READ | + base::File::FLAG_WRITE); const std::string file_contents = CreateFlatBufferString(); file.WriteAtCurrentPos(file_contents.data(), file_contents.size()); diff --git a/chromium/components/safe_browsing/content/browser/mojo_safe_browsing_impl.cc b/chromium/components/safe_browsing/content/browser/mojo_safe_browsing_impl.cc index 488bef94b27..057a5c433c7 100644 --- a/chromium/components/safe_browsing/content/browser/mojo_safe_browsing_impl.cc +++ b/chromium/components/safe_browsing/content/browser/mojo_safe_browsing_impl.cc @@ -108,8 +108,7 @@ void MojoSafeBrowsingImpl::MaybeCreate( scoped_refptr<UrlCheckerDelegate> delegate = delegate_getter.Run(); - if (!resource_context || !delegate || - !delegate->GetDatabaseManager()->IsSupported()) + if (!resource_context || !delegate) return; std::unique_ptr<MojoSafeBrowsingImpl> impl(new MojoSafeBrowsingImpl( diff --git a/chromium/components/safe_browsing/content/browser/password_protection/password_protection_request_content.cc b/chromium/components/safe_browsing/content/browser/password_protection/password_protection_request_content.cc index a5faf91fe56..b5b35ad4bcc 100644 --- a/chromium/components/safe_browsing/content/browser/password_protection/password_protection_request_content.cc +++ b/chromium/components/safe_browsing/content/browser/password_protection/password_protection_request_content.cc @@ -127,7 +127,7 @@ bool PasswordProtectionRequestContent::IsVisualFeaturesEnabled() { } void PasswordProtectionRequestContent::GetDomFeatures() { - content::RenderFrameHost* rfh = web_contents_->GetMainFrame(); + content::RenderFrameHost* rfh = web_contents_->GetPrimaryMainFrame(); PasswordProtectionService* service = static_cast<PasswordProtectionService*>(password_protection_service()); service->GetPhishingDetector(rfh->GetRemoteInterfaces(), &phishing_detector_); diff --git a/chromium/components/safe_browsing/content/browser/password_protection/password_protection_service.cc b/chromium/components/safe_browsing/content/browser/password_protection/password_protection_service.cc index d0fcc4f40b3..7e593038f7f 100644 --- a/chromium/components/safe_browsing/content/browser/password_protection/password_protection_service.cc +++ b/chromium/components/safe_browsing/content/browser/password_protection/password_protection_service.cc @@ -11,6 +11,7 @@ #include <string> #include "base/metrics/histogram_macros.h" +#include "base/strings/escape.h" #include "components/password_manager/core/browser/password_manager_metrics_util.h" #include "components/password_manager/core/browser/password_reuse_detector.h" #include "components/safe_browsing/content/browser/password_protection/password_protection_commit_deferring_condition.h" @@ -21,7 +22,6 @@ #include "content/public/browser/navigation_handle.h" #include "content/public/browser/web_contents.h" #include "google_apis/google_api_keys.h" -#include "net/base/escape.h" #include "net/base/url_util.h" #include "third_party/blink/public/common/page/page_zoom.h" diff --git a/chromium/components/safe_browsing/content/browser/password_protection/password_protection_service_unittest.cc b/chromium/components/safe_browsing/content/browser/password_protection/password_protection_service_unittest.cc index d4ac1cde92d..42dac9e9949 100644 --- a/chromium/components/safe_browsing/content/browser/password_protection/password_protection_service_unittest.cc +++ b/chromium/components/safe_browsing/content/browser/password_protection/password_protection_service_unittest.cc @@ -136,11 +136,6 @@ class TestPhishingDetector : public mojom::PhishingDetector { mojo::PendingReceiver<mojom::PhishingDetector>(std::move(handle))); } - void SetPhishingModel(const std::string& model, base::File file) override {} - - void SetPhishingFlatBufferModel(base::ReadOnlySharedMemoryRegion region, - base::File file) override {} - void StartPhishingDetection( const GURL& url, StartPhishingDetectionCallback callback) override { diff --git a/chromium/components/safe_browsing/content/browser/safe_browsing_blocking_page.cc b/chromium/components/safe_browsing/content/browser/safe_browsing_blocking_page.cc index 1d4c5f0e1ec..de195ccda6a 100644 --- a/chromium/components/safe_browsing/content/browser/safe_browsing_blocking_page.cc +++ b/chromium/components/safe_browsing/content/browser/safe_browsing_blocking_page.cc @@ -53,8 +53,6 @@ SafeBrowsingBlockingPage::SafeBrowsingBlockingPage( const BaseSafeBrowsingErrorUI::SBErrorDisplayOptions& display_options, bool should_trigger_reporting, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, SafeBrowsingNavigationObserverManager* navigation_observer_manager, SafeBrowsingMetricsCollector* metrics_collector, TriggerManager* trigger_manager, @@ -68,7 +66,6 @@ SafeBrowsingBlockingPage::SafeBrowsingBlockingPage( threat_details_in_progress_(false), threat_source_(unsafe_resources[0].threat_source), history_service_(history_service), - get_user_population_callback_(get_user_population_callback), navigation_observer_manager_(navigation_observer_manager), metrics_collector_(metrics_collector), trigger_manager_(trigger_manager) { @@ -103,7 +100,7 @@ SafeBrowsingBlockingPage::SafeBrowsingBlockingPage( trigger_manager_->StartCollectingThreatDetails( TriggerType::SECURITY_INTERSTITIAL, web_contents, unsafe_resources[0], url_loader_factory, history_service_, - get_user_population_callback_, navigation_observer_manager_, + navigation_observer_manager_, sb_error_ui()->get_error_display_options()); } } diff --git a/chromium/components/safe_browsing/content/browser/safe_browsing_blocking_page.h b/chromium/components/safe_browsing/content/browser/safe_browsing_blocking_page.h index e9732302626..644b7bb79fb 100644 --- a/chromium/components/safe_browsing/content/browser/safe_browsing_blocking_page.h +++ b/chromium/components/safe_browsing/content/browser/safe_browsing_blocking_page.h @@ -34,7 +34,6 @@ #include "base/memory/raw_ptr.h" #include "components/safe_browsing/content/browser/base_blocking_page.h" #include "components/safe_browsing/content/browser/base_ui_manager.h" -#include "components/safe_browsing/core/common/proto/csd.pb.h" namespace history { class HistoryService; @@ -112,8 +111,6 @@ class SafeBrowsingBlockingPage : public BaseBlockingPage { const BaseSafeBrowsingErrorUI::SBErrorDisplayOptions& display_options, bool should_trigger_reporting, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, SafeBrowsingNavigationObserverManager* navigation_observer_manager, SafeBrowsingMetricsCollector* metrics_collector, TriggerManager* trigger_manager, @@ -140,7 +137,6 @@ class SafeBrowsingBlockingPage : public BaseBlockingPage { private: raw_ptr<history::HistoryService> history_service_ = nullptr; - base::RepeatingCallback<ChromeUserPopulation()> get_user_population_callback_; raw_ptr<SafeBrowsingNavigationObserverManager> navigation_observer_manager_ = nullptr; raw_ptr<SafeBrowsingMetricsCollector> metrics_collector_ = nullptr; diff --git a/chromium/components/safe_browsing/content/browser/safe_browsing_navigation_observer.cc b/chromium/components/safe_browsing/content/browser/safe_browsing_navigation_observer.cc index 422a2e7fe02..87dd58cc613 100644 --- a/chromium/components/safe_browsing/content/browser/safe_browsing_navigation_observer.cc +++ b/chromium/components/safe_browsing/content/browser/safe_browsing_navigation_observer.cc @@ -322,7 +322,7 @@ void SafeBrowsingNavigationObserver::SetNavigationSourceUrl( // frame. nav_event->source_url = SafeBrowsingNavigationObserverManager::ClearURLRef( navigation_handle->GetWebContents() - ->GetMainFrame() + ->GetPrimaryMainFrame() ->GetLastCommittedURL()); } else { // If there was a URL previously committed in the current RenderFrameHost, diff --git a/chromium/components/safe_browsing/content/browser/safe_browsing_navigation_observer_manager.cc b/chromium/components/safe_browsing/content/browser/safe_browsing_navigation_observer_manager.cc index df4b069915d..88ed4b4a12c 100644 --- a/chromium/components/safe_browsing/content/browser/safe_browsing_navigation_observer_manager.cc +++ b/chromium/components/safe_browsing/content/browser/safe_browsing_navigation_observer_manager.cc @@ -687,9 +687,10 @@ void SafeBrowsingNavigationObserverManager::RecordNewWebContents( nav_event->initiator_outermost_main_frame_id = source_render_frame_host->GetOutermostMainFrame()->GetGlobalId(); } - nav_event->outermost_main_frame_id = target_web_contents->GetMainFrame() - ->GetOutermostMainFrame() - ->GetGlobalId(); + nav_event->outermost_main_frame_id = + target_web_contents->GetPrimaryMainFrame() + ->GetOutermostMainFrame() + ->GetGlobalId(); } nav_event->source_tab_id = diff --git a/chromium/components/safe_browsing/content/browser/safe_browsing_navigation_observer_unittest.cc b/chromium/components/safe_browsing/content/browser/safe_browsing_navigation_observer_unittest.cc index ae9d150041d..04f0d36ee05 100644 --- a/chromium/components/safe_browsing/content/browser/safe_browsing_navigation_observer_unittest.cc +++ b/chromium/components/safe_browsing/content/browser/safe_browsing_navigation_observer_unittest.cc @@ -375,7 +375,7 @@ TEST_F(SBNavigationObserverTest, BasicNavigationAndCommit) { TEST_F(SBNavigationObserverTest, ServerRedirect) { auto navigation = content::NavigationSimulator::CreateRendererInitiated( - GURL("http://foo/3"), web_contents()->GetMainFrame()); + GURL("http://foo/3"), web_contents()->GetPrimaryMainFrame()); auto* nav_list = navigation_event_list(); SessionID tab_id = sessions::SessionTabHelper::IdForTab(web_contents()); @@ -446,8 +446,10 @@ TEST_F(SBNavigationObserverTest, TestCleanUpStaleNavigationEvents) { base::Time::FromDoubleT(now.ToDoubleT() + 60.0 * 60.0); // Invalid GURL url_0("http://foo/0"); GURL url_1("http://foo/1"); - content::MockNavigationHandle handle_0(url_0, web_contents()->GetMainFrame()); - content::MockNavigationHandle handle_1(url_1, web_contents()->GetMainFrame()); + content::MockNavigationHandle handle_0(url_0, + web_contents()->GetPrimaryMainFrame()); + content::MockNavigationHandle handle_1(url_1, + web_contents()->GetPrimaryMainFrame()); navigation_event_list()->RecordNavigationEvent( CreateNavigationEventUniquePtr(url_0, in_an_hour)); navigation_event_list()->RecordNavigationEvent( @@ -875,9 +877,12 @@ TEST_F(SBNavigationObserverTest, TestGetLatestPendingNavigationEvent) { base::Time one_minute_ago = base::Time::FromDoubleT(now.ToDoubleT() - 60.0); base::Time two_minute_ago = base::Time::FromDoubleT(now.ToDoubleT() - 120.0); GURL url("http://foo/0"); - content::MockNavigationHandle handle_0(url, web_contents()->GetMainFrame()); - content::MockNavigationHandle handle_1(url, web_contents()->GetMainFrame()); - content::MockNavigationHandle handle_2(url, web_contents()->GetMainFrame()); + content::MockNavigationHandle handle_0(url, + web_contents()->GetPrimaryMainFrame()); + content::MockNavigationHandle handle_1(url, + web_contents()->GetPrimaryMainFrame()); + content::MockNavigationHandle handle_2(url, + web_contents()->GetPrimaryMainFrame()); navigation_event_list()->RecordPendingNavigationEvent( &handle_0, CreateNavigationEventUniquePtr(url, one_minute_ago)); navigation_event_list()->RecordPendingNavigationEvent( diff --git a/chromium/components/safe_browsing/content/browser/safe_browsing_network_context.cc b/chromium/components/safe_browsing/content/browser/safe_browsing_network_context.cc index 43daf77b1c4..d54a8f6f932 100644 --- a/chromium/components/safe_browsing/content/browser/safe_browsing_network_context.cc +++ b/chromium/components/safe_browsing/content/browser/safe_browsing_network_context.cc @@ -9,6 +9,7 @@ #include "base/bind.h" #include "base/files/file_util.h" +#include "base/trace_event/trace_event.h" #include "components/safe_browsing/core/common/safebrowsing_constants.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/network_context_client_base.h" @@ -21,6 +22,10 @@ #include "services/network/network_context.h" #include "services/network/public/mojom/url_loader_factory.mojom.h" +#if BUILDFLAG(IS_ANDROID) +#include "base/android/remove_stale_data.h" +#endif + namespace safe_browsing { class SafeBrowsingNetworkContext::SharedURLLoaderFactory @@ -115,6 +120,8 @@ class SafeBrowsingNetworkContext::SharedURLLoaderFactory ~SharedURLLoaderFactory() override = default; network::mojom::NetworkContextParamsPtr CreateNetworkContextParams() { + TRACE_EVENT0("startup", + "SafeBrowsingNetworkContext::CreateNetworkContextParams"); DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); network::mojom::NetworkContextParamsPtr network_context_params = network_context_params_factory_.Run(); @@ -132,6 +139,18 @@ class SafeBrowsingNetworkContext::SharedURLLoaderFactory base::FilePath::StringType(kSafeBrowsingBaseFilename) + kCookiesFile); network_context_params->enable_encrypted_cookies = false; +#if BUILDFLAG(IS_ANDROID) + // On Android the `data_directory` was used by some wrong builds instead of + // `unsandboxed_data_path`. Cleaning it up. See crbug.com/1331809. + // The `cookie_manager` is set by WebView, where the mistaken migration did + // not happen. + DCHECK(!trigger_migration_); + if (!network_context_params->cookie_manager) { + base::android::RemoveStaleDataDirectory( + network_context_params->file_paths->data_directory.path()); + } +#endif // BUILDFLAG(IS_ANDROID) + return network_context_params; } diff --git a/chromium/components/safe_browsing/content/browser/safe_browsing_tab_observer.cc b/chromium/components/safe_browsing/content/browser/safe_browsing_tab_observer.cc index 0cd12512dcb..13b36f0f943 100644 --- a/chromium/components/safe_browsing/content/browser/safe_browsing_tab_observer.cc +++ b/chromium/components/safe_browsing/content/browser/safe_browsing_tab_observer.cc @@ -47,8 +47,6 @@ SafeBrowsingTabObserver::SafeBrowsingTabObserver( delegate_->DoesSafeBrowsingServiceExist() && csd_service) { safebrowsing_detection_host_ = delegate_->CreateClientSideDetectionHost(web_contents); - csd_service->AddClientSideDetectionHost( - safebrowsing_detection_host_.get()); } } #endif @@ -70,8 +68,6 @@ void SafeBrowsingTabObserver::UpdateSafebrowsingDetectionHost() { if (!safebrowsing_detection_host_.get()) { safebrowsing_detection_host_ = delegate_->CreateClientSideDetectionHost(&GetWebContents()); - csd_service->AddClientSideDetectionHost( - safebrowsing_detection_host_.get()); } } else { safebrowsing_detection_host_.reset(); diff --git a/chromium/components/safe_browsing/content/browser/threat_details.cc b/chromium/components/safe_browsing/content/browser/threat_details.cc index 2c5336108bf..0d18b38e54b 100644 --- a/chromium/components/safe_browsing/content/browser/threat_details.cc +++ b/chromium/components/safe_browsing/content/browser/threat_details.cc @@ -328,8 +328,6 @@ class ThreatDetailsFactoryImpl : public ThreatDetailsFactory { const security_interstitials::UnsafeResource& unsafe_resource, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, ReferrerChainProvider* referrer_chain_provider, bool trim_to_ad_tags, ThreatDetailsDoneCallback done_callback) override { @@ -338,8 +336,8 @@ class ThreatDetailsFactoryImpl : public ThreatDetailsFactory { // used. auto threat_details = base::WrapUnique(new ThreatDetails( ui_manager, web_contents, unsafe_resource, url_loader_factory, - history_service, get_user_population_callback, referrer_chain_provider, - trim_to_ad_tags, std::move(done_callback))); + history_service, referrer_chain_provider, trim_to_ad_tags, + std::move(done_callback))); threat_details->StartCollection(); return threat_details; } @@ -361,8 +359,6 @@ std::unique_ptr<ThreatDetails> ThreatDetails::NewThreatDetails( const UnsafeResource& resource, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, ReferrerChainProvider* referrer_chain_provider, bool trim_to_ad_tags, ThreatDetailsDoneCallback done_callback) { @@ -372,8 +368,7 @@ std::unique_ptr<ThreatDetails> ThreatDetails::NewThreatDetails( factory_ = g_threat_details_factory_impl.Pointer(); return factory_->CreateThreatDetails( ui_manager, web_contents, resource, url_loader_factory, history_service, - get_user_population_callback, referrer_chain_provider, trim_to_ad_tags, - std::move(done_callback)); + referrer_chain_provider, trim_to_ad_tags, std::move(done_callback)); } // Create a ThreatDetails for the given tab. Runs in the UI thread. @@ -383,8 +378,6 @@ ThreatDetails::ThreatDetails( const UnsafeResource& resource, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, ReferrerChainProvider* referrer_chain_provider, bool trim_to_ad_tags, ThreatDetailsDoneCallback done_callback) @@ -393,7 +386,6 @@ ThreatDetails::ThreatDetails( ui_manager_(ui_manager), browser_context_(web_contents->GetBrowserContext()), resource_(resource), - get_user_population_callback_(get_user_population_callback), referrer_chain_provider_(referrer_chain_provider), cache_result_(false), did_proceed_(false), @@ -643,8 +635,9 @@ void ThreatDetails::StartCollection() { // OnReceivedThreatDOMDetails will be called when the renderer replies. // TODO(mattm): In theory, if the user proceeds through the warning DOM // detail collection could be started once the page loads. - web_contents_->GetMainFrame()->ForEachRenderFrameHost(base::BindRepeating( - &ThreatDetails::RequestThreatDOMDetails, GetWeakPtr())); + web_contents_->GetPrimaryMainFrame()->ForEachRenderFrameHost( + base::BindRepeating(&ThreatDetails::RequestThreatDOMDetails, + GetWeakPtr())); } } @@ -653,7 +646,7 @@ void ThreatDetails::RequestThreatDOMDetails(content::RenderFrameHost* frame) { frame, back_forward_cache::DisabledReason( back_forward_cache::DisabledReasonId::kSafeBrowsingThreatDetails)); - if (!frame->IsRenderFrameCreated()) { + if (!frame->IsRenderFrameLive()) { // A child frame may have been created browser-side but has not completed // setting up the renderer for it yet. In particular, this occurs if the // child frame was blocked and that's why we're showing a safe browsing page @@ -851,28 +844,11 @@ void ThreatDetails::OnCacheCollectionReady() { report_->mutable_client_properties()->set_url_api_type( GetUrlApiTypeForThreatSource(resource_.threat_source)); - if (!get_user_population_callback_.is_null()) { - *report_->mutable_population() = get_user_population_callback_.Run(); - } - // Fill the referrer chain if applicable. MaybeFillReferrerChain(); // Send the report, using the SafeBrowsingService. - std::string serialized; - if (!report_->SerializeToString(&serialized)) { - DLOG(ERROR) << "Unable to serialize the threat report."; - AllDone(); - return; - } - - content::GetUIThreadTaskRunner({})->PostTask( - FROM_HERE, - base::BindOnce(&WebUIInfoSingleton::AddToCSBRRsSent, - base::Unretained(WebUIInfoSingleton::GetInstance()), - std::move(report_))); - - ui_manager_->SendSerializedThreatDetails(browser_context_, serialized); + ui_manager_->SendThreatDetails(browser_context_, std::move(report_)); AllDone(); } @@ -890,7 +866,7 @@ void ThreatDetails::MaybeFillReferrerChain() { // We would have cancelled a prerender if it was blocked, so we can use the // primary main frame here. referrer_chain_provider_->IdentifyReferrerChainByRenderFrameHost( - web_contents_->GetMainFrame(), kThreatDetailsUserGestureLimit, + web_contents_->GetPrimaryMainFrame(), kThreatDetailsUserGestureLimit, report_->mutable_referrer_chain()); } diff --git a/chromium/components/safe_browsing/content/browser/threat_details.h b/chromium/components/safe_browsing/content/browser/threat_details.h index cbfde1009d0..3104c345750 100644 --- a/chromium/components/safe_browsing/content/browser/threat_details.h +++ b/chromium/components/safe_browsing/content/browser/threat_details.h @@ -91,8 +91,6 @@ class ThreatDetails { const UnsafeResource& resource, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, ReferrerChainProvider* referrer_chain_provider, bool trim_to_ad_tags, ThreatDetailsDoneCallback done_callback); @@ -129,8 +127,6 @@ class ThreatDetails { const UnsafeResource& resource, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, ReferrerChainProvider* referrer_chain_provider, bool trim_to_ad_tags, ThreatDetailsDoneCallback done_callback); @@ -215,8 +211,6 @@ class ThreatDetails { const UnsafeResource resource_; - base::RepeatingCallback<ChromeUserPopulation()> get_user_population_callback_; - raw_ptr<ReferrerChainProvider> referrer_chain_provider_; // For every Url we collect we create a Resource message. We keep @@ -308,8 +302,6 @@ class ThreatDetailsFactory { const security_interstitials::UnsafeResource& unsafe_resource, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, ReferrerChainProvider* referrer_chain_provider, bool trim_to_ad_tags, ThreatDetailsDoneCallback done_callback) = 0; diff --git a/chromium/components/safe_browsing/content/browser/threat_details_cache.cc b/chromium/components/safe_browsing/content/browser/threat_details_cache.cc index 1f0a5b14ba2..bd28ab4f449 100644 --- a/chromium/components/safe_browsing/content/browser/threat_details_cache.cc +++ b/chromium/components/safe_browsing/content/browser/threat_details_cache.cc @@ -191,7 +191,7 @@ void ThreatDetailsCacheCollector::ReadResponse( std::string name, value; while (headers->EnumerateHeaderLines(&iter, &name, &value)) { // Strip any Set-Cookie headers. - if (base::LowerCaseEqualsASCII(name, "set-cookie")) + if (base::EqualsCaseInsensitiveASCII(name, "set-cookie")) continue; ClientSafeBrowsingReportRequest::HTTPHeader* pb_header = pb_response->add_headers(); diff --git a/chromium/components/safe_browsing/content/browser/triggers/BUILD.gn b/chromium/components/safe_browsing/content/browser/triggers/BUILD.gn index d1447798444..b7cef3ac77d 100644 --- a/chromium/components/safe_browsing/content/browser/triggers/BUILD.gn +++ b/chromium/components/safe_browsing/content/browser/triggers/BUILD.gn @@ -63,7 +63,6 @@ source_set("ad_sampler_trigger") { "//base:base", "//components/safe_browsing/core/browser:referrer_chain_provider", "//components/safe_browsing/core/common", - "//components/safe_browsing/core/common/proto:csd_proto", "//content/public/browser", ] } @@ -81,7 +80,6 @@ source_set("suspicious_site_trigger") { "//components/prefs:prefs", "//components/safe_browsing/core/browser:referrer_chain_provider", "//components/safe_browsing/core/common", - "//components/safe_browsing/core/common/proto:csd_proto", "//content/public/browser", "//net:net", ] diff --git a/chromium/components/safe_browsing/content/browser/triggers/ad_sampler_trigger.cc b/chromium/components/safe_browsing/content/browser/triggers/ad_sampler_trigger.cc index e7b6d9b6932..d328b5521a3 100644 --- a/chromium/components/safe_browsing/content/browser/triggers/ad_sampler_trigger.cc +++ b/chromium/components/safe_browsing/content/browser/triggers/ad_sampler_trigger.cc @@ -85,8 +85,6 @@ AdSamplerTrigger::AdSamplerTrigger( PrefService* prefs, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, ReferrerChainProvider* referrer_chain_provider) : content::WebContentsObserver(web_contents), content::WebContentsUserData<AdSamplerTrigger>(*web_contents), @@ -99,7 +97,6 @@ AdSamplerTrigger::AdSamplerTrigger( prefs_(prefs), url_loader_factory_(url_loader_factory), history_service_(history_service), - get_user_population_callback_(get_user_population_callback), referrer_chain_provider_(referrer_chain_provider), task_runner_(content::GetUIThreadTaskRunner({})) {} @@ -137,7 +134,7 @@ void AdSamplerTrigger::CreateAdSampleReport() { TriggerManager::GetSBErrorDisplayOptions(*prefs_, web_contents()); const content::GlobalRenderFrameHostId primary_main_frame_id = - web_contents()->GetMainFrame()->GetGlobalId(); + web_contents()->GetPrimaryMainFrame()->GetGlobalId(); security_interstitials::UnsafeResource resource; resource.threat_type = SB_THREAT_TYPE_AD_SAMPLE; resource.url = web_contents()->GetURL(); @@ -146,8 +143,7 @@ void AdSamplerTrigger::CreateAdSampleReport() { if (!trigger_manager_->StartCollectingThreatDetails( TriggerType::AD_SAMPLE, web_contents(), resource, url_loader_factory_, - history_service_, get_user_population_callback_, - referrer_chain_provider_, error_options)) { + history_service_, referrer_chain_provider_, error_options)) { UMA_HISTOGRAM_ENUMERATION(kAdSamplerTriggerActionMetricName, NO_SAMPLE_COULD_NOT_START_REPORT, MAX_ACTIONS); return; diff --git a/chromium/components/safe_browsing/content/browser/triggers/ad_sampler_trigger.h b/chromium/components/safe_browsing/content/browser/triggers/ad_sampler_trigger.h index 59af2aadc94..b6336dab7d7 100644 --- a/chromium/components/safe_browsing/content/browser/triggers/ad_sampler_trigger.h +++ b/chromium/components/safe_browsing/content/browser/triggers/ad_sampler_trigger.h @@ -8,7 +8,6 @@ #include "base/gtest_prod_util.h" #include "base/memory/raw_ptr.h" #include "base/memory/weak_ptr.h" -#include "components/safe_browsing/core/common/proto/csd.pb.h" #include "content/public/browser/web_contents_observer.h" #include "content/public/browser/web_contents_user_data.h" @@ -83,8 +82,6 @@ class AdSamplerTrigger : public content::WebContentsObserver, PrefService* prefs, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, ReferrerChainProvider* referrer_chain_provider); // Called to create an ad sample report. @@ -116,7 +113,6 @@ class AdSamplerTrigger : public content::WebContentsObserver, raw_ptr<PrefService> prefs_; scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_; raw_ptr<history::HistoryService> history_service_; - base::RepeatingCallback<ChromeUserPopulation()> get_user_population_callback_; raw_ptr<ReferrerChainProvider> referrer_chain_provider_; // Task runner for posting delayed tasks. Normally set to the runner for the diff --git a/chromium/components/safe_browsing/content/browser/triggers/ad_sampler_trigger_unittest.cc b/chromium/components/safe_browsing/content/browser/triggers/ad_sampler_trigger_unittest.cc index 85c8017866a..54b90de5022 100644 --- a/chromium/components/safe_browsing/content/browser/triggers/ad_sampler_trigger_unittest.cc +++ b/chromium/components/safe_browsing/content/browser/triggers/ad_sampler_trigger_unittest.cc @@ -50,8 +50,7 @@ class AdSamplerTriggerTest : public content::RenderViewHostTestHarness { void CreateTriggerWithFrequency(const size_t denominator) { safe_browsing::AdSamplerTrigger::CreateForWebContents( - web_contents(), &trigger_manager_, &prefs_, nullptr, nullptr, - base::NullCallback(), nullptr); + web_contents(), &trigger_manager_, &prefs_, nullptr, nullptr, nullptr); safe_browsing::AdSamplerTrigger* ad_sampler = safe_browsing::AdSamplerTrigger::FromWebContents(web_contents()); @@ -69,7 +68,7 @@ class AdSamplerTriggerTest : public content::RenderViewHostTestHarness { // Returns the final RenderFrameHost after navigation commits. RenderFrameHost* NavigateMainFrame(const std::string& url) { - return NavigateFrame(url, web_contents()->GetMainFrame()); + return NavigateFrame(url, web_contents()->GetPrimaryMainFrame()); } // Returns the final RenderFrameHost after navigation commits. @@ -101,7 +100,7 @@ TEST_F(AdSamplerTriggerTest, TriggerDisabledBySamplingFrequency) { // zero, which disables the trigger. CreateTriggerWithFrequency(kAdSamplerFrequencyDisabled); EXPECT_CALL(*get_trigger_manager(), - StartCollectingThreatDetails(_, _, _, _, _, _, _, _)) + StartCollectingThreatDetails(_, _, _, _, _, _, _)) .Times(0); EXPECT_CALL(*get_trigger_manager(), FinishCollectingThreatDetails(_, _, _, _, _, _)) @@ -128,7 +127,7 @@ TEST_F(AdSamplerTriggerTest, DISABLED_PageWithNoAds) { CreateTriggerWithFrequency(/*denominator=*/1); EXPECT_CALL(*get_trigger_manager(), - StartCollectingThreatDetails(_, _, _, _, _, _, _, _)) + StartCollectingThreatDetails(_, _, _, _, _, _, _)) .Times(0); EXPECT_CALL(*get_trigger_manager(), FinishCollectingThreatDetails(_, _, _, _, _, _)) @@ -151,7 +150,7 @@ TEST_F(AdSamplerTriggerTest, PageWithMultipleAds) { CreateTriggerWithFrequency(/*denominator=*/1); EXPECT_CALL(*get_trigger_manager(), StartCollectingThreatDetails(TriggerType::AD_SAMPLE, - web_contents(), _, _, _, _, _, _)) + web_contents(), _, _, _, _, _)) .Times(2) .WillRepeatedly(Return(true)); EXPECT_CALL(*get_trigger_manager(), @@ -184,7 +183,7 @@ TEST_F(AdSamplerTriggerTest, ReportRejectedByTriggerManager) { CreateTriggerWithFrequency(/*denominator=*/1); EXPECT_CALL(*get_trigger_manager(), StartCollectingThreatDetails(TriggerType::AD_SAMPLE, - web_contents(), _, _, _, _, _, _)) + web_contents(), _, _, _, _, _)) .Times(1) .WillOnce(Return(false)); EXPECT_CALL(*get_trigger_manager(), @@ -216,7 +215,7 @@ TEST(AdSamplerTriggerTestFinch, FrequencyDenominatorFeature) { // given. content::BrowserTaskEnvironment task_environment; AdSamplerTrigger trigger_default(nullptr, nullptr, nullptr, nullptr, nullptr, - base::NullCallback(), nullptr); + nullptr); EXPECT_EQ(kAdSamplerDefaultFrequency, trigger_default.sampler_frequency_denominator_); @@ -232,7 +231,7 @@ TEST(AdSamplerTriggerTestFinch, FrequencyDenominatorFeature) { safe_browsing::kAdSamplerTriggerFeature, feature_params); AdSamplerTrigger trigger_finch(nullptr, nullptr, nullptr, nullptr, nullptr, - base::NullCallback(), nullptr); + nullptr); EXPECT_EQ(kDenominatorInt, trigger_finch.sampler_frequency_denominator_); } } // namespace safe_browsing diff --git a/chromium/components/safe_browsing/content/browser/triggers/mock_trigger_manager.h b/chromium/components/safe_browsing/content/browser/triggers/mock_trigger_manager.h index 13a05caab21..46833c4cee5 100644 --- a/chromium/components/safe_browsing/content/browser/triggers/mock_trigger_manager.h +++ b/chromium/components/safe_browsing/content/browser/triggers/mock_trigger_manager.h @@ -21,26 +21,22 @@ class MockTriggerManager : public TriggerManager { ~MockTriggerManager() override; - MOCK_METHOD8( + MOCK_METHOD7( StartCollectingThreatDetails, bool(TriggerType trigger_type, content::WebContents* web_contents, const security_interstitials::UnsafeResource& resource, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, ReferrerChainProvider* referrer_chain_provider, const SBErrorOptions& error_display_options)); - MOCK_METHOD9( + MOCK_METHOD8( StartCollectingThreatDetailsWithReason, bool(TriggerType trigger_type, content::WebContents* web_contents, const security_interstitials::UnsafeResource& resource, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, ReferrerChainProvider* referrer_chain_provider, const SBErrorOptions& error_display_options, TriggerManagerReason* out_reason)); diff --git a/chromium/components/safe_browsing/content/browser/triggers/suspicious_site_trigger.cc b/chromium/components/safe_browsing/content/browser/triggers/suspicious_site_trigger.cc index d4d092faeb6..eb0bc72d40f 100644 --- a/chromium/components/safe_browsing/content/browser/triggers/suspicious_site_trigger.cc +++ b/chromium/components/safe_browsing/content/browser/triggers/suspicious_site_trigger.cc @@ -61,8 +61,6 @@ SuspiciousSiteTrigger::SuspiciousSiteTrigger( PrefService* prefs, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, ReferrerChainProvider* referrer_chain_provider, bool monitor_mode) : content::WebContentsObserver(web_contents), @@ -74,7 +72,6 @@ SuspiciousSiteTrigger::SuspiciousSiteTrigger( prefs_(prefs), url_loader_factory_(url_loader_factory), history_service_(history_service), - get_user_population_callback_(get_user_population_callback), referrer_chain_provider_(referrer_chain_provider), task_runner_(content::GetUIThreadTaskRunner({})) {} @@ -102,8 +99,8 @@ bool SuspiciousSiteTrigger::MaybeStartReport() { TriggerManagerReason reason; if (!trigger_manager_->StartCollectingThreatDetailsWithReason( TriggerType::SUSPICIOUS_SITE, web_contents(), resource, - url_loader_factory_, history_service_, get_user_population_callback_, - referrer_chain_provider_, error_options, &reason)) { + url_loader_factory_, history_service_, referrer_chain_provider_, + error_options, &reason)) { UMA_HISTOGRAM_ENUMERATION(kSuspiciousSiteTriggerEventMetricName, SuspiciousSiteTriggerEvent::REPORT_START_FAILED); LOCAL_HISTOGRAM_ENUMERATION( diff --git a/chromium/components/safe_browsing/content/browser/triggers/suspicious_site_trigger.h b/chromium/components/safe_browsing/content/browser/triggers/suspicious_site_trigger.h index fc225e43e1c..88c9f90d30c 100644 --- a/chromium/components/safe_browsing/content/browser/triggers/suspicious_site_trigger.h +++ b/chromium/components/safe_browsing/content/browser/triggers/suspicious_site_trigger.h @@ -7,7 +7,6 @@ #include "base/memory/raw_ptr.h" #include "base/memory/weak_ptr.h" -#include "components/safe_browsing/core/common/proto/csd.pb.h" #include "content/public/browser/web_contents_observer.h" #include "content/public/browser/web_contents_user_data.h" @@ -132,8 +131,6 @@ class SuspiciousSiteTrigger PrefService* prefs, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, ReferrerChainProvider* referrer_chain_provider, bool monitor_mode); @@ -174,7 +171,6 @@ class SuspiciousSiteTrigger raw_ptr<PrefService> prefs_; scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_; raw_ptr<history::HistoryService> history_service_; - base::RepeatingCallback<ChromeUserPopulation()> get_user_population_callback_; raw_ptr<ReferrerChainProvider> referrer_chain_provider_; // Task runner for posting delayed tasks. Normally set to the runner for the diff --git a/chromium/components/safe_browsing/content/browser/triggers/suspicious_site_trigger_unittest.cc b/chromium/components/safe_browsing/content/browser/triggers/suspicious_site_trigger_unittest.cc index db7beb2a06b..5261da00dc0 100644 --- a/chromium/components/safe_browsing/content/browser/triggers/suspicious_site_trigger_unittest.cc +++ b/chromium/components/safe_browsing/content/browser/triggers/suspicious_site_trigger_unittest.cc @@ -55,8 +55,8 @@ class SuspiciousSiteTriggerTest : public content::RenderViewHostTestHarness { void CreateTrigger(bool monitor_mode) { safe_browsing::SuspiciousSiteTrigger::CreateForWebContents( - web_contents(), &trigger_manager_, &prefs_, nullptr, nullptr, - base::NullCallback(), nullptr, monitor_mode); + web_contents(), &trigger_manager_, &prefs_, nullptr, nullptr, nullptr, + monitor_mode); safe_browsing::SuspiciousSiteTrigger* trigger = safe_browsing::SuspiciousSiteTrigger::FromWebContents(web_contents()); // Give the trigger a test task runner that we can synchronize on. @@ -78,7 +78,7 @@ class SuspiciousSiteTriggerTest : public content::RenderViewHostTestHarness { // Returns the final RenderFrameHost after navigation commits. RenderFrameHost* NavigateMainFrame(const std::string& url) { - return NavigateFrame(url, web_contents()->GetMainFrame()); + return NavigateFrame(url, web_contents()->GetPrimaryMainFrame()); } // Returns the final RenderFrameHost after navigation commits. @@ -172,7 +172,7 @@ TEST_F(SuspiciousSiteTriggerTest, RegularPageNonSuspicious) { CreateTrigger(/*monitor_mode=*/false); EXPECT_CALL(*get_trigger_manager(), - StartCollectingThreatDetailsWithReason(_, _, _, _, _, _, _, _, _)) + StartCollectingThreatDetailsWithReason(_, _, _, _, _, _, _, _)) .Times(0); EXPECT_CALL(*get_trigger_manager(), FinishCollectingThreatDetails(_, _, _, _, _, _)) @@ -204,7 +204,7 @@ TEST_F(SuspiciousSiteTriggerTest, MAYBE_SuspiciousHitDuringLoad) { CreateTrigger(/*monitor_mode=*/false); EXPECT_CALL(*get_trigger_manager(), - StartCollectingThreatDetailsWithReason(_, _, _, _, _, _, _, _, _)) + StartCollectingThreatDetailsWithReason(_, _, _, _, _, _, _, _)) .Times(1) .WillOnce(Return(true)); EXPECT_CALL(*get_trigger_manager(), @@ -242,7 +242,7 @@ TEST_F(SuspiciousSiteTriggerTest, SuspiciousHitAfterLoad) { CreateTrigger(/*monitor_mode=*/false); EXPECT_CALL(*get_trigger_manager(), - StartCollectingThreatDetailsWithReason(_, _, _, _, _, _, _, _, _)) + StartCollectingThreatDetailsWithReason(_, _, _, _, _, _, _, _)) .Times(1) .WillOnce(Return(true)); EXPECT_CALL(*get_trigger_manager(), @@ -279,10 +279,10 @@ TEST_F(SuspiciousSiteTriggerTest, DISABLED_ReportRejectedByTriggerManager) { CreateTrigger(/*monitor_mode=*/false); EXPECT_CALL(*get_trigger_manager(), - StartCollectingThreatDetailsWithReason(_, _, _, _, _, _, _, _, _)) + StartCollectingThreatDetailsWithReason(_, _, _, _, _, _, _, _)) .Times(1) .WillOnce( - DoAll(SetArgPointee<8>(TriggerManagerReason::DAILY_QUOTA_EXCEEDED), + DoAll(SetArgPointee<7>(TriggerManagerReason::DAILY_QUOTA_EXCEEDED), Return(false))); EXPECT_CALL(*get_trigger_manager(), FinishCollectingThreatDetails(_, _, _, _, _, _)) @@ -321,7 +321,7 @@ TEST_F(SuspiciousSiteTriggerTest, NewNavigationMidLoad_NotSuspicious) { CreateTrigger(/*monitor_mode=*/false); EXPECT_CALL(*get_trigger_manager(), - StartCollectingThreatDetailsWithReason(_, _, _, _, _, _, _, _, _)) + StartCollectingThreatDetailsWithReason(_, _, _, _, _, _, _, _)) .Times(0); EXPECT_CALL(*get_trigger_manager(), FinishCollectingThreatDetails(_, _, _, _, _, _)) @@ -353,7 +353,7 @@ TEST_F(SuspiciousSiteTriggerTest, DISABLED_NewNavigationMidLoad_Suspicious) { CreateTrigger(/*monitor_mode=*/false); EXPECT_CALL(*get_trigger_manager(), - StartCollectingThreatDetailsWithReason(_, _, _, _, _, _, _, _, _)) + StartCollectingThreatDetailsWithReason(_, _, _, _, _, _, _, _)) .Times(0); EXPECT_CALL(*get_trigger_manager(), FinishCollectingThreatDetails(_, _, _, _, _, _)) @@ -391,7 +391,7 @@ TEST_F(SuspiciousSiteTriggerTest, MonitorMode_NotSuspicious) { CreateTrigger(/*monitor_mode=*/true); EXPECT_CALL(*get_trigger_manager(), - StartCollectingThreatDetailsWithReason(_, _, _, _, _, _, _, _, _)) + StartCollectingThreatDetailsWithReason(_, _, _, _, _, _, _, _)) .Times(0); EXPECT_CALL(*get_trigger_manager(), FinishCollectingThreatDetails(_, _, _, _, _, _)) @@ -418,7 +418,7 @@ TEST_F(SuspiciousSiteTriggerTest, MonitorMode_SuspiciousHitDuringLoad) { CreateTrigger(/*monitor_mode=*/true); EXPECT_CALL(*get_trigger_manager(), - StartCollectingThreatDetailsWithReason(_, _, _, _, _, _, _, _, _)) + StartCollectingThreatDetailsWithReason(_, _, _, _, _, _, _, _)) .Times(0); EXPECT_CALL(*get_trigger_manager(), FinishCollectingThreatDetails(_, _, _, _, _, _)) @@ -454,7 +454,7 @@ TEST_F(SuspiciousSiteTriggerTest, VisibleURLChangeMidLoad_NotSuspicious) { CreateTrigger(/*monitor_mode=*/false); EXPECT_CALL(*get_trigger_manager(), - StartCollectingThreatDetailsWithReason(_, _, _, _, _, _, _, _, _)) + StartCollectingThreatDetailsWithReason(_, _, _, _, _, _, _, _)) .Times(0); EXPECT_CALL(*get_trigger_manager(), FinishCollectingThreatDetails(_, _, _, _, _, _)) @@ -491,7 +491,7 @@ TEST_F(SuspiciousSiteTriggerTest, VisibleURLChangeMidLoad_Suspicious) { GURL suspicious_url(kSuspiciousUrl); EXPECT_CALL(*get_trigger_manager(), StartCollectingThreatDetailsWithReason( - _, _, ResourceHasUrl(suspicious_url), _, _, _, _, _, _)) + _, _, ResourceHasUrl(suspicious_url), _, _, _, _, _)) .Times(1) .WillOnce(Return(true)); EXPECT_CALL(*get_trigger_manager(), diff --git a/chromium/components/safe_browsing/content/browser/triggers/trigger_manager.cc b/chromium/components/safe_browsing/content/browser/triggers/trigger_manager.cc index b387d85270c..8d6f7b11851 100644 --- a/chromium/components/safe_browsing/content/browser/triggers/trigger_manager.cc +++ b/chromium/components/safe_browsing/content/browser/triggers/trigger_manager.cc @@ -148,15 +148,12 @@ bool TriggerManager::StartCollectingThreatDetails( const security_interstitials::UnsafeResource& resource, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, ReferrerChainProvider* referrer_chain_provider, const SBErrorOptions& error_display_options) { TriggerManagerReason unused_reason; return StartCollectingThreatDetailsWithReason( trigger_type, web_contents, resource, url_loader_factory, history_service, - get_user_population_callback, referrer_chain_provider, - error_display_options, &unused_reason); + referrer_chain_provider, error_display_options, &unused_reason); } bool TriggerManager::StartCollectingThreatDetailsWithReason( @@ -165,8 +162,6 @@ bool TriggerManager::StartCollectingThreatDetailsWithReason( const security_interstitials::UnsafeResource& resource, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, ReferrerChainProvider* referrer_chain_provider, const SBErrorOptions& error_display_options, TriggerManagerReason* reason) { @@ -184,8 +179,7 @@ bool TriggerManager::StartCollectingThreatDetailsWithReason( bool should_trim_threat_details = trigger_type == TriggerType::AD_SAMPLE; collectors->threat_details = ThreatDetails::NewThreatDetails( ui_manager_, web_contents, resource, url_loader_factory, history_service, - get_user_population_callback, referrer_chain_provider, - should_trim_threat_details, + referrer_chain_provider, should_trim_threat_details, base::BindOnce(&TriggerManager::ThreatDetailsDone, weak_factory_.GetWeakPtr())); return true; diff --git a/chromium/components/safe_browsing/content/browser/triggers/trigger_manager.h b/chromium/components/safe_browsing/content/browser/triggers/trigger_manager.h index d191758b4d4..edb23b7b4d4 100644 --- a/chromium/components/safe_browsing/content/browser/triggers/trigger_manager.h +++ b/chromium/components/safe_browsing/content/browser/triggers/trigger_manager.h @@ -132,8 +132,6 @@ class TriggerManager { const security_interstitials::UnsafeResource& resource, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, ReferrerChainProvider* referrer_chain_provider, const SBErrorOptions& error_display_options, TriggerManagerReason* out_reason); @@ -146,8 +144,6 @@ class TriggerManager { const security_interstitials::UnsafeResource& resource, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, ReferrerChainProvider* referrer_chain_provider, const SBErrorOptions& error_display_options); diff --git a/chromium/components/safe_browsing/content/browser/triggers/trigger_manager_unittest.cc b/chromium/components/safe_browsing/content/browser/triggers/trigger_manager_unittest.cc index a84455b3868..584e35d2f10 100644 --- a/chromium/components/safe_browsing/content/browser/triggers/trigger_manager_unittest.cc +++ b/chromium/components/safe_browsing/content/browser/triggers/trigger_manager_unittest.cc @@ -47,8 +47,6 @@ class MockThreatDetailsFactory : public ThreatDetailsFactory { const security_interstitials::UnsafeResource& unsafe_resource, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, history::HistoryService* history_service, - base::RepeatingCallback<ChromeUserPopulation()> - get_user_population_callback, ReferrerChainProvider* referrer_chain_provider, bool trim_to_ad_tags, ThreatDetailsDoneCallback done_callback) override { @@ -123,7 +121,7 @@ class TriggerManagerTest : public ::testing::Test { TriggerManager::GetSBErrorDisplayOptions(pref_service_, web_contents); return trigger_manager_.StartCollectingThreatDetails( trigger_type, web_contents, security_interstitials::UnsafeResource(), - nullptr, nullptr, base::NullCallback(), nullptr, options); + nullptr, nullptr, nullptr, options); } bool FinishCollectingThreatDetails(const TriggerType trigger_type, diff --git a/chromium/components/safe_browsing/content/browser/triggers/trigger_throttler.cc b/chromium/components/safe_browsing/content/browser/triggers/trigger_throttler.cc index b203cee28a2..8f3553ee387 100644 --- a/chromium/components/safe_browsing/content/browser/triggers/trigger_throttler.cc +++ b/chromium/components/safe_browsing/content/browser/triggers/trigger_throttler.cc @@ -18,7 +18,6 @@ namespace safe_browsing { const size_t kAdSamplerTriggerDefaultQuota = 10; const size_t kSuspiciousSiteTriggerDefaultQuota = 5; const char kSuspiciousSiteTriggerQuotaParam[] = "suspicious_site_trigger_quota"; -const char kTriggerTypeAndQuotaParam[] = "trigger_type_and_quota_csv"; namespace { const size_t kUnlimitedTriggerQuota = std::numeric_limits<size_t>::max(); @@ -49,44 +48,6 @@ void ParseTriggerTypeAndQuotaParam( trigger_type_and_quota_list->push_back( std::make_pair(TriggerType::SUSPICIOUS_SITE, suspicious_site_quota)); } - - // If the feature is disabled we just use the default list. Otherwise the list - // from the Finch param will be the one used. - if (!base::FeatureList::IsEnabled(kTriggerThrottlerDailyQuotaFeature)) { - return; - } - - const std::string& trigger_and_quota_csv_param = - base::GetFieldTrialParamValueByFeature(kTriggerThrottlerDailyQuotaFeature, - kTriggerTypeAndQuotaParam); - if (trigger_and_quota_csv_param.empty()) { - return; - } - - std::vector<std::string> split = - base::SplitString(trigger_and_quota_csv_param, ",", base::TRIM_WHITESPACE, - base::SPLIT_WANT_NONEMPTY); - // If we don't have the right number of pairs in the csv then don't bother - // parsing further. - if (split.size() % 2 != 0) { - return; - } - for (size_t i = 0; i < split.size(); i += 2) { - // Make sure both the trigger type and quota are integers. Skip them if not. - int trigger_type_int = -1; - int quota_int = -1; - if (!base::StringToInt(split[i], &trigger_type_int) || - !base::StringToInt(split[i + 1], "a_int)) { - continue; - } - trigger_type_and_quota_list->push_back( - std::make_pair(static_cast<TriggerType>(trigger_type_int), quota_int)); - } - - std::sort(trigger_type_and_quota_list->begin(), - trigger_type_and_quota_list->end(), - [](const TriggerTypeAndQuotaItem& a, - const TriggerTypeAndQuotaItem& b) { return a.first < b.first; }); } // Looks in |trigger_quota_list| for |trigger_type|. If found, sets |out_quota| @@ -241,7 +202,7 @@ void TriggerThrottler::WriteTriggerEventsToPref() { size_t TriggerThrottler::GetDailyQuotaForTrigger( const TriggerType trigger_type) const { - size_t quota_from_finch = 0; + size_t quota = 0; switch (trigger_type) { case TriggerType::SECURITY_INTERSTITIAL: case TriggerType::GAIA_PASSWORD_REUSE: @@ -253,18 +214,18 @@ size_t TriggerThrottler::GetDailyQuotaForTrigger( return 0; case TriggerType::AD_SAMPLE: - // Ad Samples have a non-zero default quota, but it can be overwritten - // through Finch. + // Check for non-default quota (needed for unit tests). if (TryFindQuotaForTrigger(trigger_type, trigger_type_and_quota_list_, - "a_from_finch)) { - return quota_from_finch; + "a)) { + return quota; } return kAdSamplerTriggerDefaultQuota; + case TriggerType::SUSPICIOUS_SITE: // Suspicious Sites are disabled unless they are configured through Finch. if (TryFindQuotaForTrigger(trigger_type, trigger_type_and_quota_list_, - "a_from_finch)) { - return quota_from_finch; + "a)) { + return quota; } break; } diff --git a/chromium/components/safe_browsing/content/browser/triggers/trigger_throttler.h b/chromium/components/safe_browsing/content/browser/triggers/trigger_throttler.h index 33fa604fdb5..8ee231b0cd3 100644 --- a/chromium/components/safe_browsing/content/browser/triggers/trigger_throttler.h +++ b/chromium/components/safe_browsing/content/browser/triggers/trigger_throttler.h @@ -27,14 +27,6 @@ extern const size_t kSuspiciousSiteTriggerDefaultQuota; // trigger. extern const char kSuspiciousSiteTriggerQuotaParam[]; -// Param name of the finch param containing the comma-separated list of trigger -// types and daily quotas. -// TODO(crbug.com/744869): This param should be deprecated after ad sampler -// launch in favour of having a unique quota feature and param per trigger. -// Having a single shared feature makes it impossible to run multiple trigger -// trials simultaneously. -extern const char kTriggerTypeAndQuotaParam[]; - enum class TriggerType { SECURITY_INTERSTITIAL = 1, AD_SAMPLE = 2, diff --git a/chromium/components/safe_browsing/content/browser/triggers/trigger_throttler_unittest.cc b/chromium/components/safe_browsing/content/browser/triggers/trigger_throttler_unittest.cc index b760b99f777..56c570004db 100644 --- a/chromium/components/safe_browsing/content/browser/triggers/trigger_throttler_unittest.cc +++ b/chromium/components/safe_browsing/content/browser/triggers/trigger_throttler_unittest.cc @@ -229,11 +229,6 @@ class TriggerThrottlerTestFinch : public ::testing::Test { const base::Feature** out_feature, std::string* out_param) { switch (trigger_type) { - case TriggerType::AD_SAMPLE: - *out_feature = &safe_browsing::kTriggerThrottlerDailyQuotaFeature; - *out_param = safe_browsing::kTriggerTypeAndQuotaParam; - break; - case TriggerType::SUSPICIOUS_SITE: *out_feature = &safe_browsing::kSuspiciousSiteTriggerQuotaFeature; *out_param = safe_browsing::kSuspiciousSiteTriggerQuotaParam; @@ -254,42 +249,12 @@ class TriggerThrottlerTestFinch : public ::testing::Test { } }; -TEST_F(TriggerThrottlerTestFinch, ConfigureQuotaViaFinch) { - base::test::ScopedFeatureList scoped_feature_list; - SetupQuotaParams(TriggerType::AD_SAMPLE, "Group_ConfigureQuotaViaFinch", 3, - &scoped_feature_list); - // Make sure that setting the quota param via Finch params works as expected. - - // The throttler has been configured (above) to allow ad samples to fire three - // times per day. - TriggerThrottler throttler(nullptr); - - // First three triggers should work - EXPECT_TRUE(throttler.TriggerCanFire(TriggerType::AD_SAMPLE)); - throttler.TriggerFired(TriggerType::AD_SAMPLE); - EXPECT_TRUE(throttler.TriggerCanFire(TriggerType::AD_SAMPLE)); - throttler.TriggerFired(TriggerType::AD_SAMPLE); - EXPECT_TRUE(throttler.TriggerCanFire(TriggerType::AD_SAMPLE)); - throttler.TriggerFired(TriggerType::AD_SAMPLE); - - // Fourth attempt will fail since we're out of quota. - EXPECT_FALSE(throttler.TriggerCanFire(TriggerType::AD_SAMPLE)); -} - TEST_F(TriggerThrottlerTestFinch, AdSamplerDefaultQuota) { - // Make sure that the ad sampler gets its own default quota when no finch - // config exists, but the quota can be overwritten through Finch. + // Make sure that the ad sampler gets its own default quota. TriggerThrottler throttler_default(nullptr); EXPECT_EQ(kAdSamplerTriggerDefaultQuota, GetDailyQuotaForTrigger(throttler_default, TriggerType::AD_SAMPLE)); EXPECT_TRUE(throttler_default.TriggerCanFire(TriggerType::AD_SAMPLE)); - - base::test::ScopedFeatureList scoped_feature_list; - SetupQuotaParams(TriggerType::AD_SAMPLE, "Group_AdSamplerDefaultQuota", 4, - &scoped_feature_list); - TriggerThrottler throttler_finch(nullptr); - EXPECT_EQ(4u, - GetDailyQuotaForTrigger(throttler_finch, TriggerType::AD_SAMPLE)); } TEST_F(TriggerThrottlerTestFinch, SuspiciousSiteTriggerDefaultQuota) { diff --git a/chromium/components/safe_browsing/content/browser/ui_manager.cc b/chromium/components/safe_browsing/content/browser/ui_manager.cc index ba29edb0b78..e7331e5729f 100644 --- a/chromium/components/safe_browsing/content/browser/ui_manager.cc +++ b/chromium/components/safe_browsing/content/browser/ui_manager.cc @@ -14,6 +14,7 @@ #include "components/prefs/pref_service.h" #include "components/safe_browsing/content/browser/safe_browsing_blocking_page.h" #include "components/safe_browsing/content/browser/threat_details.h" +#include "components/safe_browsing/content/browser/web_ui/safe_browsing_ui.h" #include "components/safe_browsing/core/browser/db/v4_protocol_manager_util.h" #include "components/safe_browsing/core/browser/ping_manager.h" #include "components/safe_browsing/core/common/features.h" @@ -203,6 +204,15 @@ void SafeBrowsingUIManager::MaybeReportSafeBrowsingHit( << hit_report.is_subresource << " " << hit_report.threat_type; delegate_->GetPingManager(web_contents->GetBrowserContext()) ->ReportSafeBrowsingHit(hit_report); + + // The following is to log this HitReport on any open chrome://safe-browsing + // pages. + auto hit_report_copy = std::make_unique<HitReport>(hit_report); + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, + base::BindOnce(&WebUIInfoSingleton::AddToHitReportsSent, + base::Unretained(WebUIInfoSingleton::GetInstance()), + std::move(hit_report_copy))); } // Static. @@ -279,18 +289,17 @@ const GURL SafeBrowsingUIManager::default_safe_page() const { // If the user had opted-in to send ThreatDetails, this gets called // when the report is ready. -void SafeBrowsingUIManager::SendSerializedThreatDetails( +void SafeBrowsingUIManager::SendThreatDetails( content::BrowserContext* browser_context, - const std::string& serialized) { + std::unique_ptr<ClientSafeBrowsingReportRequest> report) { DCHECK_CURRENTLY_ON(BrowserThread::UI); if (shut_down_) return; - if (!serialized.empty()) { - DVLOG(1) << "Sending serialized threat details."; - delegate_->GetPingManager(browser_context)->ReportThreatDetails(serialized); - } + DVLOG(1) << "Sending threat details."; + delegate_->GetPingManager(browser_context) + ->ReportThreatDetails(std::move(report)); } void SafeBrowsingUIManager::OnBlockingPageDone( diff --git a/chromium/components/safe_browsing/content/browser/ui_manager.h b/chromium/components/safe_browsing/content/browser/ui_manager.h index bc37f984b99..d2607ce152d 100644 --- a/chromium/components/safe_browsing/content/browser/ui_manager.h +++ b/chromium/components/safe_browsing/content/browser/ui_manager.h @@ -140,10 +140,11 @@ class SafeBrowsingUIManager : public BaseUIManager { // on UI thread. If shutdown is true, the manager is disabled permanently. void Stop(bool shutdown); - // Called on the IO thread by the ThreatDetails with the serialized - // protocol buffer, so the service can send it over. - void SendSerializedThreatDetails(content::BrowserContext* browser_context, - const std::string& serialized) override; + // Called on the IO thread by the ThreatDetails with the report, so the + // service can send it over. + void SendThreatDetails( + content::BrowserContext* browser_context, + std::unique_ptr<ClientSafeBrowsingReportRequest> report) override; // Calls |BaseUIManager::OnBlockingPageDone()| and triggers // |OnSecurityInterstitialProceeded| event if |proceed| is true. diff --git a/chromium/components/safe_browsing/content/browser/ui_manager_unittest.cc b/chromium/components/safe_browsing/content/browser/ui_manager_unittest.cc index a4e16e329f4..5f9c2c7a763 100644 --- a/chromium/components/safe_browsing/content/browser/ui_manager_unittest.cc +++ b/chromium/components/safe_browsing/content/browser/ui_manager_unittest.cc @@ -122,7 +122,6 @@ class TestSafeBrowsingBlockingPage : public SafeBrowsingBlockingPage { "cpn_safe_browsing"), // help_center_article_link true, // should_trigger_reporting /*history_service=*/nullptr, - /*get_user_population_callback=*/base::NullCallback(), /*navigation_observer_manager=*/nullptr, /*metrics_collector=*/nullptr, /*trigger_manager=*/nullptr) { @@ -236,7 +235,7 @@ class SafeBrowsingUIManagerTest : public content::RenderViewHostTestHarness { const char* url, bool is_subresource) { const content::GlobalRenderFrameHostId primary_main_frame_id = - web_contents()->GetMainFrame()->GetGlobalId(); + web_contents()->GetPrimaryMainFrame()->GetGlobalId(); security_interstitials::UnsafeResource resource; resource.url = GURL(url); resource.is_subresource = is_subresource; diff --git a/chromium/components/safe_browsing/content/browser/web_api_handshake_checker.cc b/chromium/components/safe_browsing/content/browser/web_api_handshake_checker.cc index 9cdd007217f..9c07406d1c3 100644 --- a/chromium/components/safe_browsing/content/browser/web_api_handshake_checker.cc +++ b/chromium/components/safe_browsing/content/browser/web_api_handshake_checker.cc @@ -45,7 +45,6 @@ class WebApiHandshakeChecker::CheckerOnIO std::move(delegate_getter_).Run(); bool skip_checks = !url_checker_delegate || - !url_checker_delegate->GetDatabaseManager()->IsSupported() || url_checker_delegate->ShouldSkipRequestCheck( url, frame_tree_node_id_, /*render_process_id=*/content::ChildProcessHost::kInvalidUniqueID, diff --git a/chromium/components/safe_browsing/content/browser/web_ui/BUILD.gn b/chromium/components/safe_browsing/content/browser/web_ui/BUILD.gn index 06bc3797497..26bdd3783db 100644 --- a/chromium/components/safe_browsing/content/browser/web_ui/BUILD.gn +++ b/chromium/components/safe_browsing/content/browser/web_ui/BUILD.gn @@ -21,6 +21,7 @@ static_library("web_ui") { "//components/safe_browsing/core/browser", "//components/safe_browsing/core/browser:download_check_result", "//components/safe_browsing/core/browser:referrer_chain_provider", + "//components/safe_browsing/core/browser/db:hit_report", "//components/safe_browsing/core/browser/db:v4_local_database_manager", "//components/safe_browsing/core/common", "//components/safe_browsing/core/common:safe_browsing_prefs", diff --git a/chromium/components/safe_browsing/content/browser/web_ui/resources/safe_browsing.html b/chromium/components/safe_browsing/content/browser/web_ui/resources/safe_browsing.html index 45423e018fa..685ee88b3b5 100644 --- a/chromium/components/safe_browsing/content/browser/web_ui/resources/safe_browsing.html +++ b/chromium/components/safe_browsing/content/browser/web_ui/resources/safe_browsing.html @@ -5,138 +5,137 @@ <title>Safe Browsing</title> <link rel="stylesheet" href="safe_browsing.css"> <link rel="stylesheet" href="chrome://resources/css/text_defaults.css"> - <link rel="stylesheet" href="chrome://resources/css/tabs.css"> </head> <body> <div id="header"> <h1 id="sb-title">Safe Browsing</h1> </div> - <tabbox id='tabbox'> - <tabs> - <tab id="preferences">Preferences</tab> - <tab id="db-manager">Database Manager</tab> - <tab id="hash-cache">Hash Cache</tab> - <tab id="csbrr">ClientSafeBrowsingReportRequests</tab> - <tab id="download-protection">Download Protection</tab> - <tab id="cpr">Client Phishing Requests</tab> - <tab id="password-protection">Password Protection</tab> - <tab id="rt-lookup">RT Lookup</tab> - <tab id="referrer-chain">Referrer Chain</tab> - <tab id="log">Log Messages</tab> - <tab id="reporting">Reporting Events</tab> - <tab id="deep-scan">Deep Scans</tab> - </tabs> - <tabpanels> - <tabpanel> - <h2>Experiments</h2> - <div class="content"> - <p id="experiments-list" class="result-container"></p> - </div> - <h2>Preferences</h2> - <div class="content"> - <p id="preferences-list" class="result-container"></p> - </div> - <h2>Policies</h2> - <div class="content"> - <p id="policies-list" class="result-container"></p> - </div> - <h2>Safe Browsing Cookie</h2> - <div class="content"> - <p id="cookie-panel" class="result-container"></p> - </div> - </tabpanel> - <tabpanel> - <h2>Database Manager</h2> - <div class="content"> - <p id="database-info-list" class="result-container"></p> - </div> - </tabpanel> - <tabpanel> - <h2>Full Hash Cache</h2> - <div class="content"> - <p id="full-hash-cache-info"></p> - </div> - </tabpanel> - <tabpanel> - <h2>CSBRRs (ClientSafeBrowsingReportRequest) sent</h2> - <div class="content"> - <p id="sent-csbrrs-list"></p> - </div> - </tabpanel> - <tabpanel> - <h2>Download URLs checked</h2> - <div class="content"> - <p id="download-urls-checked-list"></p> - </div> - <h2>Download requests (ClientDownloadRequest) sent</h2> - <div class="content"> - <p id="sent-client-download-requests-list"></p> - </div> - <h2>Download responses (ClientDownloadResponse) received</h2> - <div class="content"> - <p id="received-client-download-response-list"></p> - </div> - </tabpanel> - <tabpanel> - <h2>Client Phishing requests sent</h2> - <div class="content"> - <p id="sent-client-phishing-requests-list"></p> - </div> - <h2>Client Phishing responses sent</h2> - <div class="content"> - <p id="received-client-phishing-response-list"></p> - </div> - </tabpanel> - <tabpanel> - <h2>Saved Password Hashes</h2> - <div class="content"> - <p id="saved-passwords"></p> - </div> - <h2>Password Protection Events</h2> - <div class="content"> - <p id="pg-event-log"></p> - </div> - <h2>Security Events</h2> - <div class="content"> - <p id="security-event-log"></p> - </div> - <h2>Password Protection Pings</h2> - <table id="pg-ping-list" class="request-response"></table> - </tabpanel> - <tabpanel> - <h2>RT Lookup Pings</h2> - <table id="rt-lookup-ping-list" class="request-response"></table> - </tabpanel> - <tabpanel> - <h2>Referrer Chain</h2> - <form id="get-referrer-chain-form"> - <input type="text" id="referrer-chain-url"> - <input type="submit" value="Get Chain"> - </form> - <div class="content"> - <p id="referrer-chain-content"></p> - </div> - <h2>Most Recent Referring App Info (Android)</h2> - <p id="referring-app-info" class="result-container"></p> - </tabpanel> - <tabpanel> - <h2>Log Messages</h2> - <div class="content"> - <p id="log-messages"></p> - </div> - </tabpanel> - <tabpanel> - <h2>Reporting Events</h2> - <div class="content"> - <p id="reporting-events"></p> - </div> - </tabpanel> - <tabpanel> - <h2>Deep Scans</h2> - <table id="deep-scan-list" class="request-response"></table> - </tabpanel> - </tabpanels> - </tabbox> + <cr-tab-box id='tabbox'> + <div slot="tab" id="preferences">Preferences</div> + <div slot="tab" id="db-manager">Database Manager</div> + <div slot="tab" id="hash-cache">Hash Cache</div> + <div slot="tab" id="csbrr">ClientSafeBrowsingReportRequests</div> + <div slot="tab" id="download-protection">Download Protection</div> + <div slot="tab" id="cpr">Client Phishing Requests</div> + <div slot="tab" id="password-protection">Password Protection</div> + <div slot="tab" id="rt-lookup">RT Lookup</div> + <div slot="tab" id="referrer-chain">Referrer Chain</div> + <div slot="tab" id="log">Log Messages</div> + <div slot="tab" id="reporting">Reporting Events</div> + <div slot="tab" id="deep-scan">Deep Scans</div> + <div slot="panel"> + <h2>Experiments</h2> + <div class="content"> + <p id="experiments-list" class="result-container"></p> + </div> + <h2>Preferences</h2> + <div class="content"> + <p id="preferences-list" class="result-container"></p> + </div> + <h2>Policies</h2> + <div class="content"> + <p id="policies-list" class="result-container"></p> + </div> + <h2>Safe Browsing Cookie</h2> + <div class="content"> + <p id="cookie-panel" class="result-container"></p> + </div> + </div> + <div slot="panel"> + <h2>Database Manager</h2> + <div class="content"> + <p id="database-info-list" class="result-container"></p> + </div> + </div> + <div slot="panel"> + <h2>Full Hash Cache</h2> + <div class="content"> + <p id="full-hash-cache-info"></p> + </div> + </div> + <div slot="panel"> + <h2>CSBRRs (ClientSafeBrowsingReportRequest) sent</h2> + <div class="content"> + <p id="sent-csbrrs-list" class="result-container"></p> + </div> + <h2>Hit Reports sent</h2> + <div class="content"> + <p id="sent-hit-report-list" class="result-container"></p> + </div> + </div> + <div slot="panel"> + <h2>Download URLs checked</h2> + <div class="content"> + <p id="download-urls-checked-list"></p> + </div> + <h2>Download requests (ClientDownloadRequest) sent</h2> + <div class="content"> + <p id="sent-client-download-requests-list"></p> + </div> + <h2>Download responses (ClientDownloadResponse) received</h2> + <div class="content"> + <p id="received-client-download-response-list"></p> + </div> + </div> + <div slot="panel"> + <h2>Client Phishing requests sent</h2> + <div class="content"> + <p id="sent-client-phishing-requests-list"></p> + </div> + <h2>Client Phishing responses sent</h2> + <div class="content"> + <p id="received-client-phishing-response-list"></p> + </div> + </div> + <div slot="panel"> + <h2>Saved Password Hashes</h2> + <div class="content"> + <p id="saved-passwords"></p> + </div> + <h2>Password Protection Events</h2> + <div class="content"> + <p id="pg-event-log"></p> + </div> + <h2>Security Events</h2> + <div class="content"> + <p id="security-event-log"></p> + </div> + <h2>Password Protection Pings</h2> + <table id="pg-ping-list" class="request-response"></table> + </div> + <div slot="panel"> + <h2>RT Lookup Pings</h2> + <table id="rt-lookup-ping-list" class="request-response"></table> + </div> + <div slot="panel"> + <h2>Referrer Chain</h2> + <form id="get-referrer-chain-form"> + <input type="text" id="referrer-chain-url"> + <input type="submit" value="Get Chain"> + </form> + <div class="content"> + <p id="referrer-chain-content"></p> + </div> + <h2>Most Recent Referring App Info (Android)</h2> + <p id="referring-app-info" class="result-container"></p> + </div> + <div slot="panel"> + <h2>Log Messages</h2> + <div class="content"> + <p id="log-messages"></p> + </div> + </div> + <div slot="panel"> + <h2>Reporting Events</h2> + <div class="content"> + <p id="reporting-events"></p> + </div> + </div> + <div slot="panel"> + <h2>Deep Scans</h2> + <table id="deep-scan-list" class="request-response"></table> + </div> + </cr-tab-box> <template id="result-template"> <div> <span class="bold-span"></span> diff --git a/chromium/components/safe_browsing/content/browser/web_ui/resources/safe_browsing.js b/chromium/components/safe_browsing/content/browser/web_ui/resources/safe_browsing.js index f210463f835..6ad33bf7348 100644 --- a/chromium/components/safe_browsing/content/browser/web_ui/resources/safe_browsing.js +++ b/chromium/components/safe_browsing/content/browser/web_ui/resources/safe_browsing.js @@ -2,13 +2,11 @@ * Use of this source code is governed by a BSD-style license that can be * found in the LICENSE file. */ +import 'chrome://resources/cr_elements/cr_tab_box/cr_tab_box.js'; + import {addWebUIListener, sendWithPromise} from 'chrome://resources/js/cr.m.js'; -import {decorate} from 'chrome://resources/js/cr/ui.m.js'; -import {TabBox} from 'chrome://resources/js/cr/ui/tabs.js'; import {$} from 'chrome://resources/js/util.m.js'; -decorate('tabbox', TabBox); - /** * Asks the C++ SafeBrowsingUIHandler to get the lists of Safe Browsing * ongoing experiments and preferences. @@ -89,6 +87,15 @@ function initialize() { addSentCSBRRsInfo(result); }); + sendWithPromise('getSentHitReports', []).then((sentHitReports) => { + sentHitReports.forEach(function(hitReports) { + addSentHitReportsInfo(hitReports); + }); + }); + addWebUIListener('sent-hit-report-list', function(result) { + addSentHitReportsInfo(result); + }); + sendWithPromise('getPGEvents', []).then((pgEvents) => { pgEvents.forEach(function(pgEvent) { addPGEvent(pgEvent); @@ -184,10 +191,9 @@ function initialize() { }; // When the tab updates, update the anchor - $('tabbox').addEventListener('selectedChange', function() { - const tabbox = $('tabbox'); - const tabs = tabbox.querySelector('tabs').children; - const selectedTab = tabs[tabbox.selectedIndex]; + $('tabbox').addEventListener('selected-index-change', e => { + const tabs = document.querySelectorAll('div[slot=\'tab\']'); + const selectedTab = tabs[e.detail]; window.location.hash = 'tab-' + selectedTab.id; }, true); } @@ -310,6 +316,11 @@ function addSentCSBRRsInfo(result) { appendChildWithInnerText(logDiv, result); } +function addSentHitReportsInfo(result) { + const logDiv = $('sent-hit-report-list'); + appendChildWithInnerText(logDiv, result); +} + function addPGEvent(result) { const logDiv = $('pg-event-log'); const eventFormatted = '[' + (new Date(result['time'])).toLocaleString() + @@ -421,8 +432,11 @@ function addReferringAppInfo(info) { } function showTab(tabId) { - if ($(tabId)) { - $(tabId).selected = 'selected'; + const tabs = document.querySelectorAll('div[slot=\'tab\']'); + const index = Array.from(tabs).findIndex(t => t.id === tabId); + if (index !== -1) { + document.querySelector('cr-tab-box') + .setAttribute('selected-index', index.toString()); } } diff --git a/chromium/components/safe_browsing/content/browser/web_ui/safe_browsing_ui.cc b/chromium/components/safe_browsing/content/browser/web_ui/safe_browsing_ui.cc index 96394536699..052ea0f7636 100644 --- a/chromium/components/safe_browsing/content/browser/web_ui/safe_browsing_ui.cc +++ b/chromium/components/safe_browsing/content/browser/web_ui/safe_browsing_ui.cc @@ -14,12 +14,14 @@ #include "base/base64url.h" #include "base/bind.h" #include "base/callback.h" +#include "base/callback_helpers.h" #include "base/containers/cxx20_erase.h" #include "base/i18n/number_formatting.h" #include "base/i18n/time_formatting.h" #include "base/json/json_string_value_serializer.h" #include "base/memory/ref_counted.h" #include "base/memory/singleton.h" +#include "base/notreached.h" #include "base/strings/string_number_conversions.h" #include "base/strings/utf_string_conversions.h" #include "base/time/time.h" @@ -45,6 +47,7 @@ #include "content/public/browser/browser_task_traits.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/web_contents.h" +#include "services/network/public/mojom/content_security_policy.mojom.h" #if BUILDFLAG(SAFE_BROWSING_DB_LOCAL) #include "components/safe_browsing/core/browser/db/v4_local_database_manager.h" @@ -169,6 +172,9 @@ void WebUIInfoSingleton::AddToCSBRRsSent( for (auto* webui_listener : webui_instances_) webui_listener->NotifyCSBRRJsListener(csbrr.get()); csbrrs_sent_.push_back(std::move(csbrr)); + if (on_csbrr_logged_for_testing_) { + std::move(on_csbrr_logged_for_testing_).Run(); + } } void WebUIInfoSingleton::ClearCSBRRsSent() { @@ -176,6 +182,25 @@ void WebUIInfoSingleton::ClearCSBRRsSent() { csbrrs_sent_); } +void WebUIInfoSingleton::SetOnCSBRRLoggedCallbackForTesting( + base::OnceClosure on_done) { + on_csbrr_logged_for_testing_ = std::move(on_done); +} + +void WebUIInfoSingleton::AddToHitReportsSent( + std::unique_ptr<HitReport> hit_report) { + if (!HasListener()) + return; + + for (auto* webui_listener : webui_instances_) + webui_listener->NotifyHitReportJsListener(hit_report.get()); + hit_reports_sent_.push_back(std::move(hit_report)); +} + +void WebUIInfoSingleton::ClearHitReportsSent() { + std::vector<std::unique_ptr<HitReport>>().swap(hit_reports_sent_); +} + void WebUIInfoSingleton::AddToPGEvents( const sync_pb::UserEventSpecifics& event) { if (!HasListener()) @@ -394,12 +419,14 @@ WebUIInfoSingleton::GetReferringAppInfo(content::WebContents* web_contents) { void WebUIInfoSingleton::ClearListenerForTesting() { has_test_listener_ = false; + on_csbrr_logged_for_testing_ = base::NullCallback(); MaybeClearData(); } void WebUIInfoSingleton::MaybeClearData() { if (!HasListener()) { ClearCSBRRsSent(); + ClearHitReportsSent(); ClearDownloadUrlsChecked(); ClearClientDownloadRequestsSent(); ClearClientDownloadResponsesReceived(); @@ -904,6 +931,8 @@ std::string SerializeClientDownloadRequest(const ClientDownloadRequest& cdr) { dict_document_processing_info.SetStringKey( "maldoca_error_message", processing_info.maldoca_error_message()); } + dict_document_processing_info.SetBoolKey( + "processing_successful", processing_info.processing_successful()); dict_document_summary.SetKey("processing_info", std::move(dict_document_processing_info)); } @@ -1000,6 +1029,8 @@ std::string SerializeClientPhishingRequest( dict.SetBoolean("is_phishing", cpr.is_phishing()); if (cpr.has_model_version()) dict.SetInteger("model_version", cpr.model_version()); + if (cpr.has_dom_model_version()) + dict.SetInteger("dom_model_version", cpr.dom_model_version()); base::Value::ListStorage features; for (const auto& feature : cpr.feature_map()) { @@ -1068,8 +1099,125 @@ std::string SerializeClientPhishingResponse(const ClientPhishingResponse& cpr) { return request_serialized; } +base::Value::Dict SerializeHTTPHeader( + const ClientSafeBrowsingReportRequest::HTTPHeader& header) { + base::Value::Dict header_dict; + header_dict.Set("name", header.name()); + header_dict.Set("value", header.value()); + return header_dict; +} + +base::Value::Dict SerializeResource( + const ClientSafeBrowsingReportRequest::Resource& resource) { + base::Value::Dict resource_dict; + resource_dict.Set("id", resource.id()); + resource_dict.Set("url", resource.url()); + // HTTPRequest + if (resource.has_request()) { + base::Value::Dict request; + if (resource.request().has_firstline()) { + base::Value::Dict firstline; + firstline.Set("verb", resource.request().firstline().verb()); + firstline.Set("uri", resource.request().firstline().uri()); + firstline.Set("version", resource.request().firstline().version()); + request.Set("firstline", std::move(firstline)); + } + base::Value::List headers; + for (const ClientSafeBrowsingReportRequest::HTTPHeader& header : + resource.request().headers()) { + headers.Append(SerializeHTTPHeader(header)); + } + request.Set("headers", std::move(headers)); + resource_dict.Set("request", std::move(request)); + } + // HTTPResponse + if (resource.has_response()) { + base::Value::Dict response; + if (resource.response().has_firstline()) { + base::Value::Dict firstline; + firstline.Set("code", resource.response().firstline().code()); + firstline.Set("message", resource.response().firstline().message()); + firstline.Set("version", resource.response().firstline().version()); + response.Set("firstline", std::move(firstline)); + } + base::Value::List headers; + for (const ClientSafeBrowsingReportRequest::HTTPHeader& header : + resource.response().headers()) { + headers.Append(SerializeHTTPHeader(header)); + } + response.Set("headers", std::move(headers)); + response.Set("body", resource.response().body()); + response.Set("remote_ip", resource.response().remote_ip()); + resource_dict.Set("response", std::move(response)); + } + resource_dict.Set("parent_id", resource.parent_id()); + base::Value::List child_id_list; + for (const int& child_id : resource.child_ids()) { + child_id_list.Append(child_id); + } + resource_dict.Set("child_ids", std::move(child_id_list)); + resource_dict.Set("tag_name", resource.tag_name()); + return resource_dict; +} + +base::Value::Dict SerializeHTMLElement(const HTMLElement& element) { + base::Value::Dict element_dict; + element_dict.Set("id", element.id()); + element_dict.Set("tag", element.tag()); + base::Value::List child_id_lists; + for (const int& child_id : element.child_ids()) { + child_id_lists.Append(child_id); + } + element_dict.Set("child_ids", std::move(child_id_lists)); + element_dict.Set("resource_id", element.resource_id()); + base::Value::List attribute_list; + for (const HTMLElement::Attribute& attribute : element.attribute()) { + base::Value::Dict attribute_dict; + attribute_dict.Set("name", attribute.name()); + attribute_dict.Set("value", attribute.value()); + attribute_list.Append(std::move(attribute_dict)); + } + element_dict.Set("attribute", std::move(attribute_list)); + element_dict.Set("inner_html", element.inner_html()); + return element_dict; +} + +base::Value::Dict SerializeSafeBrowsingClientProperties( + const ClientSafeBrowsingReportRequest::SafeBrowsingClientProperties& + client_properties) { + base::Value::Dict client_properties_dict; + client_properties_dict.Set("client_version", + client_properties.client_version()); + client_properties_dict.Set( + "google_play_services_version", + static_cast<int>(client_properties.google_play_services_version())); + client_properties_dict.Set("is_instant_apps", + client_properties.is_instant_apps()); + std::string url_api_type; + switch (client_properties.url_api_type()) { + case ClientSafeBrowsingReportRequest:: + SAFE_BROWSING_URL_API_TYPE_UNSPECIFIED: + url_api_type = "SAFE_BROWSING_URL_API_TYPE_UNSPECIFIED"; + break; + case ClientSafeBrowsingReportRequest::PVER4_NATIVE: + url_api_type = "PVER4_NATIVE"; + break; + case ClientSafeBrowsingReportRequest::ANDROID_SAFETYNET: + url_api_type = "ANDROID_SAFETYNET"; + break; + case ClientSafeBrowsingReportRequest::REAL_TIME: + url_api_type = "REAL_TIME"; + break; + default: + NOTREACHED(); + url_api_type = ""; + } + client_properties_dict.Set("url_api_type", url_api_type); + return client_properties_dict; +} + std::string SerializeCSBRR(const ClientSafeBrowsingReportRequest& report) { - base::DictionaryValue report_request; + base::Value::Dict report_request; if (report.has_type()) { std::string report_type; switch (report.type()) { @@ -1125,53 +1273,142 @@ std::string SerializeCSBRR(const ClientSafeBrowsingReportRequest& report) { report_type = "BLOCKED_AD_POPUP"; break; } - report_request.SetString("type", report_type); + report_request.Set("type", report_type); + } + if (report.has_page_url()) { + report_request.Set("page_url", report.page_url()); + } + if (report.has_referrer_url()) { + report_request.Set("referrer_url", report.referrer_url()); } - if (report.has_page_url()) - report_request.SetString("page_url", report.page_url()); if (report.has_client_country()) { - report_request.SetString("client_country", report.client_country()); + report_request.Set("client_country", report.client_country()); } if (report.has_repeat_visit()) { - report_request.SetInteger("repeat_visit", report.repeat_visit()); + report_request.Set("repeat_visit", report.repeat_visit()); } if (report.has_did_proceed()) { - report_request.SetInteger("did_proceed", report.did_proceed()); + report_request.Set("did_proceed", report.did_proceed()); } if (report.has_download_verdict()) { - report_request.SetString( + report_request.Set( "download_verdict", ClientDownloadResponseVerdictToString(report.download_verdict())); } if (report.has_url()) { - report_request.SetString("url", report.url()); + report_request.Set("url", report.url()); } if (report.has_token()) { - report_request.SetString("token", report.token()); + report_request.Set("token", report.token()); } if (report.has_show_download_in_folder()) { - report_request.SetBoolean("show_download_in_folder", - report.show_download_in_folder()); + report_request.Set("show_download_in_folder", + report.show_download_in_folder()); } if (report.has_population()) { - report_request.SetKey("population", - SerializeChromeUserPopulation(report.population())); + report_request.Set("population", + SerializeChromeUserPopulation(report.population())); + } + base::Value::List resource_list; + for (const ClientSafeBrowsingReportRequest::Resource& resource : + report.resources()) { + resource_list.Append(SerializeResource(resource)); + } + report_request.Set("resources", std::move(resource_list)); + base::Value::List dom_list; + for (const HTMLElement& element : report.dom()) { + dom_list.Append(SerializeHTMLElement(element)); + } + report_request.Set("dom", std::move(dom_list)); + if (report.has_complete()) { + report_request.Set("complete", report.complete()); + } + if (report.has_client_properties()) { + report_request.Set( + "client_properties", + SerializeSafeBrowsingClientProperties(report.client_properties())); } std::string serialized; if (report.SerializeToString(&serialized)) { std::string base64_encoded; base::Base64Encode(serialized, &base64_encoded); - report_request.SetString("csbrr(base64)", base64_encoded); + report_request.Set("csbrr(base64)", base64_encoded); } - - base::Value* report_request_tree = &report_request; std::string report_request_serialized; JSONStringValueSerializer serializer(&report_request_serialized); serializer.set_pretty_print(true); - serializer.Serialize(*report_request_tree); + serializer.Serialize(report_request); return report_request_serialized; } +std::string SerializeHitReport(const HitReport& hit_report) { + base::Value::Dict hit_report_dict; + hit_report_dict.Set("malicious_url", hit_report.malicious_url.spec()); + hit_report_dict.Set("page_url", hit_report.page_url.spec()); + hit_report_dict.Set("referrer_url", hit_report.referrer_url.spec()); + hit_report_dict.Set("is_subresource", hit_report.is_subresource); + std::string threat_type; + switch (hit_report.threat_type) { + case SBThreatType::SB_THREAT_TYPE_URL_PHISHING: + threat_type = "SB_THREAT_TYPE_URL_PHISHING"; + break; + case SBThreatType::SB_THREAT_TYPE_URL_MALWARE: + threat_type = "SB_THREAT_TYPE_URL_MALWARE"; + break; + case SBThreatType::SB_THREAT_TYPE_URL_UNWANTED: + threat_type = "SB_THREAT_TYPE_URL_UNWANTED"; + break; + case SBThreatType::SB_THREAT_TYPE_URL_BINARY_MALWARE: + threat_type = "SB_THREAT_TYPE_URL_BINARY_MALWARE"; + break; + default: + threat_type = "OTHER"; + } + hit_report_dict.Set("threat_type", threat_type); + std::string threat_source; + switch (hit_report.threat_source) { + case ThreatSource::LOCAL_PVER4: + threat_source = "LOCAL_PVER4"; + break; + case ThreatSource::REMOTE: + threat_source = "REMOTE"; + break; + case ThreatSource::CLIENT_SIDE_DETECTION: + threat_source = "CLIENT_SIDE_DETECTION"; + break; + case ThreatSource::REAL_TIME_CHECK: + threat_source = "REAL_TIME_CHECK"; + break; + case ThreatSource::UNKNOWN: + threat_source = "UNKNOWN"; + break; + } + hit_report_dict.Set("threat_source", threat_source); + std::string extended_reporting_level; + switch (hit_report.extended_reporting_level) { + case ExtendedReportingLevel::SBER_LEVEL_OFF: + extended_reporting_level = "SBER_LEVEL_OFF"; + break; + case ExtendedReportingLevel::SBER_LEVEL_LEGACY: + extended_reporting_level = "SBER_LEVEL_LEGACY"; + break; + case ExtendedReportingLevel::SBER_LEVEL_SCOUT: + extended_reporting_level = "SBER_LEVEL_SCOUT"; + break; + } + hit_report_dict.Set("extended_reporting_level", extended_reporting_level); + hit_report_dict.Set("is_enhanced_protection", + hit_report.is_enhanced_protection); + hit_report_dict.Set("is_metrics_reporting_active", + hit_report.is_metrics_reporting_active); + hit_report_dict.Set("post_data", hit_report.post_data); + std::string hit_report_serialized; + JSONStringValueSerializer serializer(&hit_report_serialized); + serializer.set_pretty_print(true); + serializer.Serialize(hit_report_dict); + return hit_report_serialized; +} + base::Value SerializeReuseLookup( const PasswordReuseLookup password_reuse_lookup) { std::string lookup_result; @@ -2013,6 +2250,11 @@ SafeBrowsingUI::SafeBrowsingUI(content::WebUI* web_ui) html_source->AddResourcePath("safe_browsing.js", IDR_SAFE_BROWSING_JS); html_source->SetDefaultResource(IDR_SAFE_BROWSING_HTML); + // Static types + html_source->OverrideContentSecurityPolicy( + network::mojom::CSPDirectiveName::TrustedTypes, + "trusted-types static-types;"); + content::WebUIDataSource::Add(browser_context, html_source); } @@ -2317,6 +2559,22 @@ void SafeBrowsingUIHandler::GetSentCSBRRs(const base::Value::List& args) { ResolveJavascriptCallback(base::Value(callback_id), sent_reports); } +void SafeBrowsingUIHandler::GetSentHitReports(const base::Value::List& args) { + const std::vector<std::unique_ptr<HitReport>>& reports = + WebUIInfoSingleton::GetInstance()->hit_reports_sent(); + + base::ListValue sent_reports; + + for (const auto& report : reports) { + sent_reports.Append(base::Value(SerializeHitReport(*report))); + } + + AllowJavascript(); + DCHECK(!args.empty()); + std::string callback_id = args[0].GetString(); + ResolveJavascriptCallback(base::Value(callback_id), sent_reports); +} + void SafeBrowsingUIHandler::GetPGEvents(const base::Value::List& args) { const std::vector<sync_pb::UserEventSpecifics>& events = WebUIInfoSingleton::GetInstance()->pg_event_log(); @@ -2573,6 +2831,12 @@ void SafeBrowsingUIHandler::NotifyCSBRRJsListener( FireWebUIListener("sent-csbrr-update", base::Value(SerializeCSBRR(*csbrr))); } +void SafeBrowsingUIHandler::NotifyHitReportJsListener(HitReport* hit_report) { + AllowJavascript(); + FireWebUIListener("sent-hit-report-list", + base::Value(SerializeHitReport(*hit_report))); +} + void SafeBrowsingUIHandler::NotifyPGEventJsListener( const sync_pb::UserEventSpecifics& event) { AllowJavascript(); @@ -2702,6 +2966,10 @@ void SafeBrowsingUIHandler::RegisterMessages() { base::BindRepeating(&SafeBrowsingUIHandler::GetSentCSBRRs, base::Unretained(this))); web_ui()->RegisterMessageCallback( + "getSentHitReports", + base::BindRepeating(&SafeBrowsingUIHandler::GetSentHitReports, + base::Unretained(this))); + web_ui()->RegisterMessageCallback( "getPGEvents", base::BindRepeating(&SafeBrowsingUIHandler::GetPGEvents, base::Unretained(this))); web_ui()->RegisterMessageCallback( diff --git a/chromium/components/safe_browsing/content/browser/web_ui/safe_browsing_ui.h b/chromium/components/safe_browsing/content/browser/web_ui/safe_browsing_ui.h index ac8c92ce535..d58119605f4 100644 --- a/chromium/components/safe_browsing/content/browser/web_ui/safe_browsing_ui.h +++ b/chromium/components/safe_browsing/content/browser/web_ui/safe_browsing_ui.h @@ -12,7 +12,9 @@ #include "build/build_config.h" #include "components/safe_browsing/buildflags.h" #include "components/safe_browsing/content/browser/safe_browsing_service_interface.h" +#include "components/safe_browsing/core/browser/db/hit_report.h" #include "components/safe_browsing/core/browser/download_check_result.h" +#include "components/safe_browsing/core/browser/ping_manager.h" #include "components/safe_browsing/core/browser/safe_browsing_url_checker_impl.h" #include "components/safe_browsing/core/common/proto/csd.pb.h" #include "components/safe_browsing/core/common/proto/realtimeapi.pb.h" @@ -140,6 +142,10 @@ class SafeBrowsingUIHandler : public content::WebUIMessageHandler { // open chrome://safe-browsing tab was opened. void GetSentCSBRRs(const base::Value::List& args); + // Get the HitReports that have been collected since the oldest currently + // open chrome://safe-browsing tab was opened. + void GetSentHitReports(const base::Value::List& args); + // Get the PhishGuard events that have been collected since the oldest // currently open chrome://safe-browsing tab was opened. void GetPGEvents(const base::Value::List& args); @@ -221,6 +227,10 @@ class SafeBrowsingUIHandler : public content::WebUIMessageHandler { // sent, while one or more WebUI tabs are opened. void NotifyCSBRRJsListener(ClientSafeBrowsingReportRequest* csbrr); + // Get the new HitReport messages sent from PingManager when a ping is + // sent, while one or more WebUI tabs are opened. + void NotifyHitReportJsListener(HitReport* hit_report); + // Called when any new PhishGuard events are sent while one or more WebUI tabs // are open. void NotifyPGEventJsListener(const sync_pb::UserEventSpecifics& event); @@ -292,8 +302,12 @@ class SafeBrowsingUI : public content::WebUIController { ~SafeBrowsingUI() override; }; -class WebUIInfoSingleton : public SafeBrowsingUrlCheckerImpl::WebUIDelegate { +class WebUIInfoSingleton : public SafeBrowsingUrlCheckerImpl::WebUIDelegate, + public PingManager::WebUIDelegate { public: + WebUIInfoSingleton(); + ~WebUIInfoSingleton() override; + static WebUIInfoSingleton* GetInstance(); WebUIInfoSingleton(const WebUIInfoSingleton&) = delete; @@ -343,13 +357,24 @@ class WebUIInfoSingleton : public SafeBrowsingUrlCheckerImpl::WebUIDelegate { // Clear the list of the received ClientPhishingResponse messages. void ClearClientPhishingResponsesReceived(); + // PingManager::WebUIDelegate: // Add the new message in |csbrrs_sent_| and send it to all the open // chrome://safe-browsing tabs. - void AddToCSBRRsSent(std::unique_ptr<ClientSafeBrowsingReportRequest> csbrr); + void AddToCSBRRsSent( + std::unique_ptr<ClientSafeBrowsingReportRequest> csbrr) override; // Clear the list of the sent ClientSafeBrowsingReportRequest messages. void ClearCSBRRsSent(); + void SetOnCSBRRLoggedCallbackForTesting(base::OnceClosure on_done); + + // Add the new message in |hit_reports_sent_| and send it to all the open + // chrome://safe-browsing tabs. + void AddToHitReportsSent(std::unique_ptr<HitReport> hit_report); + + // Clear the list of the sent HitReport messages. + void ClearHitReportsSent(); + // Add the new message in |pg_event_log_| and send it to all the open // chrome://safe-browsing tabs. void AddToPGEvents(const sync_pb::UserEventSpecifics& event); @@ -475,6 +500,12 @@ class WebUIInfoSingleton : public SafeBrowsingUrlCheckerImpl::WebUIDelegate { return csbrrs_sent_; } + // Get the list of the sent HitReports that have been collected since the + // oldest currently open chrome://safe-browsing tab was opened. + const std::vector<std::unique_ptr<HitReport>>& hit_reports_sent() const { + return hit_reports_sent_; + } + // Get the list of WebUI listener objects. const std::vector<SafeBrowsingUIHandler*>& webui_instances() const { return webui_instances_; @@ -554,9 +585,6 @@ class WebUIInfoSingleton : public SafeBrowsingUrlCheckerImpl::WebUIDelegate { void ClearListenerForTesting(); private: - WebUIInfoSingleton(); - ~WebUIInfoSingleton() override; - void MaybeClearData(); friend struct base::DefaultSingletonTraits<WebUIInfoSingleton>; @@ -599,6 +627,15 @@ class WebUIInfoSingleton : public SafeBrowsingUrlCheckerImpl::WebUIDelegate { // functions that call AllowJavascript(), which is not marked const. std::vector<std::unique_ptr<ClientSafeBrowsingReportRequest>> csbrrs_sent_; + // Gets fired at the end of the AddToCSBRRsSent function. Only used for tests. + base::OnceClosure on_csbrr_logged_for_testing_; + + // List of HitReports sent since since the oldest currently open + // chrome://safe-browsing tab was opened. + // "HitReport" cannot be const, due to being used by + // functions that call AllowJavascript(), which is not marked const. + std::vector<std::unique_ptr<HitReport>> hit_reports_sent_; + // List of PhishGuard events sent since the oldest currently open // chrome://safe-browsing tab was opened. std::vector<sync_pb::UserEventSpecifics> pg_event_log_; diff --git a/chromium/components/safe_browsing/content/common/file_type_policies_policy_util.cc b/chromium/components/safe_browsing/content/common/file_type_policies_policy_util.cc index 8a787c6327a..15df3fae14a 100644 --- a/chromium/components/safe_browsing/content/common/file_type_policies_policy_util.cc +++ b/chromium/components/safe_browsing/content/common/file_type_policies_policy_util.cc @@ -64,7 +64,7 @@ bool IsInNotDangerousOverrideList(const std::string& extension, if (!domains_for_extension.GetListDeprecated().empty()) { url_matcher::URLMatcher matcher; - url_matcher::URLMatcherConditionSet::ID id(0); + base::MatcherStringPattern::ID id(0); url_matcher::util::AddFilters(&matcher, true, &id, &domains_for_extension); auto matching_set_size = matcher.MatchURL(normalized_url).size(); diff --git a/chromium/components/safe_browsing/content/common/safe_browsing.mojom b/chromium/components/safe_browsing/content/common/safe_browsing.mojom index 1f9144fb17e..9b95cbf812b 100644 --- a/chromium/components/safe_browsing/content/common/safe_browsing.mojom +++ b/chromium/components/safe_browsing/content/common/safe_browsing.mojom @@ -121,8 +121,21 @@ enum PhishingDetectorResult { }; [EnableIf=full_safe_browsing] -// Interface for setting the CSD model and to start phishing classification. +// Interface for starting phishing classification. This is scoped to a +// particular RenderFrame. interface PhishingDetector { + // Tells the renderer to begin phishing detection for the given toplevel URL + // which it has started loading. Returns the serialized request proto and a + // |result| enum to indicate failure. If the URL is phishing the request proto + // will have |is_phishing()| set to true. + StartPhishingDetection(url.mojom.Url url) + => (PhishingDetectorResult result, string request_proto); +}; + +[EnableIf=full_safe_browsing] +// Interface for setting a phishing model. This is scoped to an entire +// RenderProcess. +interface PhishingModelSetter { // A classification model for client-side phishing detection. // The string is an encoded safe_browsing::ClientSideModel protocol buffer, or // empty to disable client-side phishing detection for this renderer. The @@ -140,10 +153,15 @@ interface PhishingDetector { SetPhishingFlatBufferModel(mojo_base.mojom.ReadOnlySharedMemoryRegion region, mojo_base.mojom.ReadOnlyFile? tflite_model); - // Tells the renderer to begin phishing detection for the given toplevel URL - // which it has started loading. Returns the serialized request proto and a - // |result| enum to indicate failure. If the URL is phishing the request proto - // will have |is_phishing()| set to true. - StartPhishingDetection(url.mojom.Url url) - => (PhishingDetectorResult result, string request_proto); + // This is used in tests to ensure that the model has been set before sending + // IPCs to start classification. + SetTestObserver(pending_remote<PhishingModelSetterTestObserver>? observer) => + (); +}; + +// This is used in tests to ensure that the model has been set before sending +// IPCs to start classification. +interface PhishingModelSetterTestObserver { + // Called whenever the phishing model has changed. + PhishingModelUpdated(); }; diff --git a/chromium/components/safe_browsing/content/renderer/phishing_classifier/BUILD.gn b/chromium/components/safe_browsing/content/renderer/phishing_classifier/BUILD.gn index df0ebca7465..cba75f005b9 100644 --- a/chromium/components/safe_browsing/content/renderer/phishing_classifier/BUILD.gn +++ b/chromium/components/safe_browsing/content/renderer/phishing_classifier/BUILD.gn @@ -23,6 +23,8 @@ source_set("phishing_classifier") { "phishing_classifier_delegate.h", "phishing_dom_feature_extractor.cc", "phishing_dom_feature_extractor.h", + "phishing_model_setter_impl.cc", + "phishing_model_setter_impl.h", "phishing_term_feature_extractor.cc", "phishing_term_feature_extractor.h", "phishing_url_feature_extractor.cc", diff --git a/chromium/components/safe_browsing/content/renderer/phishing_classifier/flatbuffer_scorer.cc b/chromium/components/safe_browsing/content/renderer/phishing_classifier/flatbuffer_scorer.cc index d93c10e1c9d..0e8954fdca9 100644 --- a/chromium/components/safe_browsing/content/renderer/phishing_classifier/flatbuffer_scorer.cc +++ b/chromium/components/safe_browsing/content/renderer/phishing_classifier/flatbuffer_scorer.cc @@ -94,7 +94,7 @@ FlatBufferModelScorer::FlatBufferModelScorer() = default; FlatBufferModelScorer::~FlatBufferModelScorer() = default; /* static */ -FlatBufferModelScorer* FlatBufferModelScorer::Create( +std::unique_ptr<FlatBufferModelScorer> FlatBufferModelScorer::Create( base::ReadOnlySharedMemoryRegion region, base::File visual_tflite_model) { std::unique_ptr<FlatBufferModelScorer> scorer(new FlatBufferModelScorer()); @@ -142,7 +142,7 @@ FlatBufferModelScorer* FlatBufferModelScorer::Create( RecordScorerCreationStatus(SCORER_SUCCESS); scorer->flatbuffer_mapping_ = std::move(mapping); - return scorer.release(); + return scorer; } double FlatBufferModelScorer::ComputeRuleScore( @@ -205,6 +205,10 @@ int FlatBufferModelScorer::model_version() const { return flatbuffer_model_->version(); } +int FlatBufferModelScorer::dom_model_version() const { + return flatbuffer_model_->dom_model_version(); +} + bool FlatBufferModelScorer::has_page_term(const std::string& str) const { const flatbuffers::Vector<flatbuffers::Offset<flat::Hash>>* hashes = flatbuffer_model_->hashes(); diff --git a/chromium/components/safe_browsing/content/renderer/phishing_classifier/flatbuffer_scorer.h b/chromium/components/safe_browsing/content/renderer/phishing_classifier/flatbuffer_scorer.h index b4622f91d56..a34974ad6ac 100644 --- a/chromium/components/safe_browsing/content/renderer/phishing_classifier/flatbuffer_scorer.h +++ b/chromium/components/safe_browsing/content/renderer/phishing_classifier/flatbuffer_scorer.h @@ -43,8 +43,9 @@ class FlatBufferModelScorer : public Scorer { // Factory method which creates a new Scorer object by parsing the given // flatbuffer or tflite model. If parsing fails this method returns NULL. // Use this only if region is valid. - static FlatBufferModelScorer* Create(base::ReadOnlySharedMemoryRegion region, - base::File visual_tflite_model); + static std::unique_ptr<FlatBufferModelScorer> Create( + base::ReadOnlySharedMemoryRegion region, + base::File visual_tflite_model); double ComputeScore(const FeatureMap& features) const override; @@ -55,6 +56,7 @@ class FlatBufferModelScorer : public Scorer { #endif int model_version() const override; + int dom_model_version() const override; size_t max_words_per_term() const override; uint32_t murmurhash3_seed() const override; size_t max_shingles_per_page() const override; diff --git a/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_classifier.cc b/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_classifier.cc index 9fcd0153493..c382b715109 100644 --- a/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_classifier.cc +++ b/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_classifier.cc @@ -76,7 +76,7 @@ const float PhishingClassifier::kInvalidScore = -1.0; const float PhishingClassifier::kPhishyThreshold = 0.5; PhishingClassifier::PhishingClassifier(content::RenderFrame* render_frame) - : render_frame_(render_frame), scorer_(nullptr) { + : render_frame_(render_frame) { Clear(); } @@ -87,28 +87,8 @@ PhishingClassifier::~PhishingClassifier() { DCHECK(!page_text_); } -void PhishingClassifier::set_phishing_scorer(const Scorer* scorer) { - DCHECK(done_callback_.is_null()); - DCHECK(!page_text_); - scorer_ = scorer; - if (scorer_) { - url_extractor_ = std::make_unique<PhishingUrlFeatureExtractor>(); - dom_extractor_ = std::make_unique<PhishingDOMFeatureExtractor>(); - term_extractor_ = std::make_unique<PhishingTermFeatureExtractor>( - scorer_->find_page_term_callback(), scorer_->find_page_word_callback(), - scorer_->max_words_per_term(), scorer_->murmurhash3_seed(), - scorer_->max_shingles_per_page(), scorer_->shingle_size()); - } else { - // We're disabling client-side phishing detection, so tear down all - // of the relevant objects. - url_extractor_.reset(); - dom_extractor_.reset(); - term_extractor_.reset(); - } -} - bool PhishingClassifier::is_ready() const { - return !!scorer_; + return !!ScorerStorage::GetInstance()->GetScorer(); } void PhishingClassifier::BeginClassification(const std::u16string* page_text, @@ -125,6 +105,13 @@ void PhishingClassifier::BeginClassification(const std::u16string* page_text, // classification so that we can start in a known state. CancelPendingClassification(); + Scorer* scorer = ScorerStorage::GetInstance()->GetScorer(); + url_extractor_ = std::make_unique<PhishingUrlFeatureExtractor>(); + dom_extractor_ = std::make_unique<PhishingDOMFeatureExtractor>(); + term_extractor_ = std::make_unique<PhishingTermFeatureExtractor>( + scorer->find_page_term_callback(), scorer->find_page_word_callback(), + scorer->max_words_per_term(), scorer->murmurhash3_seed(), + scorer->max_shingles_per_page(), scorer->shingle_size()); page_text_ = page_text; done_callback_ = std::move(done_callback); @@ -172,8 +159,8 @@ void PhishingClassifier::CancelPendingClassification() { // Note that cancelling the feature extractors is simply a no-op if they // were not running. DCHECK(is_ready()); - dom_extractor_->CancelPendingExtraction(); - term_extractor_->CancelPendingExtraction(); + dom_extractor_.reset(); + term_extractor_.reset(); weak_factory_.InvalidateWeakPtrs(); Clear(); } @@ -197,7 +184,7 @@ void PhishingClassifier::TermExtractionFinished(bool success) { #if BUILDFLAG(FULL_SAFE_BROWSING) ExtractVisualFeatures(); #else - if (scorer_->HasVisualTfLiteModel()) { + if (ScorerStorage::GetInstance()->GetScorer()->HasVisualTfLiteModel()) { ExtractVisualFeatures(); } else { VisualExtractionFinished(true); @@ -280,10 +267,12 @@ void PhishingClassifier::VisualExtractionFinished(bool success) { // Hash all of the features so that they match the model, then compute // the score. + Scorer* scorer = ScorerStorage::GetInstance()->GetScorer(); FeatureMap hashed_features; std::unique_ptr<ClientPhishingRequest> verdict = std::make_unique<ClientPhishingRequest>(); - verdict->set_model_version(scorer_->model_version()); + verdict->set_model_version(scorer->model_version()); + verdict->set_dom_model_version(scorer->dom_model_version()); verdict->set_url(main_frame->GetDocument().Url().GetString().Utf8()); for (const auto& it : features_->features()) { bool result = hashed_features.AddRealFeature( @@ -296,9 +285,9 @@ void PhishingClassifier::VisualExtractionFinished(bool success) { for (const auto& it : *shingle_hashes_) { verdict->add_shingle_hashes(it); } - float score = static_cast<float>(scorer_->ComputeScore(hashed_features)); + float score = static_cast<float>(scorer->ComputeScore(hashed_features)); verdict->set_client_score(score); - bool is_dom_match = (score >= scorer_->threshold_probability()); + bool is_dom_match = (score >= scorer->threshold_probability()); verdict->set_is_phishing(is_dom_match); verdict->set_is_dom_match(is_dom_match); if (visual_features_) { @@ -306,7 +295,7 @@ void PhishingClassifier::VisualExtractionFinished(bool success) { } #if BUILDFLAG(BUILD_WITH_TFLITE_LIB) - scorer_->ApplyVisualTfLiteModel( + ScorerStorage::GetInstance()->GetScorer()->ApplyVisualTfLiteModel( *bitmap_, base::BindOnce(&PhishingClassifier::OnVisualTfLiteModelDone, weak_factory_.GetWeakPtr(), std::move(verdict))); #else @@ -317,20 +306,21 @@ void PhishingClassifier::VisualExtractionFinished(bool success) { void PhishingClassifier::OnVisualTfLiteModelDone( std::unique_ptr<ClientPhishingRequest> verdict, std::vector<double> result) { - if (static_cast<int>(result.size()) > scorer_->tflite_thresholds().size()) { + Scorer* scorer = ScorerStorage::GetInstance()->GetScorer(); + if (static_cast<int>(result.size()) > scorer->tflite_thresholds().size()) { // Model is misconfigured, so bail out. RunFailureCallback(); return; } - verdict->set_tflite_model_version(scorer_->tflite_model_version()); + verdict->set_tflite_model_version(scorer->tflite_model_version()); for (size_t i = 0; i < result.size(); i++) { ClientPhishingRequest::CategoryScore* category = verdict->add_tflite_model_scores(); - category->set_label(scorer_->tflite_thresholds().at(i).label()); + category->set_label(scorer->tflite_thresholds().at(i).label()); category->set_value(result[i]); - if (result[i] >= scorer_->tflite_thresholds().at(i).threshold()) { + if (result[i] >= scorer->tflite_thresholds().at(i).threshold()) { verdict->set_is_phishing(true); verdict->set_is_tflite_match(true); } diff --git a/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_classifier.h b/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_classifier.h index add37ba47cb..5f44a8b5587 100644 --- a/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_classifier.h +++ b/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_classifier.h @@ -28,6 +28,7 @@ #include "base/callback.h" #include "base/memory/weak_ptr.h" #include "base/time/time.h" +#include "components/safe_browsing/content/renderer/phishing_classifier/scorer.h" #include "third_party/skia/include/core/SkBitmap.h" namespace content { @@ -66,12 +67,6 @@ class PhishingClassifier { virtual ~PhishingClassifier(); - // Sets a scorer for the classifier to use in computing the phishiness score. - // This must live at least as long as the PhishingClassifier. The caller is - // expected to cancel any pending classification before setting a phishing - // scorer. - void set_phishing_scorer(const Scorer* scorer); - // Returns true if the classifier is ready to classify pages, i.e. it // has had a scorer set via set_phishing_scorer(). bool is_ready() const; @@ -152,7 +147,6 @@ class PhishingClassifier { void Clear(); content::RenderFrame* render_frame_; // owns us - const Scorer* scorer_; // owned by the caller std::unique_ptr<PhishingUrlFeatureExtractor> url_extractor_; std::unique_ptr<PhishingDOMFeatureExtractor> dom_extractor_; std::unique_ptr<PhishingTermFeatureExtractor> term_extractor_; diff --git a/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_classifier_delegate.cc b/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_classifier_delegate.cc index 21e546c0a21..c894382c9c6 100644 --- a/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_classifier_delegate.cc +++ b/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_classifier_delegate.cc @@ -24,6 +24,7 @@ #include "content/public/renderer/render_thread.h" #include "mojo/public/cpp/bindings/self_owned_receiver.h" #include "services/service_manager/public/cpp/interface_provider.h" +#include "third_party/blink/public/common/associated_interfaces/associated_interface_registry.h" #include "third_party/blink/public/platform/web_url.h" #include "third_party/blink/public/web/web_document.h" #include "third_party/blink/public/web/web_local_frame.h" @@ -41,14 +42,6 @@ GURL StripRef(const GURL& url) { return url.ReplaceComponents(replacements); } -std::set<PhishingClassifierDelegate*>& PhishingClassifierDelegates() { - static base::NoDestructor<std::set<PhishingClassifierDelegate*>> s; - return *s; -} - -base::LazyInstance<std::unique_ptr<const safe_browsing::Scorer>>:: - DestructorAtExit g_phishing_scorer = LAZY_INSTANCE_INITIALIZER; - } // namespace PhishingClassifierDelegate::PhishingClassifierDelegate( @@ -58,57 +51,21 @@ PhishingClassifierDelegate::PhishingClassifierDelegate( last_main_frame_transition_(ui::PAGE_TRANSITION_LINK), have_page_text_(false), is_classifying_(false) { - PhishingClassifierDelegates().insert(this); if (!classifier) { classifier = new PhishingClassifier(render_frame); } classifier_.reset(classifier); - if (g_phishing_scorer.Get().get()) - SetPhishingScorer(g_phishing_scorer.Get().get()); - - registry_.AddInterface( + render_frame->GetAssociatedInterfaceRegistry()->AddInterface( base::BindRepeating(&PhishingClassifierDelegate::PhishingDetectorReceiver, base::Unretained(this))); + + model_change_observation_.Observe(ScorerStorage::GetInstance()); } PhishingClassifierDelegate::~PhishingClassifierDelegate() { CancelPendingClassification(SHUTDOWN); - PhishingClassifierDelegates().erase(this); -} - -void PhishingClassifierDelegate::SetPhishingModel( - const std::string& model, - base::File tflite_visual_model) { - safe_browsing::Scorer* scorer = nullptr; - // An empty model string means we should disable client-side phishing - // detection. - if (!model.empty()) { - scorer = safe_browsing::ProtobufModelScorer::Create( - model, std::move(tflite_visual_model)); - if (!scorer) - return; - } - for (auto* delegate : PhishingClassifierDelegates()) - delegate->SetPhishingScorer(scorer); - g_phishing_scorer.Get().reset(scorer); -} - -void PhishingClassifierDelegate::SetPhishingFlatBufferModel( - base::ReadOnlySharedMemoryRegion flatbuffer_region, - base::File tflite_visual_model) { - safe_browsing::Scorer* scorer = nullptr; - // An invalid region means we should disable client-side phishing detection. - if (flatbuffer_region.IsValid()) { - scorer = safe_browsing::FlatBufferModelScorer::Create( - std::move(flatbuffer_region), std::move(tflite_visual_model)); - if (!scorer) - return; - } - for (auto* delegate : PhishingClassifierDelegates()) - delegate->SetPhishingScorer(scorer); - g_phishing_scorer.Get().reset(scorer); } // static @@ -120,29 +77,10 @@ PhishingClassifierDelegate* PhishingClassifierDelegate::Create( return new PhishingClassifierDelegate(render_frame, classifier); } -void PhishingClassifierDelegate::SetPhishingScorer( - const safe_browsing::Scorer* scorer) { - if (is_classifying_) { - // If there is a classification going on right now it means we're - // actually replacing an existing scorer with a new model. In - // this case we simply cancel the current classification. - // TODO(noelutz): if this happens too frequently we could also - // replace the old scorer with the new one once classification is done - // but this would complicate the code somewhat. - CancelPendingClassification(NEW_PHISHING_SCORER); - } - classifier_->set_phishing_scorer(scorer); -} - void PhishingClassifierDelegate::PhishingDetectorReceiver( - mojo::PendingReceiver<mojom::PhishingDetector> receiver) { - phishing_detector_receivers_.Add(this, std::move(receiver)); -} - -void PhishingClassifierDelegate::OnInterfaceRequestForFrame( - const std::string& interface_name, - mojo::ScopedMessagePipeHandle* interface_pipe) { - registry_.TryBindInterface(interface_name, interface_pipe); + mojo::PendingAssociatedReceiver<mojom::PhishingDetector> receiver) { + phishing_detector_receiver_.reset(); + phishing_detector_receiver_.Bind(std::move(receiver)); } void PhishingClassifierDelegate::StartPhishingDetection( @@ -179,6 +117,10 @@ void PhishingClassifierDelegate::DidFinishSameDocumentNavigation() { CancelPendingClassification(NAVIGATE_WITHIN_PAGE); } +bool PhishingClassifierDelegate::is_ready() { + return classifier_->is_ready(); +} + void PhishingClassifierDelegate::PageCaptured(std::u16string* page_text, bool preliminary_capture) { RecordEvent(SBPhishingClassifierEvent::kPageTextCaptured); @@ -307,4 +249,13 @@ void PhishingClassifierDelegate::OnDestruct() { delete this; } +void PhishingClassifierDelegate::OnScorerChanged() { + if (is_classifying_) { + // If there is a classification going on right now it means we're + // actually replacing an existing scorer with a new model. In + // this case we simply cancel the current classification. + CancelPendingClassification(NEW_PHISHING_SCORER); + } +} + } // namespace safe_browsing diff --git a/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_classifier_delegate.h b/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_classifier_delegate.h index 02a315a2ca2..a7a7fc6bd24 100644 --- a/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_classifier_delegate.h +++ b/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_classifier_delegate.h @@ -11,9 +11,12 @@ #include <string> #include "base/memory/read_only_shared_memory_region.h" +#include "base/scoped_observation.h" #include "components/safe_browsing/content/common/safe_browsing.mojom.h" +#include "components/safe_browsing/content/renderer/phishing_classifier/scorer.h" #include "content/public/renderer/render_frame_observer.h" #include "content/public/renderer/render_thread_observer.h" +#include "mojo/public/cpp/bindings/associated_receiver.h" #include "mojo/public/cpp/bindings/pending_receiver.h" #include "mojo/public/cpp/bindings/receiver_set.h" #include "services/service_manager/public/cpp/binder_registry.h" @@ -39,7 +42,8 @@ enum class SBPhishingClassifierEvent { }; class PhishingClassifierDelegate : public content::RenderFrameObserver, - public mojom::PhishingDetector { + public mojom::PhishingDetector, + public ScorerStorage::Observer { public: // The RenderFrame owns us. This object takes ownership of the classifier. // Note that if classifier is null, a default instance of PhishingClassifier @@ -53,19 +57,6 @@ class PhishingClassifierDelegate : public content::RenderFrameObserver, ~PhishingClassifierDelegate() override; - // mojom::PhishingDetector - void SetPhishingModel(const std::string& model, - base::File tflite_visual_model) override; - - // mojom::PhishingDetector - void SetPhishingFlatBufferModel( - base::ReadOnlySharedMemoryRegion flatbuffer_region, - base::File tflite_visual_model) override; - - // Called by the RenderFrame once there is a phishing scorer available. - // The scorer is passed on to the classifier. - void SetPhishingScorer(const safe_browsing::Scorer* scorer); - // Called by the RenderFrame once a page has finished loading. Updates the // last-loaded URL and page text, then starts classification if all other // conditions are met (see MaybeStartClassification for details). @@ -83,6 +74,8 @@ class PhishingClassifierDelegate : public content::RenderFrameObserver, // committed. We continue running the current classification. void DidFinishSameDocumentNavigation() override; + bool is_ready(); + private: friend class PhishingClassifierDelegateTest; @@ -99,7 +92,7 @@ class PhishingClassifierDelegate : public content::RenderFrameObserver, }; void PhishingDetectorReceiver( - mojo::PendingReceiver<mojom::PhishingDetector> receiver); + mojo::PendingAssociatedReceiver<mojom::PhishingDetector> receiver); // Cancels any pending classification and frees the page text. void CancelPendingClassification(CancelClassificationReason reason); @@ -109,10 +102,6 @@ class PhishingClassifierDelegate : public content::RenderFrameObserver, void OnDestruct() override; - void OnInterfaceRequestForFrame( - const std::string& interface_name, - mojo::ScopedMessagePipeHandle* interface_pipe) override; - // mojom::PhishingDetector // Called by the RenderFrame when it receives a StartPhishingDetection IPC // from the browser. This signals that it is ok to begin classification @@ -128,6 +117,9 @@ class PhishingClassifierDelegate : public content::RenderFrameObserver, // Shared code to begin classification if all conditions are met. void MaybeStartClassification(); + // ScorerStorage::Observer implementation: + void OnScorerChanged() override; + // The PhishingClassifier to use for the RenderFrame. This is created once // a scorer is made available via SetPhishingScorer(). std::unique_ptr<PhishingClassifier> classifier_; @@ -173,9 +165,11 @@ class PhishingClassifierDelegate : public content::RenderFrameObserver, // The callback from the most recent call to StartPhishingDetection. StartPhishingDetectionCallback callback_; - mojo::ReceiverSet<mojom::PhishingDetector> phishing_detector_receivers_; + mojo::AssociatedReceiver<mojom::PhishingDetector> phishing_detector_receiver_{ + this}; - service_manager::BinderRegistry registry_; + base::ScopedObservation<ScorerStorage, ScorerStorage::Observer> + model_change_observation_{this}; }; } // namespace safe_browsing diff --git a/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_dom_feature_extractor.cc b/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_dom_feature_extractor.cc index b9ec8776ca4..50d0e92c7b8 100644 --- a/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_dom_feature_extractor.cc +++ b/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_dom_feature_extractor.cc @@ -109,11 +109,7 @@ PhishingDOMFeatureExtractor::PhishingDOMFeatureExtractor() } PhishingDOMFeatureExtractor::~PhishingDOMFeatureExtractor() { - // The RenderView should have called CancelPendingExtraction() before - // we are destroyed. - DCHECK(done_callback_.is_null()); - DCHECK(!cur_frame_data_.get()); - DCHECK(cur_document_.IsNull()); + CancelPendingExtraction(); } void PhishingDOMFeatureExtractor::ExtractFeatures(blink::WebDocument document, diff --git a/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_model_setter_impl.cc b/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_model_setter_impl.cc new file mode 100644 index 00000000000..5f1521fbb58 --- /dev/null +++ b/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_model_setter_impl.cc @@ -0,0 +1,81 @@ +// Copyright 2022 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 "components/safe_browsing/content/renderer/phishing_classifier/phishing_model_setter_impl.h" + +#include "components/safe_browsing/content/renderer/phishing_classifier/flatbuffer_scorer.h" +#include "components/safe_browsing/content/renderer/phishing_classifier/protobuf_scorer.h" +#include "components/safe_browsing/content/renderer/phishing_classifier/scorer.h" +#include "third_party/blink/public/common/associated_interfaces/associated_interface_registry.h" + +namespace safe_browsing { + +PhishingModelSetterImpl::PhishingModelSetterImpl() = default; +PhishingModelSetterImpl::~PhishingModelSetterImpl() = default; + +void PhishingModelSetterImpl::RegisterMojoInterfaces( + blink::AssociatedInterfaceRegistry* associated_interfaces) { + associated_interfaces->AddInterface( + base::BindRepeating(&PhishingModelSetterImpl::OnRendererAssociatedRequest, + base::Unretained(this))); +} + +void PhishingModelSetterImpl::UnregisterMojoInterfaces( + blink::AssociatedInterfaceRegistry* associated_interfaces) { + associated_interfaces->RemoveInterface(mojom::PhishingModelSetter::Name_); +} + +void PhishingModelSetterImpl::SetPhishingModel(const std::string& model, + base::File tflite_visual_model) { + std::unique_ptr<Scorer> scorer; + + // An empty model string means we should disable client-side phishing + // detection. + if (!model.empty()) { + scorer = safe_browsing::ProtobufModelScorer::Create( + model, std::move(tflite_visual_model)); + if (!scorer) + return; + } + ScorerStorage::GetInstance()->SetScorer(std::move(scorer)); + + if (observer_for_testing_.is_bound()) { + observer_for_testing_->PhishingModelUpdated(); + } +} + +void PhishingModelSetterImpl::SetPhishingFlatBufferModel( + base::ReadOnlySharedMemoryRegion flatbuffer_region, + base::File tflite_visual_model) { + std::unique_ptr<Scorer> scorer; + // An invalid region means we should disable client-side phishing detection. + if (flatbuffer_region.IsValid()) { + scorer = safe_browsing::FlatBufferModelScorer::Create( + std::move(flatbuffer_region), std::move(tflite_visual_model)); + if (!scorer) + return; + } + ScorerStorage::GetInstance()->SetScorer(std::move(scorer)); + + if (observer_for_testing_.is_bound()) { + observer_for_testing_->PhishingModelUpdated(); + } +} + +void PhishingModelSetterImpl::SetTestObserver( + mojo::PendingRemote<mojom::PhishingModelSetterTestObserver> observer, + SetTestObserverCallback callback) { + if (observer_for_testing_.is_bound()) + observer_for_testing_.reset(); + observer_for_testing_.Bind(std::move(observer)); + std::move(callback).Run(); +} + +void PhishingModelSetterImpl::OnRendererAssociatedRequest( + mojo::PendingAssociatedReceiver<mojom::PhishingModelSetter> receiver) { + receiver_.reset(); + receiver_.Bind(std::move(receiver)); +} + +} // namespace safe_browsing diff --git a/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_model_setter_impl.h b/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_model_setter_impl.h new file mode 100644 index 00000000000..99edf2aafb0 --- /dev/null +++ b/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_model_setter_impl.h @@ -0,0 +1,51 @@ +// Copyright 2022 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. + +#ifndef COMPONENTS_SAFE_BROWSING_CONTENT_RENDERER_PHISHING_CLASSIFIER_PHISHING_MODEL_SETTER_IMPL_H_ +#define COMPONENTS_SAFE_BROWSING_CONTENT_RENDERER_PHISHING_CLASSIFIER_PHISHING_MODEL_SETTER_IMPL_H_ + +#include "components/safe_browsing/content/common/safe_browsing.mojom.h" +#include "content/public/renderer/render_thread_observer.h" +#include "mojo/public/cpp/bindings/associated_receiver.h" +#include "mojo/public/cpp/bindings/remote.h" + +namespace safe_browsing { + +class PhishingModelSetterImpl : public mojom::PhishingModelSetter, + public content::RenderThreadObserver { + public: + PhishingModelSetterImpl(); + + PhishingModelSetterImpl(const PhishingModelSetterImpl&) = delete; + PhishingModelSetterImpl& operator=(const PhishingModelSetterImpl&) = delete; + + ~PhishingModelSetterImpl() override; + + private: + // content::RenderThreadObserver overrides: + void RegisterMojoInterfaces( + blink::AssociatedInterfaceRegistry* associated_interfaces) override; + void UnregisterMojoInterfaces( + blink::AssociatedInterfaceRegistry* associated_interfaces) override; + + // mojom::PhishingModelSetter overrides: + void SetPhishingModel(const std::string& model, + base::File tflite_visual_model) override; + void SetPhishingFlatBufferModel( + base::ReadOnlySharedMemoryRegion flatbuffer_region, + base::File tflite_visual_model) override; + void SetTestObserver( + mojo::PendingRemote<mojom::PhishingModelSetterTestObserver> observer, + SetTestObserverCallback callback) override; + + void OnRendererAssociatedRequest( + mojo::PendingAssociatedReceiver<mojom::PhishingModelSetter> receiver); + + mojo::Remote<mojom::PhishingModelSetterTestObserver> observer_for_testing_; + + mojo::AssociatedReceiver<mojom::PhishingModelSetter> receiver_{this}; +}; +} // namespace safe_browsing + +#endif // COMPONENTS_SAFE_BROWSING_CONTENT_RENDERER_PHISHING_CLASSIFIER_PHISHING_MODEL_SETTER_IMPL_H_ diff --git a/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_term_feature_extractor.cc b/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_term_feature_extractor.cc index 18eb048a822..f96720515ef 100644 --- a/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_term_feature_extractor.cc +++ b/chromium/components/safe_browsing/content/renderer/phishing_classifier/phishing_term_feature_extractor.cc @@ -96,10 +96,7 @@ PhishingTermFeatureExtractor::PhishingTermFeatureExtractor( } PhishingTermFeatureExtractor::~PhishingTermFeatureExtractor() { - // The RenderView should have called CancelPendingExtraction() before - // we are destroyed. - DCHECK(done_callback_.is_null()); - DCHECK(!state_.get()); + CancelPendingExtraction(); } void PhishingTermFeatureExtractor::ExtractFeatures( diff --git a/chromium/components/safe_browsing/content/renderer/phishing_classifier/protobuf_scorer.cc b/chromium/components/safe_browsing/content/renderer/phishing_classifier/protobuf_scorer.cc index cf6e038164a..1ca421ec550 100644 --- a/chromium/components/safe_browsing/content/renderer/phishing_classifier/protobuf_scorer.cc +++ b/chromium/components/safe_browsing/content/renderer/phishing_classifier/protobuf_scorer.cc @@ -41,7 +41,7 @@ ProtobufModelScorer::ProtobufModelScorer() = default; ProtobufModelScorer::~ProtobufModelScorer() = default; /* static */ -ProtobufModelScorer* ProtobufModelScorer::Create( +std::unique_ptr<ProtobufModelScorer> ProtobufModelScorer::Create( const base::StringPiece& model_str, base::File visual_tflite_model) { std::unique_ptr<ProtobufModelScorer> scorer(new ProtobufModelScorer()); @@ -86,7 +86,7 @@ ProtobufModelScorer* ProtobufModelScorer::Create( } RecordScorerCreationStatus(SCORER_SUCCESS); - return scorer.release(); + return scorer; } double ProtobufModelScorer::ComputeScore(const FeatureMap& features) const { @@ -127,6 +127,10 @@ int ProtobufModelScorer::model_version() const { return model_.version(); } +int ProtobufModelScorer::dom_model_version() const { + return model_.dom_model_version(); +} + bool Scorer::HasVisualTfLiteModel() const { return visual_tflite_model_.IsValid(); } diff --git a/chromium/components/safe_browsing/content/renderer/phishing_classifier/protobuf_scorer.h b/chromium/components/safe_browsing/content/renderer/phishing_classifier/protobuf_scorer.h index db968ac8dee..4243d3ee20f 100644 --- a/chromium/components/safe_browsing/content/renderer/phishing_classifier/protobuf_scorer.h +++ b/chromium/components/safe_browsing/content/renderer/phishing_classifier/protobuf_scorer.h @@ -41,8 +41,9 @@ class ProtobufModelScorer : public Scorer { // Factory method which creates a new Scorer object by parsing the given // model. If parsing fails this method returns NULL. // Can use this if model_str is empty. - static ProtobufModelScorer* Create(const base::StringPiece& model_str, - base::File visual_tflite_model); + static std::unique_ptr<ProtobufModelScorer> Create( + const base::StringPiece& model_str, + base::File visual_tflite_model); double ComputeScore(const FeatureMap& features) const override; @@ -53,6 +54,7 @@ class ProtobufModelScorer : public Scorer { #endif int model_version() const override; + int dom_model_version() const override; base::RepeatingCallback<bool(uint32_t)> find_page_word_callback() const override; base::RepeatingCallback<bool(const std::string&)> find_page_term_callback() diff --git a/chromium/components/safe_browsing/content/renderer/phishing_classifier/scorer.cc b/chromium/components/safe_browsing/content/renderer/phishing_classifier/scorer.cc index 3fd872456b9..3c6899f40b7 100644 --- a/chromium/components/safe_browsing/content/renderer/phishing_classifier/scorer.cc +++ b/chromium/components/safe_browsing/content/renderer/phishing_classifier/scorer.cc @@ -10,6 +10,7 @@ #include <unordered_map> #include <unordered_set> +#include "base/logging.h" #include "base/memory/read_only_shared_memory_region.h" #include "base/memory/shared_memory_mapping.h" #include "base/metrics/histogram_functions.h" @@ -82,8 +83,13 @@ std::unique_ptr<tflite::task::vision::ImageClassifier> CreateClassifier( std::string model_data) { TRACE_EVENT0("safe_browsing", "CreateTfLiteClassifier"); tflite::task::vision::ImageClassifierOptions options; - options.mutable_model_file_with_metadata()->set_file_content( - std::move(model_data)); + tflite::task::core::BaseOptions* base_options = + options.mutable_base_options(); + base_options->mutable_model_file()->set_file_content(std::move(model_data)); + base_options->mutable_compute_settings() + ->mutable_tflite_settings() + ->mutable_cpu_settings() + ->set_num_threads(1); auto statusor_classifier = tflite::task::vision::ImageClassifier::CreateFromOptions( options, CreateOpResolver()); @@ -189,17 +195,14 @@ void Scorer::ApplyVisualTfLiteModelHelper( const SkBitmap& bitmap, int input_width, int input_height, - const std::string& model_data, + std::string model_data, scoped_refptr<base::SequencedTaskRunner> callback_task_runner, base::OnceCallback<void(std::vector<double>)> callback) { TRACE_EVENT0("safe_browsing", "ApplyVisualTfLiteModel"); base::Time before_operation = base::Time::Now(); - std::string model_data_copy = model_data; - base::UmaHistogramTimes("SBClientPhishing.ApplyTfliteTime.ModelCopy", - base::Time::Now() - before_operation); before_operation = base::Time::Now(); std::unique_ptr<tflite::task::vision::ImageClassifier> classifier = - CreateClassifier(std::move(model_data_copy)); + CreateClassifier(std::move(model_data)); base::UmaHistogramTimes("SBClientPhishing.ApplyTfliteTime.CreateClassifier", base::Time::Now() - before_operation); if (!classifier) { @@ -231,4 +234,31 @@ double Scorer::LogOdds2Prob(double log_odds) { Scorer::Scorer() = default; Scorer::~Scorer() = default; +// static +ScorerStorage* ScorerStorage::GetInstance() { + static base::NoDestructor<ScorerStorage> instance; + return instance.get(); +} + +ScorerStorage::ScorerStorage() = default; +ScorerStorage::~ScorerStorage() = default; + +void ScorerStorage::SetScorer(std::unique_ptr<Scorer> scorer) { + scorer_ = std::move(scorer); + for (Observer& obs : observers_) + obs.OnScorerChanged(); +} + +Scorer* ScorerStorage::GetScorer() const { + return scorer_.get(); +} + +void ScorerStorage::AddObserver(ScorerStorage::Observer* observer) { + observers_.AddObserver(observer); +} + +void ScorerStorage::RemoveObserver(ScorerStorage::Observer* observer) { + observers_.RemoveObserver(observer); +} + } // namespace safe_browsing diff --git a/chromium/components/safe_browsing/content/renderer/phishing_classifier/scorer.h b/chromium/components/safe_browsing/content/renderer/phishing_classifier/scorer.h index 0cbb1ca69f0..1461ed23192 100644 --- a/chromium/components/safe_browsing/content/renderer/phishing_classifier/scorer.h +++ b/chromium/components/safe_browsing/content/renderer/phishing_classifier/scorer.h @@ -26,6 +26,7 @@ #include "base/files/memory_mapped_file.h" #include "base/memory/weak_ptr.h" #include "base/metrics/histogram_macros.h" +#include "base/observer_list.h" #include "base/strings/string_piece.h" #include "build/build_config.h" #include "components/optimization_guide/machine_learning_tflite_buildflags.h" @@ -80,6 +81,8 @@ class Scorer { // Returns the version number of the loaded client model. virtual int model_version() const = 0; + virtual int dom_model_version() const = 0; + bool HasVisualTfLiteModel() const; // -- Accessors used by the page feature extractor --------------------------- @@ -131,7 +134,7 @@ class Scorer { const SkBitmap& bitmap, int input_width, int input_height, - const std::string& model_data, + std::string model_data, scoped_refptr<base::SequencedTaskRunner> callback_task_runner, base::OnceCallback<void(std::vector<double>)> callback); @@ -142,6 +145,33 @@ class Scorer { friend class PhishingScorerTest; }; +// A small wrapper around a Scorer that allows callers to observe for changes in +// the model. +class ScorerStorage { + public: + static ScorerStorage* GetInstance(); + + class Observer : public base::CheckedObserver { + public: + virtual void OnScorerChanged() = 0; + }; + + ScorerStorage(); + ~ScorerStorage(); + ScorerStorage(const ScorerStorage&) = delete; + ScorerStorage& operator=(const ScorerStorage&) = delete; + + void SetScorer(std::unique_ptr<Scorer> scorer); + Scorer* GetScorer() const; + + void AddObserver(Observer* observer); + void RemoveObserver(Observer* observer); + + private: + std::unique_ptr<Scorer> scorer_; + base::ObserverList<Observer> observers_; +}; + } // namespace safe_browsing #endif // COMPONENTS_SAFE_BROWSING_CONTENT_RENDERER_PHISHING_CLASSIFIER_SCORER_H_ diff --git a/chromium/components/safe_browsing/content/renderer/phishing_classifier/scorer_unittest.cc b/chromium/components/safe_browsing/content/renderer/phishing_classifier/scorer_unittest.cc index 559484d1493..7dcd9f30c3f 100644 --- a/chromium/components/safe_browsing/content/renderer/phishing_classifier/scorer_unittest.cc +++ b/chromium/components/safe_browsing/content/renderer/phishing_classifier/scorer_unittest.cc @@ -84,6 +84,7 @@ std::string GetFlatBufferString() { csd_model_builder.add_max_shingles_per_page(10); csd_model_builder.add_shingle_size(3); csd_model_builder.add_tflite_metadata(tflite_metadata_flat); + csd_model_builder.add_dom_model_version(123); builder.Finish(csd_model_builder.Finish()); return std::string(reinterpret_cast<char*>(builder.GetBufferPointer()), @@ -139,6 +140,7 @@ class PhishingScorerTest : public ::testing::Test { model_.set_murmur_hash_seed(12345U); model_.set_max_shingles_per_page(10); model_.set_shingle_size(3); + model_.set_dom_model_version(123); } void TearDown() override { @@ -153,36 +155,36 @@ TEST_F(PhishingScorerTest, HasValidFlatBufferModel) { std::string flatbuffer = GetFlatBufferString(); base::MappedReadOnlyRegion mapped_region = GetMappedReadOnlyRegionWithData(flatbuffer); - scorer.reset(FlatBufferModelScorer::Create(mapped_region.region.Duplicate(), - base::File())); + scorer = FlatBufferModelScorer::Create(mapped_region.region.Duplicate(), + base::File()); EXPECT_TRUE(scorer.get() != nullptr); // Invalid region. - scorer.reset(FlatBufferModelScorer::Create(base::ReadOnlySharedMemoryRegion(), - base::File())); + scorer = FlatBufferModelScorer::Create(base::ReadOnlySharedMemoryRegion(), + base::File()); EXPECT_FALSE(scorer.get()); // Invalid buffer in region. mapped_region = GetMappedReadOnlyRegionWithData("bogus string"); - scorer.reset(FlatBufferModelScorer::Create(mapped_region.region.Duplicate(), - base::File())); + scorer = FlatBufferModelScorer::Create(mapped_region.region.Duplicate(), + base::File()); EXPECT_FALSE(scorer.get()); } TEST_F(PhishingScorerTest, HasValidModel) { std::unique_ptr<Scorer> scorer; - scorer.reset( - ProtobufModelScorer::Create(model_.SerializeAsString(), base::File())); + scorer = + ProtobufModelScorer::Create(model_.SerializeAsString(), base::File()); EXPECT_TRUE(scorer.get() != nullptr); // Invalid model string. - scorer.reset(ProtobufModelScorer::Create("bogus string", base::File())); + scorer = ProtobufModelScorer::Create("bogus string", base::File()); EXPECT_FALSE(scorer.get()); // Mode is missing a required field. model_.clear_max_words_per_term(); - scorer.reset(ProtobufModelScorer::Create(model_.SerializePartialAsString(), - base::File())); + scorer = ProtobufModelScorer::Create(model_.SerializePartialAsString(), + base::File()); EXPECT_FALSE(scorer.get()); } @@ -221,8 +223,8 @@ TEST_F(PhishingScorerTest, PageTermsFlat) { std::string flatbuffer = GetFlatBufferString(); base::MappedReadOnlyRegion mapped_region = GetMappedReadOnlyRegionWithData(flatbuffer); - scorer.reset(FlatBufferModelScorer::Create(mapped_region.region.Duplicate(), - base::File())); + scorer = FlatBufferModelScorer::Create(mapped_region.region.Duplicate(), + base::File()); ASSERT_TRUE(scorer.get()); base::RepeatingCallback<bool(const std::string&)> page_terms_callback( scorer->find_page_term_callback()); @@ -271,8 +273,8 @@ TEST_F(PhishingScorerTest, PageWordsFlat) { std::string flatbuffer = GetFlatBufferString(); base::MappedReadOnlyRegion mapped_region = GetMappedReadOnlyRegionWithData(flatbuffer); - scorer.reset(FlatBufferModelScorer::Create(mapped_region.region.Duplicate(), - base::File())); + scorer = FlatBufferModelScorer::Create(mapped_region.region.Duplicate(), + base::File()); ASSERT_TRUE(scorer.get()); base::RepeatingCallback<bool(uint32_t)> page_words_callback( scorer->find_page_word_callback()); @@ -321,8 +323,8 @@ TEST_F(PhishingScorerTest, ComputeScoreFlat) { std::string flatbuffer = GetFlatBufferString(); base::MappedReadOnlyRegion mapped_region = GetMappedReadOnlyRegionWithData(flatbuffer); - scorer.reset(FlatBufferModelScorer::Create(mapped_region.region.Duplicate(), - base::File())); + scorer = FlatBufferModelScorer::Create(mapped_region.region.Duplicate(), + base::File()); EXPECT_TRUE(scorer.get() != nullptr); // An empty feature map should match the empty rule. @@ -348,4 +350,23 @@ TEST_F(PhishingScorerTest, ComputeScoreFlat) { EXPECT_DOUBLE_EQ(0.77729986117469119, scorer->ComputeScore(features)); } +TEST_F(PhishingScorerTest, DomModelVersionProtobuffer) { + std::unique_ptr<Scorer> scorer; + scorer = + ProtobufModelScorer::Create(model_.SerializeAsString(), base::File()); + ASSERT_TRUE(scorer.get() != nullptr); + EXPECT_EQ(scorer->dom_model_version(), 123); +} + +TEST_F(PhishingScorerTest, DomModelVersionFlatbuffer) { + std::unique_ptr<Scorer> scorer; + std::string flatbuffer = GetFlatBufferString(); + base::MappedReadOnlyRegion mapped_region = + GetMappedReadOnlyRegionWithData(flatbuffer); + scorer = FlatBufferModelScorer::Create(mapped_region.region.Duplicate(), + base::File()); + ASSERT_TRUE(scorer.get() != nullptr); + EXPECT_EQ(scorer->dom_model_version(), 123); +} + } // namespace safe_browsing diff --git a/chromium/components/safe_browsing/content/resources/download_file_types.asciipb b/chromium/components/safe_browsing/content/resources/download_file_types.asciipb index 05a2ba86311..8dbb6c66a63 100644 --- a/chromium/components/safe_browsing/content/resources/download_file_types.asciipb +++ b/chromium/components/safe_browsing/content/resources/download_file_types.asciipb @@ -8,7 +8,7 @@ ## ## Top level settings ## -version_id: 50 +version_id: 51 sampled_ping_probability: 0.01 max_archived_binaries_to_report: 10 default_file_type { @@ -2863,6 +2863,39 @@ file_types { auto_open_hint: ALLOW_AUTO_OPEN } } +file_types { + # Windows troubleshooting component + extension: "diagcab" + uma_value: 399 + ping_setting: FULL_PING + platform_settings { + platform: PLATFORM_WINDOWS + danger_level: ALLOW_ON_USER_GESTURE + auto_open_hint: DISALLOW_AUTO_OPEN + } +} +file_types { + # Windows troubleshooting + extension: "diagcfg" + uma_value: 400 + ping_setting: FULL_PING + platform_settings { + platform: PLATFORM_WINDOWS + danger_level: ALLOW_ON_USER_GESTURE + auto_open_hint: DISALLOW_AUTO_OPEN + } +} +file_types { + # Windows troubleshooting component + extension: "diagpkg" + uma_value: 401 + ping_setting: FULL_PING + platform_settings { + platform: PLATFORM_WINDOWS + danger_level: ALLOW_ON_USER_GESTURE + auto_open_hint: DISALLOW_AUTO_OPEN + } +} ## ## MacOS-specific files diff --git a/chromium/components/safe_browsing/content/resources/download_file_types_experiment.asciipb b/chromium/components/safe_browsing/content/resources/download_file_types_experiment.asciipb index 615c1cf3e9a..08a5047187e 100644 --- a/chromium/components/safe_browsing/content/resources/download_file_types_experiment.asciipb +++ b/chromium/components/safe_browsing/content/resources/download_file_types_experiment.asciipb @@ -12,7 +12,7 @@ ## version id is larger than the version id in download_file_types.asciipb. If ## there isn't an ongoing experiment, this version id is equal to the version id ## in download_file_types.asciipb. -version_id: 50 +version_id: 51 sampled_ping_probability: 0.01 max_archived_binaries_to_report: 10 default_file_type { @@ -2867,6 +2867,39 @@ file_types { auto_open_hint: ALLOW_AUTO_OPEN } } +file_types { + # Windows troubleshooting component + extension: "diagcab" + uma_value: 399 + ping_setting: FULL_PING + platform_settings { + platform: PLATFORM_WINDOWS + danger_level: ALLOW_ON_USER_GESTURE + auto_open_hint: DISALLOW_AUTO_OPEN + } +} +file_types { + # Windows troubleshooting + extension: "diagcfg" + uma_value: 400 + ping_setting: FULL_PING + platform_settings { + platform: PLATFORM_WINDOWS + danger_level: ALLOW_ON_USER_GESTURE + auto_open_hint: DISALLOW_AUTO_OPEN + } +} +file_types { + # Windows troubleshooting component + extension: "diagpkg" + uma_value: 401 + ping_setting: FULL_PING + platform_settings { + platform: PLATFORM_WINDOWS + danger_level: ALLOW_ON_USER_GESTURE + auto_open_hint: DISALLOW_AUTO_OPEN + } +} ## ## MacOS-specific files diff --git a/chromium/components/safe_browsing/core/browser/BUILD.gn b/chromium/components/safe_browsing/core/browser/BUILD.gn index d931494c494..f07e6908221 100644 --- a/chromium/components/safe_browsing/core/browser/BUILD.gn +++ b/chromium/components/safe_browsing/core/browser/BUILD.gn @@ -153,6 +153,20 @@ source_set("token_fetcher_unit_tests") { ] } +source_set("token_fetcher_testing_helper") { + testonly = true + sources = [ + "test_safe_browsing_token_fetcher.cc", + "test_safe_browsing_token_fetcher.h", + ] + + deps = [ + ":token_fetcher", + "//base", + "//testing/gmock", + ] +} + source_set("download_check_result") { sources = [ "download_check_result.h" ] } diff --git a/chromium/components/safe_browsing/core/browser/db/database_manager.cc b/chromium/components/safe_browsing/core/browser/db/database_manager.cc index b87902dd2cb..3ded7349599 100644 --- a/chromium/components/safe_browsing/core/browser/db/database_manager.cc +++ b/chromium/components/safe_browsing/core/browser/db/database_manager.cc @@ -150,6 +150,11 @@ void SafeBrowsingDatabaseManager::NotifyDatabaseUpdateFinished() { update_complete_callback_list_.Notify(); } +bool SafeBrowsingDatabaseManager::IsDatabaseReady() { + DCHECK(io_task_runner()->RunsTasksInCurrentSequence()); + return enabled_; +} + SafeBrowsingDatabaseManager::SafeBrowsingApiCheck::SafeBrowsingApiCheck( const GURL& url, Client* client) diff --git a/chromium/components/safe_browsing/core/browser/db/database_manager.h b/chromium/components/safe_browsing/core/browser/db/database_manager.h index 16e3ae675b6..71cb8c4ba9b 100644 --- a/chromium/components/safe_browsing/core/browser/db/database_manager.h +++ b/chromium/components/safe_browsing/core/browser/db/database_manager.h @@ -233,10 +233,6 @@ class SafeBrowsingDatabaseManager // Returns whether download protection is enabled. virtual bool IsDownloadProtectionEnabled() const = 0; - // Returns true if URL-checking is supported on this build+device. - // If false, calls to CheckBrowseUrl may dcheck-fail. - virtual bool IsSupported() const = 0; - // // Methods to indicate when to start or suspend the SafeBrowsing operations. // These functions are always called on the IO thread. @@ -267,6 +263,9 @@ class SafeBrowsingDatabaseManager // method at the bottom of it. virtual void StopOnIOThread(bool shutdown); + // Called to check if database is ready or not. + virtual bool IsDatabaseReady(); + protected: // Bundled client info for an API abuse hash prefix check. class SafeBrowsingApiCheck { diff --git a/chromium/components/safe_browsing/core/browser/db/fake_database_manager.cc b/chromium/components/safe_browsing/core/browser/db/fake_database_manager.cc index 18265d4b36a..24fab78bfd8 100644 --- a/chromium/components/safe_browsing/core/browser/db/fake_database_manager.cc +++ b/chromium/components/safe_browsing/core/browser/db/fake_database_manager.cc @@ -94,10 +94,6 @@ safe_browsing::ThreatSource FakeSafeBrowsingDatabaseManager::GetThreatSource() return safe_browsing::ThreatSource::LOCAL_PVER4; } -bool FakeSafeBrowsingDatabaseManager::IsSupported() const { - return true; -} - // static void FakeSafeBrowsingDatabaseManager::CheckBrowseURLAsync( GURL url, diff --git a/chromium/components/safe_browsing/core/browser/db/fake_database_manager.h b/chromium/components/safe_browsing/core/browser/db/fake_database_manager.h index 23d64c88d0b..cca570959d8 100644 --- a/chromium/components/safe_browsing/core/browser/db/fake_database_manager.h +++ b/chromium/components/safe_browsing/core/browser/db/fake_database_manager.h @@ -36,7 +36,6 @@ class FakeSafeBrowsingDatabaseManager : public TestSafeBrowsingDatabaseManager { Client* client) override; bool CheckUrlForSubresourceFilter(const GURL& url, Client* client) override; safe_browsing::ThreatSource GetThreatSource() const override; - bool IsSupported() const override; private: ~FakeSafeBrowsingDatabaseManager() override; diff --git a/chromium/components/safe_browsing/core/browser/db/prefix_iterator.h b/chromium/components/safe_browsing/core/browser/db/prefix_iterator.h index cd243154dde..0d065ac6514 100644 --- a/chromium/components/safe_browsing/core/browser/db/prefix_iterator.h +++ b/chromium/components/safe_browsing/core/browser/db/prefix_iterator.h @@ -16,12 +16,13 @@ namespace safe_browsing { // The prefix iterator is used to binary search within a |HashPrefixes|. It is // essentially a random access iterator that steps |PrefixSize| steps within the // underlying buffer. -class PrefixIterator - : public std::iterator<std::random_access_iterator_tag, base::StringPiece> { +class PrefixIterator { public: - using difference_type = - typename std::iterator<std::random_access_iterator_tag, - base::StringPiece>::difference_type; + using iterator_category = std::random_access_iterator_tag; + using value_type = base::StringPiece; + using difference_type = std::ptrdiff_t; + using pointer = base::StringPiece*; + using reference = base::StringPiece&; PrefixIterator(base::StringPiece prefixes, size_t index, size_t size); PrefixIterator(const PrefixIterator& rhs); diff --git a/chromium/components/safe_browsing/core/browser/db/test_database_manager.cc b/chromium/components/safe_browsing/core/browser/db/test_database_manager.cc index b4099616098..08610aa508b 100644 --- a/chromium/components/safe_browsing/core/browser/db/test_database_manager.cc +++ b/chromium/components/safe_browsing/core/browser/db/test_database_manager.cc @@ -117,11 +117,6 @@ bool TestSafeBrowsingDatabaseManager::IsDownloadProtectionEnabled() const { return false; } -bool TestSafeBrowsingDatabaseManager::IsSupported() const { - NOTIMPLEMENTED(); - return false; -} - void TestSafeBrowsingDatabaseManager::StartOnIOThread( scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, const V4ProtocolConfig& config) { diff --git a/chromium/components/safe_browsing/core/browser/db/test_database_manager.h b/chromium/components/safe_browsing/core/browser/db/test_database_manager.h index ffa46a88a1a..a4d74b45cd7 100644 --- a/chromium/components/safe_browsing/core/browser/db/test_database_manager.h +++ b/chromium/components/safe_browsing/core/browser/db/test_database_manager.h @@ -48,7 +48,6 @@ class TestSafeBrowsingDatabaseManager : public SafeBrowsingDatabaseManager { bool MatchMalwareIP(const std::string& ip_address) override; safe_browsing::ThreatSource GetThreatSource() const override; bool IsDownloadProtectionEnabled() const override; - bool IsSupported() const override; void StartOnIOThread( scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, const V4ProtocolConfig& config) override; diff --git a/chromium/components/safe_browsing/core/browser/db/v4_database_unittest.cc b/chromium/components/safe_browsing/core/browser/db/v4_database_unittest.cc index 900b9339a70..67ae5760ece 100644 --- a/chromium/components/safe_browsing/core/browser/db/v4_database_unittest.cc +++ b/chromium/components/safe_browsing/core/browser/db/v4_database_unittest.cc @@ -32,7 +32,7 @@ class FakeV4Store : public V4Store { return hash_prefix_should_match_ ? full_hash : HashPrefix(); } - bool HasValidData() const override { return true; } + bool HasValidData() override { return true; } void set_hash_prefix_matches(bool hash_prefix_matches) { hash_prefix_should_match_ = hash_prefix_matches; diff --git a/chromium/components/safe_browsing/core/browser/db/v4_get_hash_protocol_manager.cc b/chromium/components/safe_browsing/core/browser/db/v4_get_hash_protocol_manager.cc index e7ffe69cda8..221cb5c7e8a 100644 --- a/chromium/components/safe_browsing/core/browser/db/v4_get_hash_protocol_manager.cc +++ b/chromium/components/safe_browsing/core/browser/db/v4_get_hash_protocol_manager.cc @@ -11,6 +11,7 @@ #include "base/bind.h" #include "base/containers/contains.h" #include "base/memory/ptr_util.h" +#include "base/metrics/histogram_functions.h" #include "base/metrics/histogram_macros.h" #include "base/strings/string_split.h" #include "base/timer/timer.h" @@ -38,6 +39,12 @@ void RecordGetHashResult(safe_browsing::V4OperationResult result) { safe_browsing::V4OperationResult::OPERATION_RESULT_MAX); } +// Record a backoff error count +void RecordBackoffErrorCountResult(size_t count) { + base::UmaHistogramCounts100( + "SafeBrowsing.V4GetHash.Result.BackoffErrorCount", count); +} + // Enumerate parsing failures for histogramming purposes. DO NOT CHANGE // THE ORDERING OF THESE VALUES. enum ParseResultType { @@ -124,6 +131,12 @@ void RecordV4GetHashCheckResult(V4GetHashCheckResultType result_type) { GET_HASH_CHECK_RESULT_MAX); } +bool ErrorIsRetriable(int net_error, int http_error) { + return (net_error == net::ERR_INTERNET_DISCONNECTED || + net_error == net::ERR_NETWORK_CHANGED) && + http_error != net::HTTP_OK; +} + const char kPermission[] = "permission"; const char kPhaPatternType[] = "pha_pattern_type"; const char kMalwareThreatType[] = "malware_threat_type"; @@ -297,6 +310,7 @@ void V4GetHashProtocolManager::GetFullHashes( if (clock_->Now() <= next_gethash_time_) { if (gethash_error_count_) { RecordGetHashResult(V4OperationResult::BACKOFF_ERROR); + backoff_error_count_++; } else { RecordGetHashResult(V4OperationResult::MIN_WAIT_DURATION_ERROR); } @@ -708,6 +722,7 @@ void V4GetHashProtocolManager::ParseMetadata(const ThreatMatch& match, void V4GetHashProtocolManager::ResetGetHashErrors() { gethash_error_count_ = 0; gethash_back_off_mult_ = 1; + backoff_error_count_ = 0; next_gethash_time_ = base::Time(); } @@ -802,11 +817,18 @@ void V4GetHashProtocolManager::OnURLLoaderCompleteInternal( Time negative_cache_expire; if (net_error == net::OK && response_code == net::HTTP_OK) { RecordGetHashResult(V4OperationResult::STATUS_200); + if (gethash_error_count_) RecordBackoffErrorCountResult(backoff_error_count_); ResetGetHashErrors(); if (!ParseHashResponse(data, &full_hash_infos, &negative_cache_expire)) { full_hash_infos.clear(); RecordGetHashResult(V4OperationResult::PARSE_ERROR); } + } else if (ErrorIsRetriable(net_error, response_code)) { + if (net_error != net::OK) { + RecordGetHashResult(V4OperationResult::RETRIABLE_NETWORK_ERROR); + } else { + RecordGetHashResult(V4OperationResult::RETRIABLE_HTTP_ERROR); + } } else { HandleGetHashError(clock_->Now()); diff --git a/chromium/components/safe_browsing/core/browser/db/v4_get_hash_protocol_manager.h b/chromium/components/safe_browsing/core/browser/db/v4_get_hash_protocol_manager.h index b51211ef2a2..2a6eb81c89a 100644 --- a/chromium/components/safe_browsing/core/browser/db/v4_get_hash_protocol_manager.h +++ b/chromium/components/safe_browsing/core/browser/db/v4_get_hash_protocol_manager.h @@ -229,6 +229,7 @@ class V4GetHashProtocolManager { TestGetHashErrorHandlingParallelRequests); FRIEND_TEST_ALL_PREFIXES(V4GetHashProtocolManagerTest, GetCachedResults); FRIEND_TEST_ALL_PREFIXES(V4GetHashProtocolManagerTest, TestUpdatesAreMerged); + FRIEND_TEST_ALL_PREFIXES(V4GetHashProtocolManagerTest, TestBackoffErrorHistogramCount); friend class V4GetHashProtocolManagerTest; friend class V4GetHashProtocolManagerFuzzer; friend class V4GetHashProtocolManagerFactoryImpl; @@ -329,6 +330,9 @@ class V4GetHashProtocolManager { // response, used for request backoff timing. size_t gethash_error_count_; + // The number of backoff errors since the last successful HTTP response. + size_t backoff_error_count_ = 0; + // Multiplier for the backoff error after the second. size_t gethash_back_off_mult_; diff --git a/chromium/components/safe_browsing/core/browser/db/v4_get_hash_protocol_manager_unittest.cc b/chromium/components/safe_browsing/core/browser/db/v4_get_hash_protocol_manager_unittest.cc index 13ed4fd888d..c9e49a86309 100644 --- a/chromium/components/safe_browsing/core/browser/db/v4_get_hash_protocol_manager_unittest.cc +++ b/chromium/components/safe_browsing/core/browser/db/v4_get_hash_protocol_manager_unittest.cc @@ -10,6 +10,7 @@ #include "base/base64.h" #include "base/bind.h" #include "base/run_loop.h" +#include "base/strings/escape.h" #include "base/test/metrics/histogram_tester.h" #include "base/test/simple_test_clock.h" #include "base/test/task_environment.h" @@ -18,7 +19,6 @@ #include "components/safe_browsing/core/browser/db/safebrowsing.pb.h" #include "components/safe_browsing/core/browser/db/util.h" #include "components/safe_browsing/core/browser/db/v4_test_util.h" -#include "net/base/escape.h" #include "net/base/load_flags.h" #include "net/base/net_errors.h" #include "services/network/public/cpp/shared_url_loader_factory.h" @@ -232,6 +232,56 @@ TEST_F(V4GetHashProtocolManagerTest, TestGetHashErrorHandlingResponseCode) { EXPECT_TRUE(callback_called()); } +TEST_F(V4GetHashProtocolManagerTest, TestBackoffErrorHistogramCount) { + std::unique_ptr<V4GetHashProtocolManager> pm(CreateProtocolManager()); + base::HistogramTester histogram_tester; + + FullHashToStoreAndHashPrefixesMap matched_locally; + matched_locally[FullHash("AHashFull")].emplace_back(GetUrlSocEngId(), + HashPrefix("AHash")); + + std::vector<FullHashInfo> expected_results; + pm->GetFullHashes( + matched_locally, {}, + base::BindRepeating(&V4GetHashProtocolManagerTest::ValidateGetV4HashResults, + base::Unretained(this), expected_results)); + + FullHashToStoreAndHashPrefixesMap matched_locally2; + matched_locally2[FullHash("AHash2Full")].emplace_back(GetUrlSocEngId(), + HashPrefix("AHash2")); + + pm->GetFullHashes(matched_locally2, {}, + base::BindRepeating( + &V4GetHashProtocolManagerTest::ValidateGetV4HashResults, + base::Unretained(this), expected_results)); + + // Failed request status should result in error. + SetupFullHashFetcherToReturnResponse(pm.get(), FullHash("AHashFull"), + net::ERR_CONNECTION_RESET, 200, + GetStockV4HashResponse()); + + EXPECT_EQ(1ul, pm->gethash_error_count_); + EXPECT_EQ(1ul, pm->gethash_back_off_mult_); + + EXPECT_TRUE(callback_called()); + + reset_callback_called(); + + pm->GetFullHashes(matched_locally2, {}, + base::BindRepeating( + &V4GetHashProtocolManagerTest::ValidateGetV4HashResults, + base::Unretained(this), expected_results)); + + EXPECT_EQ(1ul, pm->backoff_error_count_); + + SetupFullHashFetcherToReturnResponse(pm.get(), FullHash("AHash2Full"), + net::OK, 200, GetStockV4HashResponse()); + + histogram_tester.ExpectTotalCount( + "SafeBrowsing.V4GetHash.Result.BackoffErrorCount", + 1); +} + TEST_F(V4GetHashProtocolManagerTest, TestGetHashErrorHandlingParallelRequests) { std::unique_ptr<V4GetHashProtocolManager> pm(CreateProtocolManager()); std::vector<FullHashInfo> empty_results; diff --git a/chromium/components/safe_browsing/core/browser/db/v4_local_database_manager.cc b/chromium/components/safe_browsing/core/browser/db/v4_local_database_manager.cc index 254468c76d3..0ee0f67720c 100644 --- a/chromium/components/safe_browsing/core/browser/db/v4_local_database_manager.cc +++ b/chromium/components/safe_browsing/core/browser/db/v4_local_database_manager.cc @@ -639,10 +639,6 @@ bool V4LocalDatabaseManager::IsDownloadProtectionEnabled() const { return true; } -bool V4LocalDatabaseManager::IsSupported() const { - return true; -} - void V4LocalDatabaseManager::StartOnIOThread( scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, const V4ProtocolConfig& config) { @@ -990,15 +986,18 @@ void V4LocalDatabaseManager::PerformFullHashCheck( std::unique_ptr<PendingCheck> check) { DCHECK(io_task_runner()->RunsTasksInCurrentSequence()); - DCHECK(enabled_); DCHECK(!check->full_hash_to_store_and_hash_prefixes.empty()); - FullHashToStoreAndHashPrefixesMap full_hash_to_store_and_hash_prefixes = - check->full_hash_to_store_and_hash_prefixes; - v4_get_hash_protocol_manager_->GetFullHashes( - full_hash_to_store_and_hash_prefixes, list_client_states_, - base::BindOnce(&V4LocalDatabaseManager::OnFullHashResponse, - weak_factory_.GetWeakPtr(), std::move(check))); + // If we're not enabled, we're in the middle of shutdown, so silently drop the + // check. + if (enabled_) { + FullHashToStoreAndHashPrefixesMap full_hash_to_store_and_hash_prefixes = + check->full_hash_to_store_and_hash_prefixes; + v4_get_hash_protocol_manager_->GetFullHashes( + full_hash_to_store_and_hash_prefixes, list_client_states_, + base::BindOnce(&V4LocalDatabaseManager::OnFullHashResponse, + weak_factory_.GetWeakPtr(), std::move(check))); + } } void V4LocalDatabaseManager::ProcessQueuedChecks() { diff --git a/chromium/components/safe_browsing/core/browser/db/v4_local_database_manager.h b/chromium/components/safe_browsing/core/browser/db/v4_local_database_manager.h index c27d2dd53c9..511c12ac084 100644 --- a/chromium/components/safe_browsing/core/browser/db/v4_local_database_manager.h +++ b/chromium/components/safe_browsing/core/browser/db/v4_local_database_manager.h @@ -85,7 +85,6 @@ class V4LocalDatabaseManager : public SafeBrowsingDatabaseManager { bool MatchMalwareIP(const std::string& ip_address) override; safe_browsing::ThreatSource GetThreatSource() const override; bool IsDownloadProtectionEnabled() const override; - bool IsSupported() const override; void StartOnIOThread( scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, diff --git a/chromium/components/safe_browsing/core/browser/db/v4_local_database_manager_unittest.cc b/chromium/components/safe_browsing/core/browser/db/v4_local_database_manager_unittest.cc index 8961309d140..55ec5ab867e 100644 --- a/chromium/components/safe_browsing/core/browser/db/v4_local_database_manager_unittest.cc +++ b/chromium/components/safe_browsing/core/browser/db/v4_local_database_manager_unittest.cc @@ -478,11 +478,6 @@ TEST_F(V4LocalDatabaseManagerTest, TestGetThreatSource) { v4_local_database_manager_->GetThreatSource()); } -TEST_F(V4LocalDatabaseManagerTest, TestIsSupported) { - WaitForTasksOnTaskRunner(); - EXPECT_TRUE(v4_local_database_manager_->IsSupported()); -} - TEST_F(V4LocalDatabaseManagerTest, TestCanCheckUrl) { WaitForTasksOnTaskRunner(); EXPECT_TRUE( diff --git a/chromium/components/safe_browsing/core/browser/db/v4_protocol_manager_util.cc b/chromium/components/safe_browsing/core/browser/db/v4_protocol_manager_util.cc index 4e9290e3f89..962d2b0e1bd 100644 --- a/chromium/components/safe_browsing/core/browser/db/v4_protocol_manager_util.cc +++ b/chromium/components/safe_browsing/core/browser/db/v4_protocol_manager_util.cc @@ -9,6 +9,7 @@ #include "base/hash/sha1.h" #include "base/metrics/histogram_functions.h" #include "base/rand_util.h" +#include "base/strings/escape.h" #include "base/strings/string_util.h" #include "base/strings/stringprintf.h" #include "base/time/time.h" @@ -16,7 +17,6 @@ #include "components/version_info/version_info.h" #include "crypto/sha2.h" #include "google_apis/google_api_keys.h" -#include "net/base/escape.h" #include "net/base/ip_address.h" #include "net/base/net_errors.h" #include "net/http/http_request_headers.h" @@ -47,7 +47,7 @@ std::string Unescape(const std::string& url) { int loop_var = 0; do { old_size = unescaped_str.size(); - unescaped_str = net::UnescapeBinaryURLComponent(unescaped_str); + unescaped_str = base::UnescapeBinaryURLComponent(unescaped_str); } while (old_size != unescaped_str.size() && ++loop_var <= kMaxLoopIterations); @@ -97,7 +97,7 @@ std::string GetReportUrl(const V4ProtocolConfig& config, std::string api_key = google_apis::GetAPIKey(); if (!api_key.empty()) { base::StringAppendF(&url, "&key=%s", - net::EscapeQueryParamValue(api_key, true).c_str()); + base::EscapeQueryParamValue(api_key, true).c_str()); } if (reporting_level) url.append(base::StringPrintf("&ext=%d", *reporting_level)); @@ -323,7 +323,7 @@ std::string V4ProtocolManagerUtil::ComposeUrl(const std::string& prefix, method.c_str(), request_base64.c_str()); if (!key_param.empty()) { base::StringAppendF(&url, "&key=%s", - net::EscapeQueryParamValue(key_param, true).c_str()); + base::EscapeQueryParamValue(key_param, true).c_str()); } return url; } diff --git a/chromium/components/safe_browsing/core/browser/db/v4_protocol_manager_util.h b/chromium/components/safe_browsing/core/browser/db/v4_protocol_manager_util.h index f304edb241e..0a350650d87 100644 --- a/chromium/components/safe_browsing/core/browser/db/v4_protocol_manager_util.h +++ b/chromium/components/safe_browsing/core/browser/db/v4_protocol_manager_util.h @@ -301,9 +301,16 @@ enum V4OperationResult { // Identical operation already pending. ALREADY_PENDING_ERROR = 6, + // A network error that can be retried without backoff (e.g. + // NETWORK_DISCONNECTED). + RETRIABLE_NETWORK_ERROR = 7, + + // An HTTP error code that can be retried without backoff. + RETRIABLE_HTTP_ERROR = 8, + // Memory space for histograms is determined by the max. ALWAYS // ADD NEW VALUES BEFORE THIS ONE. - OPERATION_RESULT_MAX = 7 + OPERATION_RESULT_MAX = 9 }; // A class that provides static methods related to the Pver4 protocol. diff --git a/chromium/components/safe_browsing/core/browser/db/v4_protocol_manager_util_unittest.cc b/chromium/components/safe_browsing/core/browser/db/v4_protocol_manager_util_unittest.cc index a36e312a201..63dd889d1df 100644 --- a/chromium/components/safe_browsing/core/browser/db/v4_protocol_manager_util_unittest.cc +++ b/chromium/components/safe_browsing/core/browser/db/v4_protocol_manager_util_unittest.cc @@ -9,10 +9,10 @@ #include "base/base64.h" #include "base/containers/contains.h" #include "base/logging.h" +#include "base/strings/escape.h" #include "base/strings/stringprintf.h" #include "base/time/time.h" #include "components/safe_browsing/core/browser/db/v4_test_util.h" -#include "net/base/escape.h" #include "net/http/http_request_headers.h" #include "testing/gtest/include/gtest/gtest.h" diff --git a/chromium/components/safe_browsing/core/browser/db/v4_store.cc b/chromium/components/safe_browsing/core/browser/db/v4_store.cc index 9f76b1569b9..7c6f00fa918 100644 --- a/chromium/components/safe_browsing/core/browser/db/v4_store.cc +++ b/chromium/components/safe_browsing/core/browser/db/v4_store.cc @@ -9,6 +9,7 @@ #include "base/base64.h" #include "base/bind.h" +#include "base/cpu_reduction_experiment.h" #include "base/files/file_util.h" #include "base/logging.h" #include "base/metrics/histogram_functions.h" @@ -22,6 +23,7 @@ #include "components/safe_browsing/core/common/proto/webui.pb.h" #include "crypto/secure_hash.h" #include "crypto/sha2.h" +#include "third_party/abseil-cpp/absl/types/optional.h" using base::TimeTicks; @@ -204,9 +206,14 @@ void V4Store::Initialize() { RecordStoreReadResult(store_read_result); } -bool V4Store::HasValidData() const { - RecordBooleanWithAndWithoutSuffix("SafeBrowsing.V4Store.IsStoreValid", - has_valid_data_, store_path_); +bool V4Store::HasValidData() { + // Record every 256th time (`record_has_valid_data_counter_` is 8-bit). + if (++record_has_valid_data_counter_ == 1 || + // TODO(crbug.com/1295441): Remove the condition below. + !base::IsRunningCpuReductionExperiment()) { + RecordBooleanWithAndWithoutSuffix("SafeBrowsing.V4Store.IsStoreValid", + has_valid_data_, store_path_); + } return has_valid_data_; } @@ -581,7 +588,8 @@ ApplyUpdateResult V4Store::MergeUpdate(const HashPrefixMap& old_prefixes_map, // picked is not the same as merged. A picked element isn't merged if its // index is on the raw_removals list. int total_picked_from_old = 0; - const int* removals_iter = raw_removals ? raw_removals->begin() : nullptr; + auto removals_iter = + raw_removals ? absl::make_optional(raw_removals->begin()) : absl::nullopt; while (old_has_unmerged || additions_has_unmerged) { // If the same hash prefix appears in the existing store and the additions // list, something is clearly wrong. Discard the update. @@ -605,8 +613,8 @@ ApplyUpdateResult V4Store::MergeUpdate(const HashPrefixMap& old_prefixes_map, // prefix of size |next_smallest_prefix_size| from the old store. old_iterator_map[next_smallest_prefix_size] += next_smallest_prefix_size; - if (!raw_removals || removals_iter == raw_removals->end() || - *removals_iter != total_picked_from_old) { + if (!raw_removals || *removals_iter == raw_removals->end() || + **removals_iter != total_picked_from_old) { // Append the smallest hash to the appropriate list. hash_prefix_map_[next_smallest_prefix_size] += next_smallest_prefix_old; @@ -616,7 +624,7 @@ ApplyUpdateResult V4Store::MergeUpdate(const HashPrefixMap& old_prefixes_map, } } else { // Element not added to new map. Move the removals iterator forward. - removals_iter++; + (*removals_iter)++; } total_picked_from_old++; @@ -648,7 +656,7 @@ ApplyUpdateResult V4Store::MergeUpdate(const HashPrefixMap& old_prefixes_map, } } - if (raw_removals && removals_iter != raw_removals->end()) { + if (raw_removals && *removals_iter != raw_removals->end()) { return REMOVALS_INDEX_TOO_LARGE_FAILURE; } diff --git a/chromium/components/safe_browsing/core/browser/db/v4_store.h b/chromium/components/safe_browsing/core/browser/db/v4_store.h index 9b2cdb2957e..45a6056f917 100644 --- a/chromium/components/safe_browsing/core/browser/db/v4_store.h +++ b/chromium/components/safe_browsing/core/browser/db/v4_store.h @@ -188,7 +188,7 @@ class V4Store { // True if this store has valid contents, either from a successful read // from disk or a full update. This does not mean the checksum was verified. - virtual bool HasValidData() const; + virtual bool HasValidData(); const std::string& state() const { return state_; } @@ -432,6 +432,10 @@ class V4Store { // The size of the file on disk for this store. int64_t file_size_; + // A counter used to manage how frequently the value of `has_valid_data_` + // below is recorded. + uint8_t record_has_valid_data_counter_ = 0; + // True if the file was successfully read+parsed or was populated from // a full update. bool has_valid_data_; diff --git a/chromium/components/safe_browsing/core/browser/db/v4_test_util.cc b/chromium/components/safe_browsing/core/browser/db/v4_test_util.cc index aa704c4b872..6ed5f4fd2f1 100644 --- a/chromium/components/safe_browsing/core/browser/db/v4_test_util.cc +++ b/chromium/components/safe_browsing/core/browser/db/v4_test_util.cc @@ -46,7 +46,7 @@ TestV4Store::TestV4Store( TestV4Store::~TestV4Store() = default; -bool TestV4Store::HasValidData() const { +bool TestV4Store::HasValidData() { return true; } diff --git a/chromium/components/safe_browsing/core/browser/db/v4_test_util.h b/chromium/components/safe_browsing/core/browser/db/v4_test_util.h index cbfdb7f447e..accb167247b 100644 --- a/chromium/components/safe_browsing/core/browser/db/v4_test_util.h +++ b/chromium/components/safe_browsing/core/browser/db/v4_test_util.h @@ -31,7 +31,7 @@ class TestV4Store : public V4Store { const base::FilePath& store_path); ~TestV4Store() override; - bool HasValidData() const override; + bool HasValidData() override; void MarkPrefixAsBad(HashPrefix prefix); diff --git a/chromium/components/safe_browsing/core/browser/db/v4_update_protocol_manager_unittest.cc b/chromium/components/safe_browsing/core/browser/db/v4_update_protocol_manager_unittest.cc index f077d7adb42..82b6bb1d827 100644 --- a/chromium/components/safe_browsing/core/browser/db/v4_update_protocol_manager_unittest.cc +++ b/chromium/components/safe_browsing/core/browser/db/v4_update_protocol_manager_unittest.cc @@ -10,6 +10,7 @@ #include "base/base64.h" #include "base/bind.h" +#include "base/strings/escape.h" #include "base/test/test_simple_task_runner.h" #include "base/threading/thread_task_runner_handle.h" #include "base/time/time.h" @@ -18,7 +19,6 @@ #include "components/safe_browsing/core/browser/db/safebrowsing.pb.h" #include "components/safe_browsing/core/browser/db/util.h" #include "components/safe_browsing/core/browser/db/v4_test_util.h" -#include "net/base/escape.h" #include "net/base/load_flags.h" #include "net/base/net_errors.h" #include "net/http/http_status_code.h" diff --git a/chromium/components/safe_browsing/core/browser/password_protection/password_protection_request.cc b/chromium/components/safe_browsing/core/browser/password_protection/password_protection_request.cc index 5a491a4036c..c0fe7698e6d 100644 --- a/chromium/components/safe_browsing/core/browser/password_protection/password_protection_request.cc +++ b/chromium/components/safe_browsing/core/browser/password_protection/password_protection_request.cc @@ -8,6 +8,7 @@ #include "base/bind.h" #include "base/metrics/histogram_functions.h" +#include "base/strings/escape.h" #include "base/strings/strcat.h" #include "base/strings/utf_string_conversions.h" #include "build/build_config.h" @@ -18,7 +19,6 @@ #include "components/safe_browsing/core/common/safebrowsing_constants.h" #include "components/safe_browsing/core/common/utils.h" #include "components/url_formatter/url_formatter.h" -#include "net/base/escape.h" #include "net/base/load_flags.h" #include "net/http/http_request_headers.h" #include "net/http/http_status_code.h" @@ -55,7 +55,7 @@ std::vector<std::string> GetMatchingDomains( url_formatter::kFormatUrlOmitHTTPS | url_formatter::kFormatUrlOmitTrivialSubdomains | url_formatter::kFormatUrlTrimAfterHost, - net::UnescapeRule::SPACES, nullptr, nullptr, nullptr)); + base::UnescapeRule::SPACES, nullptr, nullptr, nullptr)); matching_domains.push_back(std::move(domain)); } return base::flat_set<std::string>(std::move(matching_domains)).extract(); diff --git a/chromium/components/safe_browsing/core/browser/password_protection/password_protection_service_base.cc b/chromium/components/safe_browsing/core/browser/password_protection/password_protection_service_base.cc index 0f73eeea82c..d93eb525666 100644 --- a/chromium/components/safe_browsing/core/browser/password_protection/password_protection_service_base.cc +++ b/chromium/components/safe_browsing/core/browser/password_protection/password_protection_service_base.cc @@ -12,6 +12,7 @@ #include "base/bind.h" #include "base/callback.h" #include "base/metrics/histogram_macros.h" +#include "base/strings/escape.h" #include "base/strings/string_number_conversions.h" #include "base/strings/string_split.h" #include "base/strings/string_util.h" @@ -25,7 +26,6 @@ #include "components/safe_browsing/core/common/features.h" #include "components/safe_browsing/core/common/utils.h" #include "google_apis/google_api_keys.h" -#include "net/base/escape.h" #include "net/base/url_util.h" using password_manager::metrics_util::PasswordType; @@ -270,7 +270,7 @@ GURL PasswordProtectionServiceBase::GetPasswordProtectionRequestUrl() { GURL url(kPasswordProtectionRequestUrl); std::string api_key = google_apis::GetAPIKey(); DCHECK(!api_key.empty()); - return url.Resolve("?key=" + net::EscapeQueryParamValue(api_key, true)); + return url.Resolve("?key=" + base::EscapeQueryParamValue(api_key, true)); } // static diff --git a/chromium/components/safe_browsing/core/browser/ping_manager.cc b/chromium/components/safe_browsing/core/browser/ping_manager.cc index 7db1aeb0016..456d3d157a4 100644 --- a/chromium/components/safe_browsing/core/browser/ping_manager.cc +++ b/chromium/components/safe_browsing/core/browser/ping_manager.cc @@ -11,12 +11,12 @@ #include "base/memory/ptr_util.h" #include "base/metrics/histogram_functions.h" #include "base/notreached.h" +#include "base/strings/escape.h" #include "base/strings/string_util.h" #include "base/strings/stringprintf.h" #include "components/safe_browsing/core/browser/db/v4_protocol_manager_util.h" #include "components/safe_browsing/core/common/utils.h" #include "google_apis/google_api_keys.h" -#include "net/base/escape.h" #include "net/base/load_flags.h" #include "net/traffic_annotation/network_traffic_annotation.h" #include "services/network/public/cpp/resource_request.h" @@ -74,20 +74,32 @@ PingManager* PingManager::Create( const V4ProtocolConfig& config, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, std::unique_ptr<SafeBrowsingTokenFetcher> token_fetcher, - base::RepeatingCallback<bool()> get_should_fetch_access_token) { + base::RepeatingCallback<bool()> get_should_fetch_access_token, + WebUIDelegate* webui_delegate, + scoped_refptr<base::SequencedTaskRunner> ui_task_runner, + base::RepeatingCallback<ChromeUserPopulation()> + get_user_population_callback) { return new PingManager(config, url_loader_factory, std::move(token_fetcher), - get_should_fetch_access_token); + get_should_fetch_access_token, webui_delegate, + ui_task_runner, get_user_population_callback); } PingManager::PingManager( const V4ProtocolConfig& config, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, std::unique_ptr<SafeBrowsingTokenFetcher> token_fetcher, - base::RepeatingCallback<bool()> get_should_fetch_access_token) + base::RepeatingCallback<bool()> get_should_fetch_access_token, + WebUIDelegate* webui_delegate, + scoped_refptr<base::SequencedTaskRunner> ui_task_runner, + base::RepeatingCallback<ChromeUserPopulation()> + get_user_population_callback) : config_(config), url_loader_factory_(url_loader_factory), token_fetcher_(std::move(token_fetcher)), - get_should_fetch_access_token_(get_should_fetch_access_token) {} + get_should_fetch_access_token_(get_should_fetch_access_token), + webui_delegate_(webui_delegate), + ui_task_runner_(ui_task_runner), + get_user_population_callback_(get_user_population_callback) {} PingManager::~PingManager() {} @@ -139,19 +151,45 @@ void PingManager::ReportSafeBrowsingHit( } // Sends threat details for users who opt-in. -void PingManager::ReportThreatDetails(const std::string& report) { +PingManager::ReportThreatDetailsResult PingManager::ReportThreatDetails( + std::unique_ptr<ClientSafeBrowsingReportRequest> report) { + if (!get_user_population_callback_.is_null()) { + *report->mutable_population() = get_user_population_callback_.Run(); + } + + std::string serialized_report; + if (!report->SerializeToString(&serialized_report)) { + DLOG(ERROR) << "Unable to serialize the threat report."; + return ReportThreatDetailsResult::SERIALIZATION_ERROR; + } + if (serialized_report.empty()) { + DLOG(ERROR) << "The threat report is empty."; + return ReportThreatDetailsResult::EMPTY_REPORT; + } + if (get_should_fetch_access_token_.Run()) { token_fetcher_->Start( base::BindOnce(&PingManager::ReportThreatDetailsOnGotAccessToken, - weak_factory_.GetWeakPtr(), report)); + weak_factory_.GetWeakPtr(), serialized_report)); } else { std::string empty_access_token; - ReportThreatDetailsOnGotAccessToken(report, empty_access_token); + ReportThreatDetailsOnGotAccessToken(serialized_report, empty_access_token); } + + // The following is to log this ClientSafeBrowsingReportRequest on any open + // chrome://safe-browsing pages. + ui_task_runner_->PostTask( + FROM_HERE, + base::BindOnce(&WebUIDelegate::AddToCSBRRsSent, + // Unretained is okay because in practice, webui_delegate_ + // is a singleton + base::Unretained(webui_delegate_), std::move(report))); + + return ReportThreatDetailsResult::SUCCESS; } void PingManager::ReportThreatDetailsOnGotAccessToken( - const std::string& report, + const std::string& serialized_report, const std::string& access_token) { GURL report_url = ThreatDetailsUrl(); @@ -171,7 +209,7 @@ void PingManager::ReportThreatDetailsOnGotAccessToken( auto loader = network::SimpleURLLoader::Create(std::move(resource_request), kTrafficAnnotation); - loader->AttachStringForUpload(report, "application/octet-stream"); + loader->AttachStringForUpload(serialized_report, "application/octet-stream"); loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie( url_loader_factory_.get(), @@ -240,7 +278,7 @@ GURL PingManager::SafeBrowsingHitUrl( // Population_id should be URL-safe, but escape it and size-limit it // anyway since it came from outside Chrome. std::string up_str = - net::EscapeQueryParamValue(hit_report.population_id, true); + base::EscapeQueryParamValue(hit_report.population_id, true); if (up_str.size() > 512) { DCHECK(false) << "population_id is too long: " << up_str; up_str = "UP_STRING_TOO_LONG"; @@ -252,9 +290,10 @@ GURL PingManager::SafeBrowsingHitUrl( return GURL(base::StringPrintf( "%s&evts=%s&evtd=%s&evtr=%s&evhr=%s&evtb=%d&src=%s&m=%d%s", url.c_str(), threat_list.c_str(), - net::EscapeQueryParamValue(hit_report.malicious_url.spec(), true).c_str(), - net::EscapeQueryParamValue(hit_report.page_url.spec(), true).c_str(), - net::EscapeQueryParamValue(hit_report.referrer_url.spec(), true).c_str(), + base::EscapeQueryParamValue(hit_report.malicious_url.spec(), true) + .c_str(), + base::EscapeQueryParamValue(hit_report.page_url.spec(), true).c_str(), + base::EscapeQueryParamValue(hit_report.referrer_url.spec(), true).c_str(), hit_report.is_subresource, threat_source.c_str(), hit_report.is_metrics_reporting_active, user_population_comp.c_str())); } diff --git a/chromium/components/safe_browsing/core/browser/ping_manager.h b/chromium/components/safe_browsing/core/browser/ping_manager.h index 558e420b986..312c51463f6 100644 --- a/chromium/components/safe_browsing/core/browser/ping_manager.h +++ b/chromium/components/safe_browsing/core/browser/ping_manager.h @@ -18,6 +18,7 @@ #include "components/safe_browsing/core/browser/db/hit_report.h" #include "components/safe_browsing/core/browser/db/util.h" #include "components/safe_browsing/core/browser/safe_browsing_token_fetcher.h" +#include "components/safe_browsing/core/common/proto/csd.pb.h" #include "services/network/public/cpp/shared_url_loader_factory.h" #include "url/gurl.h" @@ -29,6 +30,25 @@ namespace safe_browsing { class PingManager : public KeyedService { public: + enum class ReportThreatDetailsResult { + SUCCESS = 0, + // There was a problem serializing the report to a string. + SERIALIZATION_ERROR = 1, + // The report is empty, so it is not sent. + EMPTY_REPORT = 2, + }; + + // Interface via which a client of this class can surface relevant events in + // WebUI. All methods must be called on the UI thread. + class WebUIDelegate { + public: + virtual ~WebUIDelegate() = default; + + // Track a client safe browsing report being sent. + virtual void AddToCSBRRsSent( + std::unique_ptr<ClientSafeBrowsingReportRequest> csbrr) = 0; + }; + PingManager(const PingManager&) = delete; PingManager& operator=(const PingManager&) = delete; @@ -39,7 +59,11 @@ class PingManager : public KeyedService { const V4ProtocolConfig& config, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, std::unique_ptr<SafeBrowsingTokenFetcher> token_fetcher, - base::RepeatingCallback<bool()> get_should_fetch_access_token); + base::RepeatingCallback<bool()> get_should_fetch_access_token, + WebUIDelegate* webui_delegate, + scoped_refptr<base::SequencedTaskRunner> ui_task_runner, + base::RepeatingCallback<ChromeUserPopulation()> + get_user_population_callback); void OnURLLoaderComplete(network::SimpleURLLoader* source, std::unique_ptr<std::string> response_body); @@ -53,9 +77,11 @@ class PingManager : public KeyedService { // SafeBrowsingtHitUrl. void ReportSafeBrowsingHit(const safe_browsing::HitReport& hit_report); - // Users can opt-in on the SafeBrowsing interstitial to send detailed - // threat reports. |report| is the serialized report. - void ReportThreatDetails(const std::string& report); + // Sends a detailed threat report after performing validation and adding extra + // details to the report. The returned object provides details on whether the + // report was successful. + ReportThreatDetailsResult ReportThreatDetails( + std::unique_ptr<ClientSafeBrowsingReportRequest> report); // Only used for tests void SetURLLoaderFactoryForTesting( @@ -65,13 +91,15 @@ class PingManager : public KeyedService { protected: friend class PingManagerTest; - // Constructs a PingManager with the given |config|, |url_loader_factory|, and - // access token fetching information. explicit PingManager( const V4ProtocolConfig& config, scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, std::unique_ptr<SafeBrowsingTokenFetcher> token_fetcher, - base::RepeatingCallback<bool()> get_should_fetch_access_token); + base::RepeatingCallback<bool()> get_should_fetch_access_token, + WebUIDelegate* webui_delegate, + scoped_refptr<base::SequencedTaskRunner> ui_task_runner, + base::RepeatingCallback<ChromeUserPopulation()> + get_user_population_callback); private: FRIEND_TEST_ALL_PREFIXES(PingManagerTest, TestSafeBrowsingHitUrl); @@ -92,7 +120,7 @@ class PingManager : public KeyedService { // Once the user's access_token has been fetched by ReportThreatDetails (or // intentionally not fetched), attaches the token and sends the report. - void ReportThreatDetailsOnGotAccessToken(const std::string& report, + void ReportThreatDetailsOnGotAccessToken(const std::string& serialized_report, const std::string& access_token); // Track outstanding SafeBrowsing report fetchers for clean up. @@ -109,6 +137,17 @@ class PingManager : public KeyedService { // based on whether they're a signed-in ESB user. base::RepeatingCallback<bool()> get_should_fetch_access_token_; + // WebUIInfoSingleton extends PingManager::WebUIDelegate to enable the + // workaround of calling AddToCSBRRsSent in WebUIInfoSingleton without /core + // having a dependency on /content. + raw_ptr<WebUIDelegate> webui_delegate_; + + // The task runner for the UI thread. + scoped_refptr<base::SequencedTaskRunner> ui_task_runner_; + + // Pulls the user population. + base::RepeatingCallback<ChromeUserPopulation()> get_user_population_callback_; + base::WeakPtrFactory<PingManager> weak_factory_{this}; }; diff --git a/chromium/components/safe_browsing/core/browser/ping_manager_unittest.cc b/chromium/components/safe_browsing/core/browser/ping_manager_unittest.cc index d5a81d64059..8a063404620 100644 --- a/chromium/components/safe_browsing/core/browser/ping_manager_unittest.cc +++ b/chromium/components/safe_browsing/core/browser/ping_manager_unittest.cc @@ -5,13 +5,14 @@ #include "components/safe_browsing/core/browser/ping_manager.h" #include "base/base64.h" +#include "base/callback_helpers.h" #include "base/run_loop.h" +#include "base/strings/escape.h" #include "base/strings/stringprintf.h" #include "base/time/time.h" #include "base/values.h" #include "components/safe_browsing/core/browser/db/v4_test_util.h" #include "google_apis/google_api_keys.h" -#include "net/base/escape.h" #include "testing/gtest/include/gtest/gtest.h" using base::Time; @@ -29,12 +30,13 @@ class PingManagerTest : public testing::Test { std::string key = google_apis::GetAPIKey(); if (!key.empty()) { key_param_ = base::StringPrintf( - "&key=%s", net::EscapeQueryParamValue(key, true).c_str()); + "&key=%s", base::EscapeQueryParamValue(key, true).c_str()); } ping_manager_.reset( new PingManager(safe_browsing::GetTestV4ProtocolConfig(), nullptr, - nullptr, base::BindRepeating([]() { return false; }))); + nullptr, base::BindRepeating([]() { return false; }), + nullptr, nullptr, base::NullCallback())); } PingManager* ping_manager() { return ping_manager_.get(); } @@ -213,4 +215,12 @@ TEST_F(PingManagerTest, TestThreatDetailsUrl) { ping_manager()->ThreatDetailsUrl().spec()); } +TEST_F(PingManagerTest, TestReportThreatDetails_EmptyReport) { + std::unique_ptr<ClientSafeBrowsingReportRequest> report = + std::make_unique<ClientSafeBrowsingReportRequest>(); + PingManager::ReportThreatDetailsResult result = + ping_manager()->ReportThreatDetails(std::move(report)); + EXPECT_EQ(result, PingManager::ReportThreatDetailsResult::EMPTY_REPORT); +} + } // namespace safe_browsing diff --git a/chromium/components/safe_browsing/core/browser/realtime/BUILD.gn b/chromium/components/safe_browsing/core/browser/realtime/BUILD.gn index 924a79816db..6c91a33316a 100644 --- a/chromium/components/safe_browsing/core/browser/realtime/BUILD.gn +++ b/chromium/components/safe_browsing/core/browser/realtime/BUILD.gn @@ -87,6 +87,7 @@ source_set("unit_tests") { "//components/safe_browsing:buildflags", "//components/safe_browsing/core/browser:referrer_chain_provider", "//components/safe_browsing/core/browser:token_fetcher", + "//components/safe_browsing/core/browser:token_fetcher_testing_helper", "//components/safe_browsing/core/browser:verdict_cache_manager", "//components/safe_browsing/core/common", "//components/safe_browsing/core/common:safe_browsing_prefs", diff --git a/chromium/components/safe_browsing/core/browser/realtime/url_lookup_service_base.cc b/chromium/components/safe_browsing/core/browser/realtime/url_lookup_service_base.cc index fe620aad119..bb4dcd5dbd2 100644 --- a/chromium/components/safe_browsing/core/browser/realtime/url_lookup_service_base.cc +++ b/chromium/components/safe_browsing/core/browser/realtime/url_lookup_service_base.cc @@ -312,7 +312,7 @@ RealTimeUrlLookupServiceBase::GetCachedRealTimeUrlVerdict(const GURL& url) { void RealTimeUrlLookupServiceBase::MayBeCacheRealTimeUrlVerdict( const GURL& url, RTLookupResponse response) { - if (response.threat_info_size() > 0) { + if (cache_manager_ && response.threat_info_size() > 0) { base::SequencedTaskRunnerHandle::Get()->PostTask( FROM_HERE, base::BindOnce(&VerdictCacheManager::CacheRealTimeUrlVerdict, cache_manager_->GetWeakPtr(), url, response, @@ -406,10 +406,10 @@ void RealTimeUrlLookupServiceBase::SendRequest( // NOTE: Pass |callback_task_runner| by copying it here as it's also needed // just below. - SendRequestInternal(std::move(resource_request), req_data, url, - access_token_string, std::move(response_callback), - callback_task_runner, - request->population().user_population()); + SendRequestInternal( + std::move(resource_request), req_data, url, access_token_string, + std::move(response_callback), callback_task_runner, + request->population().user_population(), is_sampled_report); callback_task_runner->PostTask( FROM_HERE, @@ -425,7 +425,8 @@ void RealTimeUrlLookupServiceBase::SendRequestInternal( absl::optional<std::string> access_token_string, RTLookupResponseCallback response_callback, scoped_refptr<base::SequencedTaskRunner> callback_task_runner, - ChromeUserPopulation::UserPopulation user_population) { + ChromeUserPopulation::UserPopulation user_population, + bool is_sampled_report) { std::unique_ptr<network::SimpleURLLoader> owned_loader = network::SimpleURLLoader::Create(std::move(resource_request), GetTrafficAnnotationTag()); @@ -439,7 +440,7 @@ void RealTimeUrlLookupServiceBase::SendRequestInternal( url_loader_factory_.get(), base::BindOnce(&RealTimeUrlLookupServiceBase::OnURLLoaderComplete, GetWeakPtr(), url, access_token_string, loader, - user_population, base::TimeTicks::Now(), + user_population, base::TimeTicks::Now(), is_sampled_report, std::move(callback_task_runner))); pending_requests_[owned_loader.release()] = std::move(response_callback); @@ -451,6 +452,7 @@ void RealTimeUrlLookupServiceBase::OnURLLoaderComplete( network::SimpleURLLoader* url_loader, ChromeUserPopulation::UserPopulation user_population, base::TimeTicks request_start_time, + bool is_sampled_report, scoped_refptr<base::SequencedTaskRunner> response_callback_task_runner, std::unique_ptr<std::string> response_body) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); @@ -466,9 +468,14 @@ void RealTimeUrlLookupServiceBase::OnURLLoaderComplete( int response_code = 0; if (url_loader->ResponseInfo() && url_loader->ResponseInfo()->headers) response_code = url_loader->ResponseInfo()->headers->response_code(); + std::string report_type_suffix = + is_sampled_report ? ".SampledPing" : ".NormalPing"; RecordNetworkResultWithAndWithoutSuffix("SafeBrowsing.RT.Network.Result", GetMetricSuffix(), net_error, response_code); + RecordHttpResponseOrErrorCode( + ("SafeBrowsing.RT.Network.Result" + report_type_suffix).c_str(), + net_error, response_code); if (response_code == net::HTTP_UNAUTHORIZED && access_token_string.has_value()) { @@ -481,6 +488,9 @@ void RealTimeUrlLookupServiceBase::OnURLLoaderComplete( response->ParseFromString(*response_body); RecordBooleanWithAndWithoutSuffix("SafeBrowsing.RT.IsLookupSuccessful", GetMetricSuffix(), is_rt_lookup_successful); + base::UmaHistogramBoolean( + "SafeBrowsing.RT.IsLookupSuccessful" + report_type_suffix, + is_rt_lookup_successful); is_rt_lookup_successful ? HandleLookupSuccess() : HandleLookupError(); MayBeCacheRealTimeUrlVerdict(url, *response); diff --git a/chromium/components/safe_browsing/core/browser/realtime/url_lookup_service_base.h b/chromium/components/safe_browsing/core/browser/realtime/url_lookup_service_base.h index 141c4fc2f8c..d9ecab77a37 100644 --- a/chromium/components/safe_browsing/core/browser/realtime/url_lookup_service_base.h +++ b/chromium/components/safe_browsing/core/browser/realtime/url_lookup_service_base.h @@ -229,7 +229,8 @@ class RealTimeUrlLookupServiceBase : public KeyedService { absl::optional<std::string> access_token_string, RTLookupResponseCallback response_callback, scoped_refptr<base::SequencedTaskRunner> callback_task_runner, - ChromeUserPopulation::UserPopulation user_population); + ChromeUserPopulation::UserPopulation user_population, + bool is_sampled_report); // Called when the response from the real-time lookup remote endpoint is // received. |url_loader| is the unowned loader that was used to send the @@ -243,6 +244,7 @@ class RealTimeUrlLookupServiceBase : public KeyedService { network::SimpleURLLoader* url_loader, ChromeUserPopulation::UserPopulation user_population, base::TimeTicks request_start_time, + bool is_sampled_report, scoped_refptr<base::SequencedTaskRunner> response_callback_task_runner, std::unique_ptr<std::string> response_body); diff --git a/chromium/components/safe_browsing/core/browser/realtime/url_lookup_service_unittest.cc b/chromium/components/safe_browsing/core/browser/realtime/url_lookup_service_unittest.cc index 323d521aa23..71dff161287 100644 --- a/chromium/components/safe_browsing/core/browser/realtime/url_lookup_service_unittest.cc +++ b/chromium/components/safe_browsing/core/browser/realtime/url_lookup_service_unittest.cc @@ -17,6 +17,7 @@ #include "components/safe_browsing/buildflags.h" #include "components/safe_browsing/core/browser/referrer_chain_provider.h" #include "components/safe_browsing/core/browser/safe_browsing_token_fetcher.h" +#include "components/safe_browsing/core/browser/test_safe_browsing_token_fetcher.h" #include "components/safe_browsing/core/browser/verdict_cache_manager.h" #include "components/safe_browsing/core/common/features.h" #include "components/safe_browsing/core/common/safe_browsing_prefs.h" @@ -45,28 +46,6 @@ constexpr char kTestReferrerUrl[] = "http://example.referrer/"; constexpr char kTestSubframeUrl[] = "http://iframe.example.test/"; constexpr char kTestSubframeReferrerUrl[] = "http://iframe.example.referrer/"; -class TestSafeBrowsingTokenFetcher : public SafeBrowsingTokenFetcher { - public: - TestSafeBrowsingTokenFetcher() = default; - ~TestSafeBrowsingTokenFetcher() override { - // Like SafeBrowsingTokenFetchTracer, trigger the callback when destroyed. - RunAccessTokenCallback(""); - } - - // SafeBrowsingTokenFetcher: - void Start(Callback callback) override { callback_ = std::move(callback); } - - void RunAccessTokenCallback(std::string token) { - if (callback_) - std::move(callback_).Run(token); - } - - MOCK_METHOD1(OnInvalidAccessToken, void(const std::string&)); - - private: - Callback callback_; -}; - class MockReferrerChainProvider : public ReferrerChainProvider { public: virtual ~MockReferrerChainProvider() = default; @@ -1149,11 +1128,6 @@ TEST_F(RealTimeUrlLookupServiceTest, TestShutdown_CallbackNotPostedOnShutdown) { TEST_F(RealTimeUrlLookupServiceTest, TestShutdown_CacheManagerReset) { GURL url("https://a.example.test/path1/path2"); - // Post a task to cache_manager_ to cache the verdict. - MayBeCacheRealTimeUrlVerdict(url, RTLookupResponse::ThreatInfo::DANGEROUS, - RTLookupResponse::ThreatInfo::SOCIAL_ENGINEERING, - 60, "a.example.test/path1/path2", - RTLookupResponse::ThreatInfo::COVERING_MATCH); // Shutdown and delete depending objects. rt_service()->Shutdown(); @@ -1161,6 +1135,12 @@ TEST_F(RealTimeUrlLookupServiceTest, TestShutdown_CacheManagerReset) { content_setting_map_->ShutdownOnUIThread(); content_setting_map_.reset(); + // Post a task to cache_manager_ to cache the verdict. + MayBeCacheRealTimeUrlVerdict(url, RTLookupResponse::ThreatInfo::DANGEROUS, + RTLookupResponse::ThreatInfo::SOCIAL_ENGINEERING, + 60, "a.example.test/path1/path2", + RTLookupResponse::ThreatInfo::COVERING_MATCH); + // The task to cache_manager_ should be cancelled and not cause crash. task_environment_.RunUntilIdle(); } @@ -1226,8 +1206,6 @@ TEST_F(RealTimeUrlLookupServiceTest, // Enable extended reporting. EnableExtendedReporting(); rt_service()->set_bypass_probability_for_tests(true); - // When feature is not enabled, a sampled ping should not be sent. - EXPECT_FALSE(CanSendRTSampleRequest()); feature_list_.InitAndDisableFeature( safe_browsing::kSendSampledPingsForProtegoAllowlistDomains); // After enabling the feature, a sampled ping should be sent. diff --git a/chromium/components/safe_browsing/core/browser/tailored_security_service/OWNERS b/chromium/components/safe_browsing/core/browser/tailored_security_service/OWNERS index 3a311df9c96..4f674dd2204 100644 --- a/chromium/components/safe_browsing/core/browser/tailored_security_service/OWNERS +++ b/chromium/components/safe_browsing/core/browser/tailored_security_service/OWNERS @@ -1,2 +1 @@ -bdea@chromium.org drubery@chromium.org diff --git a/chromium/components/safe_browsing/core/browser/tailored_security_service/tailored_security_service.h b/chromium/components/safe_browsing/core/browser/tailored_security_service/tailored_security_service.h index 3a7a7836579..2f85f3af1dd 100644 --- a/chromium/components/safe_browsing/core/browser/tailored_security_service/tailored_security_service.h +++ b/chromium/components/safe_browsing/core/browser/tailored_security_service/tailored_security_service.h @@ -183,7 +183,7 @@ class TailoredSecurityService : public KeyedService { bool is_shut_down_ = false; // The preferences for the given profile. - PrefService* prefs_; + raw_ptr<PrefService> prefs_; // This is used to observe when sync users update their Tailored Security // setting. diff --git a/chromium/components/safe_browsing/core/browser/test_safe_browsing_token_fetcher.cc b/chromium/components/safe_browsing/core/browser/test_safe_browsing_token_fetcher.cc new file mode 100644 index 00000000000..5f07f5324ed --- /dev/null +++ b/chromium/components/safe_browsing/core/browser/test_safe_browsing_token_fetcher.cc @@ -0,0 +1,27 @@ +// Copyright 2022 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 "components/safe_browsing/core/browser/test_safe_browsing_token_fetcher.h" + +namespace safe_browsing { + +TestSafeBrowsingTokenFetcher::TestSafeBrowsingTokenFetcher() = default; +TestSafeBrowsingTokenFetcher::~TestSafeBrowsingTokenFetcher() { + // Like SafeBrowsingTokenFetchTracer, trigger the callback when destroyed. + RunAccessTokenCallback(""); +} +void TestSafeBrowsingTokenFetcher::Start(Callback callback) { + callback_ = std::move(callback); + was_start_called_ = true; +} +void TestSafeBrowsingTokenFetcher::RunAccessTokenCallback(std::string token) { + if (callback_) { + std::move(callback_).Run(token); + } +} +bool TestSafeBrowsingTokenFetcher::WasStartCalled() { + return was_start_called_; +} + +} // namespace safe_browsing diff --git a/chromium/components/safe_browsing/core/browser/test_safe_browsing_token_fetcher.h b/chromium/components/safe_browsing/core/browser/test_safe_browsing_token_fetcher.h new file mode 100644 index 00000000000..78b7cf98eb0 --- /dev/null +++ b/chromium/components/safe_browsing/core/browser/test_safe_browsing_token_fetcher.h @@ -0,0 +1,31 @@ +// Copyright 2022 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. + +#ifndef COMPONENTS_SAFE_BROWSING_CORE_BROWSER_TEST_SAFE_BROWSING_TOKEN_FETCHER_H_ +#define COMPONENTS_SAFE_BROWSING_CORE_BROWSER_TEST_SAFE_BROWSING_TOKEN_FETCHER_H_ + +#include "base/callback.h" +#include "components/safe_browsing/core/browser/safe_browsing_token_fetcher.h" +#include "testing/gmock/include/gmock/gmock.h" + +namespace safe_browsing { + +class TestSafeBrowsingTokenFetcher : public SafeBrowsingTokenFetcher { + public: + TestSafeBrowsingTokenFetcher(); + ~TestSafeBrowsingTokenFetcher() override; + + void Start(Callback callback) override; + void RunAccessTokenCallback(std::string token); + bool WasStartCalled(); + MOCK_METHOD1(OnInvalidAccessToken, void(const std::string&)); + + private: + Callback callback_; + bool was_start_called_ = false; +}; + +} // namespace safe_browsing + +#endif // COMPONENTS_SAFE_BROWSING_CORE_BROWSER_TEST_SAFE_BROWSING_TOKEN_FETCHER_H_ diff --git a/chromium/components/safe_browsing/core/browser/verdict_cache_manager.cc b/chromium/components/safe_browsing/core/browser/verdict_cache_manager.cc index 7ee2f1b7557..ab059db7f06 100644 --- a/chromium/components/safe_browsing/core/browser/verdict_cache_manager.cc +++ b/chromium/components/safe_browsing/core/browser/verdict_cache_manager.cc @@ -428,6 +428,11 @@ void VerdictCacheManager::Shutdown() { history_service_observation_.Reset(); pref_change_registrar_.RemoveAll(); sync_observer_.reset(); + + // Clear references to other KeyedServices. + content_settings_ = nullptr; + + is_shut_down_ = true; weak_factory_.InvalidateWeakPtrs(); } @@ -438,6 +443,9 @@ void VerdictCacheManager::CachePhishGuardVerdict( ReusedPasswordAccountType password_type, const LoginReputationClientResponse& verdict, const base::Time& receive_time) { + if (is_shut_down_) { + return; + } DCHECK(content_settings_); DCHECK(trigger_type == LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE || trigger_type == LoginReputationClientRequest::PASSWORD_REUSE_EVENT); @@ -494,6 +502,9 @@ VerdictCacheManager::GetCachedPhishGuardVerdict( LoginReputationClientResponse* out_response) { DCHECK(trigger_type == LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE || trigger_type == LoginReputationClientRequest::PASSWORD_REUSE_EVENT); + if (is_shut_down_) { + return LoginReputationClientResponse::VERDICT_TYPE_UNSPECIFIED; + } std::string type_key = GetKeyOfTypeFromTriggerType(trigger_type, password_type); @@ -505,6 +516,9 @@ VerdictCacheManager::GetCachedPhishGuardVerdict( size_t VerdictCacheManager::GetStoredPhishGuardVerdictCount( LoginReputationClientRequest::TriggerType trigger_type) { + if (is_shut_down_) { + return 0; + } DCHECK(content_settings_); DCHECK(trigger_type == LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE || trigger_type == LoginReputationClientRequest::PASSWORD_REUSE_EVENT); @@ -535,6 +549,9 @@ size_t VerdictCacheManager::GetStoredPhishGuardVerdictCount( } size_t VerdictCacheManager::GetStoredRealTimeUrlCheckVerdictCount() { + if (is_shut_down_) { + return 0; + } // If we have already computed this, return its value. if (stored_verdict_count_real_time_url_check_.has_value()) return stored_verdict_count_real_time_url_check_.value(); @@ -558,6 +575,9 @@ void VerdictCacheManager::CacheRealTimeUrlVerdict( const GURL& url, const RTLookupResponse& verdict, const base::Time& receive_time) { + if (is_shut_down_) { + return; + } std::vector<std::string> visited_cache_expressions; for (const auto& threat_info : verdict.threat_info()) { // If |cache_expression_match_type| is unspecified, ignore this entry. @@ -617,6 +637,9 @@ RTLookupResponse::ThreatInfo::VerdictType VerdictCacheManager::GetCachedRealTimeUrlVerdict( const GURL& url, RTLookupResponse::ThreatInfo* out_threat_info) { + if (is_shut_down_) { + return RTLookupResponse::ThreatInfo::VERDICT_TYPE_UNSPECIFIED; + } return GetMostMatchingCachedVerdictWithHostAndPathMatching< RTLookupResponse::ThreatInfo>( url, kRealTimeUrlCacheKey, content_settings_, @@ -663,6 +686,9 @@ void VerdictCacheManager::ScheduleNextCleanUpAfterInterval( } void VerdictCacheManager::CleanUpExpiredVerdicts() { + if (is_shut_down_) { + return; + } DCHECK(content_settings_); SCOPED_UMA_HISTOGRAM_TIMER("SafeBrowsing.RT.CacheManager.CleanUpTime"); CleanUpExpiredPhishGuardVerdicts(); @@ -842,6 +868,9 @@ bool VerdictCacheManager::RemoveExpiredRealTimeUrlCheckVerdicts( void VerdictCacheManager::RemoveContentSettingsOnURLsDeleted( bool all_history, const history::URLRows& deleted_rows) { + if (is_shut_down_) { + return; + } DCHECK(content_settings_); if (all_history) { diff --git a/chromium/components/safe_browsing/core/browser/verdict_cache_manager.h b/chromium/components/safe_browsing/core/browser/verdict_cache_manager.h index 1a7bda21a02..f46d70cc51d 100644 --- a/chromium/components/safe_browsing/core/browser/verdict_cache_manager.h +++ b/chromium/components/safe_browsing/core/browser/verdict_cache_manager.h @@ -203,6 +203,8 @@ class VerdictCacheManager : public history::HistoryServiceObserver, std::unique_ptr<SafeBrowsingSyncObserver> sync_observer_; + bool is_shut_down_ = false; + base::WeakPtrFactory<VerdictCacheManager> weak_factory_{this}; static bool has_artificial_unsafe_url_; diff --git a/chromium/components/safe_browsing/core/browser/verdict_cache_manager_unittest.cc b/chromium/components/safe_browsing/core/browser/verdict_cache_manager_unittest.cc index b8e1b1f7421..4be882da08a 100644 --- a/chromium/components/safe_browsing/core/browser/verdict_cache_manager_unittest.cc +++ b/chromium/components/safe_browsing/core/browser/verdict_cache_manager_unittest.cc @@ -860,4 +860,27 @@ TEST_F(VerdictCacheManagerTest, TestClearTokenOnSyncStateChanged) { ASSERT_FALSE(token.has_token_value()); } +TEST_F(VerdictCacheManagerTest, TestShutdown) { + cache_manager_->Shutdown(); + RTLookupResponse rt_response; + // Call to cache_manager after shutdown should not cause a crash. + cache_manager_->CacheRealTimeUrlVerdict(GURL("https://www.example.com/"), + rt_response, base::Time::Now()); + RTLookupResponse::ThreatInfo out_rt_verdict; + cache_manager_->GetCachedRealTimeUrlVerdict( + GURL("https://www.example.com/path"), &out_rt_verdict); + LoginReputationClientResponse pg_response; + ReusedPasswordAccountType password_type; + cache_manager_->CachePhishGuardVerdict( + LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE, password_type, + pg_response, base::Time::Now()); + cache_manager_->GetStoredPhishGuardVerdictCount( + LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE); + LoginReputationClientResponse out_pg_verdict; + cache_manager_->GetCachedPhishGuardVerdict( + GURL("https://www.example.com/path"), + LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE, password_type, + &out_pg_verdict); +} + } // namespace safe_browsing diff --git a/chromium/components/safe_browsing/core/common/fbs/client_model.fbs b/chromium/components/safe_browsing/core/common/fbs/client_model.fbs index 2cc099860a4..7bb1c227223 100644 --- a/chromium/components/safe_browsing/core/common/fbs/client_model.fbs +++ b/chromium/components/safe_browsing/core/common/fbs/client_model.fbs @@ -39,6 +39,7 @@ table ClientSideModel { tflite_model_input_width: int (deprecated); tflite_model_input_height: int (deprecated); tflite_metadata:safe_browsing.flat.TfLiteModelMetadata; + dom_model_version:int; } root_type ClientSideModel; diff --git a/chromium/components/safe_browsing/core/common/features.cc b/chromium/components/safe_browsing/core/common/features.cc index 15ac35d23fa..f516af703a2 100644 --- a/chromium/components/safe_browsing/core/common/features.cc +++ b/chromium/components/safe_browsing/core/common/features.cc @@ -46,13 +46,25 @@ extern const base::Feature kClientSideDetectionModelTag{ const base::Feature kClientSideDetectionReferrerChain{ "ClientSideDetectionReferrerChain", base::FEATURE_ENABLED_BY_DEFAULT}; +const base::Feature kClientSideDetectionKillswitch{ + "ClientSideDetectionKillswitch", +#if BUILDFLAG(IS_MAC) + base::FEATURE_ENABLED_BY_DEFAULT +#else + base::FEATURE_DISABLED_BY_DEFAULT +#endif +}; + const base::Feature kConnectorsScanningAccessToken{ - "ConnectorsScanningAccessToken", base::FEATURE_DISABLED_BY_DEFAULT}; + "ConnectorsScanningAccessToken", base::FEATURE_ENABLED_BY_DEFAULT}; -// TODO(b/197749390): Add tests for this feature being enabled when it's -// finalized. const base::Feature kConnectorsScanningReportOnlyUI{ - "ConnectorsScanningReportOnlyUI", base::FEATURE_DISABLED_BY_DEFAULT}; + "ConnectorsScanningReportOnlyUI", base::FEATURE_ENABLED_BY_DEFAULT}; + +#if BUILDFLAG(IS_ANDROID) +const base::Feature kCreateSafebrowsingOnStartup{ + "CreateSafebrowsingOnStartup", base::FEATURE_DISABLED_BY_DEFAULT}; +#endif const base::Feature kDelayedWarnings{"SafeBrowsingDelayedWarnings", base::FEATURE_DISABLED_BY_DEFAULT}; @@ -76,6 +88,10 @@ const base::Feature kEnhancedProtection { #endif }; +const base::Feature kEnhancedProtectionPhase2IOS{ + "SafeBrowsingEnhancedProtectionPhase2IOS", + base::FEATURE_DISABLED_BY_DEFAULT}; + const base::Feature kExtensionTelemetry{"SafeBrowsingExtensionTelemetry", base::FEATURE_DISABLED_BY_DEFAULT}; @@ -86,6 +102,11 @@ const base::Feature kExtensionTelemetryPersistence{ const base::FeatureParam<int> kExtensionTelemetryUploadIntervalSeconds{ &kExtensionTelemetry, "UploadIntervalSeconds", /*default_value=*/3600}; + +const base::FeatureParam<int> kExtensionTelemetryWritesPerInterval{ + &kExtensionTelemetry, "NumberOfWritesInInterval", + /*default_value=*/4}; + const base::Feature kExtensionTelemetryTabsExecuteScriptSignal{ "SafeBrowsingExtensionTelemetryTabsExecuteScriptSignal", base::FEATURE_DISABLED_BY_DEFAULT}; @@ -110,9 +131,6 @@ const base::Feature kOmitNonUserGesturesFromReferrerChain{ const base::Feature kSafeBrowsingCsbrrWithToken{ "SafeBrowsingCsbrrWithToken", base::FEATURE_DISABLED_BY_DEFAULT}; -const base::Feature kSafeBrowsingCTDownloadWarning{ - "SafeBrowsingCTDownloadWarning", base::FEATURE_DISABLED_BY_DEFAULT}; - const base::Feature kSafeBrowsingEnterpriseCsd{ "SafeBrowsingEnterpriseCsd", base::FEATURE_ENABLED_BY_DEFAULT}; @@ -129,7 +147,7 @@ const base::Feature kSafeBrowsingRemoveCookiesInAuthRequests{ const base::Feature kSendSampledPingsForProtegoAllowlistDomains{ "SafeBrowsingSendSampledPingsForProtegoAllowlistDomains", - base::FEATURE_DISABLED_BY_DEFAULT}; + base::FEATURE_ENABLED_BY_DEFAULT}; const base::Feature kSuspiciousSiteTriggerQuotaFeature{ "SafeBrowsingSuspiciousSiteTriggerQuota", base::FEATURE_ENABLED_BY_DEFAULT}; @@ -137,10 +155,6 @@ const base::Feature kSuspiciousSiteTriggerQuotaFeature{ const base::Feature kThreatDomDetailsTagAndAttributeFeature{ "ThreatDomDetailsTagAttributes", base::FEATURE_DISABLED_BY_DEFAULT}; -const base::Feature kTriggerThrottlerDailyQuotaFeature{ - "SafeBrowsingTriggerThrottlerDailyQuota", - base::FEATURE_DISABLED_BY_DEFAULT}; - const base::Feature kUseNewDownloadWarnings{"UseNewDownloadWarnings", base::FEATURE_DISABLED_BY_DEFAULT}; @@ -168,6 +182,7 @@ constexpr struct { {&kDelayedWarnings, true}, {&kDownloadBubble, true}, {&kEnhancedProtection, true}, + {&kEnhancedProtectionPhase2IOS, true}, {&kExtensionTelemetry, true}, {&kExtensionTelemetryReportContactedHosts, true}, {&kExtensionTelemetryPersistence, true}, @@ -179,7 +194,6 @@ constexpr struct { {&kSendSampledPingsForProtegoAllowlistDomains, true}, {&kSuspiciousSiteTriggerQuotaFeature, true}, {&kThreatDomDetailsTagAndAttributeFeature, false}, - {&kTriggerThrottlerDailyQuotaFeature, false}, }; // Adds the name and the enabled/disabled status of a given feature. diff --git a/chromium/components/safe_browsing/core/common/features.h b/chromium/components/safe_browsing/core/common/features.h index 8c66f7be89f..ca50054e3a1 100644 --- a/chromium/components/safe_browsing/core/common/features.h +++ b/chromium/components/safe_browsing/core/common/features.h @@ -43,6 +43,13 @@ const char kClientSideDetectionTagParamName[] = "reporter_omaha_tag"; // Enables client side detection referrer chain. extern const base::Feature kClientSideDetectionReferrerChain; +// Killswitch for client side phishing detection. Since client side models are +// run on a large fraction of navigations, crashes due to the model are very +// impactful, even if only a small fraction of users have a bad version of the +// model. This Finch flag allows us to remediate long-tail component versions +// while we fix the root cause. +extern const base::Feature kClientSideDetectionKillswitch; + // Controls whether an access token is attached to scanning requests triggered // by enterprise Connectors. extern const base::Feature kConnectorsScanningAccessToken; @@ -53,6 +60,13 @@ extern const base::Feature kConnectorsScanningAccessToken; // instead of just showing an "Open Now" button with the blocking UI. extern const base::Feature kConnectorsScanningReportOnlyUI; +// Controls whether to connect to the Safe Browsing service early on startup. +// The alternative is to connect as soon as the first Safe Browsing check is +// made associated with a URK request. Android only. On this platform getting +// the notification about the success of establishing the connection can be +// delayed by several seconds. +extern const base::Feature kCreateSafebrowsingOnStartup; + // Controls whether the delayed warning experiment is enabled. extern const base::Feature kDelayedWarnings; // True if mouse clicks should undelay the warnings immediately when delayed @@ -65,6 +79,9 @@ extern const base::Feature kDownloadBubble; // Enables Enhanced Safe Browsing. extern const base::Feature kEnhancedProtection; +// Phase 2 of Enhanced Safe Browsing changes. +extern const base::Feature kEnhancedProtectionPhase2IOS; + // Enables collection of signals related to extension activity and uploads // of telemetry reports to SB servers. extern const base::Feature kExtensionTelemetry; @@ -75,6 +92,11 @@ extern const base::Feature kExtensionTelemetryPersistence; // Specifies the upload interval for extension telemetry reports. extern const base::FeatureParam<int> kExtensionTelemetryUploadIntervalSeconds; + +// Specifies the number of writes the telemetry service will perform during +// a full upload interval. +extern const base::FeatureParam<int> kExtensionTelemetryWritesPerInterval; + // Enables collection of telemetry signal whenever an extension invokes the // tabs.executeScript API call. extern const base::Feature kExtensionTelemetryTabsExecuteScriptSignal; @@ -96,10 +118,6 @@ extern const base::Feature kOmitNonUserGesturesFromReferrerChain; // for Enhanced Safe Browsing users extern const base::Feature kSafeBrowsingCsbrrWithToken; -// Controls whether users will see an account compromise specific warning -// when Safe Browsing determines a file is associated with stealing cookies. -extern const base::Feature kSafeBrowsingCTDownloadWarning; - // Controls whether we are performing enterprise download checks for users // with the appropriate policies enabled. extern const base::Feature kSafeBrowsingEnterpriseCsd; @@ -138,16 +156,6 @@ extern const base::Feature kTailoredSecurityIntegration; // be lower case. extern const base::Feature kThreatDomDetailsTagAndAttributeFeature; -// Controls the daily quota for data collection triggers. It's a single param -// containing a comma-separated list of pairs. The format of the param is -// "T1,Q1,T2,Q2,...Tn,Qn", where Tx is a TriggerType and Qx is how many reports -// that trigger is allowed to send per day. -// TODO(crbug.com/744869): This param should be deprecated after ad sampler -// launch in favour of having a unique quota feature and param per trigger. -// Having a single shared feature makes it impossible to run multiple trigger -// trials simultaneously. -extern const base::Feature kTriggerThrottlerDailyQuotaFeature; - // Controls whether Chrome uses new download warning UX. extern const base::Feature kUseNewDownloadWarnings; diff --git a/chromium/components/safe_browsing/core/common/proto/client_model.proto b/chromium/components/safe_browsing/core/common/proto/client_model.proto index 3c74599ef8a..920711a5700 100644 --- a/chromium/components/safe_browsing/core/common/proto/client_model.proto +++ b/chromium/components/safe_browsing/core/common/proto/client_model.proto @@ -70,9 +70,11 @@ message ClientSideModel { // Page terms in page_term contain at most this many page words. required int32 max_words_per_term = 5; - // Model version number. Every model that we train should have a different - // version number and it should always be larger than the previous model - // version. + optional int32 dom_model_version = 18; + + // The overall client model version number. Every model update should have a + // different version number and it should always be larger than the previous + // model version. optional int32 version = 6; // List of known bad IP subnets. @@ -107,7 +109,7 @@ message ClientSideModel { optional TfLiteModelMetadata tflite_metadata = 17; - // next available tag number: 18 + // next available tag number: 19 } message TfLiteModelMetadata { diff --git a/chromium/components/safe_browsing/core/common/proto/csd.proto b/chromium/components/safe_browsing/core/common/proto/csd.proto index 0ab63881eee..4112daa103f 100644 --- a/chromium/components/safe_browsing/core/common/proto/csd.proto +++ b/chromium/components/safe_browsing/core/common/proto/csd.proto @@ -109,6 +109,18 @@ message ChromeUserPopulation { // Note: This field is set as repeated to support tokens from multiple // sources. repeated PageLoadToken page_load_tokens = 14; + + // The current state of account-level enhanced safe browsing (A-ESB) as is + // known by the client. This is an optional field and represents the state of + // A-ESB as the client has observed it to be. This value will be set for sync + // users as well as signed-in users. The state on the server may be + // different from the value that the client has when setting this field. + // See: go/esb-mms-integration-dd. + optional bool is_aesb_enabled = 15; + + // The time when the account-level enhanced safe browsing (A-ESB) bit state + // was last sent updated on the client. This is an optional field. + optional int64 aesb_last_update_time_windows_epoch_micros = 16; } message ClientPhishingRequest { @@ -148,6 +160,9 @@ message ClientPhishingRequest { // sent to the scorer and which resulted in client_score being computed. repeated Feature feature_map = 5; + // The version of the DOM model used for classification + optional int32 dom_model_version = 27; + // The version number of the model that was used to compute the client-score. // Copied from ClientSideModel.version(). optional int32 model_version = 6; @@ -215,7 +230,7 @@ message ClientPhishingRequest { // users. optional VisualFeatures visual_features = 26; - // next available tag number: 27. + // next available tag number: 28. } message ClientPhishingResponse { diff --git a/chromium/components/safe_browsing/core/common/safe_browsing_prefs.cc b/chromium/components/safe_browsing/core/common/safe_browsing_prefs.cc index f63da778f92..cf55188e9e5 100644 --- a/chromium/components/safe_browsing/core/common/safe_browsing_prefs.cc +++ b/chromium/components/safe_browsing/core/common/safe_browsing_prefs.cc @@ -9,6 +9,7 @@ #include "base/metrics/histogram_macros.h" #include "base/notreached.h" #include "base/strings/string_number_conversions.h" +#include "base/time/time.h" #include "components/pref_registry/pref_registry_syncable.h" #include "components/prefs/pref_registry_simple.h" #include "components/prefs/pref_service.h" @@ -111,7 +112,8 @@ const char kAccountTailoredSecurityShownNotification[] = "safebrowsing.aesb_shown_notification"; const char kEnhancedProtectionEnabledViaTailoredSecurity[] = "safebrowsing.esb_enabled_via_tailored_security"; - +const char kExtensionTelemetryLastUploadTime[] = + "safebrowsing.extension_telemetry_last_upload_time"; } // namespace prefs namespace safe_browsing { @@ -231,6 +233,17 @@ void RegisterProfilePrefs(PrefRegistrySimple* registry) { prefs::kAccountTailoredSecurityShownNotification, false); registry->RegisterBooleanPref( prefs::kEnhancedProtectionEnabledViaTailoredSecurity, false); + registry->RegisterTimePref(prefs::kExtensionTelemetryLastUploadTime, + base::Time::Now()); +} + +base::Time GetLastUploadTimeForExtensionTelemetry(PrefService& prefs) { + return (prefs.GetTime(prefs::kExtensionTelemetryLastUploadTime)); +} + +void SetLastUploadTimeForExtensionTelemetry(PrefService& prefs, + const base::Time& time) { + prefs.SetTime(prefs::kExtensionTelemetryLastUploadTime, time); } void RegisterLocalStatePrefs(PrefRegistrySimple* registry) { diff --git a/chromium/components/safe_browsing/core/common/safe_browsing_prefs.h b/chromium/components/safe_browsing/core/common/safe_browsing_prefs.h index 05136ba29e4..c32c617df1d 100644 --- a/chromium/components/safe_browsing/core/common/safe_browsing_prefs.h +++ b/chromium/components/safe_browsing/core/common/safe_browsing_prefs.h @@ -18,6 +18,10 @@ class PrefRegistrySimple; class PrefService; class GURL; +namespace base { +class Time; +} + namespace prefs { // A list of times at which CSD pings were sent. extern const char kSafeBrowsingCsdPingTimestamps[]; @@ -123,6 +127,10 @@ extern const char kAccountTailoredSecurityShownNotification[]; // account tailored security. extern const char kEnhancedProtectionEnabledViaTailoredSecurity[]; +// The last time the Extension Telemetry Service successfully +// uploaded its data. +extern const char kExtensionTelemetryLastUploadTime[]; + } // namespace prefs namespace safe_browsing { @@ -251,6 +259,14 @@ void SetExtendedReportingPrefAndMetric(PrefService* prefs, // This variant is used to simplify test code by omitting the location. void SetExtendedReportingPrefForTests(PrefService* prefs, bool value); +// Sets the last time the Extension Telemetry Service successfully uploaded +// its data. +void SetLastUploadTimeForExtensionTelemetry(PrefService& prefs, + const base::Time& time); + +// Returns the `kExtensionTelemetryLastUploadTime` user preference. +base::Time GetLastUploadTimeForExtensionTelemetry(PrefService& prefs); + // Sets the currently active Safe Browsing Enhanced Protection to the specified // value. void SetEnhancedProtectionPrefForTests(PrefService* prefs, bool value); diff --git a/chromium/components/safe_browsing/core/common/utils.cc b/chromium/components/safe_browsing/core/common/utils.cc index e9f7c2984a5..b43b1db95cb 100644 --- a/chromium/components/safe_browsing/core/common/utils.cc +++ b/chromium/components/safe_browsing/core/common/utils.cc @@ -51,7 +51,7 @@ std::string ShortURLForReporting(const GURL& url) { ChromeUserPopulation::ProfileManagementStatus GetProfileManagementStatus( const policy::BrowserPolicyConnector* bpc) { #if BUILDFLAG(IS_WIN) - if (base::IsMachineExternallyManaged()) + if (base::IsManagedDevice()) return ChromeUserPopulation::ENTERPRISE_MANAGED; else return ChromeUserPopulation::NOT_MANAGED; |