Should we ship one that has a bad security hole, or even if one we don't host becomes widespread and then compromised. This is like the Windows registry's ActiveX blacklist, but web-wide, not per-machine (might want per-machine too for a layered administrative approach). /be

Bug 250854 is related ("XPInstall Permission Manager: Blacklist feature"). Prog.

This seems like a really good one to try to get for 1.1. Any chance it's gonna happen?

Benjamin, is this realistic at this point with everything else that's happening?

Prioritize what else is happening, and then let's schedule -- we don't have a realistic schedule right now, so I would much rather we all sort out what must, should, and would be nice to have for 1.1. This is somewhere between should and must in my book, so don't starve it for unknown "everthing elses". /be

This needs coordination with the extension update service, but after that gets sorted out this is pretty simple to implement.

That would be bug 290759. Tell us the format you want. I guess it will be something similar to http://lxr.mozilla.org/update1.0/source/update/VersionCheck.php#200

shaver, you were going to take a look at this? any reasonable chance of this getting in to 1.5? e.g. it will need to land before we lockdown for 1.8b4 within the next week or so. /cb

I was going to look at 290759, which is the client side of this work.

(I could have sworn I made this comment the other day...) This is indeed the client-side work. I'm waiting for Ben's EM patch in 296566 to land, and then I'll be able to better scope and schedule the work. I have hopes that it will land today!

-> robstrong per phone discussion

Per the phone conversation I stated I would take on the EM portion of this bug and not the other aspects (e.g. plugins, UM0 which I believe may be another bug, service for retrieving or reading the blacklist, etc.). Shaver also promised a sample file to work from that would be attached to this bug.

Per the phone conversation I will take on the EM portion of this bug - I don't have the time available to take on or drive the other aspects like disabling plugins, server side work for the blacklist, etc. Time is running short and without the base functionality of having the blacklist data to work from I may not have the time available to finish the EM portion. A simplified description of the EM portion includes: ui for user notification that an item has been blacklisted disabling of blacklisted extensions What it does not include: ui for and the disabling of plugins server side work for the blacklist periodically retrieving the blacklist or whatever is decided upon. format of the blacklist -> shaver

Per drivers discussion, this is not going to make 1.5, but we will keep it on the burner as something we'll implement in a dot release if need be to handle any security or integrity issues introduced by Extensions or plugins.

Fx2 if not earlier. (see comment 13)

Since we are adding restart capability when blacklisted items are found during the background check it would be a good thing to have session restore capability... adding deps on bug 328154

Benjamin, this adds some of what will be needed to support toolkit version knowledge in the EM... I can remove it if you think it is premature. Darin, this affects app update as well and I'd appreciate your review on the code in general as well.

Had a typo in the last patch. Also, the strings were provided by beltzner. If there are string changes I'd prefer to take care of them in bug 308916 instead of this bug though comments would be welcome.

Comment on attachment 213134 [details] [diff] [review] patch (fixed typo) > removeFile: function(file) { >+ if (!file) >+ return; Why is this necessary? I understand being defensive, but I don't see how its good to end up with null here. >+ /** >+ * The blocklist XML file looks something like this: >+ * >+ * <blocklist xmlns="http://www.mozilla.org/2006/blocklist"> >+ * <emItems> What is the "emItems" for? Do you expect to extend this format in the future to return other information? Do we handle other items gracefully? >+ _loadBlocklistFromFile: function(file) { >+ var parser = Components.classes["@mozilla.org/xmlextras/domparser;1"] >+ .createInstance(Components.interfaces.nsIDOMParser); nit: whitespace lineup >+ var doc = parser.parseFromStream(fileStream, "UTF-8", fileStream.available(), "text/xml"); >+ >+ for (var i = 0; i < doc.documentElement.childNodes.length; ++i) { >+ var emItemsElement = doc.documentElement.childNodes.item(i); >+ if (emItemsElement.nodeType == Components.interfaces.nsIDOMNode.ELEMENT_NODE || >+ emItemsElement.localName == "emItems") >+ break; >+ } Why don't we test the namespace also (? What happens here if we don't find an <emItems> element? Seems to me we need some additional defense against malformed input. Why not put this code below inside the loop, with a helper func if you think that would make it more readable. I'm vaguely worried about the fact that the EM service is popping up UI for blocked installs in various places; in an ideal world that would be handled by callbacks/observers from the client code, but I understand that's out of scope for 1.8. >+ * @param toolkitVersion >+ * The version of the toolkit we are checking for compatibility >+ * against. If this parameter is undefined, the version of the running >+ * application's toolkit is used. >- isCompatible: function (datasource, source, version) { >+ isCompatible: function (datasource, source, appVersion, toolkitVersion) { What are the situations where somebody would want/need to pass an explicit toolkitVersion instead of using gApp.platformVersion? Is this here to deal with what extensions would become incompatible in update scenarios? I don't see any usage of toolkitVersion later in the function, is that going to be added later as part of the other bug? >+ _rdfGet_detailsURL: function(item, property) { >+ var id = stripPrefix(item.Value, PREFIX_ITEM_URI); >+ if (this.getItemProperty(id, "blocklisted") == "false") >+ return EM_L("none"); nit: "none"? seems to me it would be better to return NS_RDF_NO_VALUE or an empty string. >Index: browser/app/profile/firefox.js >+// Blocklist preferences >+pref("extensions.blocklist.enabled", true); >+pref("extensions.blocklist.interval", 86400); >+pref("extensions.blocklist.url", "https://addons.mozilla.org/blocklist/firefox.xml"); >+pref("extensions.blocklist.detailsURL", "http://www.mozilla.com/blocklist/"); If these prefs are missing does the EM recover gracefully? >+ var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"] >+ .getService(Components.interfaces.nsIStringBundleService); >+ var strings = sbs.createBundle(URI_BLOCKLIST_PROPERTIES); Is there any particular reason you're using sbs directly instead of using <stringbundle> in the XUL? I'm not up on the decision to use one or the other, I just thought that the XUL version was preferred for some reason (it would block onload, perhaps, until it was loaded?). >Index: toolkit/mozapps/extensions/content/blocklist.css >=================================================================== >RCS file: toolkit/mozapps/extensions/content/blocklist.css >diff -N toolkit/mozapps/extensions/content/blocklist.css >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ toolkit/mozapps/extensions/content/blocklist.css 25 Feb 2006 00:42:55 -0000 >@@ -0,0 +1,5 @@ >+blocklistItem { >+ -moz-binding: url("chrome://mozapps/content/extensions/blocklist.xml#blocklistItem"); >+ -moz-box-orient: vertical; >+} nit: in general I'd prefer not to invent new XUL element names. How about <richlistitem class="blocklistItem">?

(In reply to comment #19) > (From update of attachment 213134 [details] [diff] [review] [edit]) > > removeFile: function(file) { > >+ if (!file) > >+ return; > Why is this necessary? I understand being defensive, but I don't see how its > good to end up with null here. That snuck in from other tests I'm doing for EM failures... I'll remove it. > >+ /** > >+ * The blocklist XML file looks something like this: > >+ * > >+ * <blocklist xmlns="http://www.mozilla.org/2006/blocklist"> > >+ * <emItems> > > What is the "emItems" for? Do you expect to extend this format in the future to > return other information? Do we handle other items gracefully? When we add plugins this same file can be used (e.g. I dislike the idea of downloading a second file for plugins and potentially other items in the future). Yes... with plugins being the only one planned. Yes, other items are handled gracefully. > >+ _loadBlocklistFromFile: function(file) { > > >+ var parser = Components.classes["@mozilla.org/xmlextras/domparser;1"] > >+ .createInstance(Components.interfaces.nsIDOMParser); > > nit: whitespace lineup Will do. > >+ var doc = parser.parseFromStream(fileStream, "UTF-8", fileStream.available(), "text/xml"); > >+ > >+ for (var i = 0; i < doc.documentElement.childNodes.length; ++i) { > >+ var emItemsElement = doc.documentElement.childNodes.item(i); > >+ if (emItemsElement.nodeType == Components.interfaces.nsIDOMNode.ELEMENT_NODE || > >+ emItemsElement.localName == "emItems") > >+ break; > >+ } > > Why don't we test the namespace also (? What happens here if we don't find an > <emItems> element? Seems to me we need some additional defense against > malformed input. Why not put this code below inside the loop, with a helper > func if you think that would make it more readable. I'll review this again this evening to make sure it does the right thing. > I'm vaguely worried about the fact that the EM service is popping up UI for > blocked installs in various places; in an ideal world that would be handled by > callbacks/observers from the client code, but I understand that's out of scope > for 1.8. It is a bit of a PITA :/ I could move the blocked install notification into extensions.js but that would also require canceling installs when the ui is closed instead of just completing the install. I believe that notification would be a bit tricky unless we just didn't care about the edgecases where the client code didn't have an observer (e.g. options panel, other chrome apps, etc.). > >+ * @param toolkitVersion > >+ * The version of the toolkit we are checking for compatibility > >+ * against. If this parameter is undefined, the version of the running > >+ * application's toolkit is used. > > >- isCompatible: function (datasource, source, version) { > >+ isCompatible: function (datasource, source, appVersion, toolkitVersion) { > > What are the situations where somebody would want/need to pass an explicit > toolkitVersion instead of using gApp.platformVersion? Is this here to deal with > what extensions would become incompatible in update scenarios? I don't see any > usage of toolkitVersion later in the function, is that going to be added later > as part of the other bug? Yes on all counts... this is the "adds some of what will be needed to support toolkit version knowledge in the EM... I can remove it if you think it is premature." as noted in comment #17 > >+ _rdfGet_detailsURL: function(item, property) { > >+ var id = stripPrefix(item.Value, PREFIX_ITEM_URI); > >+ if (this.getItemProperty(id, "blocklisted") == "false") > >+ return EM_L("none"); > > nit: "none"? seems to me it would be better to return NS_RDF_NO_VALUE or an > empty string. I used "none" to be consistent with how this was accomplished in bug 296566 for update notification... I've been working on cleaning up the ui and I believe I have a better approach. > >Index: browser/app/profile/firefox.js > > >+// Blocklist preferences > >+pref("extensions.blocklist.enabled", true); > >+pref("extensions.blocklist.interval", 86400); > >+pref("extensions.blocklist.url", "https://addons.mozilla.org/blocklist/firefox.xml"); > >+pref("extensions.blocklist.detailsURL", "http://www.mozilla.com/blocklist/"); > > If these prefs are missing does the EM recover gracefully? I believe so but I'll double check before resubmitting. > >+ var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"] > >+ .getService(Components.interfaces.nsIStringBundleService); > >+ var strings = sbs.createBundle(URI_BLOCKLIST_PROPERTIES); > > Is there any particular reason you're using sbs directly instead of using > <stringbundle> in the XUL? I'm not up on the decision to use one or the other, > I just thought that the XUL version was preferred for some reason (it would > block onload, perhaps, until it was loaded?). No and I'll fix this. > >Index: toolkit/mozapps/extensions/content/blocklist.css > >=================================================================== > >RCS file: toolkit/mozapps/extensions/content/blocklist.css > >diff -N toolkit/mozapps/extensions/content/blocklist.css > >--- /dev/null 1 Jan 1970 00:00:00 -0000 > >+++ toolkit/mozapps/extensions/content/blocklist.css 25 Feb 2006 00:42:55 -0000 > >@@ -0,0 +1,5 @@ > >+blocklistItem { > >+ -moz-binding: url("chrome://mozapps/content/extensions/blocklist.xml#blocklistItem"); > >+ -moz-box-orient: vertical; > >+} > > nit: in general I'd prefer not to invent new XUL element names. How about > <richlistitem class="blocklistItem">? Not a problem

Benjamin, sorry for the delay and I should be done with this over the weekend. I talked over a couple of changes that beltzner would like and I am looking at if / how I can utilize the existing list.xul / list.js instead of creating blocklist.xul, etc. - needing to show this from a service is a bit of a PITA but there is no other way to do so without creating cases where we wouldn't show the dialog.

Do we have something for "More Info" to redirect to?

It is in the works and will be hosted on mozilla.com. The xml file will be hosted on AMO.

Comment on attachment 214204 [details] [diff] [review] patch Benjamin, I am not going to add the changes to app update in this bug since there are other more drastic changes needed to it in Bug 324121. Otherwise, this should address your review comments. (In reply to comment #19) > What is the "emItems" for? Do you expect to extend this format in the future to > return other information? Do we handle other items gracefully? When we add plugin blocklisting the format will almost certainly be different hence the use of emItems which handle EM managed items. > >+ var doc = parser.parseFromStream(fileStream, "UTF-8", fileStream.available(), "text/xml"); > >+ > >+ for (var i = 0; i < doc.documentElement.childNodes.length; ++i) { > >+ var emItemsElement = doc.documentElement.childNodes.item(i); > >+ if (emItemsElement.nodeType == Components.interfaces.nsIDOMNode.ELEMENT_NODE || > >+ emItemsElement.localName == "emItems") > >+ break; > >+ } > > Why don't we test the namespace also (? What happens here if we don't find an > <emItems> element? Seems to me we need some additional defense against > malformed input. Why not put this code below inside the loop, with a helper > func if you think that would make it more readable. The only value I see in testing for the namespace is possibly for versioning and I added a check. If we don't find an <emItems> element the blocklist will be empty and no items will be blocklisted. I added a helper and though I could see adding a second it wouldn't provide much value. > I'm vaguely worried about the fact that the EM service is popping up UI for > blocked installs in various places; in an ideal world that would be handled by > callbacks/observers from the client code, but I understand that's out of scope > for 1.8. I changed this slightly so we pass null for notifications and attempt to use the appropriate manager when blocking an install. > What are the situations where somebody would want/need to pass an explicit > toolkitVersion instead of using gApp.platformVersion? Is this here to deal with > what extensions would become incompatible in update scenarios? I don't see any > usage of toolkitVersion later in the function, is that going to be added later > as part of the other bug? I added it so the interface wouldn't need to be changed for when the EM supports a targetApplication for toolkit@mozilla.org in bug 299716. Just to verify, is that still the name we want? > > >+ _rdfGet_detailsURL: function(item, property) { > >+ var id = stripPrefix(item.Value, PREFIX_ITEM_URI); > >+ if (this.getItemProperty(id, "blocklisted") == "false") > >+ return EM_L("none"); > > nit: "none"? seems to me it would be better to return NS_RDF_NO_VALUE or an > empty string. Fixed for the blocklist. I am planning on changing availableUpdateURL so it doesn't return "none" in bug 329045. > If these prefs are missing does the EM recover gracefully? Yes. If blocklist url is not present it bails. If blocklist enabled is not present it defaults to true If blocklist interval is not present it defaults to 86400 If blocklist details url is not present the ui doesn't display the url > nit: in general I'd prefer not to invent new XUL element names. How about > <richlistitem class="blocklistItem">? I modified lists.xul, etc. to support blocklist notification, added an info icon (I planned on doing this for 2.0 and needed it for this), and fixed a leak in list.js.

Comment on attachment 214204 [details] [diff] [review] patch Darin, with Benjamin on paternity leave I'm hoping you would review this in his stead?

Comment on attachment 214204 [details] [diff] [review] patch >Index: toolkit/mozapps/extensions/public/nsIExtensionManager.idl ... >+ /** >+ * Checks for changes to the blocklist, application disables / enables items >+ * that have been added / removed from the blocklist, and if there are >+ * additions displays a list of the items added. >+ */ >+ void checkForBlocklistChanges(); Just reading this comment, leaves me with several questions: 1) Is the checking asynchronous or synchronous? 2) If the checking is asynchronous, then what happens if I call this method twice in quick succession? 3) How does someone using this API observe the changes to the blocklist? >Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in >+const XMLURI_BLOCKLIST = "http://www.mozilla.org/2006/blocklist"; Perhaps the namespace URL should include the word "extensions" or "addons": "http://www.mozilla.org/2006/addons-blocklist" >+var Blocklist = { >+ /** >+ * Extension ID -> array of Version Ranges >+ * Each value in the version range array is a JS Object that has the >+ * following properties: >+ * "minVersion" The minimum version in a version range (default = 0). >+ * "maxVersion" The maximum version in a version range (default = *). >+ * "targetApps" Application ID -> array of Version Ranges >+ * (default = current application ID) >+ * Each value in the version range array is a JS Object that >+ * has the following properties: >+ * "minVersion" The minimum version in a version range (default = 0). >+ * "maxVersion" The maximum version in a version range (default = *). >+ */ >+ entries: null, This comment block seems out-of-date. >+ notify: function() { >+ var defaults = gPref.QueryInterface(Components.interfaces.nsIPrefService) >+ .getDefaultBranch(null); >+ try { >+ var dsURI = defaults.getCharPref(PREF_BLOCKLIST_URL); >+ if (defaults.getBoolPref(PREF_BLOCKLIST_ENABLED) == false) >+ return; So, you do not allow users to easily override the default settings for these prefs? Why? >+ request.channel.loadFlags |= Components.interfaces.nsIRequest.LOAD_BYPASS_CACHE; Hrm... are we sure we don't want to support caching the blocklist? Maybe the blocklist will grow large over time. Maybe caching will help reduce server load. How often do we check for updates? If we only check once per day, then the blocklist can be cached for a day. That could have a huge impact on performance. Just food for thought I guess. Since we control the server hosting the blocklist, we could choose to tune the cache options server-side. But, this code change prevents that altogether. >+ request.onerror = function(event) { self.onXMLError(event); }; >+ request.onload = function(event) { self.onXMLLoad(event); }; >+ request.send(null); By the way, if |send| throws an exception, then we end up leaking the request object and the scope of those anonymous functions. It is therefore better (albeit awkward) to set onerror and onload after calling send. >+ var blocklistFile = getFile(KEY_PROFILEDIR, [FILE_BLOCKLIST]); >+ if (blocklistFile.exists()) >+ blocklistFile.remove(false); If the blocklistFile is busy (windows), then this will throw. Do things return to an okay state in that case? >+ var em = Components.classes["@mozilla.org/extensions/manager;1"] >+ .getService(Components.interfaces.nsIExtensionManager) >+ .QueryInterface(Components.interfaces.nsIExtensionManager2); Why not pass nsIExtensionManager2 directly to getService? Also, for the 1.8 branch, we've been adopting the policy of naming interfaces that are exclusively for the 1.8 branch: nsIExtensionManager_MOZILLA_1_8_BRANCH It's damn ugly, but it is done for good reason: To avoid conflicting with a future nsIExtensionManager2 interface that might be introduced later on the trunk. >+ var doc = parser.parseFromStream(fileStream, "UTF-8", fileStream.available(), "text/xml"); file.fileSize is probably better than fileStream.available() since the available() method need not report the entire length of the data stream. (In this case it does, but best to avoid such dependencies IMO) >+ var itemNodes = this._getItemNodes(doc.documentElement); >+ for (var i = 0; i < itemNodes.length; ++i) { >+ var blocklistElement = itemNodes.item(i); >+ if (blocklistElement.nodeType != Components.interfaces.nsIDOMNode.ELEMENT_NODE || >+ blocklistElement.localName != "emItem") >+ continue; since you're inside a loop, it might be good to create a temp variable outside the loop body for nsIDOMNode. that way you don't pay for looking it up over and over. >+ for (var x = 0; x < blocklistElement.childNodes.length; ++x) { >+ var versionRangeElement = blocklistElement.childNodes.item(x); you might want a local var for blocklistElement.childNodes as well for similar reasons. >+ // appDisabled is determined by an item being compatible, >+ // satisfying its dependencies, and not being blocklisted So, a user cannot override the blocklist? That seems bad to me. >+ checkForBlocklistChanges: function() { ... >+ // show the blocklist notification window if there are new blocklist items. >+ if (items.length > 0) >+ showBlocklistMessage(items, false); >+ }, Ah, so the user will be prompted by this function, and it just consults the offline copy of the blocklist. OK. This should be documented in the interface IMO. >+ if (!responseXML || responseXML.documentElement.namespaceURI == XMLURI_PARSE_ERROR || >+ (request.status != 200 && request.status != 0)) { This same test is repeated elsewhere. Perhaps we should move it into a subroutine. Sorry, I am not the best person to review UI changes. I suggest getting UI review on screenshots of the feature, and then I suggest getting a xul hacker like beng to review your xul+js+css UI code changes. r=darin on the extension manager portion of this patch

Comment on attachment 213956 [details] screenshot of blocklist dialogs Mike, I went ahead and made the changes we discussed when you were at the office.

Comment on attachment 213956 [details] screenshot of blocklist dialogs thanks, Rob. I'm pretty sure I'll eventually think of a better way to do the "More information" bit, but I can't think of it at this particular moment, and it seems like the sort of thing that's easy to do in a follow-on bug. (Click here for _more information_? Find out _what is considered dangerous_? This is driving me _nuts_?)

Comment on attachment 214204 [details] [diff] [review] patch Scott, the files left to be reviewed are extensions.js, extensions.xml, extensions.xul, list.js, list.xul, and extensions.css. The ui is currently for all practical purposes overloading an element for status info which is why some of the css is a bit funky. I'll be taking care of that in bug 329045.

Comment on attachment 214204 [details] [diff] [review] patch you left me the easy part. 1) Please add the new blocklist prefs to all-thunderbird.js as well. 2) Instead of calling setAttribute to set the class attribute on infoIcon, you can use .className. You can also get rid of the switch statement, simplifying things: if (iconClass in params) document.getElementById("infoIcon").className = 'spaced ' + params["iconClass"]; that line may need tweaked to account for the default "alert-icon" case using a ?: syntax. 3) This: + message = document.getElementById("moreInfoBox") + message.hidden = false; Can just be document.getElementById("moreInfoBox").hidden = false; 4) message.setAttribute("value" can you just do message.value = ... here?

This addresses the review comments... carrying over reviews I'll be working with morgamic to improve the caching scheme. I'm thinking of adding code to manage the XMLHttpRequests especially for extension update check where it is more of an issue. It recovers gracefully if the file can't be removed and it throws.

Checked in on trunk. Please file new bugs for any regressions and assign them to me. In case there is any confusion... the bug for creating / hosting the blocklist file on AMO is bug 290759 and has not been completed yet.

Comment on attachment 214469 [details] [diff] [review] patch for checkin a=ben@mozilla.org for the 1.8.1 branch

Fixed on MOZILLA_1_8_BRANCH

Could we get an example extension (blacklisted on a.m.o) to do l10n work?

*** Bug 339949 has been marked as a duplicate of this bug. ***