URLs not blocked if they match both a V2 table and a V4 table

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
Safe Browsing
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: francois, Assigned: dimi)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
Steps to reproduce:

1. Open Nightly in default configuration.
2. Browse to https://testsafebrowsing.appspot.com/s/phishing.html
3. Refresh the page

Expected:

The page is blocked and a Safe Browsing warning page shown after steps 2 and 3.

Actual:

The page is blocked and a warning shown only the first time, not after the page is refreshed.


It turns out that if we already have a valid complete match in V2 (i.e. we don't have to do a gethash) and we have a partial match in V4, then we consider the URL safe.

This is the output of the refreshed page:

[URL Classifier]: D/UrlClassifierDbService Checking fragment testsafebrowsing.appspot.com/s/phishing.html, hash EFBD4C3AB44F327EB13CA942AD7C7F0AB47EC260A4D0B8051684A01B2EF35220 (3A4CBDEF)
[URL Classifier]: D/UrlClassifierDbService Probe in goog-malware-shavar: 3A4CBDEF, found 0
[URL Classifier]: D/UrlClassifierDbService Probe in goog-unwanted-shavar: 3A4CBDEF, found 0
[URL Classifier]: D/UrlClassifierDbService Probe in test-malware-simple: 3A4CBDEF, found 0
[URL Classifier]: D/UrlClassifierDbService Probe in test-unwanted-simple: 3A4CBDEF, found 0
[URL Classifier]: D/UrlClassifierDbService Probe in goog-phish-shavar: 3A4CBDEF, found 1
[URL Classifier]: D/UrlClassifierDbService Complete in goog-phish-shavar
[URL Classifier]: D/UrlClassifierDbService Found a result in goog-phish-shavar: complete. (Age: 1349s)
[URL Classifier]: D/UrlClassifierDbService Probe in V4 goog-phish-proto: 3A4CBDEF, found 1, complete 0
[URL Classifier]: D/UrlClassifierDbService Found a result in goog-phish-proto: Not complete. (Age: 945s)
[URL Classifier]: D/UrlClassifierDbService Probe in test-phish-simple: 3A4CBDEF, found 0
[URL Classifier]: D/UrlClassifierDbService Probe in test-block-simple: 3A4CBDEF, found 0
[URL Classifier]: D/UrlClassifierDbService Probe in mozplugin-block-digest256: 3A4CBDEF, found 0
...
[URL Classifier]: D/UrlClassifierDbService Found 2 results.
[URL Classifier]: D/UrlClassifierDbService Found 2 results.
[URL Classifier]: D/UrlClassifierDbService query took 0ms
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::LookupComplete [7f6c671de040] 0 pending completions
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::HandleResults [7f6c671de040, 0 results]
[Main Thread]: D/nsChannelClassifier nsChannelClassifier[7f6c671ddf00]:OnClassifyComplete NS_OK (suspended channel)
[URL Classifier]: D/UrlClassifierDbService nsUrlClassifierDBServiceWorker::CacheMisses [7f6c8920b8b0] 0
[Main Thread]: D/nsChannelClassifier nsChannelClassifier::MarkEntryClassified[NS_OK] https://testsafebrowsing.appspot.com/s/phishing.html
[Main Thread]: D/nsChannelClassifier nsChannelClassifier[7f6c671ddf00]: resuming channel 7f6c6dc37848 from OnClassifyComplete

If I take the goog-malware-proto list out of urlclassifier.malwareTable, then the test succeeds and I get this output instead:

[URL Classifier]: D/UrlClassifierDbService Checking fragment testsafebrowsing.appspot.com/s/malware.html, hash 5B0B89750C78F233FEE25C6BE32D928FCD805A8C5455C2110D29353C2F517FEE (75890B5B)
[URL Classifier]: D/UrlClassifierDbService Probe in goog-malware-shavar: 75890B5B, found 1
[URL Classifier]: D/UrlClassifierDbService Complete in goog-malware-shavar
[URL Classifier]: D/UrlClassifierDbService Found a result in goog-malware-shavar: complete. (Age: 1442s)
[URL Classifier]: D/UrlClassifierDbService Probe in goog-unwanted-shavar: 75890B5B, found 0
[URL Classifier]: D/UrlClassifierDbService Probe in test-malware-simple: 75890B5B, found 0
[URL Classifier]: D/UrlClassifierDbService Probe in test-unwanted-simple: 75890B5B, found 0
[URL Classifier]: D/UrlClassifierDbService Probe in goog-phish-shavar: 75890B5B, found 0
[URL Classifier]: D/UrlClassifierDbService Probe in V4 goog-phish-proto: 75890B5B, found 0, complete 0
[URL Classifier]: D/UrlClassifierDbService Probe in test-phish-simple: 75890B5B, found 0
[URL Classifier]: D/UrlClassifierDbService Probe in test-block-simple: 75890B5B, found 0
[URL Classifier]: D/UrlClassifierDbService Probe in mozplugin-block-digest256: 75890B5B, found 0
...
[URL Classifier]: D/UrlClassifierDbService Found 1 results.
[URL Classifier]: D/UrlClassifierDbService Found 1 results.
[URL Classifier]: D/UrlClassifierDbService query took 0ms
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::LookupComplete [7ffb67972540] 0 pending completions
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::HandleResults [7ffb67972540, 1 results]
[Main Thread]: D/UrlClassifierDbService Confirmed result 679FD088 from table goog-malware-shavar
[Main Thread]: D/nsChannelClassifier nsChannelClassifier[7ffb679723c0]:OnClassifyComplete NS_ERROR_MALWARE_URI (suspended channel)
[URL Classifier]: D/UrlClassifierDbService nsUrlClassifierDBServiceWorker::CacheMisses [7ffb8aa8c7d0] 0
[Main Thread]: D/nsChannelClassifier nsChannelClassifier::MarkEntryClassified[NS_ERROR_MALWARE_URI] https://testsafebrowsing.appspot.com/s/malware.html
[Main Thread]: D/nsChannelClassifier nsChannelClassifier[7ffb679723c0]: cancelling channel 7ffb68315048 for https://testsafebrowsing.appspot.com/s/malware.html with error code NS_ERROR_MALWARE_URI
[Main Thread]: D/nsChannelClassifier nsChannelClassifier[7ffb679723c0]: resuming channel 7ffb68315048 from OnClassifyComplete
(Assignee)

Updated

a year ago
Assignee: nobody → dlee
Status: NEW → ASSIGNED
(Assignee)

Comment 1

a year ago
The root cause should be related to we treat V4 lookup result as cache miss[1] and this causes subsequent lookups for that prefix will be skipped[2].

Since V4 has its own caching mechanism, the solution here will be only apply current cache miss handling for v2 lookups.


[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#1144
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#279
Blocks: 1311935
(Assignee)

Updated

a year ago
Blocks: 1333328
(Assignee)

Comment 2

a year ago
I'll provide a easier fix for this bug first, a better one will be redesigned with v4 caching mechanism.
Follow up bug is Bug 1333328
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
No longer blocks: 1167038
(Reporter)

Comment 4

a year ago
mozreview-review
Comment on attachment 8829784 [details]
Bug 1333257 - Only cache V2 misses when doing Safe Browsing lookups.

https://reviewboard.mozilla.org/r/106782/#review107952

That solution makes perfect sense :) I only have a few nits.

One other thing I would suggest is changing the commit message to something like "Bug 1333257 - Only cache V2 misses when doing Safe Browsing lookups" since the commit message is supposed to describe the commit/fix, not the underlying bug.

::: toolkit/components/url-classifier/LookupCache.h:31
(Diff revision 1)
>  class LookupResult {
>  public:
>    LookupResult() : mComplete(false), mNoise(false),
>                     mFresh(false), mProtocolConfirmed(false),
> -                   mPartialHashLength(0) {}
> +                   mPartialHashLength(0),
> +                   mProtocolVersion(Protocol_V2) {}

We should make sure to set this explicitly in the  LookupCacheV4 constructor too: http://searchfox.org/mozilla-central/rev/02a56df6474a97cf84d94bbcfaa126979970905d/toolkit/components/url-classifier/LookupCacheV4.h#23

::: toolkit/components/url-classifier/LookupCache.h:33
(Diff revision 1)
>    LookupResult() : mComplete(false), mNoise(false),
>                     mFresh(false), mProtocolConfirmed(false),
> -                   mPartialHashLength(0) {}
> +                   mPartialHashLength(0),
> +                   mProtocolVersion(Protocol_V2) {}
> +
> +  enum ProtocolVersion {

Since we are going to refactor this later, we could perhaps simplify this a little by just adding a "mProtocolV2" boolean instead of having to introduce a new enum.
Attachment #8829784 - Flags: review?(francois) → review-
(Assignee)

Comment 5

a year ago
Thanks for review! Have one question about the comment below.

(In reply to François Marier [:francois] from comment #4)
> ::: toolkit/components/url-classifier/LookupCache.h:31
> (Diff revision 1)
> >  class LookupResult {
> >  public:
> >    LookupResult() : mComplete(false), mNoise(false),
> >                     mFresh(false), mProtocolConfirmed(false),
> > -                   mPartialHashLength(0) {}
> > +                   mPartialHashLength(0),
> > +                   mProtocolVersion(Protocol_V2) {}
> 
> We should make sure to set this explicitly in the  LookupCacheV4 constructor
> too:
> http://searchfox.org/mozilla-central/rev/
> 02a56df6474a97cf84d94bbcfaa126979970905d/toolkit/components/url-classifier/
> LookupCacheV4.h#23
> 
I am not sure what do you mean here, LookupCacheV4 is not derived class of LookupResult, so LookupCacheV4 cannot set mProtocolVersion.
Comment hidden (mozreview-request)
(Reporter)

Comment 7

a year ago
(In reply to Dimi Lee[:dimi][:dlee] from comment #5)
> I am not sure what do you mean here, LookupCacheV4 is not derived class of
> LookupResult, so LookupCacheV4 cannot set mProtocolVersion.

You're right. I mistook "LookupResult" for "LookupCache". Ignore that comment and sorry about that!
(Reporter)

Comment 8

a year ago
mozreview-review
Comment on attachment 8829784 [details]
Bug 1333257 - Only cache V2 misses when doing Safe Browsing lookups.

https://reviewboard.mozilla.org/r/106782/#review108124
Attachment #8829784 - Flags: review?(francois) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed
seems there one open issue in mozreview. Could you fix this so that we can use the autoland system. Thanks!
Flags: needinfo?(dlee)
Keywords: checkin-needed
(Assignee)

Comment 10

a year ago
(In reply to Carsten Book [:Tomcat] from comment #9)
> seems there one open issue in mozreview. Could you fix this so that we can
> use the autoland system. Thanks!

I marked it fixed according to Comment 5 & Comment 7
Flags: needinfo?(dlee)
Keywords: checkin-needed
Autoland can't rebase this for landing.
Keywords: checkin-needed
Comment hidden (mozreview-request)
(Assignee)

Comment 13

a year ago
rebase the patch
Keywords: checkin-needed

Comment 14

a year ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/761f282c8d76
Only cache V2 misses when doing Safe Browsing lookups. r=francois
Keywords: checkin-needed

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/761f282c8d76
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.