477718 - Implement Phishing Protection (a.k.a. Safe Browsing) support in SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: General, enhancement)

Description

[Adrian Kalla [:adriank]]

Looks like there is no bug for that:
one of the "nice to have" features for SeaMonkey 2 or a future release would be to support (in some way) Google Safebrowsing, as it really helps to protect end users.

Hope there is someone who would have time (and the knowledge) to do that...

Comment 1

[Serge Gautherie (:sgautherie)]

Ftr,
see bug 329292 and bug 387196, (and probably others)
and https://wiki.mozilla.org/Phishing_Protection

Comment 3

[Robert Kaiser]

No need any more to tie this with the "Google" brand, it's now called "Phishing Protection" everywhere. Also, the Mozilla wiki URL is more helpful in terms of the Mozilla implementation we would share than the original Google Code URL.

Comment 4

[Philip Chee]

q.v. Bug 470876 for an implementation example.

Comment 5

[Serge Gautherie (:sgautherie)]

Bug 738884 includes the Toolkit component.
Bug 653605 will be about packaging (and enabling by default (or not)) the application code.
This bug is to port/code the latter.

http://mxr.mozilla.org/comm-central/search?string=MOZ_SAFE_BROWSING&case=1

Comment 6

[Philip Chee]

> Component: General → Security
Not a security problem.
https://bugzilla.mozilla.org/describecomponents.cgi?product=SeaMonkey#Security

Comment 7

[Philip Chee]

Attachment: Patch v0.1 WIP.

> +pref("browser.safebrowsing.updateURL", "http://safebrowsing.clients.google.com/safebrowsing/downloads?client=Firefox&appver=17&pver=2.2");
> +pref("browser.safebrowsing.keyURL", "https://sb-ssl.google.com/safebrowsing/newkey?client=Firefox&appver=17&pver=2.2");
> +pref("browser.safebrowsing.gethashURL", "http://safebrowsing.clients.google.com/safebrowsing/gethash?client=Firefox&appver=17&pver=2.2");

Firefox uses:
> pref("browser.safebrowsing.updateURL", "http://safebrowsing.clients.google.com/safebrowsing/downloads?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2");
> pref("browser.safebrowsing.keyURL", "https://sb-ssl.google.com/safebrowsing/newkey?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2");
> pref("browser.safebrowsing.gethashURL", "http://safebrowsing.clients.google.com/safebrowsing/gethash?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2");

SAFEBROWSING_ID resolves to SeaMonkey for us but we haven't registered this client name with google so we don't get any data from Google.
Also setting the appver to 2.1x also turns off fetching data from Google (no idea why).

> +++ b/suite/browser/navigator.js
> ... 
> +XPCOMUtils.defineLazyModuleGetter(this, "SafeBrowsing",
> +  "resource://gre/modules/SafeBrowsing.jsm");
> ...
> +  // Bug 778855 - Perf regression if we do this here. To be addressed in bug 779008.
> +  setTimeout(function() {SafeBrowsing.init();}, 2000);
Would this be better in nsSuiteGlue.js?

> @@ -1630,22 +1636,22 @@ function handleURLBarCommand(aUserAction
> -      var flags = nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
> -      // If the value wasn't typed, we know that we decoded the value as UTF-8.
> -      // (see losslessDecodeURI)
> -      if (browser.userTypedValue === null)
> -        flags |= nsIWebNavigation.LOAD_FLAGS_URI_IS_UTF8;
> -      browser.loadURIWithFlags(url, flags, null, null, postData.value);
> +      var flags = nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
> +      // If the value wasn't typed, we know that we decoded the value as UTF-8.
> +      // (see losslessDecodeURI)
> +      if (browser.userTypedValue === null)
> +        flags |= nsIWebNavigation.LOAD_FLAGS_URI_IS_UTF8;
> +      browser.loadURIWithFlags(url, flags, null, null, postData.value);
Remove some DOS line endings :P

> +++ b/suite/common/bindings/notification.xml
> @@ -1635,16 +1635,77 @@
>              box.installInfo = installInfo;
>              installInfo.installs.forEach(function(aInstall) {
>                aInstall.addListener(box);
>              });
>            ]]>
>          </body>
>        </method>
>  
> +      <method name="ignoreSafeBrowsingWarning">
> +        <parameter name="aIsMalware"/>
> +        <body>
> +          <![CDATA[
> +            var uri = this.activeBrowser.currentURI;
Firefox uses content.location.href hope this is the same.

+++ b/suite/common/blockedSite.xhtml

+    <link rel="icon" type="image/png" id="favicon" href="chrome://global/skin/icons/blacklist_favicon.png"/>
This causes:

Security Error: Content at about:blocked?e=malwareBlocked&u=http%3A//www.mozilla.org/firefox/its-an-attack.html&s=blacklist&c=UTF-8&d=The%20site%20at%20www.mozilla.org%20has%20been%20reported%20as%20an%20attack%20site%20and%20has%20been%20blocked%20based%20on%20your%20security%20preferences. may not load or link to chrome://global/skin/icons/blacklist_favicon.png.


Firefox does not have this problem.

> +  locale/@AB_CD@/communicator/safeBrowsing.dtd                              (%chrome/common/safeBrowsing.dtd)

Firefox uses "phishing-afterload-warning-message.dtd" I decided to shorten the name of the file.

TODO: add UI to Preferences:
> pref("browser.safebrowsing.enabled", true);
> pref("browser.safebrowsing.malware.enabled", true);

Comment 8

[Philip Chee]

Tested on:

http://mozilla.org/firefox/its-a-trap.html
http://mozilla.org/firefox/its-an-attack.html

Comment 9

[Robert Kaiser]

(In reply to Philip Chee from comment #7)
> SAFEBROWSING_ID resolves to SeaMonkey for us but we haven't registered this
> client name with google so we don't get any data from Google.

Do we know if we have permission to use "Firefox" there? That may give them a list that isn't publicly available but only under their contract.

> Security Error: Content at
> about:blocked?e=malwareBlocked&u=http%3A//www.mozilla.org/firefox/its-an-
> attack.html&s=blacklist&c=UTF-8&d=The%20site%20at%20www.mozilla.
> org%20has%20been%20reported%20as%20an%20attack%20site%20and%20has%20been%20bl
> ocked%20based%20on%20your%20security%20preferences. may not load or link to
> chrome://global/skin/icons/blacklist_favicon.png.
> 
> Firefox does not have this problem.

IIRC, they added an explicit exception for this somewhere, I remember there was a bug where they saw this as well before that fix.

Comment 10

[Philip Chee]

> Do we know if we have permission to use "Firefox" there? That may give them a list that
> isn't publicly available but only under their contract.

Official Firefox builds use a client ID of  "navclient-auto-ffox" due to provider agreement with Google.

Unofficial builds (e.g. Nightlies) use "Firefox".
We're definitely "unofficial" ;)
IceWeasel use to use "Firefox" until they registered Iceweasel with Google. We should find out how to do this eventually, but in the meantime I want to get this code into the tree for testing.

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 686491 [details] [diff] [review]
Patch v0.1 WIP.

Patch doesn't apply. The part changed in navigator.js function handleURLBarCommand is simply not there. Does this have some dependencies you forgot to mention?

Comment 12

[rsx11m]

(In reply to Philip Chee from comment #8 and comment #10)
> http://mozilla.org/firefox/its-a-trap.html

Interestingly that page claims "using an e-mail product like Mozilla Thunderbird, which detects and alerts you when it finds links to web forgeries in your e-mail" which is *not* correct as it's based on a simple, non-expandable set of rules.

Not being familiar with all the legal stuff behind it, as I recall the reason for Thunderbird not being able to use Google's phishing list was stated as not being part of the contract which Firefox has. Thus, it may not be clear that SeaMonkey can use it as it's not an unofficial build of Firefox but a separate application under the Mozilla umbrella (again, I'm just guessing but not knowing here).

Comment 13

[Philip Chee]

As far as I know, the Thunderbird phishing detector does use the url classifier tables to check for malicious email. TB of course also has a list of hard coded rules like SeaMonkey.

Comment 14

[Philip Chee]

> As far as I know, the Thunderbird phishing detector does use the url classifier tables
> to check for malicious email. TB of course also has a list of hard coded rules like
> SeaMonkey.

http://mxr.mozilla.org/comm-central/search?string=safebrowsing\..*enabled&regexp=1&case=1&find=%2Fmail%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

Comment 15

[Tony Mechelynck [:tonymec]]

In reply to comment #14: The results of this search seem to indicate that browser.safebrowsing.enabled defaults to false (= disable Safe Browsing) in Thunderbird and to undefined (= Safe Browsing does nothing) (which is what about:config shows me) in SeaMonkey.

browser.safebrowsing.remoteLookups (also required for Safe Browsing to do anything) also defaults to undefined in SeaMonkey (or at least, in SeaMonkey 2.17a1 nightlies).

See also:
    http://kb.mozillazine.org/Browser.safebrowsing.enabled
    http://kb.mozillazine.org/Browser.safebrowsing.remoteLookups
    https://wiki.mozilla.org/Phishing_Protection which says Google released it under MPL 1.1 / GPL 2.0 / LGPL 2.1

Comment 16

[Philip Chee]

> http://kb.mozillazine.org/Browser.safebrowsing.remoteLookups
Has an effect in Mozilla Firefox (nightly builds from 2006-05-12 to 2008-02-08; 2.0)
MXR shows no hits for browser.safebrowsing.remoteLookups

Comment 17

[Philip Chee]

Attachment: Patch v0.2 WIP

> Patch doesn't apply. The part changed in navigator.js function
> handleURLBarCommand is simply not there. Does this have some dependencies you
> forgot to mention?
No just screwed up my mqueue. Hopefully this one applies.

Comment 18

[Philip Chee]

> As far as I know, the Thunderbird phishing detector does use the url classifier tables
> to check for malicious email. TB of course also has a list of hard coded rules like

On closer inspection via CVS Blame. Bug 328749 (Phishing Detector APIs) ported the google safe browing code from Firefox to Thunderbird, but Bug 368635 (Hide Phishing controls for downloading local list of known phishing urls) turned this off due to:

> We aren't going to have a phishing service we can legally us in time for thunderbird 2.
> We should hide the UI for this feature from the privacy panel in the Options UI.
> Hopefully we can turn this on in a point release once we have a provider we can use.

Since then, their code has severely bit rotted and I doubt it even works anymore.

Comment 19

[Philip Chee]

http://hg.mozilla.org/mozilla-central/annotate/0352a32fde64/toolkit/components/viewsource/content/viewSource.js#l236

> 236   if (!event.isTrusted || event.target.localName != "button")
> 237     return;
So my blockedSite.xhtml uses <span> like certerror.xhtml but the toolkit code is looking for a html:button.

Comment 20

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 687508 [details] [diff] [review]
Patch v0.2 WIP

(In reply to Philip Chee from comment #8)
> Tested on:
> 
> http://mozilla.org/firefox/its-a-trap.html
> http://mozilla.org/firefox/its-an-attack.html

So I tried these two, got the two types of warnings and the notification bar when using the "Ignore this warning" button. All buttons worked as expected, and when I removed the permissions using the Data Manager, the warnings reappeared -> f=me.

I also tried the URLs in a faked feed in MailNews. The warnings appeared, but not too surprisingly none of the buttons worked (nothing on the Error Console). If this is an issue, it can surely be moved into a follow-up; I just included this for the sake of completeness.

Note that I cannot really comment on whether we're allowed to use this as-is or which URLs/params we'd need to use. And I know this is just a WIP right now, but with the next iteration you should probably add a note next to the safebrowing URL prefs that we cannot leave the hard-coded "17" versions in - just to be safe. ;-)

Comment 21

[Philip Chee]

Attachment: Patch v1.0 includes Help menu items.

Changes since the last patch:
1. Implement the Help->Report Website Forgery menu items.
2. Remove any existing "blocked-badware-page" notification before opening a new one.

> I also tried the URLs in a faked feed in MailNews. The warnings appeared, but 
> not too surprisingly none of the buttons worked (nothing on the Error Console). 
> If this is an issue, it can surely be moved into a follow-up; I just included 
> this for the sake of completeness.

Yeah not unexpected. I'll file a followup bug.

Comment 22

[Philip Chee]

Attachment: Patch v1.1 Tweak inline styles in blockedSite.xhtml

Tweak blockedSite.xhtml styles because we are using xul:buttons.

Comment 23

[Philip Chee]

Comment on attachment 690072 [details] [diff] [review]
Patch v1.1 Tweak inline styles in blockedSite.xhtml

Need more fixes.

Comment 24

[Philip Chee]

Attachment: Patch v1.2 more fixes.

> Changes since the last patch:
> 1. Implement the Help->Report Website Forgery menu items.
> 2. Remove any existing "blocked-badware-page" notification before opening a
> new one.
> 
> Patch v1.1 Tweak inline styles in blockedSite.xhtml
> 
> Tweak blockedSite.xhtml styles because we are using xul:buttons.

Changes since the last patch:

1. Use mozilla.org instead of mozilla.com in report url preferences to avoid a redirect.
2. Forgot to update suite/browser/jar.mn.
3. Allow about:blocked to link to chrome favicon.

> +    <link rel="icon" type="image/png" id="favicon" href="chrome://global/skin/icons/blacklist_favicon.png"/>
> This causes:
> 
> Security Error: Content at about:blocked?e=malwareBlocked&u=http%3A//www.mozilla.org/firefox/its-an-attack.html&s=blacklist&c=UTF-8&d=The%20site%20at%20www.mozilla.org%20has%20been%20reported%20as%20an%20attack%20site%20and%20has%20been%20blocked%20based%20on%20your%20security%20preferences. may not load or link to chrome://global/skin/icons/blacklist_favicon.png.

4. Modern fixes:
4a. blacklist_favicon.png: Use the Past Modern version instead of toolkit winstripe.
4b. blacklist_large.png: Use the toolkit winstripe version (looks better than Past Modern)

Comment 25

[Frank Wein [:mcsmurf]]

We have our own safebrowsing key now, it's
ABQIAAAALT_LuARPWqUj7bX2mqWTJRQt2QEr-yGktcva5ZhZnWk7HItT7w
We still need to check if our code does complies with the terms of services of the safebrowsing API (https://developers.google.com/safe-browsing/terms). It's likely that it does as the Firefox code uses the same API, but better be safe than sorry.

See https://developers.google.com/safe-browsing/developers_guide_v2#ProtocolBasics for how to use it, looks like we need to modify the prefs a bit.

Comment 26

[Philip Chee]

> looks like we need to modify the prefs a bit.
If we use the same API as Firefox, why do we need to "modify the prefs a bit" other than changing the client and appver fields?

http://mxr.mozilla.org/comm-central/search?string=google.com/safebrowsing
Note that none of Firefox/B2G/Fennec uses the apikey field.

Also in the toolkit SafeBrowsing.jsm the following are hardcoded:

> const phishingList = "goog-phish-shavar";
> const malwareList  = "goog-malware-shavar";

Whereas https://developers.google.com/safe-browsing/developers_guide_v2#ProtocolBasics
Says to use googpub-phish-shavar and goog-malware-shavar

We could fork SafeBrowsing.jsm of course, but then this was moved to Toolkit to avoid forking in the first place.

Comment 27

[Frank Wein [:mcsmurf]]

Maybe Firefox has a separate contract with Google for using the safebrowsing API and therefor don't need to use an API key.
The difference between goog-phish-shavar and googpub-phish-shavar is that some third-party results are not included in the googpub list (according to Bug 557752 Comment 14). Debian Iceweasel also uses uses the googpub list.

Comment 28

[Frank Wein [:mcsmurf]]

BTW: Why have phishingList and malwareList been hardcoded when there is an pref called urlclassifier.gethashtables which has as value the names of those two lists.

Comment 29

[Philip Chee]

> BTW: Why have phishingList and malwareList been hardcoded when there is an pref called
> urlclassifier.gethashtables which has as value the names of those two lists.
IIRC This question was raised in one of the firefox/toolkit bugs but never satisfactorily answered/

Comment 30

[Robert Kaiser]

BTW, note that bug 820283 currently is a grave problem with safebrowsing in Firefox and there's no known cause or solution, but bug 806422 seems to be a usable workaround. I haven't looked into the details of the bugs, just wanted to make you people here aware of those.

Comment 31

[Philip Chee]

>> BTW: Why have phishingList and malwareList been hardcoded when there is an pref called
>> urlclassifier.gethashtables which has as value the names of those two lists.
> IIRC This question was raised in one of the firefox/toolkit bugs but never
> satisfactorily answered/
q.v. Bug 557752 - Unbranded releases should use a different phishing shavar

Comment 32

[Philip Chee]

Attachment: Patch v1.3 more tweaks.

> 1. Implement the Help->Report Website Forgery menu items.
> 2. Remove any existing "blocked-badware-page" notification before opening a new one.
> 
> Patch v1.1 Tweak inline styles in blockedSite.xhtml
> 
> Tweak blockedSite.xhtml styles because we are using xul:buttons.

> 1. Use mozilla.org instead of mozilla.com in report url preferences to avoid a redirect.
Oops some mozilla.com addresses aren't redirected to .org. Reverted these changes.

> 2. Forgot to update suite/browser/jar.mn.
> 3. Allow about:blocked to link to chrome favicon.

> +    <link rel="icon" type="image/png" id="favicon" href="chrome://global/skin/icons/blacklist_favicon.png"/>
> This causes:
> 
> Security Error: Content at about:blocked?e=malwareBlocked&u=http%3A//www.mozilla.org/firefox/its-an-attack.html&s=blacklist&c=UTF-8&d=The%20site%20at%20www.mozilla.org%20has%20been%20reported%20as%20an%20attack%20site%20and%20has%20been%20blocked%20based%20on%20your%20security%20preferences. may not load or link to chrome://global/skin/icons/blacklist_favicon.png.

> 4. Modern fixes:
> 4a. blacklist_favicon.png: Use the Past Modern version instead of toolkit winstripe.
> 4b. blacklist_large.png: Use the toolkit winstripe version (looks better than Past Modern)

Changes since the last patch:

On trunk the "urlclassifier.confirm-age" preference is now "urlclassifier.max-complete-age".
Removed a couple of lines of code that aren't actually used (too much CnP from Firefox).
Use more .startsWith() instead of regexp.

Comment 33

[Philip Chee]

Attachment: Patch v1.4 minor tweaks.

> 1. Implement the Help->Report Website Forgery menu items.
> 2. Remove any existing "blocked-badware-page" notification before opening a new one.
> 3. Tweak blockedSite.xhtml styles because we are using xul:buttons.
> 4. Update suite/browser/jar.mn.
> 5. Allow about:blocked to link to chrome favicon.

> 6. Modern fixes:
> 6a. blacklist_favicon.png: Use the Past Modern version instead of toolkit winstripe.
> 6b. blacklist_large.png: Use the toolkit winstripe version (looks better than Past Modern)

> 7. On trunk the "urlclassifier.confirm-age" preference is now "urlclassifier.max-complete-age".
> 8. Removed a couple of lines of code that aren't actually used (too much CnP from Firefox).
> 9. Use more .startsWith() instead of regexp.

Changes since the last patch:

10. Port changes from Bug 825891 (Remove the code for per-client randomization in the url-classifier).
11. Use googpub-phish-shavar instead of goog-phish-shavar.
    Depends on Bug 825417 SafeBrowsing.jsm: Don't hardcode phishing/malware table names (phishingList, malwareList).

Comment 34

[Frank Wein [:mcsmurf]]

Philipp: Do you know if there is any public test URL that we could use to test if it actually downloads the hash tables from Google? Or maybe how I can manually query the API via some JS command? I found that the URL "http://malware.testing.google.test/" is supposed to be in the hash tables. But obviously this gives a DNS error in the browser. Or should safebrowsing actually kick in before trying to resolve a host?

Comment 35

[Frank Wein [:mcsmurf]]

Looks like SafeBrowsing.jsm creates its own database for testing (test-phish-simple.* and test-malware-simple.* in the Cache folder). Anyway, looks like downloading the hash tables seems to work. I was interested in this as I was testing our new API key:
-pref("browser.safebrowsing.keyURL", "https://sb-ssl.google.com/safebrowsing/newkey?client=Firefox&appver=17&pver=2.2");
-pref("browser.safebrowsing.gethashURL", "http://safebrowsing.clients.google.com/safebrowsing/gethash?client=Firefox&appver=17&pver=2.2
");
+pref("browser.safebrowsing.keyURL", "https://sb-ssl.google.com/safebrowsing/newkey?client=api&apikey=ABQIAAAALT_LuARPWqUj7bX2mqWTJRQt2
QEr-yGktcva5ZhZnWk7HItT7w&appver=2.18&pver=2.2");
+pref("browser.safebrowsing.gethashURL", "http://safebrowsing.clients.google.com/safebrowsing/gethash?client=api&apikey=ABQIAAAALT_LuAR
PWqUj7bX2mqWTJRQt2QEr-yGktcva5ZhZnWk7HItT7w&appver=2.18&pver=2.2");

(probably need to adjust the other reporting prefs as well).

Comment 36

[Philip Chee]

> Philipp: Do you know if there is any public test URL that we could use to test if it
> actually downloads the hash tables from Google?
I used this link. Be very careful, these are live phishing sites.
http://www.phishtank.com/ (live examples)

Comment 37

[Philip Chee]

> (probably need to adjust the other reporting prefs as well).
A quick look at the reporting urls, indicates that they are not Firefox specific so I don't think they need changing.

Comment 38

[[e-mail]]

Comment on attachment 702333 [details] [diff] [review]
Patch v1.4 minor tweaks.

>+  setTimeout(function() {SafeBrowsing.init();}, 2000);
[Nit: space after { and before }]

>+    // Show/hide the appropriate menu item.
>+    document.getElementById("reportPhishing").hidden = isPhishingPage || isMalwarePage;
>+    document.getElementById("reportPhishingError").hidden = !isPhishingPage || isMalwarePage;
Nit: malware pages are non-phishing-pages so you don't need to test twice.

>+              var isAllowedPage = [
>+                "about:neterror?",
>+                "about:certerror?",
>+                "about:blocked?",
>+              ].some(function(re) {return targetDoc.documentURI.startsWith(re);});
[Nit: These aren't regexps any more...]
[You could do this in a regexp /^about:(neterror|certerror|blocked)\?/]

>+      <method name="ignoreSafeBrowsingWarning">
So, this bypasses the warning and displays a notification that allows you to report the site as misclassified, right?

>+              callback: function () { getMeOutOfHere(); }
callback: getMeOutOfHere

>+    <link rel="icon" type="image/png" id="favicon" href="chrome://global/skin/icons/blacklist_favicon.png"/>
black?

>+        var error = url.search(/e\=/);
>+        var duffUrl = url.search(/\&u\=/);
Nit: these are fixed strings, just use indexOf.

>+        return decodeURIComponent(url.slice(error + 2, duffUrl));
Nit: this is ASCII, no need to decode it.

> function BrowserOnCommand(event)
This isn't going to work because you're using HTML buttons which don't generate command events. (Our certerror page has an XBL hack to make this work. In theory you could copy this because you have control over the error page, but for the offline try again button I had to put a hack in notification.xml)

>+  var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                        .getService(Components.interfaces.nsIPrefService)
Nit: Services.prefs

>+  content.location = url;
[Not sure this is idea. loadURI perhaps?]

>+<!ENTITY safeb.palm.message.p1 "This page has been reported as a web forgery designed to trick users into sharing personal or financial information. Entering any personal information on this page may result in identity theft or other fraud. &#160;">
Why the nbsp?

>+<!ENTITY safeb.palm.message.p1.linkText "Read more &#187;">
>+<!ENTITY safeb.palm.p1.linkStatusText "Read more &#133;">
Nit: use UTF-8

Comment 39

[[e-mail]]

(In reply to comment #38)
> (From update of attachment 70233 [details])
> >+              var isAllowedPage = [
> >+                "about:neterror?",
> >+                "about:certerror?",
> >+                "about:blocked?",
> >+              ].some(function(re) {return targetDoc.documentURI.startsWith(re);});
> [Nit: These aren't regexps any more...]
> [You could do this in a regexp /^about:(neterror|certerror|blocked)\?/]
[Regexp will be slower on Win64 because the fast regexp library we use doesn't work so we have to use a slower library. Otherwise I would have expected it to be faster.]

> > function BrowserOnCommand(event)
> This isn't going to work because you're using HTML buttons which don't
> generate command events. (Our certerror page has an XBL hack to make this
> work. In theory you could copy this because you have control over the error
> page, but for the offline try again button I had to put a hack in
> notification.xml)
Pointed out on IRC that this actually happens. Sorry about that.

> >+  content.location = url;
> [Not sure this is idea. loadURI perhaps?]
s/idea/ideal/

Comment 40

[[e-mail]]

Comment on attachment 702333 [details] [diff] [review]
Patch v1.4 minor tweaks.

>+            var uri = this.activeBrowser.currentURI;
>+            var flag = Components.interfaces.nsIWebNavigation.LOAD_FLAGS_BYPASS_CLASSIFIER;
>+            this.activeBrowser.loadURIWithFlags(uri.asciiSpec, flag,
>+                                                null, null, null);
...
>+            var pageUrl = uri.asciiSpec;
Nit: use variable more

>+            reportURL += ownerDoc.location.href;
No encoding here?

>+            content.location = reportURL;
[Ditto from previous comment]

Comment 41

[Philip Chee]

Attachment: blockedSite.xhtml diff with the Firefox version.

To make things clearer I'm attaching a diff with the changes I made wrt the Firefox version.

Comment 42

[Philip Chee]

Attachment: Patch v1.5 fix nits.

>>+  setTimeout(function() {SafeBrowsing.init();}, 2000);
> [Nit: space after { and before }]
Fixed.

>>+    // Show/hide the appropriate menu item.
>>+    document.getElementById("reportPhishing").hidden = isPhishingPage || isMalwarePage;
>>+    document.getElementById("reportPhishingError").hidden = !isPhishingPage || isMalwarePage;
> Nit: malware pages are non-phishing-pages so you don't need to test twice.
Fixed.

>>+              var isAllowedPage = [
>>+                "about:neterror?",
>>+                "about:certerror?",
>>+                "about:blocked?",
>>+              ].some(function(re) {return targetDoc.documentURI.startsWith(re);});
> [Nit: These aren't regexps any more...]
> [You could do this in a regexp /^about:(neterror|certerror|blocked)\?/]
> [Regexp will be slower on Win64 because the fast regexp library we use doesn't work so we have to use a slower library. Otherwise I would have expected it to be faster.]
Since we currently don't officially support Win64 I've switched back to regexp.

>>+      <method name="ignoreSafeBrowsingWarning">
> So, this bypasses the warning and displays a notification that allows you to report the site as misclassified, right?
Correct.

>>+              callback: function () { getMeOutOfHere(); }
> callback: getMeOutOfHere
Fixed.

>>+    <link rel="icon" type="image/png" id="favicon" href="chrome://global/skin/icons/blacklist_favicon.png"/>
> black?
See: Bug 380932: land netError page for malware URIs.

>>+        var error = url.search(/e\=/);
>>+        var duffUrl = url.search(/\&u\=/);
> Nit: these are fixed strings, just use indexOf.
Fixed.

>>+        return decodeURIComponent(url.slice(error + 2, duffUrl));
> Nit: this is ASCII, no need to decode it.
Fixed.

>>+  var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>>+                        .getService(Components.interfaces.nsIPrefService)
> Nit: Services.prefs
Fixed.

>>+  content.location = url;
> [Not sure this is ideal. loadURI perhaps?]
Fixed.

>>+<!ENTITY safeb.palm.message.p1 "This page has been reported as a web forgery designed to trick users into sharing personal or financial information. Entering any personal information on this page may result in identity theft or other fraud. &#160;">
> Why the nbsp?

Inherited from the old Google safe browsing extension. However this UI does not exist any more so I've pruned this and other unused strings.

>>+<!ENTITY safeb.palm.message.p1.linkText "Read more &#187;">
>>+<!ENTITY safeb.palm.p1.linkStatusText "Read more &#133;">
> Nit: use UTF-8

Deleted instead.

>>+            var uri = this.activeBrowser.currentURI;
>>+            var flag = Components.interfaces.nsIWebNavigation.LOAD_FLAGS_BYPASS_CLASSIFIER;
>>+            this.activeBrowser.loadURIWithFlags(uri.asciiSpec, flag,
>>+                                                null, null, null);
> ...
>>+            var pageUrl = uri.asciiSpec;
> Nit: use variable more
Fixed.

>>+            reportURL += ownerDoc.location.href;
> No encoding here?

No idea myself but the example on this page:
http://googleonlinesecurity.blogspot.com/2008/05/safe-browsing-diagnostic-to-rescue.html
is:
http://www.google.com/safebrowsing/diagnostic?site=http://malware.testing.google.test/testing/malware/
which doesn't appear encoded. All the other examples pulled up with Google search imply no encoding.

I've tested with: http://www.mozilla.org/firefox/its-a-trap.html
Which goes to: http://safebrowsing.clients.google.com/safebrowsing/diagnostic?client=Firefox&hl=en-US&site=http://www.mozilla.org/firefox/its-a-trap.html
Which seens to work properly.

>>+            content.location = reportURL;
> [Ditto from previous comment]
Fixed.

Comment 43

[Philip Chee]

> -pref("browser.safebrowsing.keyURL", "https://sb-ssl.google.com/safebrowsing/newkey?client=Firefox&appver=17&pver=2.2");
> -pref("browser.safebrowsing.gethashURL", "http://safebrowsing.clients.google.com/safebrowsing/gethash?client=Firefox&appver=17&pver=2.2");
> +pref("browser.safebrowsing.keyURL", "https://sb-ssl.google.com/safebrowsing/newkey?client=api&apikey=ABQIAAAALT_LuARPWqUj7bX2mqWTJRQt2QEr-yGktcva5ZhZnWk7HItT7w&appver=2.18&pver=2.2");
> +pref("browser.safebrowsing.gethashURL", "http://safebrowsing.clients.google.com/safebrowsing/gethash?client=api&apikey=ABQIAAAALT_LuARPWqUj7bX2mqWTJRQt2QEr-yGktcva5ZhZnWk7HItT7w&appver=2.18&pver=2.2");

Don't we need the "browser.safebrowsing.updateURL" too?

http://safebrowsing.clients.google.com/safebrowsing/downloads?client=api&apikey=ABQIAAAALT_LuARPWqUj7bX2mqWTJRQt2QEr-yGktcva5ZhZnWk7HItT7w&appver=2.18&pver=2.2

Comment 44

[Frank Wein [:mcsmurf]]

You're right, forgot to update the updateURL pref.

Comment 45

[Philip Chee]

> http://www.phishtank.com/ (live examples)
You need to search for verified phishing sites:
http://www.phishtank.com/phish_search.php?valid=y&active=All&Search=Search

Comment 46

[Philip Chee]

Attachment: Screenshot of phishing page with a very long URL

Random site from phishtank

Comment 47

[Ian Neal]

Comment on attachment 705357 [details] [diff] [review]
Patch v1.5 fix nits.

This is looking very good.
We should probably expose the preferences for enabling/debugging within the UI somewhere.
Debug probably under the Debug pref pane.
Enabling somewhere under Browser, Privacy & Security or Advanced?

Comment 48

[Philip Chee]

> We should probably expose the preferences for enabling/debugging within the UI
> somewhere. Debug probably under the Debug pref pane.
Sounds reasonable.

> Enabling somewhere under Browser, Privacy & Security or Advanced?
I'd say Privacy & Security.

Comment 49

[Philip Chee]

Attachment: Patch v1.6 fixed UI when phishing page has a very long URL

> phishing page with a very long URL
Use { word-wrap: break-word } so that scrollbars don't appear in the blocked site warning.

I'll update the updateURL/keyURL/gethashURL with the SeaMonkey key that mcsmurf got from google, once he confirms that they work.

Comment 50

[Philip Chee]

Comment on attachment 707041 [details] [diff] [review]
Patch v1.6 fixed UI when phishing page has a very long URL

> +<!ENTITY safeb.palm.notforgery.label2 "This isn't a web forgery…">
Oops. this is not used. Removed locally.

Comment 51

[[e-mail]]

Comment on attachment 705355 [details] [diff] [review]
blockedSite.xhtml diff with the Firefox version.

>-      /* Style warning button to look like a small text link in the
>-         bottom right. This is preferable to just using a text link
>-         since there is already a mechanism in browser.js for trapping
>-         oncommand events from unprivileged chrome pages (BrowserOnCommand).*/
As we're using XUL buttons, we should remove all the relevant styles, so I found that the best style for me was

#buttons > span {
  float: left;
}

#ignoreWarningButton {
  float: right;
}

Comment 52

[[e-mail]]

Comment on attachment 707041 [details] [diff] [review]
Patch v1.6 fixed UI when phishing page has a very long URL

>+pref("browser.safebrowsing.warning.infoURL", "http://www.mozilla.org/%LOCALE%/firefox/phishing-protection/");
[...]

>diff --git a/suite/browser/navigatorOverlay.xul b/suite/browser/navigatorOverlay.xul
>--- a/suite/browser/navigatorOverlay.xul
>+++ b/suite/browser/navigatorOverlay.xul
>@@ -6,16 +6,17 @@
> <?xul-overlay href="chrome://global/content/globalOverlay.xul"?>
> <?xul-overlay href="chrome://communicator/content/utilityOverlay.xul"?>
> <?xul-overlay href="chrome://communicator/content/viewZoomOverlay.xul"?>
> <?xul-overlay href="chrome://communicator/content/viewApplyThemeOverlay.xul"?>
> <?xul-overlay href="chrome://communicator/content/tasksOverlay.xul"?>
> <?xul-overlay href="chrome://communicator/content/bookmarks/placesOverlay.xul"?>
> <?xul-overlay href="chrome://global/content/charsetOverlay.xul"?>
> <?xul-overlay href="chrome://navigator/content/mailNavigatorOverlay.xul"?>
>+<?xul-overlay href="chrome://navigator/content/safeBrowsingOverlay.xul"?>
[Hmm, what about the Mac, when it has no windows open? Maybe this would be better in navigator.xul]

> contract @mozilla.org/network/protocol/about;1?what= {d54f2c89-8fd6-4eeb-a7a4-51d4dcdf460f}
> contract @mozilla.org/network/protocol/about;1?what=certerror {d54f2c89-8fd6-4eeb-a7a4-51d4dcdf460f}
> contract @mozilla.org/network/protocol/about;1?what=data {d54f2c89-8fd6-4eeb-a7a4-51d4dcdf460f}
> contract @mozilla.org/network/protocol/about;1?what=feeds {d54f2c89-8fd6-4eeb-a7a4-51d4dcdf460f}
> contract @mozilla.org/network/protocol/about;1?what=life {d54f2c89-8fd6-4eeb-a7a4-51d4dcdf460f}
> contract @mozilla.org/network/protocol/about;1?what=rights {d54f2c89-8fd6-4eeb-a7a4-51d4dcdf460f}
> contract @mozilla.org/network/protocol/about;1?what=sessionrestore {d54f2c89-8fd6-4eeb-a7a4-51d4dcdf460f}
> contract @mozilla.org/network/protocol/about;1?what=sync-tabs {d54f2c89-8fd6-4eeb-a7a4-51d4dcdf460f}
>+contract @mozilla.org/network/protocol/about;1?what=blocked {d54f2c89-8fd6-4eeb-a7a4-51d4dcdf460f}
Nit: these were alphabetical...

>+  blockedFlags: SCRIPT | UNTRUSTED | HIDE,
>+  blockedURI: "chrome://communicator/content/blockedSite.xhtml",
... as were these.

>+            content.location = Services.urlFormatter.formatURLPref("browser.safebrowsing.warning.infoURL");
Nit: loadURI

>+<!ENTITY safeb.palm.notforgery.label2 "This isn't a web forgeryâ¦">
Unused?

>+/* Custom styling for 'blacklist' error class */
>+:root.blacklist #errorTitle, :root.blacklist #errorLongContent,
>+:root.blacklist #errorShortDesc, :root.blacklist #errorLongDesc,
>+:root.blacklist a {
>+  background-color: #772222; /* Dark red */
>+  color: #FFFFFF;
>+}
No point repeating yourself, just set the colour on the error page container.

>+:root.blacklist #errorTryAgain {
>+  display: none;
>+}
Doesn't exist.

Comment 53

[[e-mail]]

Comment on attachment 707041 [details] [diff] [review]
Patch v1.6 fixed UI when phishing page has a very long URL

>+              oncommand="openUILinkIn(gSafeBrowsing.getReportURL('Error'), 'tab');"
>+              onclick="checkForMiddleClick(this, event);"/>
No point checking for middle click if you're always going to open in a tab (and should that be tabfocused?)

>+        document.title = document.getElementById("errorTitleText_malware")
>+                                 .innerHTML;
IMHO these should use .textContent since titles can't contain HTML.

>+        <!-- Action buttons -->
>+        <div id="buttons">
>+          <!-- Commands handled in utilityOverlay.js -->
>+          <span id="getMeOutOfHereButton" label="&safeb.palm.accept.label;"/>
>+          <span id="reportButton" label="&safeb.palm.reportPage.label;"/>
>+        </div>
>+      </div>
>+      <div id="ignoreWarning">
>+        <span id="ignoreWarningButton" label="&safeb.palm.decline.label;"/>
>+      </div>
[This could be simplified slightly if you were to use my suggested styles]

>+  if (docURI.startsWith("about:neterror?e=nssBadCert") ||
>+      docURI.startsWith("about:certerror?")) {
>+
[While you're here, we originally kept this in case someone disabled
about:certerror in about:config but that won't work as of bug 590725]

>+safebrowsing.notAForgeryButton.accessKey=F
Won't this conflict with the File menu?

Comment 54

[Philip Chee]

Attachment: Patch v1.7 fix more review issues.

> As we're using XUL buttons, we should remove all the relevant styles, so I found that the best style for me was
> 
> #buttons > span {
>   float: left;
> }
> 
> #ignoreWarningButton {
>   float: right;
> }
Fixed.

> [Hmm, what about the Mac, when it has no windows open? Maybe this would be better in navigator.xul]
Fixed.

> Nit: these were alphabetical...
Fixed.

>>+  blockedFlags: SCRIPT | UNTRUSTED | HIDE,
>>+  blockedURI: "chrome://communicator/content/blockedSite.xhtml",
> ... as were these.
Fixed.

>>+            content.location = Services.urlFormatter.formatURLPref("browser.safebrowsing.warning.infoURL");
> Nit: loadURI
Fixed.

>>+<!ENTITY safeb.palm.notforgery.label2 "This isn't a web forgery…">
> Unused?
Removed.

> No point repeating yourself, just set the colour on the error page container.
Fixed.

>>+:root.blacklist #errorTryAgain {
>>+  display: none;
>>+}
> Doesn't exist.
Removed.

>>+              oncommand="openUILinkIn(gSafeBrowsing.getReportURL('Error'), 'tab');"
>>+              onclick="checkForMiddleClick(this, event);"/>
> No point checking for middle click if you're always going to open in a tab (and should that be tabfocused?)
Removed and using tabfocused now.

>>+        document.title = document.getElementById("errorTitleText_malware")
>>+                                 .innerHTML;
> IMHO these should use .textContent since titles can't contain HTML.
Fixed.

>>+        <!-- Action buttons -->
>>+        <div id="buttons">
>>+          <!-- Commands handled in utilityOverlay.js -->
>>+          <span id="getMeOutOfHereButton" label="&safeb.palm.accept.label;"/>
>>+          <span id="reportButton" label="&safeb.palm.reportPage.label;"/>
>>+        </div>
>>+      </div>
>>+      <div id="ignoreWarning">
>>+        <span id="ignoreWarningButton" label="&safeb.palm.decline.label;"/>
>>+      </div>
> [This could be simplified slightly if you were to use my suggested styles]
Fixed.

>>+  if (docURI.startsWith("about:neterror?e=nssBadCert") ||
>>+      docURI.startsWith("about:certerror?")) {
>>+
> [While you're here, we originally kept this in case someone disabled
> about:certerror in about:config but that won't work as of bug 590725]
Fixed.

>>+safebrowsing.notAForgeryButton.accessKey=F
> Won't this conflict with the File menu?
Now using "s"

Comment 55

[Philip Chee]

Comment on attachment 708092 [details] [diff] [review]
Patch v1.7 fix more review issues.

oops. need to fix something.

Comment 56

[Philip Chee]

Attachment: Patch v1.7a fix more review issues.

> As we're using XUL buttons, we should remove all the relevant styles, so I found that the best style for me was
> 
> #buttons > span {
>   float: left;
> }
> 
> #ignoreWarningButton {
>   float: right;
> }
Fixed.

> [Hmm, what about the Mac, when it has no windows open? Maybe this would be better in navigator.xul]
Fixed.

> Nit: these were alphabetical...
Fixed.

>>+  blockedFlags: SCRIPT | UNTRUSTED | HIDE,
>>+  blockedURI: "chrome://communicator/content/blockedSite.xhtml",
> ... as were these.
Fixed.

>>+            content.location = Services.urlFormatter.formatURLPref("browser.safebrowsing.warning.infoURL");
> Nit: loadURI
Fixed.

>>+<!ENTITY safeb.palm.notforgery.label2 "This isn't a web forgery…">
> Unused?
Removed.

> No point repeating yourself, just set the colour on the error page container.
Fixed.

>>+:root.blacklist #errorTryAgain {
>>+  display: none;
>>+}
> Doesn't exist.
Removed.

>>+              oncommand="openUILinkIn(gSafeBrowsing.getReportURL('Error'), 'tab');"
>>+              onclick="checkForMiddleClick(this, event);"/>
> No point checking for middle click if you're always going to open in a tab (and should that be tabfocused?)
Removed and using tabfocused now.

>>+        document.title = document.getElementById("errorTitleText_malware")
>>+                                 .innerHTML;
> IMHO these should use .textContent since titles can't contain HTML.
Fixed.

>>+        <!-- Action buttons -->
>>+        <div id="buttons">
>>+          <!-- Commands handled in utilityOverlay.js -->
>>+          <span id="getMeOutOfHereButton" label="&safeb.palm.accept.label;"/>
>>+          <span id="reportButton" label="&safeb.palm.reportPage.label;"/>
>>+        </div>
>>+      </div>
>>+      <div id="ignoreWarning">
>>+        <span id="ignoreWarningButton" label="&safeb.palm.decline.label;"/>
>>+      </div>
> [This could be simplified slightly if you were to use my suggested styles]
Fixed.

>>+  if (docURI.startsWith("about:neterror?e=nssBadCert") ||
>>+      docURI.startsWith("about:certerror?")) {
>>+
> [While you're here, we originally kept this in case someone disabled
> about:certerror in about:config but that won't work as of bug 590725]
Fixed.

>>+safebrowsing.notAForgeryButton.accessKey=F
> Won't this conflict with the File menu?
Now using "s"

Comment 57

[Frank Wein [:mcsmurf]]

Comment on attachment 705357 [details] [diff] [review]
Patch v1.5 fix nits.

Using the SeaMonkey API key (as mentioned in Comment 35/Comment 41) seems to work fine. I created a new testing profile and used a URL from phishtank.com to test if it works. After creating a new profile, it took a few minutes(? maybe it was only a minute, not sure) for it to fetch the hash tables. Now I have 12 files in the safebrowsing folder in the profile with 1,17MB in size.
What confused me a bit: With the old (default, daily use) profile the hash tables are in the %USERPROFILE%\AppData\Local\Mozilla\SeaMonkey\... folder. With the new profile the hash tables are in the %USERPROFILE%\AppData\Roaming\Mozilla\SeaMonkey\... folder. I'm not sure why that is, did the patch maybe change the locations of the files once (I tested a few patches in the last weeks)?

Comment 58

[Frank Wein [:mcsmurf]]

Actually, ignore the Local/Roaming thing, this might be a bug on trunk not related to your patch (need to update my build&check again). It also stores the Cache directory in Roaming, so that's wrong.

Comment 59

[[e-mail]]

Comment on attachment 708095 [details] [diff] [review]
Patch v1.7a fix more review issues.

Nit: there were some extra blank lines floating around at the beginning of blocks i.e.
/* stuff here */ {

  // useless blank line above
}

>+        document.title = document.getElementById("errorTitleText_phishing")
>+                                 .innerHTML;
Oops, there were two of these :-( r=me with that fixed.

Comment 60

[Philip Chee]

Attachment: Patch v1.7b as checked-in r=Neil

> Nit: there were some extra blank lines floating around at the beginning of blocks i.e.
Fixed.

>>+        document.title = document.getElementById("errorTitleText_phishing")
>>+                                 .innerHTML;
> Oops, there were two of these :-( r=me with that fixed.
Fixed.

Also since mcsmurf confirms that the apikey versions of the google lookup URLs are working I've now switched the relevant preferences to:

+pref("browser.safebrowsing.updateURL", "http://safebrowsing.clients.google.com/safebrowsing/downloads?client=api&apikey=ABQIAAAALT_LuARPWqUj7bX2mqWTJRQt2QEr-yGktcva5ZhZnWk7HItT7w&appver=%VERSION%&pver=2.2");
+pref("browser.safebrowsing.keyURL", "https://sb-ssl.google.com/safebrowsing/newkey?client=api&apikey=ABQIAAAALT_LuARPWqUj7bX2mqWTJRQt2QEr-yGktcva5ZhZnWk7HItT7w&appver=%VERSION%&pver=2.2");
+pref("browser.safebrowsing.gethashURL", "http://safebrowsing.clients.google.com/safebrowsing/gethash?client=api&apikey=ABQIAAAALT_LuARPWqUj7bX2mqWTJRQt2QEr-yGktcva5ZhZnWk7HItT7w&appver=%VERSION%&pver=2.2");

Comment 61

[rsx11m]

For the sake of completeness, that was changeset
http://hg.mozilla.org/comm-central/rev/eb9aa93d401c

Comment 62

[Philip Chee]

> For the sake of completeness, that was changeset
> http://hg.mozilla.org/comm-central/rev/eb9aa93d401c
Oops thanks.

Comment 20:
> I also tried the URLs in a faked feed in MailNews. The warnings appeared, 
> but not too surprisingly none of the buttons worked (nothing on the Error 
> Console). If this is an issue, it can surely be moved into a follow-up; I 
> just included this for the sake of completeness.

I filed a new bug on this:
Bug 836769 (Hook up MailNews feeds display to the Safe Browsing code so that the buttons and notifications work).

Comment 63

[Stefan [:stefanh]]

Looks like everyone forgot to look in themes/classic/mac. Reopen and fix here?

Comment 64

[Philip Chee]

> Looks like everyone forgot to look in themes/classic/mac. Reopen and fix here?
Oops.

Comment 65

[Philip Chee]

Attachment: Patch Mv1.0 Theme changes for classic/mac.

> Looks like everyone forgot to look in themes/classic/mac
Quick fix.

Comment 66

[Stefan [:stefanh]]

Comment on attachment 709364 [details] [diff] [review]
Patch Mv1.0 Theme changes for classic/mac.

Thanks!

Comment 67

[Philip Chee]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/17fd50faed6c