1331534 - Add a pref to ignore V4 matches

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[François Marier [:francois]]

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.

Comment 1

[Henry Chang [:hchang]]

Attachment: Bug 1331534 - Temporarily ignore v4 hash completion result.

Review commit: https://reviewboard.mozilla.org/r/105400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105400/

Comment 2

[Henry Chang [:hchang]]

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.

Comment 3

[François Marier [:francois]]

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?

Comment 4

[Henry Chang [:hchang]]

(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"?

Comment 5

[Henry Chang [:hchang]]

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.

Comment 6

[Dimi Lee [:dimi][:dlee]]

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

Comment 7

[François Marier [:francois]]

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?

Comment 8

[François Marier [:francois]]

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.

Comment 9

[François Marier [:francois]]

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!

Comment 10

[Henry Chang [:hchang]]

Comment on attachment 8827807 [details]
Bug 1331534 - Temporarily ignore v4 hash completion result.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105400/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a00c204385df
Temporarily ignore v4 hash completion result. r=francois

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a00c204385df