1274685 - Unowned Safe Browsing tables break list updates

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[François Marier [:francois]]

If urlclassifier.trackingTable refers to a list that's not included in browser.safebrowsing.provider.mozilla.lists, Safe Browsing updates break.

Steps to reproduce:

1. start Firefox and create a new profile (./firefox --no-remote --ProfileManager)
2. go into about:config
3. set browser.safebrowsing.debug = 1
4. remove "mozstd-track-digest256" from browser.safebrowsing.provider.mozilla.lists
5. close Firefox
6. delete the safe browsing diretory (rm ~/.cache/mozilla/firefox/XXXX/safebrowsing/*)
7. start Firefox and look at the terminal output
8. look in the safe browsing directory (ls ~/.cache/mozilla/firefox/XXXX/safebrowsing/)


EXPECTED:

There should be files in the safe browsing directory:

goog-badbinurl-shavar.cache    mozstd-track-digest256.pset          test-malware-simple.sbstore
goog-badbinurl-shavar.pset     mozstd-track-digest256.sbstore       test-phish-simple.cache
goog-badbinurl-shavar.sbstore  mozstd-trackwhite-digest256.cache    test-phish-simple.pset
goog-malware-shavar.cache      mozstd-trackwhite-digest256.pset     test-phish-simple.sbstore
goog-malware-shavar.pset       mozstd-trackwhite-digest256.sbstore  test-track-simple.cache
goog-malware-shavar.sbstore    test-block-simple.cache              test-track-simple.pset
goog-phish-shavar.cache        test-block-simple.pset               test-track-simple.sbstore
goog-phish-shavar.pset         test-block-simple.sbstore            test-trackwhite-simple.cache
goog-phish-shavar.sbstore      test-forbid-simple.cache             test-trackwhite-simple.pset
goog-unwanted-shavar.cache     test-forbid-simple.pset              test-trackwhite-simple.sbstore
goog-unwanted-shavar.pset      test-forbid-simple.sbstore           test-unwanted-simple.cache
goog-unwanted-shavar.sbstore   test-malware-simple.cache            test-unwanted-simple.pset
mozstd-track-digest256.cache   test-malware-simple.pset             test-unwanted-simple.sbstore

and there should be output that shows that the browser is downloading the list:

listmanager: 14:16:07 GMT-0700 (PDT): Enabling table updates for goog-phish-shavar
listmanager: 14:16:07 GMT-0700 (PDT): Enabling table updates for goog-malware-shavar
listmanager: 14:16:07 GMT-0700 (PDT): Enabling table updates for goog-unwanted-shavar
listmanager: 14:16:07 GMT-0700 (PDT): Enabling table updates for goog-badbinurl-shavar
listmanager: 14:16:07 GMT-0700 (PDT): Enabling table updates for mozstd-track-digest256
listmanager: 14:16:07 GMT-0700 (PDT): Enabling table updates for mozstd-trackwhite-digest256
listmanager: 14:16:07 GMT-0700 (PDT): Disabling table updates for mozplugin-block-digest256
listmanager: 14:16:07 GMT-0700 (PDT): Starting managing lists
listmanager: 14:16:07 GMT-0700 (PDT): needsUpdate: {
  "https://safebrowsing.google.com/safebrowsing/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2&key=[trimmed-google-api-key]": {
    "goog-phish-shavar": true,
    "goog-malware-shavar": true,
    "goog-unwanted-shavar": true,
    "goog-badbinurl-shavar": true
  },
  "https://shavar.services.mozilla.com/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2": {
    "mozstd-track-digest256": true,
    "mozstd-trackwhite-digest256": true,
    "mozplugin-block-digest256": false
  }
}
listmanager: 14:16:07 GMT-0700 (PDT): No updates needed or already initialized for https://safebrowsing.google.com/safebrowsing/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2&key=[trimmed-google-api-key]
listmanager: 14:16:07 GMT-0700 (PDT): No updates needed or already initialized for https://shavar.services.mozilla.com/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2
listmanager: 14:17:54 GMT-0700 (PDT): checkForUpdates with https://safebrowsing.google.com/safebrowsing/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2&key=[trimmed-google-api-key]
listmanager: 14:17:54 GMT-0700 (PDT): this.tablesData: {
  "goog-phish-shavar": {
    "updateUrl": "https://safebrowsing.google.com/safebrowsing/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2&key=[trimmed-google-api-key]",
    "gethashUrl": "https://safebrowsing.google.com/safebrowsing/gethash?client=navclient-auto-ffox&appver=49.0a&pver=2.2",
    "provider": "google"
  },
  "goog-malware-shavar": {
    "updateUrl": "https://safebrowsing.google.com/safebrowsing/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2&key=[trimmed-google-api-key]",
    "gethashUrl": "https://safebrowsing.google.com/safebrowsing/gethash?client=navclient-auto-ffox&appver=49.0a&pver=2.2",
    "provider": "google"
  },
  "goog-unwanted-shavar": {
    "updateUrl": "https://safebrowsing.google.com/safebrowsing/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2&key=[trimmed-google-api-key]",
    "gethashUrl": "https://safebrowsing.google.com/safebrowsing/gethash?client=navclient-auto-ffox&appver=49.0a&pver=2.2",
    "provider": "google"
  },
  "goog-badbinurl-shavar": {
    "updateUrl": "https://safebrowsing.google.com/safebrowsing/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2&key=[trimmed-google-api-key]",
    "gethashUrl": "https://safebrowsing.google.com/safebrowsing/gethash?client=navclient-auto-ffox&appver=49.0a&pver=2.2",
    "provider": "google"
  },
  "mozstd-track-digest256": {
    "updateUrl": "https://shavar.services.mozilla.com/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2",
    "gethashUrl": "https://shavar.services.mozilla.com/gethash?client=navclient-auto-ffox&appver=49.0a&pver=2.2",
    "provider": "mozilla"
  },
  "mozstd-trackwhite-digest256": {
    "updateUrl": "https://shavar.services.mozilla.com/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2",
    "gethashUrl": "https://shavar.services.mozilla.com/gethash?client=navclient-auto-ffox&appver=49.0a&pver=2.2",
    "provider": "mozilla"
  },
  "mozplugin-block-digest256": {
    "updateUrl": "https://shavar.services.mozilla.com/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2",
    "gethashUrl": "https://shavar.services.mozilla.com/gethash?client=navclient-auto-ffox&appver=49.0a&pver=2.2",
    "provider": "mozilla"
  }
}


ACTUAL:

The safebrowsing directory is empty and the only messages we see are:

SafeBrowsing: 14:24:28 GMT-0700 (PDT): getLists: urlclassifier.phishTable
SafeBrowsing: 14:24:28 GMT-0700 (PDT): getLists: urlclassifier.malwareTable
SafeBrowsing: 14:24:28 GMT-0700 (PDT): getLists: urlclassifier.downloadBlockTable
SafeBrowsing: 14:24:28 GMT-0700 (PDT): getLists: urlclassifier.downloadAllowTable
SafeBrowsing: 14:24:28 GMT-0700 (PDT): getLists: urlclassifier.trackingTable
SafeBrowsing: 14:24:28 GMT-0700 (PDT): getLists: urlclassifier.trackingWhitelistTable
SafeBrowsing: 14:24:28 GMT-0700 (PDT): getLists: urlclassifier.forbiddenTable
SafeBrowsing: 14:24:28 GMT-0700 (PDT): getLists: urlclassifier.blockedTable
SafeBrowsing: 14:24:28 GMT-0700 (PDT): reading prefs
SafeBrowsing: 14:24:28 GMT-0700 (PDT): initializing safe browsing URLs, client id navclient-auto-ffox
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Child: mozilla.lists.mozstd.name
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Child: mozilla.lists.mozstd.description
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Child: mozilla.updateURL
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Child: google.lists
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Child: google.lastupdatetime
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Child: google.reportURL
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Child: mozilla.lists.mozfull.description
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Child: mozilla.lists.mozfull.name
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Child: google.nextupdatetime
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Child: mozilla.gethashURL
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Child: mozilla.lastupdatetime
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Child: mozilla.nextupdatetime
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Child: google.updateURL
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Child: mozilla.lists
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Child: google.gethashURL
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Providers: mozilla, google
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Provider: mozilla updateURL=https://shavar.services.mozilla.com/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Provider: mozilla gethashURL=https://shavar.services.mozilla.com/gethash?client=navclient-auto-ffox&appver=49.0a&pver=2.2
SafeBrowsing: 14:24:28 GMT-0700 (PDT): getLists: browser.safebrowsing.provider.mozilla.lists
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Provider: google updateURL=https://safebrowsing.google.com/safebrowsing/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2&key=[trimmed-google-api-key]
SafeBrowsing: 14:24:28 GMT-0700 (PDT): Provider: google gethashURL=https://safebrowsing.google.com/safebrowsing/gethash?client=navclient-auto-ffox&appver=49.0a&pver=2.2
SafeBrowsing: 14:24:28 GMT-0700 (PDT): getLists: browser.safebrowsing.provider.google.lists
listmanager: 14:24:28 GMT-0700 (PDT): Initializing list manager
listmanager: 14:24:28 GMT-0700 (PDT): registering goog-phish-shavar with https://safebrowsing.google.com/safebrowsing/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2&key=[trimmed-google-api-key]
listmanager: 14:24:28 GMT-0700 (PDT): Creating request backoff for https://safebrowsing.google.com/safebrowsing/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2&key=[trimmed-google-api-key]
listmanager: 14:24:28 GMT-0700 (PDT): registering goog-malware-shavar with https://safebrowsing.google.com/safebrowsing/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2&key=[trimmed-google-api-key]
listmanager: 14:24:28 GMT-0700 (PDT): registering goog-unwanted-shavar with https://safebrowsing.google.com/safebrowsing/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2&key=[trimmed-google-api-key]
listmanager: 14:24:28 GMT-0700 (PDT): registering goog-badbinurl-shavar with https://safebrowsing.google.com/safebrowsing/downloads?client=navclient-auto-ffox&appver=49.0a&pver=2.2&key=[trimmed-google-api-key]

Comment 1

[François Marier [:francois]]

We should fix this because it caused problems during Richard's end-to-end shavar tests.

Henry, is this something you want to take a look at?

I suspect that Dimi's change in registerTableWithURLs might solve this:

https://reviewboard.mozilla.org/r/52133/diff/2#0

Comment 2

[Henry Chang [:hchang]]

Dimi's fix is indeed the solution. However, what I wonder is the actual end-to-end tests requirement. If the unownded list is not needed at all, we can skip this unowned list. If it needs to be downloaded for some reason, maybe we need to infer the provider from the list name.

Richard, do you mind providing your requirement of the tests? Thanks :)

Comment 3

[François Marier [:francois]]

Henry Chang [:henry] from comment #2)
> Richard, do you mind providing your requirement of the tests? Thanks :)

Richard's tests set a number of prefs:

  https://wiki.mozilla.org/Services/TrackingProtection/Shavar_Server_-_Testing#SET_CUSTOM_PREFS

in order to ensure that all of the lists served by the shavar server are working.

Him and I are trying to work around this bug and simplify the prefs:

  https://github.com/mozilla-services/services-test/blob/dev/shavar/e2e-test/prefs.ini

> Dimi's fix is indeed the solution. However, what I wonder is the actual
> end-to-end tests requirement. If the unownded list is not needed at all, we
> can skip this unowned list. If it needs to be downloaded for some reason,
> maybe we need to infer the provider from the list name.

It doesn't need to be downloaded. If we see an unowned list, we should (ideally) print a warning and skip it.

Comment 4

[Henry Chang [:hchang]]

Attachment: Part 1: Avoid registering unowned list

Comment 5

[Henry Chang [:hchang]]

Comment on attachment 8759013 [details] [diff] [review]
Part 1: Avoid registering unowned list

Hi Francois,

I've just attached a patch for review. However, I haven't figured out a test case for it. I'll provide one once I make it. Thanks!

Comment 6

[François Marier [:francois]]

Comment on attachment 8759013 [details] [diff] [review]
Part 1: Avoid registering unowned list

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

Thanks Henry!

Comment 7

[Henry Chang [:hchang]]

Attachment: Part 2: Test case

Comment 8

[Henry Chang [:hchang]]

Comment on attachment 8759043 [details] [diff] [review]
Part 2: Test case

This is the test case to make sure the owned lists don't break table update.

Comment 9

[Henry Chang [:hchang]]

Attachment: Part 2: Test case

Comment 10

[Henry Chang [:hchang]]

Attachment: Part 2: Test case

Comment 11

[François Marier [:francois]]

Comment on attachment 8759050 [details] [diff] [review]
Part 2: Test case

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

That's a nice way to test this.

Just a small thing to fix with the initial value of the pref and it's ready to land.

::: toolkit/components/url-classifier/tests/unit/test_bug1274685_unowned_list.js
@@ +17,5 @@
> +
> +  try {
> +    // Remove 'goog-malware-shavar' from the original.
> +    Services.prefs.setCharPref("browser.safebrowsing.provider.google.lists",
> +                               "goog-unwanted-shavar,test-malware-simple,test-unwanted-simple");

That value is not quite right because it includes lists that are not supposed to be in there (the test-*-simple ones) and also is missing more than just "goog-malware-shavar".

The default value is this:

  goog-badbinurl-shavar,goog-downloadwhite-digest256,goog-phish-shavar,goog-malware-shavar,goog-unwanted-shavar

So what I'd recommend for this test is that you first do a Services.prefs.getCharPref() and then a string replacement:

  s/goog-malware-shavar,//

on the string that you get. That way you only remove the malware one and you're not accidentally removing anything else that we might add there in the future.

Comment 12

[Henry Chang [:hchang]]

Attachment: Part 2: Test case

Comment 13

[Henry Chang [:hchang]]

Comment on attachment 8759471 [details] [diff] [review]
Part 2: Test case

Hi Francois,

I've addressed your comments. Could you review again? Thanks :)

Comment 14

[François Marier [:francois]]

Comment on attachment 8759471 [details] [diff] [review]
Part 2: Test case

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

r+ once you address this last comment.

::: toolkit/components/url-classifier/tests/unit/test_bug1274685_unowned_list.js
@@ +16,5 @@
> +  let origList = Services.prefs.getCharPref("browser.safebrowsing.provider.google.lists");
> +
> +  try {
> +    // Remove 'goog-malware-shavar' from the original.
> +    let trimmedList = origList.replace('goog-malware-shavar,', '');

It might be better to move the string replacement and the setCharPref() call out of the try..catch block because we only want to pass the test if the exception is thrown from SafeBrowsing.registerTable().

Comment 15

[Henry Chang [:hchang]]

Attachment: Part 2: Test case (r+'ed)

Comment 16

[Henry Chang [:hchang]]

Attachment: Part 2: Test case (r+'ed)

Comment 17

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a052fbf9c642

Comment 18

[François Marier [:francois]]

Comment on attachment 8759505 [details] [diff] [review]
Part 2: Test case (r+'ed)

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

Oops this one needs to move out too.

::: toolkit/components/url-classifier/tests/unit/test_bug1274685_unowned_list.js
@@ +17,5 @@
> +  let trimmedList = origList.replace('goog-malware-shavar,', '');
> +
> +  try {
> +    // Remove 'goog-malware-shavar' from the original.
> +    Services.prefs.setCharPref("browser.safebrowsing.provider.google.lists", trimmedList);

Could you also move this one out of the try..catch?

It would be better not to pass the test if this throws an exception.

Comment 19

[Henry Chang [:hchang]]

Attachment: Part 2: Test case (carry r+)

Comment 20

[Henry Chang [:hchang]]

(In reply to François Marier [:francois] from comment #18)
> Comment on attachment 8759505 [details] [diff] [review]
> Part 2: Test case (r+'ed)
> 
> Review of attachment 8759505 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Oops this one needs to move out too.
> 
> :::
> toolkit/components/url-classifier/tests/unit/test_bug1274685_unowned_list.js
> @@ +17,5 @@
> > +  let trimmedList = origList.replace('goog-malware-shavar,', '');
> > +
> > +  try {
> > +    // Remove 'goog-malware-shavar' from the original.
> > +    Services.prefs.setCharPref("browser.safebrowsing.provider.google.lists", trimmedList);
> 
> Could you also move this one out of the try..catch?
> 
> It would be better not to pass the test if this throws an exception.

Done! Thanks for reminding :)

Comment 21

[François Marier [:francois]]

Comment on attachment 8759534 [details] [diff] [review]
Part 2: Test case (carry r+)

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

Looks great, thanks Henry!

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/2df25135bcd3
Part 1: Avoid registering lists which have no provider info. r=francois.
https://hg.mozilla.org/integration/fx-team/rev/30700073705d
Part 2: Test case. r=francois.

Comment 23

[Richard Pappalardo [:rpapa][:rpappalardo]]

Thanks, :henry, :francois!  when these new prefs land, I assume they'll only do so in Nightly?  I'm pretty sure that's the case but if so, I'll need to adapt our test tool to now also handle unique pref sets for different browser versions.  The matrix of pref combinations is getting quite complex so I want to confirm before i make that change

Comment 24

[François Marier [:francois]]

Moving over to Matt to verify because it's really a client bug :)

Comment 25

[François Marier [:francois]]

(In reply to Richard Pappalardo [:rpapa][:rpappalardo] from comment #23)
> Thanks, :henry, :francois!  when these new prefs land, I assume they'll only
> do so in Nightly?  I'm pretty sure that's the case but if so, I'll need to
> adapt our test tool to now also handle unique pref sets for different
> browser versions.  The matrix of pref combinations is getting quite complex
> so I want to confirm before i make that change

This patch doesn't change any of the prefs so you shouldn't have to change anything in the test tool or end-to-end test plan.

If anything, the client will be more tolerant of pref mistakes in the future :)

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2df25135bcd3
https://hg.mozilla.org/mozilla-central/rev/30700073705d

Comment 27

[Matt Wobensmith [:mwobensmith][:matt:]]

Using steps from comment 0:

Confirmed problem on Fx49 from 2016-05-10.
Verified fixed on Fx49 and Fx50, 2016-06-07.