Closed
Bug 1333257
Opened 7 years ago
Closed 7 years ago
URLs not blocked if they match both a V2 table and a V4 table
Categories
(Toolkit :: Safe Browsing, defect, P1)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: francois, Assigned: dlee)
References
Details
Attachments
(1 file)
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•7 years ago
|
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•7 years 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 | ||
Comment 2•7 years 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•7 years ago
|
No longer blocks: safebrowsingv4
Reporter | ||
Comment 4•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
Keywords: checkin-needed
Comment 9•7 years ago
|
||
seems there one open issue in mozreview. Could you fix this so that we can use the autoland system. Thanks!
Flags: needinfo?(dlee)
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 10•7 years 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
Comment hidden (mozreview-request) |
Comment 14•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/761f282c8d76
Status: ASSIGNED → RESOLVED
Closed: 7 years 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.
Description
•