417798 - History can fill FF3 addressbar suggestions (awesomebar) with unwanted bookmarklets that look almost like real pages but execute in the current page's scope.

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Hans Schmucker]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008021404 Minefield/3.0b4pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008021404 Minefield/3.0b4pre

javascript: urls that return a html page when visited currently produce a history entry just like normal urls... while this may be desired for the classical history view, it's very dangerous that these urls end up in the address bar suggestion list as these entries can be made to look just like the real page, including FavIcon, Title and a pseudo url that only differs from the real url in that http:// is replaced with javascript:'//. However opening the javascript: url will execute it in the scope of the currently viewed page, thereby exposing all sensitive data on the page to the javascript code. I think javascript: urls should only be viewed in the suggestion list when they actually are bookmarklets and the user has made a conscious effort to add them, not when he just happened to come by one (which he may not even has noticed, the javascript: url in the sample is opened by an onclick handler and redirects to the correct page).

A simple filter for the suggestion box to not include history entries that start with javascript: should already be sufficient.

Reproducible: Always

Steps to Reproduce:
1.Visit http://tapper-ware.net/devel/js/historypoisoning/
2.Click on the link (http://www.news.com/8301-13578_3-9872464-38.html)
3.Open a new tab and start typing "news" into the addressbar
Actual Results:  
The javascript url appears in the suggestion list as "[CNet Icon] news.com BitTorrent firms: Comc(...)" with url "javascript:'//www.news.com/8301-13578_3-9872464-38.html______(...)"

Expected Results:  
The real news.com page should be the only listed suggestion as the javascript url is not a location that can safely be visited.

The favicon doesn't always work... I'm still trying to figure out when it does and when it doesn't but while it may be relevant to the practical severity of this bug, it doesn't change anything for the underlying flaw.

Comment 1

[Hans Schmucker]

No takers? I think this is really quite critical since a malicious page can redirect thru a bookmarklet for any number of times thereby increasing its rank in the suggestion list... or it could make subtle changes to the bookmarklet each time it redirects so that all entries in the suggestion list consist of this bookmarklet... combine that with the situation that almost anybody activates a suggestion entry by accident every now and then because the mouse cursor often jumps around a pixel or two and you have a semi-automatic cross-domain attack.

Comment 2

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

Seems like a pretty straightforward way to exploit the new awesomebar feature (I mean, even I understand it). How many javascript URIs are revisited such that we need to include them in the awesomebar results? Easier just to filter all those out?

(Can we get a confirmation?)

Comment 3

[Hans Schmucker]

Well, I can confirm it (obviously) :)

There are a couple of good solutions IMHO:
1. Don't list javascript: history entries in the awesomebar
2. For history javascript: urls (NOT for bookmarklets) create a blank page first and execute the javascript url in that page's scope.
3. Save the page url along with the javascript url when adding such an entry to the history... when the user opens such an url, open the page url first, and execute the javascript: url onload.

#1 and #2 seem more "right", but #3 might be the most practical if we assume that a number of pages might generate anonymous popups that the user might want to revisit (not the case so far in my oppinion).

#2 has the problem that we'll probably populate the awesome bar with a lot of non-working javascript urls.

So my preference would be #1 by far, followed by #3 (which might be non-trivial to implement as we'd need a referer field in places which I don't think exists so far) and #2 if we really want to get on peoples nerves...

Comment 4

[Hans Schmucker]

P.S. I'd appreciate it if you didn't kill the JS url icons in the process as these are at least in my opinion incredibly useful, as they allow for bookmarklets with an icon in a way that doesn't need a special API or Addon and doesn't break anything.

Example:
http://tapper-ware.net/devel/js/bookmarklets/
(Don't try to drag the two buttons: Click them, then drag the tab title of the appearing description page to the bookmarks bar)

Comment 5

[Ed Lee :Mardak]

Would it be okay to only show javascript: URIs if the user explicitly types out "javascript:" as the first part of the query? Not for "javascript" or "java", "alert javascript:", etc. Would it be safe to assume that if the user is typing out javascript:, s/he should know the consequences?

dietrich, gavin: Should we only filter out these results in the UI but still let the backend provide those results if some other autocomplete consumer wants those results?

Comment 6

[Hans Schmucker]

Sounds good to me...

Comment 7

[Jesse Ruderman]

Mardak's idea sounds great.

Comment 8

[Ed Lee :Mardak]

Attachment: v1

Only allow searches that start with "javascript:" to find javascript URIs. + testcase! :)

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 304911 [details] [diff] [review]
v1

what's the use-case for browser.urlbar.filter.javascript = false?

Comment 10

[Ed Lee :Mardak]

If someone wants to do multi word matches without needing to type out javascript:? Or someone else using nsNavHistory that does want javascript uris to always show for some other app? I figured we shouldn't need to set policy for everyone.

Comment 11

[Ed Lee :Mardak]

With the pref set to false, an advanced user could use a saved javascript: bookmark by typing just the name of the bookmark instead of needing to type "javascript: bookmark"

Comment 12

[Hans Schmucker]

I think we should treat bookmarklets separately... and also with a separate pref.

0:Show matches in title or url (without requiring javascript:)
1:Show matches in title (without requiring javascript:)
2:Show matches in title or url (requiring javascript:)
3:Show matches in title (requiring javascript:)
4:Show no matches

browser.urlbar.filter.javascript.bookmarks: Default 1
browser.urlbar.filter.javascript.history: Default 3

That way users can have fine grained control over what results they want, but medium-advanced users get a setting they can work with right away as bookmarklets get listed by title, while average users (as long as they don't accidentally add bookmarklets) have nothing to worry about...

Comment 13

[Ed Lee :Mardak]

Well, already providing a pref to turn it on and off is providing quite a bit more functionality than now. Dietrich, do we want a pref at all?

Comment 14

[Hans Schmucker]

Definately... but is there a reason not to add two prefs? I mean, the about:config list has reached the point where you can't navigate it without the search years ago . And if the prefs are only read at startup, it shouldn't really affect performance either, right?

Comment 15

[Dietrich Ayala (:dietrich)]

Comment on attachment 304911 [details] [diff] [review]
v1

this is fine for now, better to start with this constraint and enhance as appropriate afterwards. the suggestions in comment #12 should be filed as a followup bug.

Comment 16

[Reed Loden [:reed]]

Are data: URLs affected by this bug? Usually when javascript: is mentioned, data: URLs have the same problem.

Comment 17

[Ed Lee :Mardak]

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.283; previous revision: 1.282
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.259; previous revision: 1.258
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.141; previous revision: 1.140
done
Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <--  nsNavHistoryAutoComplete.cpp
new revision: 1.43; previous revision: 1.42
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_417798.js,v
done
Checking in toolkit/components/places/tests/unit/test_417798.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_417798.js,v  <--  test_417798.js
initial revision: 1.1
done

Comment 19

[Mike Lierman]

*** VERIFIED

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

-Mike

Comment 20

[Tracy Walker [:tracy]]

verified with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5) Gecko/2008032606 Firefox/3.0b5
and
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-GB; rv:1.9b5) Gecko/2008032604 Firefox/3.0b5

Comment 22

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv