Closed Bug 920951 Opened 11 years ago Closed 8 years ago

Update SeaMonkey Safebrowsing preferences to sync with Mozilla-Central

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set
normal

Tracking

(seamonkey2.41 wontfix, seamonkey2.42 fixed, seamonkey2.43 fixed, seamonkey2.44 fixed)

RESOLVED FIXED
seamonkey2.44
Tracking Status
seamonkey2.41 --- wontfix
seamonkey2.42 --- fixed
seamonkey2.43 --- fixed
seamonkey2.44 --- fixed

People

(Reporter: philip.chee, Assigned: frg)

References

Details

Attachments

(1 file, 4 obsolete files)

From Bug 842828:

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #0)
> In Chrome, before the target file is set, there's a call to check locally if
> the download already has a verdict (good or bad), before calling out to the
> application reputation API.
> 
> http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/download/
> chrome_download_manager_delegate.cc&q=checkdownloadurl%20file:%5Esrc/chrome/
> browser/&l=222
> 
> We should do the same thing. This also involves changing the safebrowsing
> code to download the allowlists and blocklists that Chrome is using.
Things to track:
Bug 842828 - Suppress application reputation checks for binaries already found in malware/whitelists
Bug 842828 Comment 30/31: Question does our API key allow use to use these two new tables?
Bug 842828 has landed and has been on m-c for a few days now, NEEDINFO per bug 842828 comment 29.
Flags: needinfo?(philip.chee)
I have a working patch. However I am not getting the two new files in %profile%/safebrowsing/ what I have are the files that were there previously (but updated):

10/10/2013  19:29              12  goog-malware-shavar.cache
10/10/2013  19:29         712,010  goog-malware-shavar.pset
10/10/2013  19:29       1,648,264  goog-malware-shavar.sbstore
10/10/2013  19:29              12  googpub-phish-shavar.cache
10/10/2013  19:29         893,298  googpub-phish-shavar.pset
10/10/2013  19:29         710,544  googpub-phish-shavar.sbstore
10/10/2013  19:28              44  test-malware-simple.cache
10/10/2013  19:28              16  test-malware-simple.pset
10/10/2013  19:28             232  test-malware-simple.sbstore
10/10/2013  19:28              44  test-phish-simple.cache
10/10/2013  19:28              16  test-phish-simple.pset
10/10/2013  19:28             232  test-phish-simple.sbstore

Is this normal?
Flags: needinfo?(philip.chee) → needinfo?(paolo.mozmail)
Flags: needinfo?(paolo.mozmail) → needinfo?(mmc)
Philip, you won't get the new files without using the API key

https://bugzilla.mozilla.org/show_bug.cgi?id=887044

However, we don't have an agreement with Google to use this API for SeaMonkey, so the correct thing to do is to set the new preferences (urlclassifier.download_block_table, or whatever I called it) to empty in SeaMonkey only so that the list manager ignores updates.

Thanks,
Monica
Flags: needinfo?(mmc)
Attached patch Patch v1.0 update prefs. (obsolete) — Splinter Review
This syncs with Firefox. We are not getting new allow/block tables due to not having a contract/agreement with Google. However this patch allows Safe Browsing to continue to work normally.
Attachment #815943 - Flags: review?(iann_bugzilla)
Attachment #815943 - Flags: feedback?(neil)
Attachment #815943 - Flags: feedback?(bugzilla)
So basically we can currently no longer use the malware table from urlclassifier.malware_table as it also requires the use of the tables from urlclassifier.download_block_table and urlclassifier.download_allow_table which we cannot download with our API key?
Oh sorry, nevermind. We just override that one pref, ok, that's fine then. But we should set those new prefs to null then anyway (as mentioned in Comment 4).
Comment on attachment 815943 [details] [diff] [review]
Patch v1.0 update prefs.

r=me with changes as suggested in comment 4
Attachment #815943 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 815943 [details] [diff] [review]
Patch v1.0 update prefs.

I think we should explicitly set appropriate values for all four preferences to avoid confusion. (Then you won't need such a big comment.)
Attachment #815943 - Flags: feedback?(neil)
Comment on attachment 815943 [details] [diff] [review]
Patch v1.0 update prefs.

I agree with Neil, explicitly set those prefs in browser-prefs.js, makes it easier to see what's going on.
Attachment #815943 - Flags: feedback?(bugzilla) → feedback-
Summary: Update SeaMonkey Safebrowsing preferences to sync with Mozilla-Central Bug 842828. → Update SeaMonkey Safebrowsing preferences to sync with Mozilla-Central
I think the safebrowsing feature is currently broken in all trees. 

I am getting a:

Timestamp: 12/19/2015 1:41:43 PM
Error: TypeError: provider is undefined
Source File: resource://gre/modules/SafeBrowsing.jsm
Line: 83

at startup in release, beta and nightly. Didn't try Aurora. Prefs for Safebrowsing.jsm were updated in Bug 1107372. Seamonkey still uses the old prefs.

I merged the new ones from browser/app/profile/firefox.js and modules/libpref/init/all.js.

With the work in progress patch the error goes away and output with debug true looks ok but I do not see google lists in my local profile. I also think the feature itself is still broken. Visiting a phishing site still is possible so at least the urlclassifiers might also need changes.

FRG
Reporting sites also do not longer work:

Timestamp: 19.12.2015 14:16:02
Error: TypeError: URI is undefined
Source File: resource://gre/modules/SafeBrowsing.jsm
Line: 137

suite/browser/safeBrowsingOverlay.js needs an update.
Download lists from google fail with a 403:

listmanager: 16:07:35 GMT+0100 (W. Europe Standard Time): makeUpdateRequestForEntry_: request goog-phish-shavar;
goog-malware-shavar;
goog-unwanted-shavar;
 update: https://safebrowsing.google.com/safebrowsing/downloads?client=api&apikey=ABQIAAAALT_LuARPWqUj7bX2mqWTJRQt2QEr-yGktcva5ZhZnWk7HItT7w&appver=2.43a1&pver=2.2 tablelist: goog-phish-shavar,goog-malware-shavar,goog-unwanted-shavar


listmanager: 16:07:40 GMT+0100 (W. Europe Standard Time): download error for goog-phish-shavar,goog-malware-shavar,goog-unwanted-shavar: 403

Is 2.2 still good?

Fromm google:

+++ 
This document describes the DEPRECATED version 2.2 of the Safe Browsing API. This version will be turned off after December 1, 2014. See the current developer's guide for information on the latest version of the API.
Apikey name has changed but even if I change the name form apikey to key and update the prefs I get a 403. This time with an autorization error. Looks like a new supersecret key is needed. See bug 957704

https://bug957704.bmoattachments.org/attachment.cgi?id=8357841

Best for your privacy to get rid of google safebrowsing and be done with it.

FRG
Attached patch 920951-safebrowsing-list.patch (obsolete) — Splinter Review
List updates for tracking protection work. Safebrowsing list updates also work with this patch if you replace %SUPER_SECRET_APIKEY% with the actual Mozilla FF 43.0.1 key (which I am sure should not be used here).

Safebrowsing itself is still broken. Websites will not be blocked.

No errors shown in web console with this patch.

User agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:46.0) Gecko/20100101 Firefox/46.0 SeaMonkey/2.43a1
Build identifier: 20151220105535
Attachment #8700304 - Attachment is obsolete: true
(In reply to Frank-Rainer Grahl from comment #16)
> Apikey name has changed but even if I change the name form apikey to key and
> update the prefs I get a 403. This time with an autorization error. Looks
> like a new supersecret key is needed. See bug 957704
> 
> https://bug957704.bmoattachments.org/attachment.cgi?id=8357841
> 
> Best for your privacy to get rid of google safebrowsing and be done with it.
> 
> FRG

Probably that would be the best solution. Yes, you need a secret app key. IIRC there was a problem getting such a key for SeaMonkey, unfortunately I can't find anymore why there was a problem.
See Also: → 1243545
Attached file 920951-safebrowsing_lists-V2.patch (obsolete) (deleted) —
This patch seems to work if used with a valid google api key. The api key is set at build time in the .mozconfig file with ac_add_options --with-google-api-keyfile=<your key directory>/googleapi.key

Blocks access and shows block page. Tested with key on a standard phishing site in the google blacklist and the itsatrap sites found on
https://support.mozilla.org/en-US/kb/how-does-phishing-and-malware-protection-work

VS2015 x64 en-US Windows 7 

User agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0 SeaMonkey/2.44a1
Build identifier: 20160213192448

Ratty,

should the patch replace yours? Then I would clean it up and remove the parts from Bug 1200263 from it.
Attachment #8700411 - Attachment is obsolete: true
Flags: needinfo?(philip.chee)
Comment on attachment 8719200 [details]
920951-safebrowsing_lists-V2.patch

Pay attention to Bug 1109475 - Firefox should use HTTPS instead of HTTP for Safe Browsing URLs

>  pref("browser.safebrowsing.warning.infoURL", "http://www.mozilla.org/%LOCALE%/firefox/phishing-protection/");
https: works for the above line
> -pref("browser.safebrowsing.malware.reportURL", "http://safebrowsing.clients.google.com/safebrowsing/diagnostic?client=Firefox&hl=%LOCALE%&site=");

>      var pageUri = getBrowser().currentURI.cloneIgnoringRef();
Unfortunately SafeBrowsing.jsm uses clone() and not cloneIgnoringRef() so the above line can't be removed at the moment.

>      // Remove the query to avoid including potentially sensitive data
>      if (pageUri instanceof Components.interfaces.nsIURL)
>        pageUri.query = "";
You can get rid of the above chunk since SafeBrowsing.jsm already does this.

> -              reportName = "MalwareError";
> +              reportName = "MalwareMistake";
>              }
>              else {
>                title = "safebrowsing.reportedWebForgery";
>                label = "safebrowsing.notAForgeryButton.label";
>                accessKey = "safebrowsing.notAForgeryButton.accessKey";
> -              reportName = "Error";
> +              reportName = "PhishMistake";
>              }
>              title = this._stringBundle.GetStringFromName(title);
 
>              var tmp = {};

>              Components.utils.import("resource://gre/modules/SafeBrowsing.jsm", tmp);
>              var reportUrl = tmp.SafeBrowsing.getReportURL(reportName);
>              reportUrl += "&url=" + encodeURIComponent(asciiSpec);
You need to fix getReportURL() here too e.g.:

  var reportUrl = tmp.SafeBrowsing.getReportURL(reportName, uri)

>   else if (docURI.startsWith("about:blocked")) {
>     // The event came from a button on a malware/phishing block page
>     // First check whether it's malware or phishing, so that we can
>     // use the right strings/links
>     let isMalware = /e=malwareBlocked/.test(docURI);
In place of a boolean isMalware, Firefox is now using a string "reason"

>   onAboutBlocked: function (targetElement, ownerDoc) {
>     var reason = 'phishing';
>     if (/e=malwareBlocked/.test(ownerDoc.documentURI)) {
>       reason = 'malware';
>     } else if (/e=unwantedBlocked/.test(ownerDoc.documentURI)) {
>       reason = 'unwanted';
>     } else if (/e=forbiddenBlocked/.test(ownerDoc.documentURI)) {
>       reason = 'forbidden';
We want "unwanted" but probably not forbidden as we don't have the parental controls code.

grep firefox for "onAboutBlocked", "Browser:SiteBlockedError", and "ignoreWarningButton"
Flags: needinfo?(philip.chee)
Attachment #815943 - Attachment is obsolete: true
I think we should keep this bug to just updating the prefs. The other changes should go into a new bug. Can you come up with a new patch with just the changes to the browser-prefs.js file? Thanks.
Assignee: philip.chee → frgrahl
Flags: needinfo?(frgrahl)
Patch for the download lists.
Flags: needinfo?(frgrahl)
Depends on: 1250600
The content of attachment 8719200 [details] has been deleted for the following reason:

as per bug 1251279
Comment on attachment 8722583 [details] [diff] [review]
920951-safebrowsing_lists-V3.patch

Almost there!

> +pref("browser.safebrowsing.downloads.remote.block_dangerous",            true);
> +pref("browser.safebrowsing.downloads.remote.block_dangerous_host",       true);
> +pref("browser.safebrowsing.downloads.remote.block_potentially_unwanted", false);
> +pref("browser.safebrowsing.downloads.remote.block_uncommon",             false);
Please collapse the whitespace. Yes I know some parts of this file align vertically, but not this part.

> -pref("browser.safebrowsing.id", "SeaMonkey");
> +//pref("browser.safebrowsing.id", "SeaMonkey");
> +pref("browser.safebrowsing.id", "navclient-auto-ffox");
Please remove the comment out line. Also we aren't Firefox. Does using "SeaMonkey" make a difference?

> +pref("browser.safebrowsing.provider.mozilla.lists", "mozstd-track-digest256,mozstd-trackwhite-digest256,mozfull-track-digest256");
> +pref("browser.safebrowsing.provider.mozilla.updateURL", "https://tracking.services.mozilla.com/downloads?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2");
> +pref("browser.safebrowsing.provider.mozilla.gethashURL", "https://tracking.services.mozilla.com/gethash?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2");
These three lines are already in all.js so they are redundant. Please remove.
Attachment #8722583 - Flags: feedback+
Philip,

>> Please collapse the whitespace. Yes I know some parts of this file align vertically, but not this part.

Can do but it's straight 1:1 out of mozilla\browser\app\profile\firefox.js. Shouldn't it be left in as is if future merges are needed?

>> Please remove the comment out line. Also we aren't Firefox. Does using "SeaMonkey" make a difference?

Will check. Only tested with the straight FF urls till now.

FRG
Flags: needinfo?(philip.chee)
Cleand up patch for prefs. 

Changing the default settings result in a 503 so Seamonkey as a client id not currently possible:

>> listmanager: 11:58:59 GMT+0100 (W. Europe Standard Time): download error for 
>> goog-phish-shavar,goog-malware-shavar,goog-unwanted-shavar,goog-badbinurl-
>> shavar,goog-downloadwhite-digest256: 503

Let me know if review should be set?
Attachment #8722583 - Attachment is obsolete: true
Comment on attachment 8724383 [details] [diff] [review]
920951-safebrowsing_lists-V4.patch

>> Please collapse the whitespace. Yes I know some parts of this file align
>> vertically, but not this part.

> Can do but it's straight 1:1 out of
> mozilla\browser\app\profile\firefox.js.
> Shouldn't it be left in as is if future merges are needed?
Not unless you're running some sort of automated merge tool, which I assume you're not...

>> Please remove the comment out line. Also we aren't Firefox. Does using
>> "SeaMonkey" make a difference?
> Changing the default settings result in a 503 so Seamonkey as a client id
> not currently possible:
> 
>>> listmanager: 11:58:59 GMT+0100 (W. Europe Standard Time): download error for 
>>> goog-phish-shavar,goog-malware-shavar,goog-unwanted-shavar,goog-badbinurl-
>>> shavar,goog-downloadwhite-digest256: 503
Yuck.  I guess this is OK as long as we don't use the Firefox API KEY.

>  // Normally the "client ID" sent in updates is appinfo.name, but
> -// official Firefox releases from Mozilla use a special identifier.
> -// This is currently unused as we are using the apikey method.
> +// official Firefox releases from Mozilla use navclient-auto-ffox 
> +// as a special identifier.
The above comment is now wrong. Perhaps something more informative like:

//Theoretically the "client ID" sent in updates should be appinfo.name but 
//anything except "Firefox" or "navclient-auto-ffox" will cause safebrowsing 
//updates to fail. So we pretend to be Firefox here.

> Let me know if review should be set?
No worries. I'll set it.
Flags: needinfo?(philip.chee)
Attachment #8724383 - Flags: review+
http://hg.mozilla.org/comm-central/rev/9e71c34f77d7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.44
> status-seamonkey2.41: --- → affected
> status-seamonkey2.42: --- → affected
> status-seamonkey2.43: --- → affected
I'm just guessing. How far back should these be backported to?
>> I'm just guessing. How far back should these be backported to?

I would just leave it in c-c for now. I am quite sure without an api key and maybe even a unique client id it will just produce silent 503 infos in the log. That what I got first without a key. I will see that I get the other bug in reviewable shape so that at least the code is clean. 

Maybe someone from the council should open a third bug to decide what to do about the api key?
See Also: → 1260457
Comment on attachment 8724383 [details] [diff] [review]
920951-safebrowsing_lists-V4.patch

[Approval Request Comment]
Regression caused by (bug #): 842828 and others.
User impact if declined: Tracking protection and safe browsing does not work
Testing completed (on m-c, etc.): c-r and c-b
Risk to taking this patch (and alternatives if risky): It's broken already and can only get better. See bug 1260457.
String changes made by this patch: none
Attachment #8724383 - Flags: approval-comm-release?
Attachment #8724383 - Flags: approval-comm-beta?
Target Milestone: seamonkey2.44 → seamonkey2.42
frg: "Target Milestone" is the version which was trunk at the time this was fixed. It remains at that version even if it lands on the branches (which in turn is documented by the "Tracking Flags").
Target Milestone: seamonkey2.42 → seamonkey2.44
Comment on attachment 8724383 [details] [diff] [review]
920951-safebrowsing_lists-V4.patch

a=me for comm-beta and comm-release
Attachment #8724383 - Flags: approval-comm-release?
Attachment #8724383 - Flags: approval-comm-release+
Attachment #8724383 - Flags: approval-comm-beta?
Attachment #8724383 - Flags: approval-comm-beta+
reopened.

checkin-needed for comm-beta and comm-release only.
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Pushed to comm-beta and comm-release
http://hg.mozilla.org/releases/comm-beta/rev/67e87b488a5f
http://hg.mozilla.org/releases/comm-release/rev/e7524e71a31f
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 1269773
You need to log in before you can comment on or make changes to this bug.