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

VERIFIED FIXED in Firefox 30

Status

()

defect
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: mmc, Assigned: mmc)

Tracking

(Blocks 1 bug)

unspecified
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 disabled, firefox30 fixed, firefox31 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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.
Depends on: 985627
No longer blocks: 985722
Depends on: 985722
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Attachment #8394301 - Flags: review?(gpascutto)
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 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?
Attachment #8394301 - Flags: review?(gpascutto) → review-
Attachment #8394301 - Attachment is obsolete: true
Attachment #8394401 - Flags: review?(gpascutto)
(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.
Attachment #8394401 - Flags: review?(gpascutto) → review+
>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.
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.
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 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.
Attachment #8394878 - Flags: review?(gpascutto) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/e7a44e5b762f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Attachment #8394878 - Attachment is obsolete: true
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
Attachment #8395974 - Flags: review+
Attachment #8395974 - Flags: approval-mozilla-beta?
Attachment #8395974 - Flags: approval-mozilla-aurora?
Attachment #8395974 - Flags: approval-mozilla-beta?
Attachment #8395974 - Flags: approval-mozilla-beta+
Attachment #8395974 - Flags: approval-mozilla-aurora?
Attachment #8395974 - Flags: approval-mozilla-aurora+
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.
Flags: needinfo?(mmc)
Hi Ryan,

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

Thanks,
Monica
Flags: needinfo?(mmc)
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.
Keywords: checkin-needed
QA Whiteboard: qa+

Updated

5 years ago
Blocks: 989232

Updated

5 years ago
No longer blocks: 989232
Depends on: 989232
This already landed on beta. So updating the flags.
OS: Linux → All
Hardware: x86_64 → All

Updated

5 years ago
Depends on: 995801
Is there anything manual QA can cover here? If yes can you please offer guidance on what should I test?
Flags: needinfo?(mmc)
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(mmc)
Thanks Monica, marking this [qa-]
QA Whiteboard: qa+ → qa-
QA Whiteboard: qa- → [qa-]
Depends on: 997759
No longer depends on: 995801
Depends on: 1110628
You need to log in before you can comment on or make changes to this bug.