Persona is no longer an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 371179 - Don't allow bookmarking an evaluated+loaded javascript: URL
: Don't allow bookmarking an evaluated+loaded javascript: URL
Status: NEW
[sg:want][xss with nearly normal user...
: sec-want
Product: Firefox
Classification: Client Software
Component: Security (show other bugs)
: unspecified
: All All
: -- normal with 6 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
: 528772 (view as bug list)
Depends on:
Blocks: 527530 372035
  Show dependency treegraph
Reported: 2007-02-21 15:45 PST by Michal Zalewski
Modified: 2015-01-26 18:49 PST (History)
38 users (show)
mconnor: blocking‑firefox3-
mconnor: wanted‑firefox3+
samuel.sidler+old: blocking1.8.1.13-
dveditz: wanted1.8.1.x+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (432 bytes, text/html)
2007-02-25 16:25 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
branch patch (1.72 KB, patch)
2007-02-26 09:40 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Splinter Review
branch patch v2 (2.04 KB, patch)
2007-02-27 04:42 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Splinter Review

Description Michal Zalewski 2007-02-21 15:45:41 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv: Gecko/20061204 Firefox/
Build Identifier: 

It should be not possible to bookmark data: and other inline / special URL schemes, or, if this functionality is needed, Javascript execution context should be properly initialized. Otherwise, such a bookmark, when invoked, will be allowed to execute Javascript code in the context of a currently viewed resource (Google start page = stealing GMail authentication cookies, for example).

See this demonstration:

Reproducible: Always
Comment 1 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-02-21 15:53:35 PST
I don't see any difference with javascript: urls, data url's get just like javascript: urls the principals of the page.
Maybe this should be 'fixed' by giving data/javascript bookmarks a special bookmark icon? (There is bug 72374 for that)
Comment 2 Michal Zalewski 2007-02-21 15:56:30 PST
(In reply to comment #1)
> I don't see any difference with javascript: urls

New windows with javascript: target seem to be non-bookmarkable (internally, Firefox considers them to be just about:blank).

Comment 3 Michal Zalewski 2007-02-21 16:05:37 PST
Oh, I see the reason for confusion: truth to be told, I didn't know about "bookmarklets". To make myself more clear, then: it should be not possible for data: windows to be semi-automatically bookmarked through Ctrl-D in a way that turns them into bookmarklets without user's consent (see the demo). Manually adding javascript: or data: bookmarks is OK, because the user is likely making an informed decision.

Icons probably won't suffice - novices would be most vulnerable, anyway, and they might not be able to tell a difference.

Comment 4 Daniel Veditz [:dveditz] 2007-02-21 16:14:42 PST
It's probably slightly easier to fool someone into bookmarking an open data: page than a bookmarklet link (but that's just a right-click away) . We can't take away bookmarklets but we can probably block this without breaking any current legitimate use.

A fix might fall out of bug 255107, or we could have the bookmark code clear
the current content before loading the data: URL as we do when loading them from external apps into existing windows (the old default).
Comment 5 Jesse Ruderman 2007-02-21 17:09:55 PST
> It's probably slightly easier to fool someone into bookmarking an open data:
> page than a bookmarklet link

javascript: URLs that don't result in "undefined" result in an open page, too:
Comment 6 Michal Zalewski 2007-02-21 17:23:03 PST
(In reply to comment #5)
> javascript: URLs that don't result in "undefined" result in an open page, 
> too: javascript:"<html><body><h3>Foo"

True, data: and javascript: then. Visiting such a window and clicking Ctrl-D doesn't seem to be a documented and practiced way of adding bookmarklets, and severely violates the principle of least astonishment when a piece of code actually tries to mimick a "physically" existing page - so maybe as a bare minimum, we should have a warning when the user attempts to bookmark these URL schemes, and have them confirm?

Comment 7 Michal Zalewski 2007-02-21 17:31:08 PST
MSIE7 refuses to bookmark javascript: and data: URLs through Ctrl+D in the resulting window. It always bookmarks the last seen "physical" page instead (or about:blank if none found). 

It is still possible to implement bookmarklets by manually entering or right-clicking the link, but that seems sane.
Comment 8 Carsten Book [:Tomcat] 2007-02-23 00:26:01 PST

requesting blocking 
Comment 9 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-02-25 16:25:58 PST
Created attachment 256397 [details]
Comment 10 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-02-26 09:40:26 PST
Created attachment 256474 [details] [diff] [review]
branch patch

So there are different things that could be done:
1- Disallow Ctrl+D for javascript:/data: uri pages.
2- Disallow Ctrl+D and "Bookmark This Page" for javascript:/data: uri pages.
3- Do what IE does and store the last seen "physical" page (might be tricky to do, haven't figured that out, yet)

This patch does 2. I'm not sure if it would make sense to do what IE doesn. IE doesn't show the javascript: url in the url bar, while Firefox does show the javascript: url. You can still bookmark javascript/data url links with right-click bookmark this link with the patch.
Comment 11 Jesse Ruderman 2007-02-26 14:13:30 PST
It looks like this patch causes "Bookmark all tabs..." to bail silently if one of the tabs happens to have a javascript: or data: URL.  Is that really what we want to do?
Comment 12 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-02-26 14:51:42 PST
I don't know what the best solution is.
As far as I know, string changes are not allowed for branch, so alert/confirm dialogs are not possible for branch.
If someone knows a good solution, I'll be happy to give it a try.
Comment 13 Jesse Ruderman 2007-02-26 15:01:13 PST
How about skipping the javascript: and data: tabs and bookmarking the rest?
Comment 14 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-02-26 15:05:15 PST
Ah, yeah, that makes sense. But for the single bookmark, that still would mean that nothing happens. Someone should say if that's ok or not.
And I guess for trunk it might be better to add some annoying warning dialog like IE does.
Comment 15 Jesse Ruderman 2007-02-26 15:47:42 PST
For the single-bookmark case (or the "none of these tabs can be bookmarked" case) I'd rather see an error dialog than a security dialog.  Perhaps something simple like "This page cannot be bookmarked".

A security dialog might be appropriate for adding a bookmarklet through "Bookmark this link...", but that's a discussion for another bug (bug 28387).
Comment 16 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-02-27 04:42:54 PST
Created attachment 256620 [details] [diff] [review]
branch patch v2

Ok, this includes Jesse's suggestions.
When bookmarking a group of tabs, it skips all tabs that have javascript:/data: urls (which is a little odd, when you only have javascript:/data: urls for tabs, you get an empty folder).
When bookmarking a single page, you get the alert.

I filed bug 371923 for adding a warning dialog, like IE does.
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2007-02-27 08:58:54 PST
Might need similar changes to other apps....
Comment 18 Samuel Sidler (old account; do not CC) 2007-02-27 09:04:45 PST
Indeed. Camino is also vulnerable.
Comment 19 Chris Lawson (gone) 2007-02-27 09:10:49 PST
Does this mean that javascript: and data: URLs will never *work* as bookmarks, or simply that such bookmarks couldn't be made without manually pasting the URL into an already-created bookmark?

Not that I know many people who have data: bookmarks, and it seems the bookmarklet case is being addressed, but it's not clear to me from reading the bug so far what the eventual effect will be.

Comment 20 Martynas Venckus 2007-02-27 13:58:27 PST
Attachment 256474 [details] [diff] works fine (C-d ignored for javascript:/data:).
Attachment 256620 [details] [diff] does not at all (the old behavior allowing to bookmark javascript:/data: pages).
Tested on firefox
Comment 21 Samuel Sidler (old account; do not CC) 2007-02-27 14:19:46 PST
Camino version of this bug is bug 372020.
Comment 22 Chris Thomas (CTho) [formerly] 2007-02-27 15:55:41 PST
SeaMonkey version of this bug is bug 372035.
Comment 23 Chris Casciano 2007-02-27 21:47:44 PST
Where does dragging a bookmarkable item fall into the use cases for this bug? Is it enough of a direct action to be "OK" to let happen in all cases?

- dragging data:/javascript: uri from a link in a page to the bookmark toolbar
- dragging data:/javascript: uri from the location bar [via handle like the favicon] into the bookmark toolbar

I realize you will not get the extreme cases of having a suspicious tab burried in the middle of a tab group, but it is a bookmarking action with no current path for UI or warnings.
Comment 24 2007-02-28 04:24:37 PST
Excluding bookmarking all tabs for a moment, would it suffice to force the add bookmark dialog to appear when bookmarking a javascript: or data: link?
Comment 25 Daniel Veditz [:dveditz] 2007-04-04 11:08:38 PDT
Mike: please find an appropriate person from your team to assign this to.
Comment 26 Daniel Veditz [:dveditz] 2007-04-23 13:09:58 PDT
(In reply to comment #24)
> would it suffice to force the add bookmark dialog to appear when bookmarking
> a javascript: or data: link?

It does appear, but (in Firefox) only shows the page title not the URL itself. Not that naive users would know what a data: url can do anyway.
Comment 27 Daniel Veditz [:dveditz] 2007-04-27 11:46:59 PDT
No one likes this approach, FF devs would prefer waiting until we have a better solution.
Comment 28 Daniel Veditz [:dveditz] 2007-07-09 11:39:14 PDT
waiting for a trunk fix, pushing out to the next branch release.
Comment 29 Mike Connor [:mconnor] 2007-10-01 13:36:51 PDT
moving out bugs that don't need to block b1
Comment 30 Daniel Veditz [:dveditz] 2007-10-01 15:17:50 PDT
Following suit on the branch, need a trunk fix first.
Comment 31 :Gavin Sharp [email:] 2008-01-11 13:59:42 PST
I think the only real option for fixing this bug on the branch while avoiding the l10n addition pain would be to go with Martijn's patch in comment 10 - it does cause bookmarking to fail silently if you attempt it in the attack scenario ("trick user into bookmarking javascript:/data: URI"), but that's probably not all that common anyways, and it does mitigate the attack.

We could take a better approach to fixing this on the trunk, perhaps by just using a dialog as suggested. That's the "uiwanted" part, I guess.
Comment 32 Samuel Sidler (old account; do not CC) 2008-01-11 16:57:35 PST
On the branch, I think we should disallow Ctrl+D and "Bookmark This Page" for javascript:/data: URIs as Martijn does in comment 10, but also incorporate the part of his second patch that causes bookmarking all tabs to work and just removes the javascript:/data: URIs from the set that gets bookmarked.
Comment 33 :Gavin Sharp [email:] 2008-01-11 18:17:01 PST
Right, that's what I was proposing.

I forgot about the drag and drop case that Chris mentions in comment 23, though. That change wouldn't help with the drag and drop case, which is just as easy to convince the user to do. In fact drag and drop is the most common way to "install" a bookmarklet, so any attempts to cover-up that hole will surely draw the ire of bookmarklet authors and users.
Comment 34 Samuel Sidler (old account; do not CC) 2008-02-29 12:03:20 PST
We won't take this for because the fix we want might be more complicated. Dan will comment on it shortly.
Comment 35 Mike Connor [:mconnor] 2008-03-28 11:56:39 PDT
Wanted, but not going to block on something where we don't have a viable solution in place that won't destroy bookmarklets as useful things.
Comment 36 SmoothPorcupine Pirate 2008-06-03 21:30:17 PDT
I like having data: and javascript: URLs in my bookmarks. I have quite a few of them. I also wouldn't be fooled into bookmarking such a URL. For those that would be tricked, a security dialog would be the best choice. Just give it one of those  "Don't show this dialog again." checkboxes so I can turn it off.
Comment 37 Tyler Downer [:Tyler] 2009-11-15 06:59:57 PST
*** Bug 528772 has been marked as a duplicate of this bug. ***
Comment 38 Mike Hommey [:glandium] 2009-12-15 05:34:11 PST
(In reply to comment #34)
> We won't take this for because the fix we want might be more
> complicated. Dan will comment on it shortly.

(almost 2 years later, Dan didn't comment)

How should this issue be addressed ? Is the security popup the considered solution ?
Comment 39 Michal Zalewski 2011-01-02 14:02:03 PST
I still think it would be productive to fix the vector shown here, seems like a low effort:

While bookmarklets are useful, it seems doubtful that there is a need to have javascript: and data: bookmarkable via Ctrl-D without any warning, especially if this does not work in other browsers.
Comment 40 Jesse Ruderman 2011-05-12 18:40:28 PDT
For data: URLs, fixing bug 656823 would be better than e.g. refusing to bookmark.

In fact, assuming we fix that bug, we could make it so attempting to bookmark the page "javascript:1+2" could bookmark "data:text/html,3" rather than doing nothing.
Comment 41 Alex Limi (:limi) — Firefox UX Team 2012-01-20 15:22:13 PST
I don't think there's any UI work here, removing uiwanted.
Comment 42 Jim Mathies [:jimm] 2015-01-06 12:46:13 PST
Not sure why this was nominated for e10s triage, it's not e10s specific.

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