Open Bug 371179 Opened 18 years ago Updated 2 years ago

Don't allow bookmarking an evaluated+loaded javascript: URL

Categories

(Firefox :: Security, defect)

Product:
Firefox
Component:
Security
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
e10s - ---

People

(Reporter: lcamtuf, Unassigned)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: sec-want, Whiteboard: [sg:want][xss with nearly normal user interaction and no upside])

Attachments

(3 files)

testcase
(deleted), text/html
Details
branch patch
(deleted), patch
Details | Diff | Splinter Review
branch patch v2
(deleted), patch
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1 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: http://lcamtuf.coredump.cx/ffbook/ Reproducible: Always
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)
(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). /mz
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. /mz
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).
Whiteboard: [sg:want]
> 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: javascript:"<html><body><h3>Foo"
(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? /mz
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.
Summary: bookmarked data: URLs can be used to cross domains → bookmarked data: or javascript: URLs can be used to cross domains
-> http://www.heise-security.co.uk/news/85728 requesting blocking 1.8.1.3
Flags: blocking1.8.1.3?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached file testcase (deleted) —
Attached patch branch patch (deleted) — — Splinter Review
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.
Attachment #256474 - Attachment is patch: true
Attachment #256474 - Attachment mime type: application/octet-stream → text/plain
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?
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.
Keywords: uiwanted
How about skipping the javascript: and data: tabs and bookmarking the rest?
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.
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).
Attached patch branch patch v2 (deleted) — — Splinter Review
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.
Might need similar changes to other apps....
Indeed. Camino is also vulnerable.
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. cl
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 2.0.0.2.
Camino version of this bug is bug 372020.
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.
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?
Flags: blocking1.8.1.4? → blocking1.8.1.4+
Mike: please find an appropriate person from your team to assign this to.
Assignee: nobody → mconnor
Assignee: mconnor → gavin.sharp
(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.
No one likes this approach, FF devs would prefer waiting until we have a better solution.
Flags: blocking1.8.1.5+
Flags: blocking1.8.1.4+
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 alpha6
Target Milestone: Firefox 3 alpha6 → Firefox 3 beta1
waiting for a trunk fix, pushing out to the next branch release.
Flags: blocking1.8.1.5+ → blocking1.8.1.6?
Target Milestone: Firefox 3 M7 → Firefox 3 M8
Flags: blocking1.8.1.7? → blocking1.8.1.7+
Target Milestone: Firefox 3 M8 → Firefox 3 M9
moving out bugs that don't need to block b1
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Following suit on the branch, need a trunk fix first.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.9+
Flags: blocking1.8.1.8+
Priority: -- → P3
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Blocks: 376635
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.
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.
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.
Flags: blocking1.8.1.12+ → blocking1.8.1.13?
Flags: blocking1.8.1.13? → blocking1.8.1.13+
We won't take this for 1.8.1.13 because the fix we want might be more complicated. Dan will comment on it shortly.
Flags: blocking1.8.1.14+
Flags: blocking1.8.1.13-
Flags: blocking1.8.1.13+
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.
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Flags: blocking-firefox3+
Flags: blocking1.8.1.15+
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.
Assignee: gavin.sharp → nobody
Priority: P3 → --
Target Milestone: Firefox 3 beta3 → ---
(In reply to comment #34) > We won't take this for 1.8.1.13 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 ?
No longer blocks: 376635
I still think it would be productive to fix the vector shown here, seems like a low effort: http://lcamtuf.coredump.cx/ffbook/ 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.
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.
Summary: bookmarked data: or javascript: URLs can be used to cross domains → Don't allow bookmarking an evaluated+loaded javascript: URL
Whiteboard: [sg:want] → [sg:want][xss with nearly normal user interaction and no upside]
Blocks: bookmarklet-xss
I don't think there's any UI work here, removing uiwanted.
Keywords: uiwanted
tracking-e10s: --- → ?
Not sure why this was nominated for e10s triage, it's not e10s specific.
tracking-e10s: ? → -
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: