Protego: Reduce timeout; perform hash-based checks when cached verdict is SAFE

Fixed: 1119987
Change-Id: I0a6ed16da25a0ae09ad188d21fa1223dc063c007
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2368413
Commit-Queue: Varun Khaneja <vakh@chromium.org>
Auto-Submit: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800405}

chrome/browser/safe_browsing/chrome_enterprise_url_lookup_service_unittest.cc

@@ -143,7 +143,8 @@ TEST_F(ChromeEnterpriseRealTimeUrlLookupServiceTest,
 
   // |request_callback| should not be called if the verdict is already cached.
   EXPECT_CALL(request_callback, Run(_, _)).Times(0);
-  EXPECT_CALL(response_callback, Run(/* is_rt_lookup_successful */ true, _));
+  EXPECT_CALL(response_callback, Run(/* is_rt_lookup_successful */ true,
+                                     /* is_cached_response */ true, _));
 
   task_environment_.RunUntilIdle();
 }
@@ -174,7 +175,8 @@ TEST_F(ChromeEnterpriseRealTimeUrlLookupServiceTest,
           }),
       response_callback.Get());
 
-  EXPECT_CALL(response_callback, Run(/* is_rt_lookup_successful */ true, _));
+  EXPECT_CALL(response_callback, Run(/* is_rt_lookup_successful */ true,
+                                     /* is_cached_response */ false, _));
 
   task_environment_.RunUntilIdle();
 

components/safe_browsing/core/browser/safe_browsing_url_checker_impl.cc

@@ -588,6 +588,7 @@ void SafeBrowsingUrlCheckerImpl::OnRTLookupRequest(
 
 void SafeBrowsingUrlCheckerImpl::OnRTLookupResponse(
     bool is_rt_lookup_successful,
+    bool is_cached_response,
     std::unique_ptr<RTLookupResponse> response) {
   DCHECK(CurrentlyOnThread(ThreadID::IO));
   bool is_expected_resource_type =
@@ -616,7 +617,12 @@ void SafeBrowsingUrlCheckerImpl::OnRTLookupResponse(
         RealTimeUrlLookupServiceBase::GetSBThreatTypeForRTThreatType(
             response->threat_info(0).threat_type());
   }
-  OnUrlResult(url, sb_threat_type, ThreatMetadata());
+  if (is_cached_response && sb_threat_type == SB_THREAT_TYPE_SAFE) {
+    // TODO(vakh): Add a UMA metric.
+    PerformHashBasedCheck(url);
+  } else {
+    OnUrlResult(url, sb_threat_type, ThreatMetadata());
+  }
 }
 
 }  // namespace safe_browsing

components/safe_browsing/core/browser/safe_browsing_url_checker_impl.h

@@ -193,7 +193,10 @@ class SafeBrowsingUrlCheckerImpl : public mojom::SafeBrowsingUrlChecker,
   // Called when the |response| from the real-time lookup service is received.
   // |is_rt_lookup_successful| is true if the response code is OK and the
   // response body is successfully parsed.
+  // |is_cached_response| is true if the response is a cache hit. In such a
+  // case, fall back to hash-based checks if the cached verdict is |SAFE|.
   void OnRTLookupResponse(bool is_rt_lookup_successful,
+                          bool is_cached_response,
                           std::unique_ptr<RTLookupResponse> response);
 
   // Logs |request| on any open chrome://safe-browsing pages.

components/safe_browsing/core/browser/safe_browsing_url_checker_impl_unittest.cc

@@ -194,10 +194,11 @@ class MockRealTimeUrlLookupService : public RealTimeUrlLookupService {
     threat_info.set_threat_type(threat_type);
     threat_info.set_verdict_type(verdict_type);
     *new_threat_info = threat_info;
-    base::PostTask(FROM_HERE, CreateTaskTraits(ThreadID::IO),
-                   base::BindOnce(std::move(response_callback),
-                                  /* is_rt_lookup_successful */ true,
-                                  std::move(response)));
+    base::PostTask(
+        FROM_HERE, CreateTaskTraits(ThreadID::IO),
+        base::BindOnce(std::move(response_callback),
+                       /* is_rt_lookup_successful */ true,
+                       /* is_cached_response */ false, std::move(response)));
   }
 
   void SetThreatTypeForUrl(const GURL& gurl, SBThreatType threat_type) {

components/safe_browsing/core/proto/realtimeapi.proto

@@ -43,6 +43,8 @@ message RTLookupRequest {
   // Version 1:
   //   * Client supports caching with |COVERING_MATCH| verdicts.
   //   * Client supports the |os_type| field.
+  //   * Client falls back to hash-based checks if there's a SAFE match in the
+  //     cache.
   optional int32 version = 6 [default = 0];
 
   // The operating system of the client.

components/safe_browsing/core/realtime/url_lookup_service_base.cc

@@ -35,7 +35,7 @@ const size_t kMaxFailuresToEnforceBackoff = 3;
 const size_t kMinBackOffResetDurationInSeconds = 5 * 60;   //  5 minutes.
 const size_t kMaxBackOffResetDurationInSeconds = 30 * 60;  // 30 minutes.
 
-const size_t kURLLookupTimeoutDurationInSeconds = 10;  // 10 seconds.
+const size_t kURLLookupTimeoutDurationInSeconds = 3;
 
 constexpr char kAuthHeaderBearer[] = "Bearer ";
 
@@ -313,6 +313,7 @@ void RealTimeUrlLookupServiceBase::StartLookup(
     base::PostTask(FROM_HERE, CreateTaskTraits(ThreadID::IO),
                    base::BindOnce(std::move(response_callback),
                                   /* is_rt_lookup_successful */ true,
+                                  /* is_cached_response */ true,
                                   std::move(cache_response)));
     return;
   }
@@ -415,9 +416,10 @@ void RealTimeUrlLookupServiceBase::OnURLLoaderComplete(
                                      GetMetricSuffix(),
                                      response->threat_info_size());
 
-  base::PostTask(FROM_HERE, CreateTaskTraits(ThreadID::IO),
-                 base::BindOnce(std::move(it->second), is_rt_lookup_successful,
-                                std::move(response)));
+  base::PostTask(
+      FROM_HERE, CreateTaskTraits(ThreadID::IO),
+      base::BindOnce(std::move(it->second), is_rt_lookup_successful,
+                     /* is_cached_response */ false, std::move(response)));
 
   delete it->first;
   pending_requests_.erase(it);
@@ -473,10 +475,12 @@ bool RealTimeUrlLookupServiceBase::IsHistorySyncEnabled() {
 
 void RealTimeUrlLookupServiceBase::Shutdown() {
   for (auto& pending : pending_requests_) {
-    // Treat all pending requests as safe.
+    // Treat all pending requests as safe, and not from cache so that a
+    // hash-based check isn't performed.
     auto response = std::make_unique<RTLookupResponse>();
     std::move(pending.second)
-        .Run(/* is_rt_lookup_successful */ true, std::move(response));
+        .Run(/* is_rt_lookup_successful */ true, /* is_cached_response */ false,
+             std::move(response));
     delete pending.first;
   }
   pending_requests_.clear();

components/safe_browsing/core/realtime/url_lookup_service_base.h

@@ -42,7 +42,7 @@ using RTLookupRequestCallback =
     base::OnceCallback<void(std::unique_ptr<RTLookupRequest>, std::string)>;
 
 using RTLookupResponseCallback =
-    base::OnceCallback<void(bool, std::unique_ptr<RTLookupResponse>)>;
+    base::OnceCallback<void(bool, bool, std::unique_ptr<RTLookupResponse>)>;
 
 class VerdictCacheManager;
 

components/safe_browsing/core/realtime/url_lookup_service_unittest.cc

@@ -575,7 +575,8 @@ TEST_F(RealTimeUrlLookupServiceTest, TestStartLookup_ResponseIsAlreadyCached) {
 
   // |request_callback| should not be called.
   EXPECT_CALL(request_callback, Run(_, _)).Times(0);
-  EXPECT_CALL(response_callback, Run(/* is_rt_lookup_successful */ true, _));
+  EXPECT_CALL(response_callback, Run(/* is_rt_lookup_successful */ true,
+                                     /* is_cached_response */ true, _));
 
   task_environment_->RunUntilIdle();
 
@@ -608,7 +609,8 @@ TEST_F(RealTimeUrlLookupServiceTest,
           }),
       response_callback.Get());
 
-  EXPECT_CALL(response_callback, Run(/* is_rt_lookup_successful */ true, _));
+  EXPECT_CALL(response_callback, Run(/* is_rt_lookup_successful */ true,
+                                     /* is_cached_response */ false, _));
 
   WaitForAccessTokenRequestIfNecessaryAndRespondWithToken(
       "access_token_string");
@@ -642,7 +644,8 @@ TEST_F(RealTimeUrlLookupServiceTest, TestStartLookup_NoTokenWhenNotSignedIn) {
           }),
       response_callback.Get());
 
-  EXPECT_CALL(response_callback, Run(/* is_rt_lookup_successful */ true, _));
+  EXPECT_CALL(response_callback, Run(/* is_rt_lookup_successful */ true,
+                                     /* is_cached_response */ false, _));
 
   task_environment_->RunUntilIdle();
 
@@ -672,7 +675,8 @@ TEST_F(RealTimeUrlLookupServiceTest,
           }),
       response_callback.Get());
 
-  EXPECT_CALL(response_callback, Run(/* is_rt_lookup_successful */ true, _));
+  EXPECT_CALL(response_callback, Run(/* is_rt_lookup_successful */ true,
+                                     /* is_cached_response */ false, _));
 
   task_environment_->RunUntilIdle();