192355 - web page can put text on clipboard (cut/copy) using Midas

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

The "copy" and "cut" buttons on http://www.mozilla.org/editor/midasdemo/ work. 
They should not.  The most important reason is that people paste text into
powerful command-line apps such as mIRC and command prompts (see bug 97284
comment 157).  Another reason is that malicious porn sites (of which there are
many) might start putting ads in the clipboard.

As long as we're breaking one button ("paste") on existing pages, we might as
well break several of them, so backwards compatibility is not a good argument
for keeping "copy" and "cut" around.  Breaking "paste" without also breaking
"cut" is arguably dataloss, but that's not why I'm filing this bug.

It seems intentional that "copy" and "cut" work while "paste" does not:
http://lxr.mozilla.org/seamonkey/source/editor/docs/midas-spec.html.

Note that's it's fine that the standard keyboard shortcuts for copy and cut
(Ctrl+C and Ctrl+X on Windows) work in Midas.

Comment 1

[Jesse Ruderman]

Attachment: demo for IE: waits for you to paste from mIRC to mIRC

I haven't figured out all of Midas's commands yet, so here's a demo that only
works in IE.

Comment 2

[Joe Francis]

-> brade

Comment 3

[Kathleen :Brade]

There was some discussion of this issue in another bug.
Also, the intention is to enable paste (with an appropriate prompt).

Rhetorical question: why would someone do a paste when they hadn't copied anything?

Comment 4

[Daniel Veditz [:dveditz]]

> why would someone do a paste when they hadn't copied anything?

To see if there are any leftover goodies in the clipboard worth stealing. Given
a popular enough page it'd be worth trolling through everyone's clipboard for
the small percentage that contain credit card or SSN numbers, phone numbers,
addresses, potential passwords, especially if the page had some sort of
"membership" or cookie that would allow you to tie the data to a specific account.

Comment 5

[Mitchell Stoltz (not reading bugmail)]

Jesse made a good point, and I'm worried about all clipboard functionality in
Midas. Can we disable all of the clipboard functions, or perhaps implement a
"local" clipboard that can be used by one Midas frame only?

Comment 6

[Mike Kaply [:mkaply]]

That would defeat the purpose.

We should provide checkboxes on the Advanced->Security page to turn this off if
people want to.

I think we are looking for security holes where there aren't any.

Any application can wipe out the clipboard - you aren't supposed to put
important stuff there. It's temporary.

Comment 7

[Kathleen :Brade]

dveditz (comment 4)--I don't understand your comments in the context of this
bug.  Web pages can't call paste.  Enabling that is covered in a different bug.

One issue with Jesse's testcase is that it only works in IE.  In mozilla,
execCommand only works on the document that has been edited.  The only way I've
come up for someone to grab a credit card number or other secure/private
information is to get you to type it or paste into that page.

Mitch (comment 5)--I don't think we need to go this far.  In mozilla we don't
blindly allow execCommand to work as IE (apparently) does.

Mike (comment 6)--I disagree, I don't want prefs.

I'd really like to resolve this bug as WONTFIX based on previous discussions
unless someone can convince me that copy/cut within Midas exposes a real exploit.

Comment 8

[timeless]

i actually do put important stuff on the clipboard, sometimes because i don't
know the values. sometimes because i really don't want to retype something that
i just typed. i also paste into command prompts where the newline character will
trigger execution of instructions.

one solution would be for the midas team to create a mozilla clipboard
application (perhaps similar to clipbrd.exe, clibbook.exe and xclipboard), it
would have a very simple feature set:
0. allow the user to view each piece of clipboard data
1. allow the user to control which content types are available
2. allow the user to control access from mozilla webpages to the system clipboard

(1 would solve the annoying paste as plaintext/quoted/rich text stuff, 2 solves
the security concerns)

Actually. if an application did something like request to handle speicific mime
types that might make me happier.  your typical html page that uses midas might
only ask for text/html, and image/*.  I would grant it permission for those
types. that should prevent me from exposing my passwords to the midas consumer
and it should prevent the midas consumer from poking stuff into mirc or a
console app.

Comment 9

[timeless]

ok, we talked about this on irc. the approach that midas people didn't reject
was requiring the midas panel to be active and in the active window and making
sure that the user would see the selection.

this would prevent a hidden / minimized / not focussed app from
stealing/poisoning. it's not perfect, and some additional conditions would
probably be needed, but it's a better compromise than the extremes...

Comment 10

[Jesse Ruderman]

Mike, comment 6:
> Any application can wipe out the clipboard - you aren't supposed to put
> important stuff there. It's temporary.

Any application can delete your files, too.  Web pages are not applications. 
Also, it sounds like you're underestimating the impact of a web page being able
to put text of its choice on the clipboard (see comment 0 and comment 1).


Mike, comment 6:
> We should provide checkboxes on the Advanced->Security page to turn this off 
> if people want to.

IE uses a pref (one checkbox for allowing scripted copy and paste), but I don't
like that solution.  Web pages should not be able to copy or paste by default. 
And if the default is to off, I don't think a pref would be very useful.


Brade, comment 7:
> One issue with Jesse's testcase is that it only works in IE.  In mozilla,
> execCommand only works on the document that has been edited....  In mozilla we 
> don't blindly allow execCommand to work as IE (apparently) does.

When I tried to make a demo for Mozilla, I couldn't figure out how to load or
create a non-empty designMode document using a script.  If there isn't a way
now, I expect there to be a way in the future because of the importance of being
able to edit existing documents.

Can you elaborate on "execCommand only works on the document that has been
edited"?  Is there a distinction between "designMode, has been edited" and
"designMode, has not been edited", and if so, when does a designMode document
become "edited"?


Brade, comment 3:
> Also, the intention is to enable paste (with an appropriate prompt).
Brade, comment 7:
> Web pages can't call paste.  Enabling that is covered in a different bug.

Bug number?  (Tried "ALL past prompt,dialog,warn,enabl,allow,midas".)

I don't like the dialog solution.  The dialog will annoy anyone trying to paste.
 Also, what would the wording be?
* "A web page is trying to..." is wrong if I just clicked a paste button.
* "Are you sure you want to..." is wrong if I wasn't trying to paste
* "To paste, press Ctrl+V or select Paste from the Edit menu" is strange because
you're giving the user instructions even though know what he's trying to do.

In what way is having a button with a dialog better than not having a button at
all?  Menus and keyboard shortcuts seem to be adequate for the Cut, Copy, and
Paste commands in plain-text textareas.


Brade, comment 7:
> I'd really like to resolve this bug as WONTFIX based on previous 
> discussions unless someone can convince me that copy/cut within Midas exposes 
> a real exploit.

Bug number(s)?  I haven't seen these discussions.  The only previous discussion
I'm aware of is in bug 97284 around bug 97284 comment 157, where one commenter
agreed with me and nobody disagreed.


Timeless, comment 9:
> ... requiring the midas panel to be active and in the active window and making
> sure that the user would see the selection.

Web pages can focus themselves, and windows don't always overlap, so this
wouldn't work.  Also, I often activate extra windows as I switch between my text
editor and command prompt, and sometimes those windows are Mozilla windows.

Comment 11

[Jesse Ruderman]

Attachment: file used by mozilla demo

Comment 12

[Jesse Ruderman]

Attachment: demo for Mozilla: waits for you to paste from mIRC to mIRC

This demo works in Mozilla build 2003 022408 on Windows XP.

Comment 13

[Daniel Veditz [:dveditz]]

I've got to go w/Jesse on this one--allowing a script to write to the clipboard
(let alone paste) without user action is trouble waiting to happen. Not
Klez-scale trouble--it's unlikely to affect my Mom and Pop--but it wouldn't be
all that hard to socially engineer an attack that would work against some
percentage of the slashdot-type crowd, either.

To put it in real world terms, it doesn't make people feel any better to be told
vandalism is a petty crime when it's their smashed mailbox, and they may not
take too kindly to the guy down the street handing out promotional bats.

If only one person unknowingly pastes a rogue "rd /s c:\windows<CRLF>" into a
windows command prompt that's too many.

Comment 14

[Mike Kaply [:mkaply]]

Dan:

So are you voting for totally disallow or add some protection?

Comment 15

[Daniel Veditz [:dveditz]]

There appear to be disscussions on the topic still in progress, but to put down
an answer for Mike:

(unsigned) scripts should not be able to touch the clipboard without human
action. IE offers three options for clipboard scriptability: allow, deny, and
prompt. We should only support two: deny and prompt.

I'm OK with prompt being the default -- deny would be the same as not
implementing the feature at all. UI for the pref could be buried on the Advanced
Scripts and Plugins page which includes similar "potential for abuse" items that
most normal people don't need to worry about.

I lean toward a single clipboard pref for simplicity. I'm open to arguments that
paste and cut/copy are different enough to warrant separate prefs, but most
people won't turn these off and the sorts of people who do would most likely do
both.

Comment 16

[Mike Kaply [:mkaply]]

Attachment: First attempt at a patch

OK, this is my first attempt at using script security, so work with me here.

This patch requires script access to access cut/copy and paste.

Changes to JS file look like this:

pref("capability.policy.default.clipboard.cutcopy", "noAccess");
pref("capability.policy.default.clipboard.paste", "noAccess");
pref("capability.policy.policynames","allowclipboard");
pref("capability.policy.allowclipboard.sites", "http://www.mozilla.org");
pref("capability.policy.allowclipboard.clipboard.cutcopy", "allAccess");
pref("capability.policy.allowclipboard.clipboard.paste", "allAccess");

At this point, the primary downside to this method is the error on the
javascript console:

Error: uncaught exception: [Exception... "Access to XPConnect service denied" 
code: "1011" nsresult: "0x805303f3 (NS_ERROR_DOM_XPCONNECT_ACCESS_DENIED)" 
location: "http://www.mozilla.org/editor/midasdemo/ Line: 192"]

Which doesn't seem to reflect what is actually happening. Or maybe it is.

If anyone can come up with a more creative way to avoid the double string check
of the cut, copy and paste, please let me know.

Comment 17

[Kathleen :Brade]

Comment on attachment 115765 [details] [diff] [review]
First attempt at a patch

add an "else" in these places:
+      if (sCutCopyInternal_id == JSVAL_VOID) {
+	 sCutCopyInternal_id =
+	   STRING_TO_JSVAL(::JS_InternString(cx, "cutcopy"));
+      }
+      ***ELSE*** if (sPasteInternal_id == JSVAL_VOID) {

+      if (commandID.Equals(NS_LITERAL_STRING("cut")) ||
+	   commandID.Equals(NS_LITERAL_STRING("copy"))) {
+	 rv = secMan->CheckPropertyAccess(cx, nsnull, "clipboard",
sCutCopyInternal_id,
+				       
nsIXPCSecurityManager::ACCESS_GET_PROPERTY);
+      }
+      ***ELSE*** if (commandID.Equals(NS_LITERAL_STRING("paste"))) {

if you're going to change this line:
+  } else 
+  {
change it to:
+  }
+  else
+  {

(preferably use whatever style is consistent in the rest of the file)

in the header, you added spaces to the blank line above the statics you are
adding; remove the spaces from the blank line.

Comment 18

[Mike Kaply [:mkaply]]

Attachment: Patch addressing brade's comments

Fixed spacing issues.
Added else cases.
Removed erroneous modify of another else.

Comment 19

[Mitchell Stoltz (not reading bugmail)]

Whoops, I was writing some comments, but you beat me to it...

The first one's my fault. I made a mistake in the prefs I gave you. Please
remove the line
pref("capability.policy.policynames","allowclipboard");

and instead modify this line that's already present in all.js:
pref("capability.policy.default_policynames", "mailnews");

to this:
pref("capability.policy.default_policynames", "mailnews allowclipboard");

I disagree with Kathy's first "else" suggestion. Please do not add an else
between the two jsstring initializations. Adding an else there will cause
sPasteInternal_id to be used before it is initialized.

I agree with her second "else" suggestion, except that as Mike mentioned, the
double string compare could be avoided. This is just a suggestion; you can take
it or leave it, but you could break the security check out into a separate
function with a boolean parameter to select "cutcopy" or "paste." Then you could
do like this:

if (command.Equals("paste"))
   DoSecurityCheck(true);
else if (command.Equals("cut") || command.Equals("copy"))
   DoSecurityCheck(false);
// otherwise, continue without security check...

Are the command strings case sensitive? Should they be?

One more nit: please capitalize "Clipboard" as the aClassName argument to
CheckPropertyAccess (that's the convention), and wrap it in an
NS_NAMED_LITERAL_STRING so it doesn't have to be defined twice, like so:
NS_NAMED_LITERAL_STRING(classNameStr, "Clipboard");
...
secMan->CheckPropertyAccess(cx, nsnull, classNameStr.get(),...

As for the error message in the console (again, this is just a suggestion), you
could post a more descriptive error message if you want. If CheckPropertyAccess
returns failure, you can call this:
JS_SetPendingException(cx, STRING_TO_JSVAL(JS_NewStringCopyZ(cx,
NS_LITERAL_STRING("error message of choice").get())));

to set a new error message.

Please post a new patch, and add the all.js changes to it, and I'll give an r=.

Comment 20

[Mike Kaply [:mkaply]]

Attachment: Take 3

I went with the DoSecurityCheck method although I had to make the two statics
public to do that since I didn't make DoSecurityCheck a member function because
it seemed unnecessary.

Comment 21

[Mike Kaply [:mkaply]]

For the record, that is supposed to be an NS_NAMED_LITERAL_CSTRING - I already
made the change.

Comment 22

[Asa Dotzler [:asa]]

We need quick turnaround on reviews for this. Time is short for 1.3. 

Comment 23

[Kathleen :Brade]

Comment on attachment 115796 [details] [diff] [review]
Take 3

add parens around this:
+  if NS_FAILED(rv)
changed to:
+  if (NS_FAILED(rv))

I'm not particularly fond of "fPaste"; I'd prefer "isPasteCommand" or "aPaste"
or "bPaste" ...

r=brade but I defer the official r= to Mitch Stoltz

Comment 24

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 115796 [details] [diff] [review]
Take 3

All of the c++ changes look fine, except for the parens that Kathy mentioned.

The prefs still aren't exactly right; again, I apologize for their complexity. 
+pref("capability.policy.default_policynames", "mailnews clipboard");

should be
+pref("capability.policy.default_policynames", "mailnews allowclipboard");

and you need to REMOVE this line:
+pref("capability.policy.policynames","allowclipboard");

r=mstoltz with those changes.

Comment 25

[Kathleen :Brade]

Attachment: patch with changes from brade and mstoltz

Comment 26

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 115922 [details] [diff] [review]
patch with changes from brade and mstoltz

r=mstoltz.

Comment 27

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 115922 [details] [diff] [review]
patch with changes from brade and mstoltz

Why are sCutCopyInternal_id and sPasteInternal_id static? They are set to
JSVAL_VOID every time an HTML document is destroyed, is that intentional? 

In any case I'd like them not be public. You could make DoSecurityCheck() a
member function (and even make it static if you want to save the |this| pointer
from being passed in).

Comment 28

[Kathleen :Brade]

Attachment: revised patch (interim)

Comment 29

[Mike Kaply [:mkaply]]

Attachment: Hopefully final patch

OK, DoSecurityCheck is a member function.

Removed setting of prefs in destructor, this caused unnecessary work.

Comment 30

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 115937 [details] [diff] [review]
Hopefully final patch

This looks good as far as I know, so sr=heikki

What I am not completely sure about is the unreleased jsval's now. They might
show as leaks on shutdown but that shouldn't really matter, right?

Comment 31

[Mike Kaply [:mkaply]]

Comment on attachment 115937 [details] [diff] [review]
Hopefully final patch

This is a security issue so we need this for 1.3.

Comment 32

[Johnny Stenback (:jst)]

The "unreleased" jsvals won't cause leaks, but they could possibly cause
problems if this code was ever to run in a situation where the JS engine was
"unloaded" and then "reloaded", in such a case these static jsvals would hold
garbage. But there's no way that could happen in todays Mozilla, and we have
other places in the code where we do the same thing, so for now, I say this is fine.

Comment 33

[Asa Dotzler [:asa]]

Comment on attachment 115937 [details] [diff] [review]
Hopefully final patch

a=asa (on behalf of drivers) for checkin to 1.3

Comment 34

[Mike Kaply [:mkaply]]

I put this on 1.3. I'll put it on the trunk on Monday.

Comment 35

[Mike Kaply [:mkaply]]

OK, trunk fix is in. should we close it? Or keep it open for UI pieces?

Comment 36

[Kathleen :Brade]

-->mkaply for resolution / morphing

Comment 37

[Boris Zbarsky [:bzbarsky]]

So wait.  We're giving mozilla.org unfettered access to people's clipboards? 
That's not cool....

Comment 38

[Mitchell Stoltz (not reading bugmail)]

Yes, I think exempting mozilla.org was ill-considerd. Let's remove that exemption.

Comment 39

[Kathleen :Brade]

resolving this bug as fixed; it was fixed long ago