Closed Bug 1725487 Opened 3 years ago Closed 2 years ago

Don't allow dragging javascript: links to create bookmarklet

Categories

(Firefox :: Bookmarks & History, enhancement, P2)

Firefox 90
enhancement

Tracking

()

RESOLVED FIXED
98 Branch
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.

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

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE

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:

  1. context-menu "Bookmark Link" => shows the "add bookmark" panel (which does NOT show the URL).
  2. drag it to the bookmark toolbar => URL never visible
  3. drag it to the bookmark sidebar => URL never visible
  4. drag it to the "Manage Bookmarks dialog sidebar => URL never visible
  5. drag it to the main secton of "Manage Bookmarks" (Library) dialog => URL column visible
  6. "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.

Status: RESOLVED → REOPENED
Type: defect → enhancement
Flags: needinfo?(gijskruitbosch+bugs)
Resolution: DUPLICATE → ---
Summary: Warn before adding bookmarklet → Don't allow dragging javascript: links to create bookmarklet

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.

See Also: → 1721092

(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...

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mak)

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.

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.

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:

  1. 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.
  2. 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?

Flags: needinfo?(mak) → needinfo?(dveditz)

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

Flags: needinfo?(dveditz)
Severity: -- → S3
Priority: -- → P2

We are hoping to look at this in the next couple of months.

Assignee: nobody → jenyakotovich

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.

(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?

Flags: needinfo?(mak)

(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?

Flags: needinfo?(mak) → needinfo?(gijskruitbosch+bugs)

(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.

Flags: needinfo?(gijskruitbosch+bugs)
Attachment #9255378 - Attachment description: Bug 1725487 - Don't allow dragging javascript: links to create bookmarklet. r?mak → Bug 1725487 - Show the Add Bookmark Dialog when dragging javascript url to create a bookmarklet. r?mak
Flags: needinfo?(mak)
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

Backed out for causing mochitest failures on browser_toolbar_drop_bookmarklet.js

Flags: needinfo?(jenyakotovich)
Attachment #9256343 - Attachment description: Bug 1725487 - Don't allow dragging multiple uris if one is javascript:link. r?mak → Bug 1725487 - Don't allow dragging multiple uris if one is javascript:link. r?mak,standard8

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

Regressions: 1751649
Attachment #9256343 - Attachment description: Bug 1725487 - Don't allow dragging multiple uris if one is javascript:link. r?mak,standard8 → Bug 1725487 - Don't allow dragging multiple uris if one is javascript:link. r?mak
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
Status: REOPENED → RESOLVED
Closed: 3 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch
Flags: needinfo?(jenyakotovich)
Blocks: 1751649
No longer regressions: 1751649
See Also: → 1779542

It seems that this technique is being used against Discord users: https://krebsonsecurity.com/2023/05/discord-admins-hacked-by-malicious-bookmarks/

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

Attachment

General

Created:
Updated:
Size: