Enable client side phishing detection on https sites. (issue 2927123004 by jialiul@chromium.org)

347 views
Skip to first unread message

jia...@chromium.org

unread,
Jun 8, 2017, 7:07:38 PM6/8/17
to va...@chromium.org, npa...@chromium.org, chromium...@chromium.org, asvitki...@chromium.org, grt+...@chromium.org, vakh+...@chromium.org, timvo...@chromium.org
Reviewers: vakh (Varun Khaneja), Nathan Parker
CL: https://codereview.chromium.org/2927123004/

Message:
A tiny CL to turn on CSD phishing detection on Https sites.

Description:
Enable client side phishing detection on https sites.

(Client side malware detection is already enabled on https).

BUG=730895

Affected files (+3, -11 lines):
M chrome/browser/safe_browsing/client_side_detection_host.cc
M chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
M tools/metrics/histograms/enums.xml


Index: chrome/browser/safe_browsing/client_side_detection_host.cc
diff --git a/chrome/browser/safe_browsing/client_side_detection_host.cc b/chrome/browser/safe_browsing/client_side_detection_host.cc
index 838bc9dd920e488eda7236efc83c785606b34673..a3ec5533965b76b89ec9b365b9b6fe7e493ec2ad 100644
--- a/chrome/browser/safe_browsing/client_side_detection_host.cc
+++ b/chrome/browser/safe_browsing/client_side_detection_host.cc
@@ -110,14 +110,6 @@ class ClientSideDetectionHost::ShouldClassifyUrlRequest
DontClassifyForMalware(NO_CLASSIFY_PRIVATE_IP);
}

- // For phishing we only classify HTTP pages.
- if (!url_.SchemeIs(url::kHttpScheme)) {
- DVLOG(1) << "Skipping phishing classification for URL: " << url_
- << " because it is not HTTP: "
- << socket_address_.host();
- DontClassifyForPhishing(NO_CLASSIFY_NOT_HTTP_URL);
- }
-
// Don't run any classifier if the tab is incognito.
if (web_contents_->GetBrowserContext()->IsOffTheRecord()) {
DVLOG(1) << "Skipping phishing and malware classification for URL: "
@@ -166,7 +158,7 @@ class ClientSideDetectionHost::ShouldClassifyUrlRequest
NO_CLASSIFY_KILLSWITCH,
NO_CLASSIFY_CANCEL,
NO_CLASSIFY_RESULT_FROM_CACHE,
- NO_CLASSIFY_NOT_HTTP_URL,
+ DEPRECATED_NO_CLASSIFY_NOT_HTTP_URL,

NO_CLASSIFY_MAX // Always add new values before this one.
};
Index: chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
diff --git a/chrome/browser/safe_browsing/client_side_detection_host_unittest.cc b/chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
index 05437fd9a0a1827fc43ed481771e2bcca924fe24..464cb9bd661a4a4ff11dcb4c73924dc65d3e4e60 100644
--- a/chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
+++ b/chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
@@ -1172,7 +1172,7 @@ TEST_F(ClientSideDetectionHostTest, TestPreClassificationCheckHttpsUrl) {
NavigateAndCommit(url);
WaitAndCheckPreClassificationChecks();

- ExpectStartPhishingDetection(NULL);
+ ExpectStartPhishingDetection(&url);
ExpectShouldClassifyForMalwareResult(true);
}

Index: tools/metrics/histograms/enums.xml
diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml
index 074632578aa956f3ca48f908026978c486f99772..a5d34ed3a68ba9d29225e761489f1f43c2f17405 100644
--- a/tools/metrics/histograms/enums.xml
+++ b/tools/metrics/histograms/enums.xml
@@ -31801,7 +31801,7 @@ from previous Chrome versions.
<int value="7" label="KILLSWITCH"/>
<int value="8" label="CANCEL"/>
<int value="9" label="RESULT_FROM_CACHE"/>
- <int value="10" label="NOT_HTTP_URL"/>
+ <int value="10" label="DEPRECATED: NOT_HTTP_URL"/>
</enum>

<enum name="SBClientDownloadCheckDownloadStats" type="int">


va...@chromium.org

unread,
Jun 8, 2017, 7:30:32 PM6/8/17
to jia...@chromium.org, npa...@chromium.org, chromium...@chromium.org, asvitki...@chromium.org, grt+...@chromium.org, vakh+...@chromium.org, timvo...@chromium.org

phishing we only classify HTTP pages.
Any chance this was intended as: this URL is not HTTP (or HTTPS)
but the code only checked for HTTP.

We do this for DB checks also:
https://cs.chromium.org/chromium/src/components/safe_browsing_db/v4_local_database_manager.cc?l=202&rcl=1733d120eb79e2d8d49b18892e60ee1297f646f7

https://codereview.chromium.org/2927123004/

jia...@chromium.org

unread,
Jun 8, 2017, 8:26:14 PM6/8/17
to va...@chromium.org, npa...@chromium.org, chromium...@chromium.org, asvitki...@chromium.org, grt+...@chromium.org, vakh+...@chromium.org, timvo...@chromium.org
Thanks vakh@! CL updated based on your feedback.


https://codereview.chromium.org/2927123004/diff/1/chrome/browser/safe_browsing/client_side_detection_host.cc
File chrome/browser/safe_browsing/client_side_detection_host.cc (left):

https://codereview.chromium.org/2927123004/diff/1/chrome/browser/safe_browsing/client_side_detection_host.cc#oldcode113
chrome/browser/safe_browsing/client_side_detection_host.cc:113: // For
phishing we only classify HTTP pages.
On 2017/06/08 23:30:31, vakh (Varun Khaneja) wrote:
> Any chance this was intended as: this URL is not HTTP (or HTTPS)
> but the code only checked for HTTP.
>
> We do this for DB checks also:
>
https://cs.chromium.org/chromium/src/components/safe_browsing_db/v4_local_database_manager.cc?l=202&rcl=1733d120eb79e2d8d49b18892e60ee1297f646f7

Good catch. You're right. I should be aware of other schemes.

https://codereview.chromium.org/2927123004/

va...@chromium.org

unread,
Jun 10, 2017, 3:11:33 AM6/10/17
to jia...@chromium.org, npa...@chromium.org, chromium...@chromium.org, asvitki...@chromium.org, grt+...@chromium.org, vakh+...@chromium.org, timvo...@chromium.org

jia...@chromium.org

unread,
Jun 12, 2017, 1:36:01 PM6/12/17
to va...@chromium.org, npa...@chromium.org, chromium...@chromium.org, asvitki...@chromium.org, grt+...@chromium.org, vakh+...@chromium.org, timvo...@chromium.org

npa...@chromium.org

unread,
Jun 12, 2017, 5:43:57 PM6/12/17
to jia...@chromium.org, va...@chromium.org, chromium...@chromium.org, asvitki...@chromium.org, grt+...@chromium.org, vakh+...@chromium.org, timvo...@chromium.org
lgtm




https://codereview.chromium.org/2927123004/diff/40001/chrome/browser/safe_browsing/client_side_detection_host.cc
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

https://codereview.chromium.org/2927123004/diff/40001/chrome/browser/safe_browsing/client_side_detection_host.cc#newcode170
chrome/browser/safe_browsing/client_side_detection_host.cc:170:
NO_CLASSIFY_SCHEME_NOT_SUPPORTED,
nit: add integer values to these to verify they match up with UMA
histogram.

https://codereview.chromium.org/2927123004/

jia...@chromium.org

unread,
Jun 12, 2017, 5:59:36 PM6/12/17
to va...@chromium.org, npa...@chromium.org, chromium...@chromium.org, asvitki...@chromium.org, grt+...@chromium.org, vakh+...@chromium.org, timvo...@chromium.org
Thanks!



https://codereview.chromium.org/2927123004/diff/40001/chrome/browser/safe_browsing/client_side_detection_host.cc
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

https://codereview.chromium.org/2927123004/diff/40001/chrome/browser/safe_browsing/client_side_detection_host.cc#newcode170
chrome/browser/safe_browsing/client_side_detection_host.cc:170:
NO_CLASSIFY_SCHEME_NOT_SUPPORTED,
On 2017/06/12 21:43:56, Nathan Parker wrote:
> nit: add integer values to these to verify they match up with UMA
histogram.

commit-bot@chromium.org via codereview.chromium.org

unread,
Jun 12, 2017, 6:00:21 PM6/12/17
to jia...@chromium.org, va...@chromium.org, npa...@chromium.org, commi...@chromium.org, chromium...@chromium.org, asvitki...@chromium.org, grt+...@chromium.org, vakh+...@chromium.org, timvo...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Jun 12, 2017, 7:10:37 PM6/12/17
to jia...@chromium.org, va...@chromium.org, npa...@chromium.org, commi...@chromium.org, chromium...@chromium.org, asvitki...@chromium.org, grt+...@chromium.org, vakh+...@chromium.org, timvo...@chromium.org

va...@chromium.org

unread,
Jun 15, 2017, 4:51:27 PM6/15/17
to jia...@chromium.org, npa...@chromium.org, chromium...@chromium.org, asvitki...@chromium.org, grt+...@chromium.org, vakh+...@chromium.org, timvo...@chromium.org
On 2017/06/12 23:08:46, commit-bot: I haz the power wrote:
> Committed patchset #4 (id:60001) as
>
https://chromium.googlesource.com/chromium/src/+/837b32b0bc91016102f09448259fb6048516f0a1

jia...@chromium.org

unread,
Jun 15, 2017, 5:25:28 PM6/15/17
to va...@chromium.org, npa...@chromium.org, chromium...@chromium.org, asvitki...@chromium.org, grt+...@chromium.org, vakh+...@chromium.org, timvo...@chromium.org
Good catch! I missed this one. I'll reopen this bug and send a follow-up CL
shortly.

https://codereview.chromium.org/2927123004/
Reply all
Reply to author
Forward
0 new messages