Search engine aliases can "take over" the location bar

RESOLVED FIXED in Firefox 3 beta4

Status

()

Firefox
Search
P2
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Gavin, Assigned: rflint)

Tracking

({verified1.8.1.13})

Trunk
Firefox 3 beta4
verified1.8.1.13
Points:
---
Bug Flags:
blocking-firefox3 +
blocking1.8.0.next -
wanted1.8.0.x -
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low spoof] post 1.8-branch)

Attachments

(4 attachments)

Search engines can define "aliases" that act as shortcuts for searching with the engine. We expose these shortcuts in the URL bar, so if the engine's alias is "google", the user can type "google foo bar" to search for "foo bar" using Google. We don't do any filtering of aliases, though, so if you convince the user to install an engine with an alias set to "http://www.google.com", your search engine's URL would load whenever the user entered "http://www.google.com" into the location bar.

I'm not sure exactly how this should be fixed. We could attempt to only allow "simple" aliases that don't look like URIs, but it's probably safer to just not allow engines to define aliases themselves, and instead only let users define them themselves (using the engine manager UI).
Flags: wanted1.8.1.x?
I don't know if any existing engines really rely on this behaviour, but it feels like we could probably do some basic filtering and still leave the function mostly intact, couldn't we?  I know that l10n might complicate things, and that blacklists are suboptimal, but compared to what we currently do, what about just doing:

s/[\\\/:.@#?]//g

Maybe this is what all naive filterers say, but surely the number of characters with semantic meaning in URLs is a small, and known, set?
I should have noted that the reason I'm not reluctant to just pull the feature is that I don't know of any in-the-wild plugins that use it. It's a Mozilla-specific extension that is no longer part of the OpenSearch spec, and we've done almost no advertising of the feature. URL bar shortcut support landed a while ago on the trunk in bug 378553, so it doesn't even apply to shipping versions of Firefox (so I don't know why I requested wanted1.8.1.x!).
Flags: wanted1.8.1.x?
Or maybe we could just limit aliases to alpha-numeric ASCII? Might draw ire from  users of non-Latin alphabets.
I think the best option would be to simply remove support for our alias extension and only allow fiddling with them though the UI. I've yet to see any engines making use of aliases and if we were to ship our defaults with aliases, there's currently nothing preventing a random web-installed engine from taking over the alias of a default or even a bookmark keyword.

> Or maybe we could just limit aliases to alpha-numeric ASCII? Might draw ire
> from  users of non-Latin alphabets.
If bug 387723 is any indication, I don't think that'll go over too well :)
Plus it'd leave us with two different behaviors for bookmark and engine keywords/aliases.
Assignee: nobody → rflint
Flags: blocking-firefox3?
Priority: -- → P1
Target Milestone: --- → Firefox 3 beta4
(In reply to comment #4)
> > Or maybe we could just limit aliases to alpha-numeric ASCII? Might draw ire
> > from  users of non-Latin alphabets.
> If bug 387723 is any indication, I don't think that'll go over too well :)

Well, that was about using the keyword to search for non-latin text, not about the the keyword itself having "strange characters".

But I'm not opposed to just disabling it. It is simpler to fix, and also solves the problem of having to deal with conflicts.
We should fix this, and if this isn't part of the opensearch spec, or a documented extension, I vote we drop support for engine-specified ones.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: P1 → P2
Yeah, I didn't realise this wasn't something the spec expected to be there anymore, either.  Sounds like pulling it is easiest after all.
Created attachment 305501 [details] [diff] [review]
Patch (checked in)
Attachment #305501 - Flags: review?(gavin.sharp)
Comment on attachment 305501 [details] [diff] [review]
Patch (checked in)

You'll need to change _initFromMetadata to use the "alias" setter rather than setting _alias directly, otherwise we won't save the aliases added that way.

r=me with that.
Attachment #305501 - Flags: review?(gavin.sharp) → review+
mozilla/browser/components/search/nsSearchService.js 	1.109
mozilla/browser/components/search/test/Makefile.in 	1.2
mozilla/browser/components/search/test/browser_415700.js 	1.1
mozilla/browser/components/search/test/testEngine.xml 	1.1
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Comment on attachment 305501 [details] [diff] [review]
Patch (checked in)

I think we want to take this on branch as it's a fairly simple way to remove the incentive for anyone to start distributing engines with evil aliases now in hopes catching users once they upgrade to 3.0.
Attachment #305501 - Flags: approval1.8.1.13?
Flags: in-testsuite+
Flags: in-litmus-
Does that patch apply cleanly to the branch or will you need another one?
Created attachment 305614 [details] [diff] [review]
Branch patch

It applies cleanly enough - the only real change with this patch is the inclusion of gavin's review comment.
Attachment #305614 - Flags: approval1.8.1.13?
Attachment #305501 - Attachment description: Patch → Patch (checked in)
Attachment #305501 - Flags: approval1.8.1.13?
Comment on attachment 305614 [details] [diff] [review]
Branch patch

approved for 1.8.1.13, a=dveditz for release-drivers
Attachment #305614 - Flags: approval1.8.1.13? → approval1.8.1.13+
The security problem itself doesn't affect 1.8, right? We just don't want the attribute present for when those users upgrade to the trunk.
Whiteboard: [sg:low spoof] post 1.8-branch
Ah, indeed. In fact even the upgrade case isn't a problem, given the way we used to store this (we don't save engine-defaulted aliases in the user pref database, and we won't read in any existing ones once we upgrade). So this patch isn't needed on the branch, though it won't hurt to keep them in sync.
Ah, I forgot we didn't touch the database for default aliases.
I guess this at least protects users of the extension I wrote prior to adding this functionality on trunk ;)
Keywords: fixed1.8.1.13
Created attachment 309544 [details]
example search plugin 

This search plugin, when installed, will "takeover" http://www.google.com . Note that you must type in exactly that string (no trailing slash) for the test to work. In a trunk build without this patch but with that engine installed, typing in "http://www.google.com" exactly will bring you to http://notgoogle.example.com .
Attachment #309544 - Attachment is patch: false
Attachment #309544 - Attachment mime type: text/plain → application/xml
Created attachment 309545 [details]
test install page

Just a page to make ease installation of the previous plugin.
This was checked into branch but it is really an issue for trunk. Per IRC conversation with Gavin, we will not worry about this for branch (for verification for release).
Keywords: fixed1.8.1.13 → verified1.8.1.13

Comment 21

10 years ago
nsSearchService.js doesn't exist on branch 1.8.0.
Flags: wanted1.8.0.x-
Flags: blocking1.8.0.15-
Group: security

Comment 22

10 years ago
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050718 Minefield/3.0pre

Verified.  Other platforms need verification, too.
You need to log in before you can comment on or make changes to this bug.