483086 - non-http[s] SearchForm URIs should be ignored (possible XSS if a malicious search plugin is installed)

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[Prateek]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.7) Gecko/2009030422 Ubuntu/8.04 (hardy) Firefox/3.0.7
Build Identifier: 3.0.7

If the attacker the trick the user into his search plugin, then the attacker can inject JS into any web site using the XML below.  

<SearchPlugin xmlns="http://www.mozilla.org/2006/browser/search/">
<ShortName>FOO.com</ShortName>
<Description>Foo.com Search Engine - Better Web Search</Description>
<InputEncoding>UTF-8</InputEncoding>
<Image width="16" height="16">data:image/x-icon;base64,AAABAAEAEBAAAAEACABoBQAAFgAAACgAAAAQAAAAIAAAAAEACAAAAAAAQAEAAAAAAAAAAAAAAAEAAAAAAACbm5sAAADJAAAA1gAAANMAAADYAAAAyACcnJ0AAADLAAEBywDCwsAAjIy2AH9/gwBdXa4AAAC5AAAAygAwMOEAMjLgADMzvQAEBHwAaGipAAAAzwCtsLEAhYWgAAsLygDq6toAQkK3AGZnigA5O84ACAiuANjY3wAICIsAAADAAGVljgAEBIQAAADfAGNiegAREZAAwsLzAAcHkwAEBZ0Ag4jOAC0twAALC7QAqK7dAKSq1wBycqQABATQAPT04ACYm74ARkaOALO09wAmJnQAJSXJADw9fwAzNc4ALzDCAHp6pACdnZkApKevAL29sQD5+foAkpKcAOzs4AAMDMoAPj7iAKGinACvr6oAT0/wAAwNyQAAANIAlJTaAN/f0QCcnZ0AAADRAF5erQAAAJ8AAADDAAAAzAA1NrwAMTGfAAkJ1QAAAMQAWlqvAHJ31wDMzMAAFhbHAAAAiQBERNIA8fHmAAgI1gCys6oAu7v4AD09ngDDwfoABgbOAM7N9wCYmPUAKyuOABkZmgAyMsEAiYq8AHp68gDDw5sAAgLPALW2rwCdnZgAAAC9AAMD1wCenpgA9fX1AAAArQCCgrIA0dLYAK+vnwB3e9kA5ubSAOfn+AClp6cA3NzDAMHHygBQU9MABATCAG5z2gAcHMQAio/aAAAA2QD19fwAnaHIAIaGogDm5uoAk5N0AK+ytAAMDNUAAAAAAP///wAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA//////////////////////////////////////////////////////////////////+DBgAAAAAASBX///////8GEzQ/Ll5VEUo9aXX///86e08hAg1qUUUiaymAOXf/NxQlglZvIzgzHksFeQxs/xcBEG1aWC1lc2YgCmEEUkE2BwFdaHAkQ3RGRxgqAmcWKwgFD1Q7Yn4xYzx2En0DGf9TBwFbLyZgCVwdQgtuBE7//3xEQD4nA1BZgTVXHwIw/////3pkHEkOTV8aTIR//////////yxyeBsycSj/////////////////////////////////////////////////AAD//wAA//8AAMAfAACABwAAAAEAAAABAAAAAAAAAAAAAAAAAACAAAAAwAAAAPABAAD8BwAA//8AAP//AAA=</Image>
<Url type="text/html" method="GET" template="http://www.ask.com/web">
  <Param name="q" value="{searchTerms}"/>
  <Param name="o" value="1576"/>
  <Param name="l" value="dis"/>
</Url>
<SearchForm>javascript:{aaa=function(){alert('You are owned');document.location="http://www.ask.com";}; aaa();}</SearchForm>
</SearchPlugin>

Reproducible: Always

Comment 1

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #0)
> If the attacker the trick the user into his search plugin, then the attacker
> can inject JS into any web site using the XML below.

... assuming they can convince the user to also complete an empty search (pressing enter into the search bar with no text entered), since that's the only time we use the SearchForm value.

But yeah, we should filter the SearchForm value the same way we already filter templateURI and the IconURL.

Comment 2

[Adam Barth]

Prateek put up a test case here:

http://webblaze.org/prateeks/bad.html

Comment 3

[Daniel Veditz [:dveditz]]

"moderate", really? An untrustworthy search plugin can already snoop on all your searches and potentially return hacked results which seems much more reliably bad than hoping you accidentally do a blank search on an interesting site. But it just sits there waiting and you only have to make that mistake once; OK, I'll buy "moderate".

Comment 4

[Daniel Veditz [:dveditz]]

Gavin, if you're not doing search plugins stuff anymore please assign to the correct person.

Comment 5

[Ryan Flint [:rflint] (ping via IRC for reviews)]

I can take this.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Don't think we'd hold Shiretoko on this, but would obviously take a fix if there's a trivial one to be taken.

Comment 7

[Daniel Veditz [:dveditz]]

If the fix is as trivial as comment 1 implies we'd really like a patch in 1.9.0.8 -- but code-freeze is tomorrow night (Mar 17) so we'd need a reviewed patch ASAP to approve. Otherwise we'll block on the next release.

Comment 8

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

This patch quietly drops SearchForms which aren't http(s). I've gone the quiet route because it's the most self-contained for a branch landing and will protect, but not outright break users who may have plugins containing SearchForms with invalid schemes already installed (plus I don't need to stub out prompts to test it).
The tests are in browser/ to make them easier to add to 1.9.0, I'll be sure to add them to the search test suite so they get migrated over to toolkit for 1.9.1+.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 367754 [details] [diff] [review]
Patch

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>+  set _searchForm(aValue) {
>+    if (/^https?:/i.test(aValue))
>+      this.__searchForm = aValue;
>+    else
>+      LOG("_searchForm: Invalid URL dropped for " + this._name ||
>+          "the current engine");

Can _name really be null here?

Should the cache switch to setting/saving __searchForm rather than going through the setter each time? I suppose that would require a version bump...

I still think we should try to consolidate the scheme filtering for template/iconURI/searchForm, get a followup filed on that perhaps?

Comment 10

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Comment on attachment 367754 [details] [diff] [review]
Patch

(In reply to comment #9)
> Can _name really be null here?
Yeah, during parsing if SearchForm comes before ShortName in the description.

> Should the cache switch to setting/saving __searchForm rather than going
> through the setter each time? I suppose that would require a version bump...
I'll change it do that and bump the cache version on checkin.

> I still think we should try to consolidate the scheme filtering for
> template/iconURI/searchForm, get a followup filed on that perhaps?
Sure, I left it out of this version because of the different scheme requirements for each, but I'll file a bug to see if we can't change that.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 367754 [details] [diff] [review]
Patch

Approved for 1.9.0.8, a=dveditz for release-drivers

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 367754 [details] [diff] [review]
Patch

a191=beltzner

Comment 13

[Ryan Flint [:rflint] (ping via IRC for reviews)]

mozilla/browser/components/search/nsSearchService.js 1.112
mozilla/browser/components/search/test/483086-1.xml 1.1
mozilla/browser/components/search/test/483086-2.xml 1.1
mozilla/browser/components/search/test/Makefile.in 1.4
mozilla/browser/components/search/test/browser_483086.js 1.1

Comment 14

[Ryan Flint [:rflint] (ping via IRC for reviews)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/87ebf1fc30f1

Comment 15

[Ryan Flint [:rflint] (ping via IRC for reviews)]

http://hg.mozilla.org/mozilla-central/rev/198155c14694

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

On which machine the mochitests will be run? I'm not able to find the results. It's not the unit test one...

Comment 17

[:Gavin Sharp [email: [e-mail]]]

It's not a mochitest, it's a browser chrome test. It runs on the normal unit test machines, e.g.:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1237468237.1237475679.27591.gz&fulltext=1

Search for "chrome://mochikit/content/browser/browser/components/search/test/browser_483086.js".

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified based on results from the automated tests on trunk and 1.9.1.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.8 using browser_483086.js test.

Comment 20

[Martin Stránský [:stransky] (ni? me)]

For 1.8 line it affects only firefox, right? Seamonkey looks safe.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

AFAIK SeaMonkey doesn't use this code at all, on any branch.

Comment 22

[Alexander Sack]

Attachment: same for browser/ nsSearchService on 1.8 branch

verified that the "owned" alert does not pop up with this patch.

Comment 23

[Samuel Sidler (old account; do not CC)]

Comment on attachment 374772 [details] [diff] [review]
same for browser/ nsSearchService on 1.8 branch

Approved for 1.8.1.22. a=ss for release-drivers

Comment 24

[Daniel Veditz [:dveditz]]

Fix checked into the 1.8.1 branch