This bug isn't alternative for bug 167475 and bug 167473, but it's highly desired addition to its. According bugs 163648 and 172498, we need a whitelist for safe external protocols (such as irc:, magnet:, realaudio:). Any external protocol which isn't included in a whitelist must be ignored. if software developers will want to introduce their own additional protocols, they may register it in Mozilla whitelist during application's installation process. But remembered user decisions for confirmation dialogs (see bug 167473) must be saved in user profile in order to prevent developers from launching their applications without user confirmation. Please, enter here your suggestions for more safe and usable protocols.

I have marked this bug security-confidential as it discusses pending security issues.

I don't think this should be secret. We're already adding a blacklist to protect us against known-bad protocols which should keep us out of trouble for a while. We need this bug to be public so that we can thrash out the issues: -- should we have a whitelist? -- what should the UI be? -- what should be on the whitelist?

>-- should we have a whitelist? Whitelist is better because we can't know what strange and dangerous protocols will be introduced by Microsoft an could be exploited in future. This was discussed in bug 163648 and I fully agree with Georgi Guninsky's comments about this issue. -- what should the UI be? See my proposals in bug 167473 -- what should be on the whitelist? Oh, this is a question :) IMHO, irc:, magnet: and realaudio: (and, of course, mailto: and news:) are a first candidates.

to mitch for investigation.

I think we should have dialogs similar to the unknown content type dialog for file downloads - an external protocol handler is, after all, very much like a helper app. The dialog would ask: - open this URL using Windows default helper app - Open this URL using a different app - don't open this type of URL at all - do/don't ask me again

Mitchel, we already have bug 167473 for your considerations.

I feel my proposals at http://bugzilla.mozilla.org/show_bug.cgi?id=75915#c53 have some bearing.

A fix for this bug would have prevented the security hole (the "shell" exploit) that we recently had. http://mozilla.org/security/shell.html Operating system manufacturers will continue to add protocols that may very well be highly questionable. It would be best to assume all protocols are dangerous unless known to be safe.

Taking bug. I doubt this will block 1.8a2 but should block 1.8 itself.

http://www.sacarny.com/blog/index.php?p=105 points out that if we add whitelist UI, we need to keep the existing blacklist.

UI freeze for preview release means this needs to get on aviary branch fairly soon.

If UI freeze is the only think keeping this out of Aviary, perhaps someone should just make the UI, which would have the two fields (one for the whitelist and one for the blacklist). The blacklist could be implemented, since it currently exists; while the whitelist could just be greyed out until the feature can be added (hopefully for the 1.0 full release). Obviously, the best solution would be for someone to write the full whitelist functionality, but it doesn't look like that is going to happen.

Dan, what's the status of this? Its getting pretty late to take on something that's going to be pretty major if done right. Based on comment 11, minusing for 1.0 final, since if it doesn't make PR, it waits for 1.5, which might be the best course of action unless we have time to test the hell out of it.

Consider adding ed2k: (under Windows) into your whitelist.

This asks the user before loading any unknown external protocol handler. The default can be changed via hidden pref to either NEVER load unknown handlers, or to ALWAYS load unknown handlers (the default behavior up to now). As is currently the case, any explicit pref setting for a specific protocol will take precedence over the default behavior. Bug 167475 would be a nice addition to this one (restrict such protocols to explicit links, not images or frame source)

Comment on attachment 158392 [details] [diff] [review] Ask for permission before loading unknown schemes Seeking r= from mscott who was responsible for some of the early code in this area.

Comment on attachment 158392 [details] [diff] [review] Ask for permission before loading unknown schemes - In nsExtProtocolChannel::SetNotificationCallbacks(): { + mCallbacks = aNotificationCallbacks; return NS_OK; // don't fail when trying to set this Wanna replace that tab with two spaces while you're here? sr=jst

I see a few problems with this patch... 1) The dialog comes up sync off an AsyncOpen() call. That's a violation of the API, really. The dialog needs to come off an event (perhaps just have the event do the LoadURL() call). 2) This patch reuses the existing protocol handler preferences. This has some unintended side effects, I think. For example, if I have a protocol we can't handle internally and I click on a link of that type and say it's ok to use the external app every time and _then_ I install an extension that adds an internal handler for the protocol, we will continue using the external app, ignoring the internal handler. There are similar issues with the "false" value for the pref... All of this is compounded by the lack of UI to manage said prefs. 3) The prompt doesn't tell the user what app will be used. I think it should; as it is, the user has no basis for deciding whether to allow the load...

> 1) The dialog comes up sync off an AsyncOpen() call. That's a violation of the > API, really. The dialog needs to come off an event (perhaps just have the > event do the LoadURL() call). Yeah, this is a major problem. Launching a XUL dialog from LoadURL means spinning up an event queue, etc. That could lead to the caller of AsyncOpen being re-entered unexpectedly. AsyncOpen is sometimes called deep in some stack, and re-entering that stack could cause all sorts of problems. Let's not risk that. Instead, we should post a PLEvent to the current event queue, and call LoadURL from that PLEvent. It should be fine for ::Open to do the modal dialog stuff... callers of nsIChannel::open already have to accept such dialogs. > 2) This patch reuses the existing protocol handler preferences. This has some > unintended side effects, I think. For example, if I have a protocol we > can't handle internally and I click on a link of that type and say it's ok > to use the external app every time and _then_ I install an extension that > adds an internal handler for the protocol, we will continue using the > external app, ignoring the internal handler. There are similar issues with > the "false" value for the pref... All of this is compounded by the lack of > UI to manage said prefs. Yeah, it seems like we need UI to manage these settings just like we have UI to manage the handlers for unknown mime types. We have the same issue today with extensions adding content handlers. Of course, it is possible that an extension could reset the pref. > 3) The prompt doesn't tell the user what app will be used. I think it should; > as it is, the user has no basis for deciding whether to allow the load... I agree with this point too. It probably wouldn't be that hard to add something like this... we'd probably have to add an API that nsOSHelperAppService would implement.

Comment on attachment 158392 [details] [diff] [review] Ask for permission before loading unknown schemes minus'ing after reading comments from Boris and Darin.

Updated to address the AsyncOpen concern. I think this is the right set of prefs to use rather than make up a second "ask me" set that would only take effect after nsIOService looks at the first set. The lack of a managing UI is a little problem, but not one that needs to keep this security feature out of the tree (and could even be added as an extension later). I'll file a bug for this. Showing the appname would be nice, but we currently don't have a clue what the OS is going to use. As Darin said, we'd need a new entry point on the OS helper service, and then have to write the implementation for all platforms. It's not realistic to get that into 1.0PR, but simply showing the suspicious link is easy. I'm not sure how useful knowing the app would be, at least we would still want to show the link because I'd much rather know someone's trying an aim:GoAway?hahaha link on me than just that it's launching AIM. I will file a bug here, too. Ignore minor tab cleanups.

Comment on attachment 158476 [details] [diff] [review] updated to handle AsyncOpen correctly >Index: mozilla/uriloader/exthandler/nsExternalProtocolHandler.cpp >+PRBool nsExtProtocolChannel::PromptForScheme(nsIURI* aURI, >+ nsACString& aScheme) >+{ >+ // deny the load if we aren't able to ask but prefs say we should >+ nsresult rv; >+ NS_ASSERTION(mCallbacks, "Notification Callbacks not set!"); >+ if (!mCallbacks) >+ return PR_FALSE; This may be the one case that breaks something. It's possible that some consumer may have not provided a prompt or notification callbacks. We'd of course like to fix any of those cases, so maybe it is for the best that we block by default and hope for bug reports during the PR cycle. >+ NS_ASSERTION(prompt, "No prompt interface on channel"); >+ if (!prompt) >+ return PR_FALSE; you could also write: if (!prompt) { NS_ERROR("No prompt interface on channel"); return PR_FALSE; } same applies elsewhere in this patch. >Index: mozilla/docshell/resources/locale/en-US/appstrings.properties >+externalProtocolPrompt=An external application must be launched to handle %1$S: links. Requested link:\n\n\n%2$S\n\n\nIf you were not expectng this request it may be an attempt to exploit a weakness in that other program. Cancel this request unless you are sure it is not malicious.\n "If you were not expectng this request it may..." should be: "If you were not expecting this request, it may..." sr=darin

I think that UI for this security feature should be a requirement for Firefox 1.0. We should probably leverage the "banner"-based UI notification, and provide a management interface similar to what is currently provided for other things like XPI whitelisting. Only, this would apply to URL schemes instead of hostnames.

Comment on attachment 158476 [details] [diff] [review] updated to handle AsyncOpen correctly plussing after talking to jst (who already reviewed the original patch) and shaver. Dan can you make sure we file a spin off 1.0 blocker for adding the UI per Darin's comments?

Fixed typo and formatting nits filed bug 258799 and bug 258802 on requested enhancements to the UI Fix checked into aviary, waiting for bustage to clear on trunk. Not sure if we want this fix on the 1.7.x branch since it introduces new strings. We could maybe tweak the patch to have hardcoded english defaults if the stringbundle contents are missing...

I believe this falls into the "core functionality that should be the same between 1.7 and aviary" category. If we're so desperate to have this on aviary, we need to take this on 1.7.

fixed on trunk

re comment #23 If the UI is a requirement for 1.0, then any text it is going touse needs to be defined realy quickly because we are up againt the l10n freeze for the 1.0 release.

Comment on attachment 158476 [details] [diff] [review] updated to handle AsyncOpen correctly Index: mozilla/uriloader/exthandler/nsExternalProtocolHandler.cpp + PRBool PromptForScheme(nsIURI *aURI, nsACString& aScheme); since aScheme seems to be an in parameter it should be const. but the scheme could be gotten off the URI too, why pass it here? + NS_ENSURE_ARG_POINTER(aNotificationCallbacks); this should not be a runtime check, since people who pass NULL to a get function totally deserve what they get. also, scripts can't do that. + return NS_OK; // don't fail when trying to set this the comment seems to be pointless now that this function actually does something +static void *PR_CALLBACK handleExtProtoEvent(PLEvent *event) +{ + nsExtProtocolChannel *channel = + NS_STATIC_CAST(nsExtProtocolChannel*, PL_GetEventOwner(event)); + + if (channel) when can this be null? same in destroyExtProtoEvent

since this bug is fixed, I filed Bug 259084 about the issues from comment 30.

Comment on attachment 158515 [details] [diff] [review] 1.7 branch patch (handles missing localized strings) looks fine to me, r=darin

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

OK, great work! Is bug 167473 fixed by this?

Comment on attachment 158515 [details] [diff] [review] 1.7 branch patch (handles missing localized strings) a=asa for 1.7.x checkin.

A patch for bug 173010 and bug 263546 was checked in on MOZILLA_1_7_BRANCH on 2004-10-20 09:26. So is this fixed1.7.5?

1.7.5 has shipped. Moving request to 1.7.6.

This one's fixed, don't need blocking flag.