Closed
Bug 1331534
Opened 6 years ago
Closed 6 years ago
Add a pref to ignore V4 matches
Categories
(Toolkit :: Safe Browsing, defect, P2)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: francois, Assigned: hchang)
References
Details
Attachments
(1 file)
Now that bug 1305486 has landed, it is possible that the Safe Browsing V4 lists will be used to block URLs. It's unlikely right now because completions are disabled (until bug 1329808) and any hash prefix will fail silently, but if we received any full hashes, they could be used to block. I suggest we add a new pref (browser.safebrowsing.temporary.block_pver4_matches) which will cause the URL Classifier to ignore any matches it found on a pver4 provider. While the matches won't lead to anything, we will still be able to keep track of them in telemetry.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → hchang
Reporter | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Updated•6 years ago
|
No longer blocks: safebrowsingv4
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8827807 -
Flags: review?(francois)
Assignee | ||
Comment 2•6 years ago
|
||
I have pushed a patch for review with a slight difference from what comment 0 describes: 1) I turned the negative prefs to a positive one, which is easier for testers to "turn on some pref to enable some feature". 2) "matches" is a little ambiguous. It can be prefix matches or the entire hash matches. So, I just use a more specific term: "completion result" With the new preference, we can have 2-step roll-out plan 1) While we have sufficient telemetry for v4 hash completion, we can put the actual gethash URL and have an early sense that if v4 hash completion result is always equal to v2. 2) Once we are confident enough of the v4 hash completion results, we can remove this temporary pref.
Reporter | ||
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8827807 [details] Bug 1331534 - Temporarily ignore v4 hash completion result. https://reviewboard.mozilla.org/r/105400/#review106498 (In reply to Henry Chang [:henry][:hchang] from comment #2) > 2) "matches" is a little ambiguous. It can be prefix matches or the entire > hash matches. > So, I just use a more specific term: "completion result" Right, I should have said "full hash match". However, I don't think that limiting completions is sufficient. To fully control the roll-out of V4 lookups (bug 1329817), we need to have the ability to ignore both: 1. full hashes returned by gethash 2. full hashes sent as part of updates (not common, but could occur) Having this will allow us to enable V4 lookups on our pre-release channels in a "monitoring mode" and collect telemetry like: - URLs that are blocked in V2 only - URLs that are blocked in V4 only - URLs that are blocked in both The last case is what we expect most of the time since the underlying data is the same, but if we had a bug in our V4 code that someone flagged all URLs that end in .com.tw as bad, then we would see a big spike in V4 and we would investigate. So by disabling cases 1 and 2 above, we ensure that our data collection can be done without any harmful effects on people. ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp (Diff revision 1) > nsUrlClassifierDBServiceWorker::QueueLookup(const nsACString& spec, > const nsACString& tables, > nsIUrlClassifierLookupCallback* callback) > { > MutexAutoLock lock(mPendingLookupLock); > - if (gShuttingDownThread) { Is that an accidental change?
Attachment #8827807 -
Flags: review?(francois) → review-
Assignee | ||
Comment 4•6 years ago
|
||
(In reply to François Marier [:francois] from comment #3) > Comment on attachment 8827807 [details] > Bug 1331534 - Temporarily ignore v4 hash completion result. > > https://reviewboard.mozilla.org/r/105400/#review106498 > > (In reply to Henry Chang [:henry][:hchang] from comment #2) > > 2) "matches" is a little ambiguous. It can be prefix matches or the entire > > hash matches. > > So, I just use a more specific term: "completion result" > > Right, I should have said "full hash match". > > However, I don't think that limiting completions is sufficient. To fully > control the roll-out of V4 lookups (bug 1329817), we need to have the > ability to ignore both: > > 1. full hashes returned by gethash > 2. full hashes sent as part of updates (not common, but could occur) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ I fully understand (1), which is the intention of the patch. Even though this patch doesn't block the hash completion result from the completer's point of view, the completion result will not be considered at all. We would bail out the "complete" callback function if the completion match is for v4 tables. In other words, we may do the completion (as long as the gethash URL is not empty) but will do nothing even if the completion result matches full hash. As for (2), I am a little confused. Did you mean "not to do v4 hash completion"?
Flags: needinfo?(francois)
Assignee | ||
Comment 5•6 years ago
|
||
Per discussion with Francois, case (2) is the case where we hit a 32-byte prefix in v4 tables. In that case, the URL may just be reported malicious without hash completion, which is the case I missed. That said, Dimi just reminded me his patch in Bug 1328821 will force to do hash completion even if we matched 32-byte prefix if the entry is not found in the cache. ni Dimi to let him explain this more clearly than me :) Anyways, I will rename the preference and see whereever the preference needs to be used.
Flags: needinfo?(francois) → needinfo?(dlee)
Comment 6•6 years ago
|
||
The rule to decide if we should send gethash request is different in v2 and v4. In v2, we won't send gethash when match fullhash(see bug 1288833). As for V4, the "ONLY" case we treat a url as Un-Safe without sending gethash is when we found a completion in cache and cache is not expired. I'm sorry about not describing this clearly in Bug 1328821. In the patch of Bug 1328821, i changed the |Confirmed|[1] value to not depend on |mComplete|, and until v4 caching is landed, |Confirmed| will always return false, that means, we will never treat a url as Un-Safe without receiving a match result from gethash response. [1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/LookupCache.h#56
Flags: needinfo?(dlee)
Reporter | ||
Comment 7•6 years ago
|
||
Ah, so what you're saying is that case #2 in comment 3 will never happen and that Henry's patch is all we need?
Flags: needinfo?(dlee)
Reporter | ||
Comment 8•6 years ago
|
||
After talking to Dimi, he pointed out that once bug 1328821 has landed, Henry's patch will be all that's needed to cover cases #1 and #2.
Flags: needinfo?(dlee)
Reporter | ||
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8827807 [details] Bug 1331534 - Temporarily ignore v4 hash completion result. https://reviewboard.mozilla.org/r/105400/#review106922 If you resolve the issue I pointed out (possible accidental change), then I think this is good to go!
Attachment #8827807 -
Flags: review- → review+
Comment hidden (mozreview-request) |
Comment 11•6 years ago
|
||
Pushed by hchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a00c204385df Temporarily ignore v4 hash completion result. r=francois
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a00c204385df
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•