420356 - Ship blocklist.xml with Firefox

Status: VERIFIED FIXED

(Firefox :: Security, defect, P1)

Description

[Dave Townsend [:mossop]]

I find myself wondering whether it would be a good idea to ship a base blocklist file with Firefox. Currently if you are a first time user starting Firefox there will be no blocklist present until you have been running Firefox for a continuous 10 minutes. It seems to me that that is ample time for already installed addon to crash you or for you to install a bad one.

This could be as simple as including a blocklist.xml in the default profile so it'd be there for newly created profiles. People migrating from Firefox 2 already have an update to date blocklist in their profile. So long as we remember to do an update of the blocklist before each main release, the average user could be better protected for that first startup experience.

Comment 1

[Dave Townsend [:mossop]]

I have to correct myself. The blocklist is not downloaded until at least 1 day after the first run of Firefox.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking for resolution on this; I'm curious to know what we'd populate it with? Would we update it when shipping dot-dot release updates? That sounds like goodness, too.

Comment 3

[Dave Townsend [:mossop]]

Yes my theory was that at some point during each release (maybe as we freeze or something or bump versions) we would take the current blocklist.xml and put it in for the release.

Comment 4

[Mike Connor [:mconnor]]

What I think we want to do here is:

* Add blocklist.xml to the set of profile defaults
* Tweak the "required files" code to replace if the file date is newer (optional, but probably right, consider the case where the user hasn't run firefox in a while but downloads Firefox 3 to take a spin.
* Figure out the best way to get latest-and-greatest blocklist.xml into the build process.  Could be as simple as a smart wget call during the build process.  Not sure at all how we'd want to do this, ccing build folks for ideas.

Comment 5

[Dave Townsend [:mossop]]

We currently use a different url for downloading the blocklist depending on the application and version. I'm assuming that in reality there is just a single blocklist file that we can use for our purposes here though, can you confirm morgamic?

Comment 6

[(not currently active) Ted Mielczarek]

Any reason we can't just have this live in CVS?

Comment 7

[Mike Connor [:mconnor]]

Ted: define have it live in CVS?  Meaning we'd maintain the blocklist as a CVS file and import it to the web service we're pointing at?  Pretty sure we have code to generate the blocklist snippets so we don't have to push any more data than needed (IOW we should only push blocklist data that applies to the current version, anything more is wasted bandwidth and diskspace).

If we had a bunch of "broken in Fx2, fine in Fx3" extensions, that single file would just add to startup time parsing, in any case.  Why not just grab the appropriate blocklist for the app+version we're building?

Comment 8

[(not currently active) Ted Mielczarek]

Either have the source data in CVS, or just a snapshot of the data stored in CVS that we can update. Pre-processing it to only have data relevant to the branch it's on would be fine. Just thinking that would simplify the build logic a lot.

Comment 9

[Benjamin Smedberg]

Having a build process do "wget" is a non-starter, because it leads to non-reproducable builds and depends on whatever machine is doing the build having wget and access to the internet... most build machines are firewalled from the internet.

Comment 10

[Dave Townsend [:mossop]]

(In reply to comment #4)
> * Add blocklist.xml to the set of profile defaults
> * Tweak the "required files" code to replace if the file date is newer
> (optional, but probably right, consider the case where the user hasn't run
> firefox in a while but downloads Firefox 3 to take a spin.

Actually it need not necessarily be in the profile default. The blocklist service could be adjusted to look in two different places for the blocklist file, the profile and somewhere in the appdir. It could either use the newest one, or maybe even combine the two.

Comment 11

[Nick Thomas [:nthomas] (UTC+12)]

Agree with comment #9. In a perfect world
* the info would be in CVS and end up in the default profile/appdir
* updating the information in the AMO db would cause a checkin to CVS (might be able to do something similar to what egg does with configure.in)

Comment 12

[Michael Morgan [:morgamic]]

I am actually against shipping a default blocklist.xml for two reasons:
1) user will update this via service anyway
2) we can't take it back if we ship it this way...

It sucks that it isn't processed or updated for 24 hours -- what is the reason/cause for the delay?  Don't want new users to see "your crap is broken" I imagine... but remember that even if you ship with Firefox that'll be the case.

So why not remove the 24 hour window and update on first run or update?

Comment 13

[Dave Townsend [:mossop]]

(In reply to comment #12)
> It sucks that it isn't processed or updated for 24 hours -- what is the
> reason/cause for the delay?  Don't want new users to see "your crap is broken"
> I imagine... but remember that even if you ship with Firefox that'll be the
> case.

It's mainly a result of the original timer service being written for software update. It was decided that an automatic update wouldn't be offered run for at least a day after you ran firefox to avoid the "ran firefox and immediately I needed to update" experience. The timer service was then used by lots of other components but with the same basic behaviour the app update scenario needed.

> So why not remove the 24 hour window and update on first run or update?

It is a possibility sure. However to actually achieve the same effect as shipping the blocklist you would have to delay the startup until the blocklist was retrieved and processed. And you can potentially be running the app without network access at the start.

Comment 14

[Jesse Ruderman]

> 2) we can't take it back if we ship it this way...

How about grabbing the blacklist from the server IFF something in the local blacklist matches?

Or we could just say that our strategy for "taking it back" is to ship a new point release of Firefox.

Comment 15

[Dave Townsend [:mossop]]

(In reply to comment #12)
> 2) we can't take it back if we ship it this way...

We can't take it back, but it will get replaced with a new version from the server within around a day.

Comment 16

[Michael Morgan [:morgamic]]

True -- that makes it more reasonable to me so I'm out of objections.

Comment 17

[Dave Townsend [:mossop]]

Attachment: blocklist service changes

This implements what would be necessary for the application code. It makes the blocklist check two locations for an existing file, the profile and the application directory. Whichever is newest, if both exist, wins.

This doesn't handle the issue of getting the latest blocklist for the build, for now I have just included the latest blocklist to be added. We could ship b5 with it like this and then bring in some update process for the blocklist in tree at a later date.

This works for the case of installations using the installer, OSX dmg or tarball all of which preserve the time of the original build (i.e. older than the install date). It isn't ideal for the auto-update case which sets the modification time of the new file to be the time of the update. This will mean that if you auto-update then you would end up using the blocklist shipped in the update instead of the file in the profile which could be newer, until the next time the blocklist is retrieved from the server. There isn't an easy way around that without encoding the time into the app's blocklist file and parsing it to find it which would be costly.

Thoughts?

Comment 18

[Dave Townsend [:mossop]]

Nick says that egg auto checks in a new configure based on these steps:

"it checks out configure.in, creates configure, does a cvs diff on that, and does a checkin on a diff"

This is pretty much ideal for keeping an in-CVS blocklist file up to date, just checkout the blocklist from the tree, use wget to pull a new copy and the diff and checkin as necessary.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Nick, how do we feel about this? I'd like to get this for B5 as it would help us determine if we can actually stop a lot of our initial crashing by applying some hot-hot blocking action right up front. Not sure it blocks the beta, though :(

Comment 20

[(not currently active) Ted Mielczarek]

beltzner: the automated-blocklist-in-CVS stuff shouldn't block the beta, should it? That will just make sure we're shipping the latest blocklist in future releases. Mossop's patch has the existing blocklist in it, so that should be enough for b5.

(Incidentally, I bet it would be pretty easy to adapt the configure-update script to do the blocklist.)

Comment 21

[Dave Townsend [:mossop]]

Yeah there is no need to block b5 on automating the blocklist checkin. This patch is all that is necessary.

Comment 22

[Mike Shaver (:shaver emeritus)]

I think this has to block the beta: we can't be targetting RC1 for things that need in-the-field validation; RC1 should be "we think this is good to go, prove otherwise", not "we're holding our breath on the blocklist thing".

But really then we would want 2 more betas, so that we can test shipping blocklist rev1 with the installer, rev2 over the wire, and rev3 (or rev1 again!) with the installer.  Given that we're not going to do that, and that I don't think nightlies give us the test coverage we want, I think that means that we have to have this in b5, and then push an update over the wire, and then push an update via the update-installer.

Timestamp comparisons also make me extremely nervous.  Restoring backups by dragging and dropping, mishaps with clocks that put the file stamp deep into the future -- all sorts of things can make the timestamp on the disk not match our expectations of meaning.  If we're going to put a timestamp in play, we should put it in the file, and we should compare it with Last-Modified from the HTTP response to guard against a bug in the update script adding a zero or otherwise promoting it to the deep future.

Comment 23

[Dave Townsend [:mossop]]

Attachment: always use profile blocklist when available

This is a slightly different version that only uses the application shipped blocklist if there is no blocklist in the profile.

Comment 24

[Dave Townsend [:mossop]]

Checking in toolkit/mozapps/extensions/src/nsBlocklistService.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsBlocklistService.js,v  <--  nsBlocklistService.js
new revision: 1.10; previous revision: 1.9
done
Checking in browser/app/Makefile.in;
/cvsroot/mozilla/browser/app/Makefile.in,v  <--  Makefile.in
new revision: 1.153; previous revision: 1.152
done
RCS file: /cvsroot/mozilla/browser/app/blocklist.xml,v
done
Checking in browser/app/blocklist.xml;
/cvsroot/mozilla/browser/app/blocklist.xml,v  <--  blocklist.xml
initial revision: 1.1
done
Checking in browser/installer/unix/packages-static;
/cvsroot/mozilla/browser/installer/unix/packages-static,v  <--  packages-static
new revision: 1.158; previous revision: 1.157
done
Checking in browser/installer/windows/packages-static;
/cvsroot/mozilla/browser/installer/windows/packages-static,v  <--  packages-static
new revision: 1.160; previous revision: 1.159
done

Comment 25

[Tony Chung [:tchung]]

hi dave, how can this be easily tested?

Comment 26

[Dave Townsend [:mossop]]

(In reply to comment #25)
> hi dave, how can this be easily tested?

On a clean machine (no existing firefox profile), run firefox 3 and try to install one of the blocklisted addons (IDM and FDM right now). Equally on a clean machine install IDM before starting firefox then run it and you should still see the addon blocklisted.

Comment 27

[Justin Wood (:Callek)]

This caused SeaMonkey Linux tinderbox to go orange (tUnit failures).

Comment 28

[Justin Wood (:Callek)]

Attachment: fix SeaMonkey TUnit

This should fix the SeaMonkey test

Comment 29

[Justin Wood (:Callek)]

Attachment: fix SeaMonkey TUnit take 2 (Checked in)

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 310705 [details] [diff] [review]
fix SeaMonkey TUnit take 2 (Checked in)

>Index: toolkit/mozapps/extensions/src/nsBlocklistService.js

>+    LOG("Blocklist::_loadBlocklist: no XML File not found");


Fix that typo :)

Comment 31

[Justin Wood (:Callek)]

Comment on attachment 310705 [details] [diff] [review]
fix SeaMonkey TUnit take 2 (Checked in)

Fixed with nit addressed

Checking in toolkit/mozapps/extensions/src/nsBlocklistService.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsBlocklistService.js,v  <--  nsBlocklistService.js
new revision: 1.11; previous revision: 1.10
done

Comment 32

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032405 Minefield/3.0b5pre

Tested on FDM 1.0 and IDM 3.3, and both are disabled from Addons Manager.   

Comment 33

[Tony Chung [:tchung]]

Need to include a few litmus test cases.   Testcases should reflect comment #26, and also include uninstalling extensions that are blocklisted from the OS.

Comment 34

[Tony Chung [:tchung]]

Added litmus tests:

https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=5218
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=5219