User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050408 Firefox/1.0.3 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050408 Firefox/1.0.3 By creating a special sherlock file it is possible to overwrite an existing search engine without a chance for the user to see what is going on. The displayed name in the confirmation dialog is given as the third parameter of sidebar.addSearchEngine(), but the displayed name in the search dropdown is taken from the sherlock file. This way it is possible to overwrite the default Google search with a modified version that monitors the data and/or waits for a chance to run code (see Firesearching 1 on demo page). The string "google.src" in the source URL got also be moved out of the dialog by supplying a really long URL to the sherlock file (the dialog just cuts the source URL when it's getting too long). Reproducible: Always Steps to Reproduce: 1. Open http://bugzilla:Je5Zw8k@www.mikx.de/firesearching/ 2. Follow instructions The user will probably think the search engine installation just failed, because after confirming the installation dialog Firefox never displays an error messages if the installation failed because e.g. the sherlock file is broken or not found. Since there is no UI to see details about the installed searches a common user will probably never find out that the default Google search got modified. Using the built in sherlock update feature an attacker also gets a decent update mechanism to modifiy the scripts beyond the initial infection.

The fix in bug 290037 takes much of the sting out by eliminating the ability to execute javscript, but there are still bad things that could be done. One that comes to mind is spying on someone's searches and then redirecting to the real engine so they don't notice. Timeless suggested a URL command to some server the victim is likely to be logged into, but that would probably require targetting a specific victim. Changing 'Source:' to something like "google.src from http://www.mikx.de" might help, assuming all search plugins have reasonably distinguishable names. We could build in some collision avoidance, but then people would end up with multiple copies of ones they reinstalled for whatever reason. Maybe a simple "You're about to overwrite .... OK?" confirm.

What about just allowing a .src file to be overwritten by the host that initially installed the file? Would require to set a host for the .src file coming default with Firefox. Or it least throw a warning if the inital host and the new host is different. But even if the google.src file doesn't get's overwritten anymore. An attacker could still create a search that is named "Google" and has the same icon. I doubt a casual user can tell the difference. What about making the search drop down the way that the real host the request goes to is displayed: Google [on google.com] Yahoo [on yahoo.com] Amazon [on amazon.con] Dictionary [on dictionary.com] eBay [on ebay.com] Google [on mikx.de] Maybe just display the name if the name of the search engine and the domain name don't match exactly? Google Yahoo Amazon Dictionary eBay Google [on mikx.de]

This screen shot shows the output of the upcoming patch. It's not much better than what we have now. People *might* notice the google.src filename, but it's still pretty spoofable.

This puts the filename onto the dialog, might help some people notice this type of spoof overwrite. This works well with localizations, if they don't update the string then they simply continue to have the bug.

Comment on attachment 180555 [details] [diff] [review] stopgap patch approval-aviary1.0.3-

Comment on attachment 180621 [details] [diff] [review] variation that doesn't require l10n updates (for branches) It might be worth adding a comment like: + // Note that this use of match (rather than URI parsing) is correct + // since it matches what InternetSearchDataSource::saveContents + // does, and we want srcfile to match the filename we're saving to + // (see bug 290038). (I also slightly prefer parentheses to newlines, although I'd have to double-check the bidi rules to see if that would cause problems. (We'd be OK if parentheses associated with what was inside them rather than what was outside them.))

(In reply to comment #8) > (I also slightly prefer parentheses to newlines, although I'd have to > double-check the bidi rules I did too, but it didn't look as good when I tried it. Also there's the worry about language scripts that don't use '(' and ')' as parens. I think this is decent enough for a branch stopgap. Thanks for the comment suggestion.

Comment on attachment 180621 [details] [diff] [review] variation that doesn't require l10n updates (for branches) Yeah, this works. I like dbaron's comment suggestion, but as long as it points to the bug, that'll work.

Comment on attachment 180621 [details] [diff] [review] variation that doesn't require l10n updates (for branches) a=caillon for the branches.

I still haven't sr'd, and I really don't want to fry my brain further on this. What is the practical threat? Naive users don't install .src files -- they don't even know there are search box drop-down options. If we're not fixing this for all locales, we're not really fixing it, even with a stopgap. And if the users likely to trust an evil Sherlock plugin don't read the whole URL, why do we think they'll notice the filename? We need a better approach here, if this is truly a bug for us to fix. Again, not for 1.0.3 IMO. /be

(In reply to comment #12) > What is the practical threat? Naive users don't install .src files -- they > don't even know there are search box drop-down options. User don't need to know about the feature. They just visit a website and stumble into a script like this: while(true) { window.sidebar.addSearchEngine( "http://www.mikx.de/firesearching/google.src", "http://www.mikx.de/firesearching/google.png", "Firesearching", "Web" ); } I found no way out of this beside killing Firefox in Task Manager. And you can bet a naive user will click on OK to get out since the other option left for him is a reboot. It should never be possible to overwrite the default google search. To trick the user to add a new seach plugin is on thing (i doubt more than 50% of the users know that they can change the google search to e.g. yahoo), but overwriting the default search is a threat especially to naive users. If you believe naive users don't need/know/understand this feauture then disable it by default and add an about:config entry to enable. But don't create a feature and than blame security bugs on naive users if patching it is uncomfortable.

For 1.0.3, I think we should prevent addSearchEngine from overwriting any installed search plugin. If you really want it, you delete it from the filesystem by hand, sorry. We know we need a better search-plugin-management story (profile storage, user removal UI, etc.) in 1.1 or later, but until then I'm all about the default-deny. mconnor: can you make it so, quickly?

Sorry, I wrote comment 12 based on stale info. I still maintain that wording or filename showing don't cut it, so I completely agree with shaver's comment 14. /be

The idea here is not to replace existing files for search engine download (but to ensure that we still do so on update if, somehow, this function is ever called for update (looks like it's not). Not yet tested.

Comment on attachment 180621 [details] [diff] [review] variation that doesn't require l10n updates (for branches) Actually, let's fix this for real. New patch being worked on.

Comment on attachment 180642 [details] [diff] [review] alternative approach (prevent overwriting an existing file) I've tested that this prevents the file from being overwritten. Still need to test non-overwriting-install and update.

Comment on attachment 180642 [details] [diff] [review] alternative approach (prevent overwriting an existing file) The mozilla.org search plugin install at http://www.mozilla.org/projects/search/technical.html works.

This should fix updates; still testing.

oops, forgot to call my new function in both places :-)

Comment on attachment 180652 [details] [diff] [review] alternative approach (prevent overwriting an existing file) ok, figured out how to test updates -- we don't check for updates until the second time a user uses a search plugin -- and the initial time to validate against is the time of first use. So I've tested that this patch: * doesn't break updates * doesn't break new installs * fixes the bug by preventing a new install from overwriting an existing search plugin

I filed bug 290254 on the bizarre update code.

Comment on attachment 180652 [details] [diff] [review] alternative approach (prevent overwriting an existing file) So this passes sr=brendan muster, which depends on peer or moa as usual, even if only for security release (higher than usual review standards). Use HEAD in the constant names that do HEAD checks, and I'll be extra-happy. Not clear ben will surface in time to review for 1.0.3, so mconnor should feel free. /be

Comment on attachment 180652 [details] [diff] [review] alternative approach (prevent overwriting an existing file) just little comment glitches, you might have noticed them already. the patch itself is good. >+ // The download the .src file for a *new* engine. >+ const unsigned long ENGINE_DOWNLOAD_NEW_CONTEXT = 2; "download of the" >+ >+ // The download of the icon file for a new engine. >+ const unsigned long ICON_DOWNLOAD_NEW_CONTEXT = 3; >+ >+ // The head request to see if we need to update the engine. > const unsigned long ENGINE_UPDATE_CONTEXT = 4; like brendan said on IRC, this possibly should have HEAD or something similar to better distinguish it from ENGINE_DOWNLOAD_UPDATE_CONTEXT >- const unsigned long ICON_UPDATE_CONTEXT = 5; >+ >+ // The download the .src file for an engine update. >+ const unsigned long ENGINE_DOWNLOAD_UPDATE_CONTEXT = 5; >+ "of the" again here >+ // Just like AddSearchEngine, but with an extra fifth parameter to say >+ // whether it's our internal update or whether it's a new engine or an >+ // update. this comment is a little confused. I think you meant something like this: // Just like AddSearchEngine, but with an extra fifth parameter to say // whether it's our internal update or whether it's a new engine.

fix released

Fix checked in to trunk, 2005-04-15 21:37 -0700.

Comment on attachment 180555 [details] [diff] [review] stopgap patch patch is obsolete, removing review requests