Closed Bug 192355 Opened 19 years ago Closed 19 years ago

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

Categories

(Core :: DOM: Editor, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: mkaply)

References

()

Details

(Whiteboard: midas fixed1.3 security)

Attachments

(4 files, 5 obsolete files)

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.
I haven't figured out all of Midas's commands yet, so here's a demo that only
works in IE.
-> brade
Assignee: jfrancis → 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?
Status: NEW → ASSIGNED
Whiteboard: midas
> 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.
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?
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.
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.
Summary: web page can put text on clipboard → web page can put text on clipboard (cut/copy) if in Midas document
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.
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...
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.
This demo works in Mozilla build 2003 022408 on Windows XP.
Flags: blocking1.3?
Summary: web page can put text on clipboard (cut/copy) if in Midas document → web page can put text on clipboard (cut/copy) using Midas
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.
Dan:

So are you voting for totally disallow or add some protection?
Flags: blocking1.3? → blocking1.3+
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.
Attached patch First attempt at a patch (obsolete) — Splinter Review
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 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.
Fixed spacing issues.
Added else cases.
Removed erroneous modify of another else.
Attachment #115765 - Attachment is obsolete: true
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=.
Attached patch Take 3 (obsolete) — Splinter Review
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.
Attachment #115792 - Attachment is obsolete: true
For the record, that is supposed to be an NS_NAMED_LITERAL_CSTRING - I already
made the change.
We need quick turnaround on reviews for this. Time is short for 1.3. 
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 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.
Attachment #115796 - Attachment is obsolete: true
Comment on attachment 115922 [details] [diff] [review]
patch with changes from brade and mstoltz

r=mstoltz.
Attachment #115922 - Flags: superreview?(heikki)
Attachment #115922 - Flags: review+
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).
Attached patch revised patch (interim) (obsolete) — Splinter Review
Attachment #115922 - Attachment is obsolete: true
OK, DoSecurityCheck is a member function.

Removed setting of prefs in destructor, this caused unnecessary work.
Attachment #115931 - Attachment is obsolete: true
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?
Attachment #115937 - Flags: superreview+
Comment on attachment 115937 [details] [diff] [review]
Hopefully final patch

This is a security issue so we need this for 1.3.
Attachment #115937 - Flags: approval1.3?
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 on attachment 115937 [details] [diff] [review]
Hopefully final patch

a=asa (on behalf of drivers) for checkin to 1.3
Attachment #115937 - Flags: approval1.3? → approval1.3+
I put this on 1.3. I'll put it on the trunk on Monday.
Flags: blocking1.3+
Flags: blocking1.3+
Whiteboard: midas → midas fixed1.3
OK, trunk fix is in. should we close it? Or keep it open for UI pieces?
-->mkaply for resolution / morphing
Assignee: brade → mkaply
Status: ASSIGNED → NEW
OS: Windows XP → All
Hardware: PC → All
So wait.  We're giving mozilla.org unfettered access to people's clipboards? 
That's not cool....
Yes, I think exempting mozilla.org was ill-considerd. Let's remove that exemption.
resolving this bug as fixed; it was fixed long ago
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: midas fixed1.3 → midas fixed1.3 security
You need to log in before you can comment on or make changes to this bug.