Text fields give scripts access to the user's clipboard

VERIFIED FIXED

Status

()

Core
DOM: Events
--
critical
VERIFIED FIXED
13 years ago
10 years ago

People

(Reporter: Wladimir Palant, Assigned: Wladimir Palant)

Tracking

({fixed-aviary1.0, fixed1.7, regression})

Trunk
fixed-aviary1.0, fixed1.7, regression
Points:
---
Bug Flags:
blocking1.7.5 +
blocking-aviary1.0PR +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix] fixed1.7.3)

Attachments

(5 attachments)

(Assignee)

Description

13 years ago
Text fields accept script-generated events. You cannot send Ctrl-C - this is a
menu shortcut, so untrusted events will be rejected. But you can send Ctrl-Ins,
which is implemented in the text field itself. This way you can put every text
you want into the user's clipboard. And you can send Shift-Ins to insert
whatever you have in your clipboard into the text field.

I've tested this on Windows but it should work as well at least with KDE, it has
same shortcuts.
(Assignee)

Comment 1

13 years ago
Created attachment 157492 [details]
Testcase - clipboard read
(Assignee)

Comment 2

13 years ago
Created attachment 157493 [details]
Testcase - clipboard write
(Assignee)

Comment 3

13 years ago
These shortcuts are defined in
content/xbl/builtin/(win|unix)/platformHTMLBindings.xml. XBL only rejects
non-trusted events for chrome content but these text fields are not chrome...
Assignee: nobody → jst
Component: Layout: Form Controls → DOM: Events
Whiteboard: [sg:fix]

Comment 4

13 years ago
Severity critical, because I keep copy&pasting passwords, assume others do as well.
Reproduced both read and write on Mozilla 1.7.x branch on Linux. Only works when
I copied using the explicit "CLIPBOARD" (via menu, shortcut etc.), not the
"PRIMARY" clipboard (just select).
<http://freedesktop.org/Standards/clipboards-spec/clipboards.txt>
Severity: major → critical
Flags: blocking1.8a4?
Flags: blocking1.7.x?
Flags: blocking-aviary1.0mac?
Flags: blocking-aviary1.0PR?
Flags: blocking-aviary1.0?
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 5

13 years ago
Created attachment 157608 [details] [diff] [review]
Proposed patch

This prevents commands from being triggered by non-trusted events, even if the
widget itself isn't trusted.
(Assignee)

Updated

13 years ago
Attachment #157608 - Flags: review?(jst)
Comment on attachment 157608 [details] [diff] [review]
Proposed patch

Seems reasonable to me. r=jst
Attachment #157608 - Flags: review?(jst) → review+
Comment on attachment 157608 [details] [diff] [review]
Proposed patch

>   // See if our event receiver is a content node (and not us).
>   PRBool isXULKey = (mType & NS_HANDLER_TYPE_XUL);
> 
>-  //XUL handlers shouldn't be triggered by non-trusted events.
>-  if (isXULKey) {
>+  //XUL handlers and commands shouldn't be triggered by non-trusted events.
>+  if (isXULKey || (mType & NS_HANDLER_TYPE_XBL_COMMAND)) {

Since there's another (mType & NS_HANDLER_TYPE_XBL_COMMAND) a few lines down
you could define isXBLCommand like isXULKey above for readability. Or not --
your call.

sr=dveditz
Attachment #157608 - Flags: superreview+
Attachment #157608 - Flags: approval1.7.x?
Attachment #157608 - Flags: approval-aviary?
Is there anything that could be legitimately relying on the old behavior? I was
thinking of some sort of QA test harness poking in events, but I guess a chrome
app could create trusted events for that.

Comment 9

13 years ago
Comment on attachment 157608 [details] [diff] [review]
Proposed patch

a=chofmann for the branches
Attachment #157608 - Flags: approval1.7.x?
Attachment #157608 - Flags: approval1.7.x+
Attachment #157608 - Flags: approval-aviary?
Attachment #157608 - Flags: approval-aviary+

Updated

13 years ago
Flags: blocking1.7.x?
Flags: blocking1.7.x+
Flags: blocking-aviary1.0PR?
Flags: blocking-aviary1.0PR+

Comment 10

13 years ago
we run as chrome and i actually have keyhandling off. supporting midas is going
to be so much fun.
Created attachment 157668 [details] [diff] [review]
Patch that was checked in
Fixed on aviary and 1.7 branches, and on the trunk. Marking FIXED.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed-aviary1.0, fixed1.7.x
Resolution: --- → FIXED
Fix checked into the 1.7.2 branch also
Whiteboard: [sg:fix] → [sg:fix] fixed1.7.2+

Updated

13 years ago
Flags: blocking-aviary1.0mac?
Flags: blocking-aviary1.0?

Comment 14

13 years ago
Verified with the two testcases on Mozilla/5.0 (Windows; U; Windows NT 5.1;
en-US; rv:1.7.3) Gecko/20040910
Group: security
Whiteboard: [sg:fix] fixed1.7.2+ → [sg:fix] fixed1.7.3

Updated

13 years ago
Flags: blocking1.8a4?

Comment 15

13 years ago
The test cases still work (i.e. the bug still exists) for me in Linux with the
GTK2/XFT nightly builds of both Mozilla and Firefox; however, it does seems
fixed with the GTK1 builds.
Reopening, as this is still not fixed on linux.
Status: RESOLVED → REOPENED
Keywords: fixed-aviary1.0, fixed1.7.x
Resolution: FIXED → ---
(Assignee)

Comment 17

13 years ago
Just a guess - is this because of bug 257405?
(Assignee)

Comment 18

13 years ago
Looking at the patch - this seems to be the case. DOMEventToNativeKeyEvent()
should return PR_FALSE for non-trusted events.
(Assignee)

Comment 19

13 years ago
Created attachment 160498 [details] [diff] [review]
Patch for the regression

This fixes the regression caused by bug 257405. AFAICT this is the only place
where we need a check.
(Assignee)

Updated

13 years ago
Attachment #160498 - Flags: superreview?(dveditz)
Attachment #160498 - Flags: review?(jst)

Updated

13 years ago
Keywords: regression
Comment on attachment 160498 [details] [diff] [review]
Patch for the regression

Thanks for the quick fix, Wladimir!

r=jst
Attachment #160498 - Flags: review?(jst) → review+
Comment on attachment 160498 [details] [diff] [review]
Patch for the regression

sr=dveditz

bug 257523 landed on the branches, so we'll want this regression fix there as
well.
Attachment #160498 - Flags: superreview?(dveditz)
Attachment #160498 - Flags: superreview+
Attachment #160498 - Flags: approval1.7.x?
Attachment #160498 - Flags: approval-aviary?

Comment 22

13 years ago
Comment on attachment 160498 [details] [diff] [review]
Patch for the regression

a=mkaply for both
Attachment #160498 - Flags: approval1.7.x?
Attachment #160498 - Flags: approval1.7.x+
Attachment #160498 - Flags: approval-aviary?
Attachment #160498 - Flags: approval-aviary+
Fixed on trunk, aviary and 1.7 branches. Thanks for the patch, Wladimir!
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Keywords: fixed-aviary1.0, fixed1.7
Resolution: --- → FIXED
verified with windows 1.7.5 12/15
Status: RESOLVED → VERIFIED
verified with Mac 1.75 2004-12-16-09
(Assignee)

Updated

11 years ago
Assignee: jst → trev

Comment 26

10 years ago
I only see Ctrl-Ins and Shift-Ins mentioned.

Are we taking care of "Shift-Del" which is equivalent to ctrl-x

Also if we can get a test case, it will be great help.
(Assignee)

Comment 27

10 years ago
The fix here is general and takes care of all keys handled by XBL bindings.
You need to log in before you can comment on or make changes to this bug.