Last Comment Bug 483086 - non-http[s] SearchForm URIs should be ignored (possible XSS if a malicious search plugin is installed)
: non-http[s] SearchForm URIs should be ignored (possible XSS if a malicious se...
Status: VERIFIED FIXED
[sg:moderate]
: fixed1.8.1.22, verified1.9.0.9, verified1.9.1
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: 3.5 Branch
: All All
: P1 normal (vote)
: Firefox 3.6a1
Assigned To: Ryan Flint [:rflint] (ping via IRC for reviews)
:
Mentors:
Depends on: 920487
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-12 16:33 PDT by Prateek
Modified: 2013-09-26 11:00 PDT (History)
11 users (show)
mbeltzner: blocking‑firefox3.5-
dveditz: wanted1.9.0.x+
dveditz: blocking1.8.1.next+
dveditz: wanted1.8.1.x+
rflint: in‑testsuite+
rflint: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (8.23 KB, patch)
2009-03-17 04:35 PDT, Ryan Flint [:rflint] (ping via IRC for reviews)
gavin.sharp: review+
mbeltzner: approval1.9.1+
dveditz: approval1.9.0.9+
Details | Diff | Review
same for browser/ nsSearchService on 1.8 branch (1.26 KB, patch)
2009-04-27 09:07 PDT, Alexander Sack
samuel.sidler+old: approval1.8.1.next+
Details | Diff | Review

Description Prateek 2009-03-12 16:33:38 PDT
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: gavin@gavinsharp.com] 2009-03-12 16:47:43 PDT
(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 2009-03-12 16:48:26 PDT
Prateek put up a test case here:

http://webblaze.org/prateeks/bad.html
Comment 3 Daniel Veditz [:dveditz] 2009-03-12 17:43:36 PDT
"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] 2009-03-12 17:45:04 PDT
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) 2009-03-12 17:51:21 PDT
I can take this.
Comment 6 Mike Beltzner [:beltzner, not reading bugmail] 2009-03-15 20:43:46 PDT
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] 2009-03-16 14:31:57 PDT
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) 2009-03-17 04:35:41 PDT
Created attachment 367754 [details] [diff] [review]
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: gavin@gavinsharp.com] 2009-03-17 11:22:03 PDT
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) 2009-03-17 13:36:34 PDT
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] 2009-03-17 13:57:01 PDT
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] 2009-03-17 16:10:33 PDT
Comment on attachment 367754 [details] [diff] [review]
Patch

a191=beltzner
Comment 13 Ryan Flint [:rflint] (ping via IRC for reviews) 2009-03-17 17:08:09 PDT
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) 2009-03-17 21:31:58 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/87ebf1fc30f1
Comment 15 Ryan Flint [:rflint] (ping via IRC for reviews) 2009-03-18 00:56:41 PDT
http://hg.mozilla.org/mozilla-central/rev/198155c14694
Comment 16 Henrik Skupin (:whimboo) 2009-03-19 02:43:56 PDT
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: gavin@gavinsharp.com] 2009-03-19 09:19:47 PDT
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) 2009-03-20 05:33:27 PDT
Verified based on results from the automated tests on trunk and 1.9.1.
Comment 19 Al Billings [:abillings] 2009-03-23 15:01:37 PDT
Verified for 1.9.0.8 using browser_483086.js test.
Comment 20 Martin Stránský 2009-04-09 09:24:04 PDT
For 1.8 line it affects only firefox, right? Seamonkey looks safe.
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-04-09 18:24:26 PDT
AFAIK SeaMonkey doesn't use this code at all, on any branch.
Comment 22 Alexander Sack 2009-04-27 09:07:43 PDT
Created attachment 374772 [details] [diff] [review]
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) 2009-05-20 15:48:38 PDT
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] 2009-05-30 00:01:23 PDT
Fix checked into the 1.8.1 branch

Note You need to log in before you can comment on or make changes to this bug.