290038 - Search plugins can silently overwrite existing search plugins

Status: RESOLVED FIXED

(SeaMonkey :: Search, defect, P1)

Description

[Michael Krax]

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:[e-mail]/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.

Comment 1

[Daniel Veditz [:dveditz]]

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.

Comment 2

[Michael Krax]

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]

Comment 3

[Daniel Veditz [:dveditz]]

Attachment: how a stopgap might look

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.

Comment 4

[Daniel Veditz [:dveditz]]

Attachment: stopgap patch

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 5

[Chase Phillips]

Comment on attachment 180555 [details] [diff] [review]
stopgap patch

approval-aviary1.0.3-

Comment 6

[Daniel Veditz [:dveditz]]

Attachment: variation that doesn't require l10n updates (for branches)

Comment 7

[Daniel Veditz [:dveditz]]

Attachment: screenshot of variant w/out l10n change

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.))

Comment 9

[Daniel Veditz [:dveditz]]

(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 10

[Mike Connor [:mconnor]]

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 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

a=caillon for the branches.

Comment 12

[Brendan Eich [:brendan]]

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

Comment 13

[Michael Krax]

(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.

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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?

Comment 15

[Brendan Eich [:brendan]]

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

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: alternative approach (prevent overwriting an existing file)

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 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

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 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: alternative approach (prevent overwriting an existing file)

This should fix updates; still testing.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: alternative approach (prevent overwriting an existing file)

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

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I filed bug 290254 on the bizarre update code.

Comment 24

[Brendan Eich [:brendan]]

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 25

[Mike Connor [:mconnor]]

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.

Comment 26

[Daniel Veditz [:dveditz]]

fix released

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 28

[Simon Paquet [:sipaq]]

Comment on attachment 180555 [details] [diff] [review]
stopgap patch

patch is obsolete, removing review requests