Search plugins can silently overwrite existing search plugins

RESOLVED FIXED in mozilla1.8beta2

Status

SeaMonkey
Search
P1
critical
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: Michael Krax, Assigned: dbaron)

Tracking

(Blocks: 1 bug, {fixed-aviary1.0.3, fixed1.7.7})

1.7 Branch
mozilla1.8beta2
fixed-aviary1.0.3, fixed1.7.7
Bug Flags:
blocking1.7.7 +
blocking-aviary1.0.3 +
blocking1.8b2 +
blocking-aviary1.5 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix][patch], URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

13 years ago
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.
(Reporter)

Updated

13 years ago
Summary: Seach plugins can silently overwrite existing search plugins including google default → Search plugins can silently overwrite existing search plugins including google default
Assignee: p_ch → dveditz
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-aviary1.1+
Flags: blocking-aviary1.0.3+
Whiteboard: [sg:fix]
Component: Search → Search
Product: Firefox → Core
Version: unspecified → 1.7 Branch
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.
Flags: blocking1.8b2+
Flags: blocking1.7.7+
(Reporter)

Comment 2

13 years ago
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]
Created attachment 180554 [details]
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.
Created attachment 180555 [details] [diff] [review]
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.
Attachment #180555 - Flags: superreview?(brendan)
Attachment #180555 - Flags: review?

Comment 5

13 years ago
Comment on attachment 180555 [details] [diff] [review]
stopgap patch

approval-aviary1.0.3-
Attachment #180555 - Flags: approval-aviary1.0.3-
Created attachment 180621 [details] [diff] [review]
variation that doesn't require l10n updates (for branches)
Attachment #180555 - Attachment is obsolete: true
Attachment #180621 - Flags: superreview?(dbaron)
Attachment #180621 - Flags: review?(mconnor)
Attachment #180621 - Flags: approval1.7.7?
Attachment #180621 - Flags: approval-aviary1.0.3?
Created attachment 180622 [details]
screenshot of variant w/out l10n change
Attachment #180554 - Attachment is obsolete: true
(Assignee)

Comment 8

13 years ago
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.))
Attachment #180621 - Flags: superreview?(dbaron) → superreview+
(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.
Attachment #180621 - Flags: review?(mconnor) → review+
Comment on attachment 180621 [details] [diff] [review]
variation that doesn't require l10n updates (for branches)

a=caillon for the branches.
Attachment #180621 - Flags: approval1.7.7?
Attachment #180621 - Flags: approval1.7.7+
Attachment #180621 - Flags: approval-aviary1.0.3?
Attachment #180621 - Flags: approval-aviary1.0.3+
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
(Reporter)

Comment 13

13 years ago
(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
(Assignee)

Comment 16

13 years ago
Created attachment 180642 [details] [diff] [review]
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 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.
Attachment #180621 - Flags: approval1.7.7-
Attachment #180621 - Flags: approval1.7.7+
Attachment #180621 - Flags: approval-aviary1.0.3-
Attachment #180621 - Flags: approval-aviary1.0.3+
(Assignee)

Comment 18

13 years ago
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.
Attachment #180642 - Attachment description: alternative approach → alternative approach (prevent overwriting an existing file)
(Assignee)

Comment 19

13 years ago
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.
(Assignee)

Comment 20

13 years ago
Created attachment 180649 [details] [diff] [review]
alternative approach (prevent overwriting an existing file)

This should fix updates; still testing.
Attachment #180642 - Attachment is obsolete: true
(Assignee)

Comment 21

13 years ago
Created attachment 180652 [details] [diff] [review]
alternative approach (prevent overwriting an existing file)

oops, forgot to call my new function in both places :-)
Attachment #180649 - Attachment is obsolete: true
(Assignee)

Comment 22

13 years ago
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
Attachment #180652 - Flags: superreview?(brendan)
Attachment #180652 - Flags: review?(bugs)
Attachment #180652 - Flags: approval1.8b2?
Attachment #180652 - Flags: approval1.7.7?
Attachment #180652 - Flags: approval-aviary1.0.3?
(Assignee)

Comment 23

13 years ago
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.
Attachment #180652 - Flags: review?(bugs) → review+

Updated

13 years ago
Attachment #180652 - Flags: superreview?(brendan) → superreview+
Attachment #180652 - Flags: approval1.8b2?
Attachment #180652 - Flags: approval1.8b2+
Attachment #180652 - Flags: approval1.7.7?
Attachment #180652 - Flags: approval1.7.7+
Attachment #180652 - Flags: approval-aviary1.0.3?
Attachment #180652 - Flags: approval-aviary1.0.3+
(Assignee)

Updated

13 years ago
Assignee: dveditz → dbaron
Severity: normal → critical
Keywords: fixed-aviary1.0.3, fixed1.7.7
Priority: -- → P1
Whiteboard: [sg:fix] → [sg:fix][patch]
Target Milestone: --- → mozilla1.8beta2
Summary: Search plugins can silently overwrite existing search plugins including google default → Search plugins can silently overwrite existing search plugins
fix released
Group: security
(Assignee)

Comment 27

13 years ago
Fix checked in to trunk, 2005-04-15 21:37 -0700.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Comment on attachment 180555 [details] [diff] [review]
stopgap patch

patch is obsolete, removing review requests
Attachment #180555 - Flags: superreview?(brendan)
Attachment #180555 - Flags: review?
Blocks: 256199

Updated

12 years ago
Flags: testcase+

Updated

11 years ago
Flags: in-testsuite+ → in-testsuite?
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.