985623 - goog-badbinurl-shavar and goog-whitedigest-sha256 are causing too many gethash requests

Status: VERIFIED FIXED

(Core :: DOM: Security, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

We should not be checking these for non-application reputation requests. There are too many collisions and the remote servers are experiencing higher load than expected.

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Force non-testing url classifier clients to specify which tables to lookup (

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8394301 [details] [diff] [review]
Force non-testing url classifier clients to specify which tables to lookup (

Review of attachment 8394301 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +1159,5 @@
>  
>    return NS_OK;
>  }
>  
> +// nsChannelClassifier is the only consumer of this interface.

I'd like to get rid of this interface, but this seemed like the easiest route for now.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8394301 [details] [diff] [review]
Force non-testing url classifier clients to specify which tables to lookup (

Review of attachment 8394301 [details] [diff] [review]:
-----------------------------------------------------------------

r- based on the test-only API. I think we can do that a lot cleaner by dealing with it in the tests themselves. Also would like your feedback on caching the active tables.

::: toolkit/components/url-classifier/Classifier.cpp
@@ +45,5 @@
> +    FindCharInReadable(',', iter, end);
> +    tables.AppendElement(Substring(begin, iter));
> +    begin = iter;
> +    if (begin != end)
> +      begin++;

goto fail :) Our coding guideline requires braces.

@@ +226,5 @@
>    nsresult rv = LookupCache::GetLookupFragments(aSpec, &fragments);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    nsTArray<nsCString> activeTables;
> +  SplitTables(aTables, activeTables);

I think this makes all the mActiveTablesCache machinery redundant. From reading the code, they're now only used on (1) caching a completion (2) during an update. Both of those already generate IO. So I think it can go. On the other hand, it's not like it uses much memory. What do you think?

::: toolkit/components/url-classifier/nsIUrlClassifierDBService.idl
@@ +85,5 @@
> +   * WARNING: USE ONLY FOR TESTING. Looking up in tables can have network
> +   * side-effects.
> +   */
> +  void lookupAllTables(in nsIPrincipal principal,
> +                       in nsIUrlClassifierCallback c);

I don't think we should be making the existing testing-only API madness worse. It's also the one requiring you to expose ActiveTables() upwards.

::: toolkit/components/url-classifier/tests/unit/head_urlclassifier.js
@@ +199,5 @@
>    var doLookup = function() {
>      if (urls.length > 0) {
>        var fragment = urls.shift();
>        var principal = secMan.getNoAppCodebasePrincipal(iosvc.newURI("http://" + fragment, null, null));
> +      dbservice.lookupAllTables(principal,

Adding the tables in the tests seems very feasible, and avoids making the API uglier and introducing more test-only API usage. In fact, the tests already hardcode the table names, so why not do it here too?

Comment 4

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Force non-testing url classifier clients to specify which tables to lookup (

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #3)
> Comment on attachment 8394301 [details] [diff] [review]
> Force non-testing url classifier clients to specify which tables to lookup (
> 
> Review of attachment 8394301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- based on the test-only API. I think we can do that a lot cleaner by
> dealing with it in the tests themselves. Also would like your feedback on
> caching the active tables.

test-only API gone! I guess I was trying to avoid the work in tracking down all of the implicit usages, but I think I got them all now...

> ::: toolkit/components/url-classifier/Classifier.cpp
> @@ +45,5 @@
> > +    FindCharInReadable(',', iter, end);
> > +    tables.AppendElement(Substring(begin, iter));
> > +    begin = iter;
> > +    if (begin != end)
> > +      begin++;
> 
> goto fail :) Our coding guideline requires braces.

I think by careful inspection of hg blame on the original location we can identify the culprit as the person who disavowed all knowledge of this code :) Fixed.

> @@ +226,5 @@
> >    nsresult rv = LookupCache::GetLookupFragments(aSpec, &fragments);
> >    NS_ENSURE_SUCCESS(rv, rv);
> >  
> >    nsTArray<nsCString> activeTables;
> > +  SplitTables(aTables, activeTables);
> 
> I think this makes all the mActiveTablesCache machinery redundant. From
> reading the code, they're now only used on (1) caching a completion (2)
> during an update. Both of those already generate IO. So I think it can go.
> On the other hand, it's not like it uses much memory. What do you think?

I tried removing it for about 10 minutes, then realized that listmanager.js is using it in production. I think it's OK to keep this since it's so small, and to minimize the number of changes in this patch.

Comment 6

[[:mmc] Monica Chew (no longer reading bugmail)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=09f6cb3b8deb

Comment 7

[Gian-Carlo Pascutto [:gcp]]

>I guess I was trying to avoid the work in tracking down all of the implicit usages, but I think I got them all now...

This was probably worth it. I mean those tests that could apparently pass even if the phishing tables were disabled or the pref broken...glad that that's gone!

>the person who disavowed all knowledge of this code

You're talking about dcamp, right? :-)

>I tried removing it for about 10 minutes, then realized that listmanager.js is using it in production.

That's just calling ActiveTables on updates? I was talking about the mActiveTablesCache only, not removing ActiveTables. It's probably not worth fiddling with this now anyhow.

Comment 8

[[:mmc] Monica Chew (no longer reading bugmail)]

Per IRC, this change will break http://www.mozilla.org/firefox/its-a-trap.html and http://www.itisatrap.org/firefox/its-a-trap.html in production if we don't set test-phish-simple and test-malware-simple in prefs (or hardcode it in the ChannelClassifier check, which is worse).

Here are a couple of solutions. None of them are great, because we need to rely on these fake tables in both production and for testing.

1) Add test-phish-simple and friends to the preferences, which can already be a comma-separated list.
Change Confirmed() to always confirm if mTableName is test-phish-simple or test-malware-simple.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/LookupCache.h#38

2) Add the tables to prefs, change GetCompleter to return false when tablename is test-phish-simple or test-malware-simple even if contained in mGetHashtables. This falls through to http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#842

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#1392

3) Get google to add the itisatrap links to their DB. This is the best, because it gives us an automatic regression test, but we need something in the meantime anyway.

I think option 2) is better than 1) because it makes it more clear we're avoiding the completers.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

3) Doesn't work because there will be no warning until the users DB is fully up to date. I'm all for making Google add a "is-my-safebrowsing-updating" link in their DB but we still need separate always-present ones.

I would propose to add a list of tables (either dont_complete or complete_tables or something like that) that identify which tables we want to run (or don't want to run) the hashcompleter on. This seems more flexible than trying to hardcode the logic in the C++ code. You would then for example put "goog-whitedigest-sha256,test-phish-simple,test-malware-simple" in that pref.

Comment 10

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Bug 985623: Force classifier clients to specify which tables to lookup, add a pref for skipping hash completions (r=gcp)

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8394878 [details] [diff] [review]
Bug 985623: Force classifier clients to specify which tables to lookup, add a pref for skipping hash completions (r=gcp)

Review of attachment 8394878 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpref/src/init/all.js
@@ +4393,3 @@
>  pref("urlclassifier.download_block_table", "");
>  pref("urlclassifier.download_allow_table", "");
> +pref("urlclassifier.disallow_completions", "test-malware-simple,test-phish-simple,goog-downloadwhite-digest256");

This is slightly weird because "goog-downloadwhite-digest256" isn't actually used in the prefs above. On the other hand you can't just append to this list at the place where you'll set download_allow_table (AFAIK) so I guess there's no clean solution.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +76,5 @@
>  
>  #define CONFIRM_AGE_PREF        "urlclassifier.max-complete-age"
>  #define CONFIRM_AGE_DEFAULT_SEC (45 * 60)
>  
> +#define DISALLOW_COMPLETIONS_PREF "urlclassifier.disallow_completions"

Move this up with the other table prefs, and name it DISALLOW_COMPLETION_TABLE_PREF to keep consistency.

@@ +1179,5 @@
> +  }
> +  nsAutoCString phishing;
> +  Preferences::GetCString(PHISH_TABLE_PREF, &phishing);
> +  if (!phishing.IsEmpty()) {
> +    tables.Append(",");

So what happens if I clear the malware pref (trying to save some space on our new $1 FirefoxOS phone that doesn't download anything anyway) but keep the phishing table?

I'll end up with:
",goog-phish-shavar,test-phish-simple"
right?

@@ +1397,5 @@
>  bool
>  nsUrlClassifierDBService::GetCompleter(const nsACString &tableName,
>                                         nsIUrlClassifierHashCompleter **completer)
>  {
> +  // If we have specified a completer, go ahead and query it.

Document that this is only used by the tests.

Comment 12

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #11)
> So what happens if I clear the malware pref (trying to save some space on
> our new $1 FirefoxOS phone that doesn't download anything anyway) but keep
> the phishing table?
> 
> I'll end up with:
> ",goog-phish-shavar,test-phish-simple"
> right?

I changed SplitTables to ignore empty strings.

Try looked good, so fixed and checked in:

https://tbpl.mozilla.org/?tree=Try&rev=8d0d672ba352
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e7a44e5b762f

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Force url classifier clients to specify which tables to lookup, add a pref to skip hash completion checks (

Comment 15

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8395974 [details] [diff] [review]
Force url classifier clients to specify which tables to lookup, add a pref to skip hash completion checks (

Btw, the patch is in https://hg.mozilla.org/mozilla-central/rev/e7a44e5b762f, not in this attachment (I tried to re-upload it to make bugzilla happy but it got munged)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 985627
User impact if declined: Per discussion with Sylvestre, we don't get a release cycle downloading tables for application reputation before they're needed in 30.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Delays deployment of application reputation, which cuts downloaded malware by 50% (https://wiki.mozilla.org/Security/Features/Application_Reputation/Preliminary_Results)
String or IDL/UUID changes made by this patch: nsIUrlClassifierDBService, nsIUrlClassifierDBServiceWorker

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e1282ac83e49

This has conflicts on beta. I tried pinging you on IRC but couldn't get ahold of you.

Comment 17

[[:mmc] Monica Chew (no longer reading bugmail)]

Hi Ryan,

Thanks for trying, I'll checkout beta later today and try to resolve conflicts.

Thanks,
Monica

Comment 18

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: split-lookups for mozilla-beta

Try: https://tbpl.mozilla.org/?tree=Try&rev=a359fef53622

Comment 19

[[:mmc] Monica Chew (no longer reading bugmail)]

Try looks terrible on b2g on 29 but OK elsewhere, and RyanVM says that we don't care about b2g on 29. Orange bc for browser_bug410900.js passes locally on mac.

Comment 20

[[:mmc] Monica Chew (no longer reading bugmail)]

Tree reopened!
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/363950e031c0

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This already landed on beta. So updating the flags.

Comment 22

[Bogdan Maris, Desktop Test Engineering]

Is there anything manual QA can cover here? If yes can you please offer guidance on what should I test?

Comment 23

[[:mmc] Monica Chew (no longer reading bugmail)]

Hi Bogdan,

There's nothing manual to cover here. Google confirmed on March 24:

> The spike in HTTP 204 replies to Firefox 28 clients returned to its
> previous level by Saturday.  Thanks again for the quick fix!

Thanks,
Monica

Comment 24

[Bogdan Maris, Desktop Test Engineering]

Thanks Monica, marking this [qa-]