825417 - SafeBrowsing.jsm: Don't hardcode phishing/malware table names (phishingList, malwareList).

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Philip Chee]

I'm working on implementing Safe Browsing in SeaMonkey. Based on our discussions with Google, we will need to use the "googpub-phish-shavar" hashtable instead of "goog-phish-shavar". In order to use the toolkit SafeBrowsing.jsm it should read the table names from "urlclassifier.gethashtables" instead of hard coding them. e.g.:

> -const phishingList = "goog-phish-shavar";
> -const malwareList  = "goog-malware-shavar";
> +const [phishingList, malwareList] =
> +  Services.prefs.getCharPref("urlclassifier.gethashtables").split(",").map(function(value) value.trim());

By the way I notice in nsUrlClassifierDBService.cpp, there is a pref observer which reloads the hash tables if the "urlclassifier.gethashtables" pref is changed.

http://hg.mozilla.org/mozilla-central/annotate/0bb4773db082/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#l1469

Shouldn't SafeBrowsing.jsm also do the same?

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Proposed fix.

Comment 2

[Philip Chee]

Comment on attachment 696496 [details] [diff] [review]
Patch v1.0 Proposed fix.

Changing reviewer to :gcp

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 696496 [details] [diff] [review]
Patch v1.0 Proposed fix.

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

Good fix.

Comment 4

[Frank Wein [:mcsmurf]]

Philip: One small suggestion in addition to this patch: You could add a comment to browser/app/profile/firefox.js (and maybe also to the other pref files) that the order of the hash tables in the urlclassifier.gethashtables pref now matters with this patch (first entry phishing list, second entry malware list).

Comment 5

[Gian-Carlo Pascutto [:gcp]]

>By the way I notice in nsUrlClassifierDBService.cpp, there is a pref observer 
>which reloads the hash tables if the "urlclassifier.gethashtables" pref is changed.

I'm not really sure I understand the design of that code. It seems that the tables that are configured in gethash will be forwarded to the default completer if we don't do a SetHashCompleter for them. So who's doing SetHashCompleter? As far as I can tell, no-one does. So why does it exist? Because we want to test the code. Why does the testing code take a totally separate path from what is used in the field? Because this is the SafeBrowsing component, young grasshopper!

>Shouldn't SafeBrowsing.jsm also do the same?

Yes, probably you can try something similar to this, it's putting an observer on "browser.safebrowsing" that re-reads and reinitialized most prefs if they change:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/SafeBrowsing.jsm#34

Based on the above, I think that currently, changing that settings on a running Firefox just won't work. (And can't possibly be detected by the tests) But this is also not something you're likely to do.

Comment 6

[Philip Chee]

Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/740cf756cf33

Comment 7

[Philip Chee]

> Philip: One small suggestion in addition to this patch: You could add a comment to
> browser/app/profile/firefox.js (and maybe also to the other pref files) that the order
> of the hash tables in the urlclassifier.gethashtables pref now matters with this patch
> (first entry phishing list, second entry malware list).
Suggestions for a suitable comment?
// The order of these tables is significant because it is read by SafeBrowsing.jsm.
// The first table is the phishing list and the second is the malware list.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Sounds good to me (the "because it is read by SafeBrowsing.jsm" part is a bit superfluous, though).

Maybe it's good to file a follow-up bug for the problem of changing them at runtime that you noticed.

Comment 9

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/740cf756cf33