New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 714300 link

Starred by 1 user
Status: Fixed
Owner:
█ lpz@chromium.org
Last visit > 30 days ago
Closed: Aug 2017
Cc:
vakh@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocked on:
issue 730053


Sign in to add a comment

Make the CSD phishing whitelist checking asynchronous

Project Member Reported by nparker@chromium.org, Apr 21 2017 
Checking this whitelist shouldn't send full-hash checks if the full 32B hash is in the DB, but should send one if there's a hash-prefix match.

The existing CSD phishing check is synchronous since it expects 32 byte hashes. We'll probably need to create a new CheckCsdWhitelistUrl() interface to replace MatchCsdWhitelistUrl(), and change all the existing calls to handle asynchronicity.

This will be just for Pver4, but we'll need to not break Pver3.

Comment 1 by jialiul@chromium.org, May 5 2017

Labels: SafeBrowsing-Triaged
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 1 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/56e1ea730204dbc043f58b14fec3cb6dfb6a4739

commit 56e1ea730204dbc043f58b14fec3cb6dfb6a4739
Author: nparker <nparker@chromium.org>
Date: Thu Jun 01 18:53:40 2017

Add the ability to check the CSD Whitelist asynchronously,for PhishGuard.

This adds V4LocalDatabaseManager::CheckCsdWhitelistUrl() and tests for
it. A later CL will switch the three callers of MatchCsdWhitelistUrl()
to this new function, and remove the Match method.

BUG= 714300 

Review-Url: https://codereview.chromium.org/2890293004
Cr-Commit-Position: refs/heads/master@{#476372}

[modify] https://crrev.com/56e1ea730204dbc043f58b14fec3cb6dfb6a4739/chrome/browser/safe_browsing/local_database_manager.cc
[modify] https://crrev.com/56e1ea730204dbc043f58b14fec3cb6dfb6a4739/chrome/browser/safe_browsing/local_database_manager.h
[modify] https://crrev.com/56e1ea730204dbc043f58b14fec3cb6dfb6a4739/chrome/browser/ssl/security_state_tab_helper.cc
[modify] https://crrev.com/56e1ea730204dbc043f58b14fec3cb6dfb6a4739/components/safe_browsing_db/database_manager.h
[modify] https://crrev.com/56e1ea730204dbc043f58b14fec3cb6dfb6a4739/components/safe_browsing_db/remote_database_manager.cc
[modify] https://crrev.com/56e1ea730204dbc043f58b14fec3cb6dfb6a4739/components/safe_browsing_db/remote_database_manager.h
[modify] https://crrev.com/56e1ea730204dbc043f58b14fec3cb6dfb6a4739/components/safe_browsing_db/test_database_manager.cc
[modify] https://crrev.com/56e1ea730204dbc043f58b14fec3cb6dfb6a4739/components/safe_browsing_db/test_database_manager.h
[modify] https://crrev.com/56e1ea730204dbc043f58b14fec3cb6dfb6a4739/components/safe_browsing_db/v4_local_database_manager.cc
[modify] https://crrev.com/56e1ea730204dbc043f58b14fec3cb6dfb6a4739/components/safe_browsing_db/v4_local_database_manager.h
[modify] https://crrev.com/56e1ea730204dbc043f58b14fec3cb6dfb6a4739/components/safe_browsing_db/v4_local_database_manager_unittest.cc
[modify] https://crrev.com/56e1ea730204dbc043f58b14fec3cb6dfb6a4739/components/safe_browsing_db/v4_protocol_manager_util.h

Comment 4 by nparker@chromium.org, Jun 6 2017

Blockedon: 730053
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 17 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/59d5b2b8766a4f7eefe357f60f7af2d240c3e1d7

commit 59d5b2b8766a4f7eefe357f60f7af2d240c3e1d7
Author: Nathan Parker <nparker@chromium.org>
Date: Sat Jun 17 02:07:01 2017

Switch to async whitelist checking in PasswordProtectionRequest

This adds a new WhitelistCheckerClient class that lets the caller
provide a callback rather than extending
SafeBrowsingDatabaseManager::Client, and makes ownership a bit simpler.

Bug:  714300 
Change-Id: I1005debdbd9afe6314962537b48ce5f25c521614
Reviewed-on: https://chromium-review.googlesource.com/524694
Commit-Queue: Nathan Parker <nparker@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#480271}
[modify] https://crrev.com/59d5b2b8766a4f7eefe357f60f7af2d240c3e1d7/components/safe_browsing/password_protection/BUILD.gn
[modify] https://crrev.com/59d5b2b8766a4f7eefe357f60f7af2d240c3e1d7/components/safe_browsing/password_protection/password_protection_request.cc
[modify] https://crrev.com/59d5b2b8766a4f7eefe357f60f7af2d240c3e1d7/components/safe_browsing/password_protection/password_protection_request.h
[modify] https://crrev.com/59d5b2b8766a4f7eefe357f60f7af2d240c3e1d7/components/safe_browsing/password_protection/password_protection_service.cc
[modify] https://crrev.com/59d5b2b8766a4f7eefe357f60f7af2d240c3e1d7/components/safe_browsing/password_protection/password_protection_service_unittest.cc
[modify] https://crrev.com/59d5b2b8766a4f7eefe357f60f7af2d240c3e1d7/components/safe_browsing_db/BUILD.gn
[add] https://crrev.com/59d5b2b8766a4f7eefe357f60f7af2d240c3e1d7/components/safe_browsing_db/whitelist_checker_client.cc
[add] https://crrev.com/59d5b2b8766a4f7eefe357f60f7af2d240c3e1d7/components/safe_browsing_db/whitelist_checker_client.h
[add] https://crrev.com/59d5b2b8766a4f7eefe357f60f7af2d240c3e1d7/components/safe_browsing_db/whitelist_checker_client_unittest.cc

Comment 6 by nparker@chromium.org, Jul 19 2017

Owner: lpz@chromium.org
The work remaining here is to convert the two other callers of MatchCsdWhitelistUrl() (See the design doc) to use the new method.  I'd assume you could use the WhitelistCheckerClient class I added in the last CL which makes it easier.

The priority: As Pver4 rolls out, clients calling the two un-converted methods will whitelist a bit more than they should since they'll check only the partial hash until this is fixed. This just slightly diminishes the effectiveness of the CSD Phishing model -- not a huge issue.  But we should fix it by M62.

Comment 7 by nparker@chromium.org, Jul 28 2017 

(I pinged the backend bug to check on readiness)
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 22 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0c534f203cbe5e049a3d533ceb9dc0d7f403c79a

commit 0c534f203cbe5e049a3d533ceb9dc0d7f403c79a
Author: Luke Zielinski <lpz@chromium.org>
Date: Tue Aug 22 20:08:39 2017

Use async CSD whitelist check in ClientSideDetectionHost

Bug:  714300 
Change-Id: I700c5d26ff3035d6eb84aa73c574583d2259714d
Reviewed-on: https://chromium-review.googlesource.com/625763
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Commit-Queue: Luke Z <lpz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496417}
[modify] https://crrev.com/0c534f203cbe5e049a3d533ceb9dc0d7f403c79a/chrome/browser/safe_browsing/BUILD.gn
[modify] https://crrev.com/0c534f203cbe5e049a3d533ceb9dc0d7f403c79a/chrome/browser/safe_browsing/client_side_detection_host.cc
[modify] https://crrev.com/0c534f203cbe5e049a3d533ceb9dc0d7f403c79a/chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 23 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cd781edb452caa582f6921d6703ebe026f90f737

commit cd781edb452caa582f6921d6703ebe026f90f737
Author: Luke Zielinski <lpz@chromium.org>
Date: Wed Aug 23 14:00:20 2017

Use async CSD whitelist check in NotificationImageReporter.

Bug:  714300 
Change-Id: I3349cb44fcf7b464b5675ee07de45877d2860911
Reviewed-on: https://chromium-review.googlesource.com/624437
Commit-Queue: Luke Z <lpz@chromium.org>
Reviewed-by: Varun Khaneja (slow) <vakh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496674}
[modify] https://crrev.com/cd781edb452caa582f6921d6703ebe026f90f737/chrome/browser/safe_browsing/notification_image_reporter.cc
[modify] https://crrev.com/cd781edb452caa582f6921d6703ebe026f90f737/chrome/browser/safe_browsing/notification_image_reporter.h
[modify] https://crrev.com/cd781edb452caa582f6921d6703ebe026f90f737/chrome/browser/safe_browsing/notification_image_reporter_unittest.cc

Comment 10 by lpz@chromium.org, Aug 23 2017

Status: Fixed (was: Assigned)
Project Member

Comment 11 by bugdroid1@chromium.org, Nov 16 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/80ab613e67a52907c2c850bc59c11695bb8f8846

commit 80ab613e67a52907c2c850bc59c11695bb8f8846
Author: Nathan Parker <nparker@chromium.org>
Date: Thu Nov 16 06:33:48 2017

Remove MatchCsdWhitelistUrl() from safe browsing code

Bug:  714300 , 712304 
Change-Id: I95df2d633b8838fe77971f1816e0e424e239b60c
Reviewed-on: https://chromium-review.googlesource.com/773658
Commit-Queue: Nathan Parker <nparker@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516999}
[modify] https://crrev.com/80ab613e67a52907c2c850bc59c11695bb8f8846/chrome/browser/safe_browsing/local_database_manager.h
[modify] https://crrev.com/80ab613e67a52907c2c850bc59c11695bb8f8846/components/safe_browsing/android/remote_database_manager.cc
[modify] https://crrev.com/80ab613e67a52907c2c850bc59c11695bb8f8846/components/safe_browsing/android/remote_database_manager.h
[modify] https://crrev.com/80ab613e67a52907c2c850bc59c11695bb8f8846/components/safe_browsing/db/database_manager.h
[modify] https://crrev.com/80ab613e67a52907c2c850bc59c11695bb8f8846/components/safe_browsing/db/test_database_manager.cc
[modify] https://crrev.com/80ab613e67a52907c2c850bc59c11695bb8f8846/components/safe_browsing/db/test_database_manager.h
[modify] https://crrev.com/80ab613e67a52907c2c850bc59c11695bb8f8846/components/safe_browsing/db/v4_local_database_manager.cc
[modify] https://crrev.com/80ab613e67a52907c2c850bc59c11695bb8f8846/components/safe_browsing/db/v4_local_database_manager.h
[modify] https://crrev.com/80ab613e67a52907c2c850bc59c11695bb8f8846/components/safe_browsing/db/v4_local_database_manager_unittest.cc
►

Sign in to add a comment