Don't allow dragging javascript: links to create bookmarklet
Categories
(Firefox :: Bookmarks & History, enhancement, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox98 | --- | fixed |
People
(Reporter: michel, Assigned: janey)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
Hello,
I saw websites recommending adding listed "links" to bookmarks by dragging them to to the bookmarks toolbar. Those "links" however were malicious bookmarklets.
I think that Firefox should warn before adding bookmarklets since probably most users never heard of bookmarklets and don't expect bookmarks to have capabilities like stealing cookies, performing requests or replacing page content preserving omnibox content.
Bookmarklets can look and behave like normal links opening a defined url, but perform malicious actions when specified websites are open and the user clicks on the bookmarklet. That can make them very difficult to be noticed by an average user.
Reporter | ||
Comment 1•3 years ago
|
||
I wrote a simple example https://lebihan.pl/bookmarklet-poc/ and recorded a video of how it works: https://lebihan.pl/files/2021-08-13_11-21-24.mp4
Updated•3 years ago
|
Comment 3•3 years ago
|
||
Comment 4•3 years ago
|
||
Gijs: I'm morphing this bug slightly and reopening it. I'm not sure all the reasons we haven't wanted to implement bug 371923 generally, but dragging links to the bookmarks toolbar is a dangerous subset that could be addressed separately if we wanted to.
There are several ways to create a bookmarklet. Unlike a normal web page you can't just "star" it or use Ctrl/Cmd-D to create a bookmark, but all the other ways work:
- context-menu "Bookmark Link" => shows the "add bookmark" panel (which does NOT show the URL).
- drag it to the bookmark toolbar => URL never visible
- drag it to the bookmark sidebar => URL never visible
- drag it to the "Manage Bookmarks dialog sidebar => URL never visible
- drag it to the main secton of "Manage Bookmarks" (Library) dialog => URL column visible
- "Add Bookmark" in various places => must manually paste or type the URL
5 and 6 give victims a fighting chance -- we can leave those methods alone.
2-4 need to fail for javascript: links. Could argue to fail #5, too, for consistency or just because it's easier.
1 could be made safe-ish if we added the URL field back to the panel (like the Edit panel), but if we aren't going to do that we should disable or fail "Bookmark Link" on a javascript: link.
needinfo? to see if you agree or want to object.
Comment hidden (obsolete) |
Comment 6•3 years ago
|
||
Reopening this one in the face of many dupes was fairly arbitrary, but the use of drag got me thinking and checking the Chrome behavior.
Comment 7•3 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #4)
Gijs: I'm morphing this bug slightly and reopening it. I'm not sure all the reasons we haven't wanted to implement bug 371923 generally
I'm out; 302 Marco but I would have expected it's less "we don't want to" and more "nobody has bothered because it hasn't seemed all that important (and there's some UX stuff to figure out in order to not annoy people who do use this regularly)"
but dragging links to the bookmarks toolbar is a dangerous subset that could be addressed separately if we wanted to.
Maybe; I'm not sure if that's really easier than bookmark creation in general, and if that wouldn't turn into whack-a-mole with "right click this link and then click bookmark link", in combination with a mousedown handler that changes the link target, and whatever other mechanisms we provide...
Reporter | ||
Comment 8•3 years ago
|
||
but dragging links to the bookmarks toolbar is a dangerous subset that could be addressed separately if we wanted to.
Maybe; I'm not sure if that's really easier than bookmark creation in general, and if that wouldn't turn into whack-a-mole with "right click this link and then click bookmark link", in combination with a mousedown handler that changes the link target, and whatever other mechanisms we provide...
That's why I proposed to not block any bookmarklet creation method, but to display a warning when adding one. Firefox already displays a popup when the user attempts to install an addon.
Comment 9•3 years ago
|
||
Yes, that's why Gijs initially marked this as a duplicate of bug 371923 that requests such a warning. That bug seems stalled on figuring out the appropriate user experience, plus designing and implementing UI interactions takes more work and coordination between several groups. I reopened this to see if there was a subset that could be resolved with a simple UI-less implementation that would be more likely to get done in a nearer future.
but dragging links to the bookmarks toolbar is a dangerous subset that could be addressed separately if we wanted to.
Maybe;
Yeah, I should have gone back and revised that after my enumeration of drag targets got longer and longer.
(In reply to Daniel Veditz [:dveditz] from comment #5)
FWIW Safari lets you drag-create these to the favorites bar, Chrome does not.
That's incorrect: Chrome lets you drag-create bookmarklets to the bookmark toolbar, too.
Comment 10•3 years ago
•
|
||
We don't have numbers to be able to tell how common is to create a bookmarklet by dragging to the toolbar, so it's hard to tell whether this breaks many or not. If we disallow it, we still provide ways to do it, maybe not as quickly but still decent.
I'm also not a fan of confirm/warning dialogs. If we introduce a warning dialog, users that don't use bookmarklets should always refuse, while users that use them would probably not want to see it every time, and will need the usual "don't ask me again" management. It doesn't sound like a nice experience for any of the sides.
Thus, I see 2 potential alternatives:
- Just disallow creating a bookmarklet by dropping a js url. Users will have to pick Add Bookmark. This removes some coherence from the bookmarking system and may break user expectations.
- When dropping a js url, after creating the bookmarklet, open the Add Bookmark dialog for it. This will keep the feature, adding one additional confirmation step that shows the url. We could add an inline warning in the dialog itself under the Location field, to clarify bookmarklets may be dangerous. We'll need proper message and styling for that. Note we must add the bookmark first for technical reasons, but I am filing an Outreachy proposal to change this behavior, so that we create bookmarks AFTER showing the dialog.
I'd probably go for 2, because it sounds like may have potential to also resolve bug 371923, the inline warning if made visible enough, would work as a confirmation hint.
wdyt, would this address our sec concerns?
Comment 11•3 years ago
|
||
If we disallow it, we still provide ways to do it, maybe not as quickly but still decent.
Right-click on bookmark toolbar, "Add Bookmark...", paste URL, give it a better name -- done. Not terrible, given the power of bookmarklets.
It doesn't sound like a nice experience for any of the sides.
That's the argument that has kept bug 371923 stalled forever.
I un-duped this bug to suggest what you call option #1 -- a "worse is better" hack to get some progress. Not elegant, but gets the job done without having to involve UI changes and zero localization impact. May put a little extra burden on folks who use bookmarklets, although maybe not! I use bookmarklets nearly daily and I've always added them by copying script out of my scratchpad where I'm writing/debugging it and then pasting it into one of the "Add" fields.
Do we even have telemetry on bookmarklets? It's nice to treat power users well, but surely this is a vanishingly small esoteric feature.
Your option #2 is better, of course. If it doesn't involve so many pieces and approvals that stops it from happening. I'd consider that pretty much a solution for bug 371923 if it includes the warning string (when when it's not triggered by a drag). The first version doesn't have to include the warning string -- just turning a drag into the "add" dialog would be a decent speedbump
Updated•2 years ago
|
Comment 12•2 years ago
|
||
We are hoping to look at this in the next couple of months.
Assignee | ||
Comment 13•2 years ago
|
||
Updated•2 years ago
|
Comment 14•2 years ago
|
||
Jane is implementing option #2 here, when dropping a single javascript URL onto a Places view, we'll open the Add Bookmark dialog.
Dropping multiple elements won't do it, but I think that's a much rarer case that is unlikely to happen in a malicious way.
This first implementation won't include a warning message, we should use bug 371923 for adding it.
Comment 15•2 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #14)
Jane is implementing option #2 here, when dropping a single javascript URL onto a Places view, we'll open the Add Bookmark dialog.
Dropping multiple elements won't do it, but I think that's a much rarer case that is unlikely to happen in a malicious way.
This first implementation won't include a warning message, we should use bug 371923 for adding it.
When dragging from a website, doesn't the website control the drag data and is therefore able to trigger the multi-element drag/drop case?
Comment 16•2 years ago
|
||
(In reply to :Gijs (he/him) from comment #15)
When dragging from a website, doesn't the website control the drag data and is therefore able to trigger the multi-element drag/drop case?
Good point, but we can only show one dialog at a time... What should we do in that case?
The simplest thing that comes to my mind is disallow dropping multiple elements if one of them is a javascript url in text format. I don't see many use cases for that. wdyt?
Comment 17•2 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #16)
(In reply to :Gijs (he/him) from comment #15)
When dragging from a website, doesn't the website control the drag data and is therefore able to trigger the multi-element drag/drop case?
Good point, but we can only show one dialog at a time... What should we do in that case?
The simplest thing that comes to my mind is disallow dropping multiple elements if one of them is a javascript url in text format. I don't see many use cases for that. wdyt?
That (blocking the drag entirely if any of them is a JS URL) sounds good to me.
Assignee | ||
Comment 18•2 years ago
|
||
Depends on D133813
Updated•2 years ago
|
Comment 19•2 years ago
|
||
Look at the failure in https://treeherder.mozilla.org/jobs?repo=try&revision=ae6acace95dd6923ebfd6e918ad28ad0f3f977bc&group_state=expanded&selectedTaskRun=XZVO71p9QOqTfofNsh8Pzg.0
Updated•2 years ago
|
Updated•2 years ago
|
Comment 20•2 years ago
|
||
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/ae5ab5c447c3 Show the Add Bookmark Dialog when dragging javascript url to create a bookmarklet. r=mak https://hg.mozilla.org/integration/autoland/rev/1564124c0309 Don't allow dragging multiple uris if one is javascript:link. r=mak
Comment 21•2 years ago
|
||
Backed out for causing mochitest failures on browser_toolbar_drop_bookmarklet.js
- Backout link
- Push with failures
- Failure Log
- Failure line: TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_toolbar_drop_bookmarklet.js | Test timed out
Updated•2 years ago
|
Comment 22•2 years ago
•
|
||
this is very likely the same cause as other failures that made us disable bookmark tests on TSAN, we should for now just do the same, that means skipping the test there, because it's unclear how scheduler changes are breaking expectations here. See https://searchfox.org/mozilla-central/search?q=tsan&path=places
Updated•2 years ago
|
Comment 23•2 years ago
|
||
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/a32000478bbf Show the Add Bookmark Dialog when dragging javascript url to create a bookmarklet. r=mak https://hg.mozilla.org/integration/autoland/rev/547e11aae5fe Don't allow dragging multiple uris if one is javascript:link. r=mak
Comment 24•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a32000478bbf
https://hg.mozilla.org/mozilla-central/rev/547e11aae5fe
Updated•2 years ago
|
Updated•2 years ago
|
Reporter | ||
Comment 26•10 months ago
|
||
It seems that this technique is being used against Discord users: https://krebsonsecurity.com/2023/05/discord-admins-hacked-by-malicious-bookmarks/
Description
•