Closed Bug 1592237 Opened 6 years ago Closed 6 years ago

Websites can change content inside the selection and thus influence what the user ends up copying to the clipboard

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Version:
70 Branch
Platform:
Desktop
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1591698

People

(Reporter: kontakt.luck, Unassigned)

Details

+++ This bug was initially created as a clone of Bug #1591698 +++
+++ I cloned the bug because it is a horrible security risk and i have the same interest to get this fixed asap +++

Steps to reproduce:

I went to metro.co.uk, marked some text, used ctrl-v to copy it, then pasted it someplace else.

Actual results:

Instead of the text, I got the text, a newline, and additional text.

In this case it was a Read more: [url] \n Twitter: https://twitter.com/MetroUK | Facebook: https://www.facebook.com/MetroUK/

Expected results:

It should have copied only the text I marked.

Note that this is a horrible security issue. The newlines cause the text to be immediately executed if I pasted it into a command line window. My browser tells the web site whether I'm on Linux or Windows, so the web site can even send "\nformat c: /s\n" on Windows and "\nrm -rf ~\n" on Linux.

I have a solution for this: Just copy things from DOM to the clipboard that the user actually marked NOT any other code.

(In reply to drlellinger from comment #1)

I have a solution for this: Just copy things from DOM to the clipboard that the user actually marked NOT any other code.

I explained why this doesn't work in bug 1591698 comment 10:

(In reply to :Gijs (he/him) from bug 1591698 comment #10)

e.g. opacity: 0 elements are invisible and will still take clicks. It'd also be possible to hide content into an overflow:hidden container, or use a foreground colour that blends in with the background colour, or...

All of these would have a representation in the layout frame tree and would thus be part of what the user "actually marked".

Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE

if they created it (visible) right before copying and removed it right after, users probably wouldn't notice and certainly wouldn't be able to do anything about it.

Websites don't create code the moment you copy code. There must be a mechanism that discards code like "\nformat c: /s\n" on Windows and "\nrm -rf ~\n" on Unix Systems.
Also a mechanism could be implemented that reviews the acutal clipboard with the marked selection, if it does not compare it should be also discarded.

Closing this type of bugs is not helping the situation.

(In reply to drlellinger from comment #3)

There must be a mechanism that discards code like "\nformat c: /s\n" on Windows and "\nrm -rf ~\n" on Unix Systems.

There isn't, and even if there was it wouldn't help for the problem as originally reported, which didn't involve shell code but copyright/social-media link insertion.

Also a mechanism could be implemented that reviews the acutal clipboard with the marked selection, if it does not compare it should be also discarded.

There's no meaningful distinction for the computer between the "marked selection" and what currently gets copied, so no, such a mechanism could not be implemented.

Closing this type of bugs is not helping the situation.

We try to be clear about bugs that we think cannot or will not be fixed. That's better than leaving them open indefinitely.

Note that I restricted comments on the original bug for a reason. Cloning it and continuing to argue by repeating points made elsewhere wastes people's time (and spams the CC list of the previous bug which you've cloned into this one). Please stop and let engineers like me get back to fixing bugs and making Firefox better.

There's no meaningful distinction for the computer between the "marked selection"
What i was thinking of was some kind of visual approach: like an OCR just review the marked text and compare it.

But whatever I do not give this any priority anymore after this end to the conversation.

(In reply to drlellinger from comment #5)

What i was thinking of was some kind of visual approach: like an OCR just review the marked text and compare it.

This doesn't work in "rich" web apps that have to provide their own copy/paste support, like google sheets, image editors, etc.

Yeah you cannot overlook every website in the world but this would work on a website like this: http://thejh.net/misc/website-terminal-copy-paste and that's why it should be there: At least try to give a bit more security you cannot fix everything.
You can close this now if you do not want to take any further action.

Component: Untriaged → DOM: Events
Product: Firefox → Core

(In reply to :Gijs (he/him) from comment #6)

(In reply to drlellinger from comment #5)

What i was thinking of was some kind of visual approach: like an OCR just review the marked text and compare it.

This doesn't work in "rich" web apps that have to provide their own copy/paste support, like google sheets, image editors, etc.

As I've already mentioned in the original Bug:
Implement access rights for modifying the clipboard much like we already have for GPS position, Autoplay, Notifications, webcam, etc...

Reading and participating in these 2 bugreports feels a lot like running a against a wall.
I feel like the issue doesn't get a proper evaluation and is being closed immediately.
I would love for other engineers (and security-aware people) to chime in and give their evaluation and considerations, instead of this getting closed before it was even triaged.

Implement access rights for modifying the clipboard much like we already have for GPS position, Autoplay, Notifications, webcam, etc...
This would be a feature you can implement for those "rich" web apps. Manual copy selection from user should be possible without extra permissions though.

We could have a way to disable copy/cut altogether, but to avoid what gets to the clipboard if copy/cut is supported is harder - web page could after all just catch ctrl+c event and at that point modify DOM to contain some other content and then that would get copied (this all even if modifying datatransfer object wasn't supported).

(In reply to Olli Pettay [:smaug] from comment #10)

We could have a way to disable copy/cut altogether

We have supported that for many years: the dom.allow_cut_copy pref. Setting that preference to false will prevent web pages from automatically copying to the clipboard.

but to avoid what gets to the clipboard if copy/cut is supported is harder - web page could after all just catch ctrl+c event and at that point modify DOM to contain some other content and then that would get copied (this all even if modifying datatransfer object wasn't supported).

Indeed.

So I took a look at the code on metro.co.uk that does this, and here's how it looks like:

      _onCopy: function (e) {
        - 1 < clip.BLACKLIST.indexOf(e.srcElement.type) || clip.appendAttribution()
      },
      appendAttribution: function () {
        var e = document.getElementsByTagName('body') [0],
        t = window.getSelection(),
        n = t.getRangeAt(0),
        i = document.createElement('p');
        i.innerHTML = t;
        var o = document.createElement('p');
        o.innerHTML = clip.ATTRIBUTION_SITE;
        var c = document.createElement('p');
        c.innerHTML = clip.ATTRIBUTION_TWITTER + clip.ATTRIBUTION_FB;
        var T = document.createElement('br'),
        l = document.createElement('div');
        l.style.position = 'absolute',
        l.style.left = '-99999px',
        l.appendChild(i),
        l.appendChild(T),
        l.appendChild(o),
        l.appendChild(c),
        e.appendChild(l),
        t.selectAllChildren(l),
        window.setTimeout(function () {
          e.removeChild(l),
          t.removeAllRanges(),
          t.addRange(n)
        }, 0)
      }

While I agree with Gijs that a very generic intervention around the lines of "make what the user sees on the screen be copied to the clipboard" is probably unattainable, it seems that it might be possible to add some simpler interventions that would at least fix the case at hand.

For example, it is reasonable for websites to change the active selection ranges during the course of disptching cut/copy events? By making the selection objects (and their associated ranges) inert during that period we should be able to very simply break this code from working and address the original bug as filed. Not sure if that won't break some legitimate use case of the clipboard events but perhaps it is worth a try?

Also, I'd like to ask for the bug to be reopened. I think maybe the discussion has diverged a little bit away from the original bug report into other unrelated topics, and I'd like to try to focus it back to the original bug report: It is an unreasonable behaviour as far as users are concerned for a website like metro.co.uk to be able to add invisible text to what the user is trying to copy to the clipboard. Firefox should just behave like any other application which has cut/copy functionality here.

This bug is not about fixing all potential ways of websites doing this (so saying that we won't act on this very well known and common technique of doing so because other potential techniques which could be made to work in the future may emerge is a bit of a non-sequitur). This bug is also not about fixing other potential issues where the user has indeed selected something that is barely visible on the screen (e.g. through playing with almost invisible selection/background/foreground colours) and that thing getting copied to the clipboard. It is also not about the browser learning to interpret shell commands and sanitize clipboard data to ensure no shell code can ever get copied to the clipboard from a web page. If folks want to discuss any of those types of issues, please file other bugs (and I agree that all of those other bugs are probably WONTFIX.)

We will know when this bug is fixed if the user can use Firefox to go to an article on metro.co.uk and copy a piece of text and paste it into another application and see what they copied being pasted there.

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to :ehsan akhgari from comment #13)

Also, I'd like to ask for the bug to be reopened.

Aside: if any bug is getting reopened it should be the original, not the dupe.

I think maybe the discussion has diverged a little bit away from the original bug report into other unrelated topics, and I'd like to try to focus it back to the original bug report: It is an unreasonable behaviour as far as users are concerned for a website like metro.co.uk to be able to add invisible text to what the user is trying to copy to the clipboard. Firefox should just behave like any other application which has cut/copy functionality here.

This bug is not about fixing all potential ways of websites doing this (so saying that we won't act on this very well known and common technique of doing so because other potential techniques which could be made to work in the future may emerge is a bit of a non-sequitur).

But this contradicts what you wrote in your previous paragraph, namely that it is "unreasonable" for websites to be able to do this and that should motivate us to fix it. If so, it should be unreasonable for the website to do this irrespective what technical means it uses to accomplish the (to the user, seemingly) unreasonable action. If we accept the metro.co.uk case as a valid "Firefox bug", it would also imply that whenever any other website does similar dodgy things through different technical means, we implicitly agree that that too, is a bug. I don't think that's a tenable position.

I'll repeat myself from earlier comments, but I fully understand that intuitively, from the web user's perspective, this is not helpful and broken and whatever other adjectives you'd like to apply.

However, reasonableness-to-the-user isn't a result of the technical means used by the website, it's a result of the contents of what is copy-pasted (whether facebook/twitter goop you didn't want or dodgy shell commands). You could use the same techniques to e.g. improve readability of selections of table copy paste to include table headers, or to remove off-screen content that is in a contenteditable for editor-management reasons but shouldn't be in the copy-pasted string, or just to make things even work at all in google sheets, or canvas-rendered editor apps, or ...

I don't think we can break this behaviour without breaking web compatibility because its "unreasonable" uses are indistinguishable from "reasonable" uses of the exact same APIs. Breaking web compat without due reason when you're a minority player is unwise.

I also, additionally, don't think that there's much point to breaking this specific way of doing what the website is doing, because if the website insists on doing this it'll find other techniques to do the same obnoxious thing. I think our time is better spent elsewhere - it's not like we're sitting around waiting for things to do...

(In reply to :ehsan akhgari from comment #12)

For example, it is reasonable for websites to change the active selection ranges during the course of disptching cut/copy events? By making the selection objects (and their associated ranges) inert during that period we should be able to very simply break this code from working and address the original bug as filed. Not sure if that won't break some legitimate use case of the clipboard events but perhaps it is worth a try?

So I think the answer to this is firmly "no", it's not worth the effort given that it's unlikely to actually achieve the desired goal and likely to have negative side effects, besides the actual cost of implementing something like this sanely. If you feel strongly that this is the wrong answer, I suggest you ask the triage folks in Core DOM Events (or Core Selection, it's a bit of a wash where this would belong) to reconsider the state of the original bug.

Flags: needinfo?(gijskruitbosch+bugs)

This could also be implemented as an additinal copy WYSIWYG option to alleviate compatibility concerns. There already is paste vs paste raw in other applications so this concept wouldn't be entirely new to users.

FWIW, I don't understand what inert selection object or range would mean.
How should a range be serialized when it is "inert", but the underlying DOM has already changed?

(In reply to :Gijs (he/him) from comment #14)

I think maybe the discussion has diverged a little bit away from the original bug report into other unrelated topics, and I'd like to try to focus it back to the original bug report: It is an unreasonable behaviour as far as users are concerned for a website like metro.co.uk to be able to add invisible text to what the user is trying to copy to the clipboard. Firefox should just behave like any other application which has cut/copy functionality here.

This bug is not about fixing all potential ways of websites doing this (so saying that we won't act on this very well known and common technique of doing so because other potential techniques which could be made to work in the future may emerge is a bit of a non-sequitur).

But this contradicts what you wrote in your previous paragraph, namely that it is "unreasonable" for websites to be able to do this and that should motivate us to fix it. If so, it should be unreasonable for the website to do this irrespective what technical means it uses to accomplish the (to the user, seemingly) unreasonable action. If we accept the metro.co.uk case as a valid "Firefox bug", it would also imply that whenever any other website does similar dodgy things through different technical means, we implicitly agree that that too, is a bug. I don't think that's a tenable position.

The only contradiction is if you view my arguments in a with the mathematical logic eyeglasses: "If it's unreasonable for site A to do x, then it must be unreasonable for any site S to do x. Therefore if we cannot prevent any site S to do x it is futile to prevent site A to do x."

But I think our users actually do not care about any site S at all. They care that the sites they actually visit in the real world work in the way they expect. So if we fix the problem they're having with site A and not for any site S, they would appreciate that.

I understand that it is an arms race and if we fixed this sites like metro.co.uk may change their code to do something more sophisticated but we deal with these types of arms race questions all the time and employ good-enough-in-practice-but-not-bullet-proof solutions to them, I don't see this problem as a special case.

However, reasonableness-to-the-user isn't a result of the technical means used by the website, it's a result of the contents of what is copy-pasted (whether facebook/twitter goop you didn't want or dodgy shell commands). You could use the same techniques to e.g. improve readability of selections of table copy paste to include table headers, or to remove off-screen content that is in a contenteditable for editor-management reasons but shouldn't be in the copy-pasted string, or just to make things even work at all in google sheets, or canvas-rendered editor apps, or ...

I don't think we can break this behaviour without breaking web compatibility because its "unreasonable" uses are indistinguishable from "reasonable" uses of the exact same APIs. Breaking web compat without due reason when you're a minority player is unwise.

Do I understand correctly that you're saying this same technique is used in other applications to clean up the copied content? If yes do you mind sharing some examples? I certainly agree that concrete web compat concerns are a great reason not to do what I was suggesting before.

I also, additionally, don't think that there's much point to breaking this specific way of doing what the website is doing, because if the website insists on doing this it'll find other techniques to do the same obnoxious thing. I think our time is better spent elsewhere - it's not like we're sitting around waiting for things to do...

There is a difference between saying we won't fix a bug (aka we will not accept a patch to it) vs we can't prioritize it right now. Surely lack of resources to work on something at the moment isn't a good reason to say we're not open to the idea of someone else doing the work for us, or us picking it up later?

(In reply to Olli Pettay [:smaug] from comment #16)

FWIW, I don't understand what inert selection object or range would mean.
How should a range be serialized when it is "inert", but the underlying DOM has already changed?

Something really simple, like this was what I had in mind:

  • Teach mozilla::Selection about whether a copy/cut event is currently being dispatched.
  • For Selection methods which check IsValidSelectionPoint(), throw an exception if copy/cut is currently being dispatched.
  • In Range::DoSetRange, if the range has a selection and it tells us a copy/cut event is in progress, detach it from mSelection.

(In reply to :ehsan akhgari from comment #17)

I understand that it is an arms race and if we fixed this sites like metro.co.uk may change their code to do something more sophisticated but we deal with these types of arms race questions all the time and employ good-enough-in-practice-but-not-bullet-proof solutions to them, I don't see this problem as a special case.

I think "good enough but not bullet proof" works if you raise the bar - if you make smash-proof glass that you could maybe still break with heavier machinery, to go with the metaphor. I think that's not what this is, this is planking up the window while the door is open.

You could use the same techniques to e.g. improve readability of selections of table copy paste to include table headers, or to remove off-screen content that is in a contenteditable for editor-management reasons but shouldn't be in the copy-pasted string, or just to make things even work at all in google sheets, or canvas-rendered editor apps, or ...

I don't think we can break this behaviour without breaking web compatibility because its "unreasonable" uses are indistinguishable from "reasonable" uses of the exact same APIs. Breaking web compat without due reason when you're a minority player is unwise.

Do I understand correctly that you're saying this same technique is used in other applications to clean up the copied content? If yes do you mind sharing some examples? I certainly agree that concrete web compat concerns are a great reason not to do what I was suggesting before.

I think pretty much any editor (gdocs, codemirror, office online, ...) uses cut/copy/paste event handlers. I would be astonished if they never manipulated the selected DOM in those handlers. Some trivial searching finds e.g. https://github.com/codemirror/CodeMirror/blob/c18094eab567211f73050beaa5c3fcceb2133888/src/input/ContentEditableInput.js#L59-L72 . I could probably find better examples if github codesearch wasn't absolute garbage and let you search for quotes, because obviously the string "copy" occurs a lot more often without the qutoes... Anyway, I haven't looked into the details, so perhaps the naive reading of that code is wrong.

It's possible we could work around most of this by allowing the manipulation if part of a contenteditable, but of course that creates additional complexity.

I also, additionally, don't think that there's much point to breaking this specific way of doing what the website is doing, because if the website insists on doing this it'll find other techniques to do the same obnoxious thing. I think our time is better spent elsewhere - it's not like we're sitting around waiting for things to do...

There is a difference between saying we won't fix a bug (aka we will not accept a patch to it) vs we can't prioritize it right now. Surely lack of resources to work on something at the moment isn't a good reason to say we're not open to the idea of someone else doing the work for us, or us picking it up later?

I think we also wouldn't want other people to do this work and add code to gecko to cover this specific corner-case. It's added code that needs review, that needs tests, that will need to be kept working when refactoring code, etc. etc. It's just not the case that we can make 0-cost changes, or that the only cost is writing the patch. Given that that is the case, we should focus on changes with significant benefit given the cost they incur. It seems to me that the complexity of addressing this isn't worth the negligible benefit of metro.co.uk not inserting a twitter link - until they figure out a workaround for whatever we implement.

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