184436 - password stealing using prompt() or getSelection()

Status: VERIFIED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[Jake Howlett]

User-Agent:       Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.3a) Gecko/20021207 Phoenix/0.5
Build Identifier: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.3a) Gecko/20021207 Phoenix/0.5

The website xrefer.com have a bookmarklet called XreferIt that lets you search
there online reference material. If you have any text selected then the
JavaScript should pick this up and pass it on to Xrefer. Here's the JS:

javascript:var x;self.onerror = errorHandler;
function errorHandler(message, url, line){
if(x==null || x==''){ 
 void(x=prompt('Please enter text to search xrefer - the web\'s reference
engine.',''));
}
if(x!=null && x!=''){
location.href='http://www.xrefer.com/results.jsp?term='+escape(x)+'&source=xreferit';}return
true;
}
if(frames.length == 0){
x=window.getSelection();
}else{
for(i=0;i<frames.length;i++){if(frames[i].frames.length == 0){
x = frames[i].window.getSelection();
}else{
for(j=0;j<frames[i].frames.length;j++){
x = frames[i].frames[j].window.getSelection();
if(x){
break;
}
}
}
if(x){break;}}}if(x==null || x==''){
void(x=prompt('Enter text to search xrefer - the web\'s reference engine.',''));
}
if(x!=null && x!=''){
location.href= 'http://www.xrefer.com/results.jsp?term=' +
escape(x)+'&source=xreferit';
}

This used to work in 0.4 but in 0.5 it seems that 90% of the time "x" works out
to be my proxy password (the only place Phoenix has this particular password
stored).

Reproducible: Sometimes

Steps to Reproduce:
1. Download the bookmarklet from http://www.xrefer.com/xreferit.jsp
2. Configure the browser to surf via a Proxy server.
3. Store your password and continue to use the bookmarklet.
Actual Results:  
Xrefer receives my password via the URL!

Expected Results:  
Not my password!

Comment 1

[Stefan Moebius]

Using Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021207
Phoenix/0.5

I haven't configured a proxy password, however, window.getSelection() returns an
empty string (instead of an undefined). On the other hand,
document.getSelection() returns a password. Reproducibly, this is the first one
from the password list (in the ".s"-file)

Comment 2

[Stefan Moebius]

Attachment: Testcase

It appears that the problem is not only getSelection(), but also the prompt()
method.

Comment 3

[Jesse Ruderman]

This problem was fixed once before, in bug 148520.

Comment 4

[Daniel Veditz [:dveditz]]

Confirming bug, and this happens in Mozilla as well. Since bug 148520 Jesse
references was in singsign.cpp and Darin just rewrote a chunk of that (checked
in 12/5) I'm going to reassign to him.

Comment 5

[Daniel Veditz [:dveditz]]

The original summary "Bookmarklet using prompt() or getSelection() sends my
proxy password" minimizes the problem. Users initiate bookmarklets, but any
script can do document.getSelection() without user action, and then send off the
password it gets.

The password appears to be the first one in the .s file (as mentioned by Jesse
in the older bug). The attacker might not know what site it's for, but any
password could be useful in attempting attacks.

Comment 6

[Darin Fisher]

Attachment: v1 patch

simple, low-risk fix.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 109150 [details] [diff] [review]
v1 patch

fix yup that comment, and add a comment above si_GetURL explaining why passing
null is seriously wrong.

Comment 8

[Daniel Veditz [:dveditz]]

Darin thinks this may be the cause of a mail password regression Bienvenu is seeing.

Comment 9

[David :Bienvenu]

it certainly looks like that would fix the problem we're having in mail/news. I
can try applying the patch to make sure.

Comment 10

[David :Bienvenu]

yes, thx, it does fix the mailnews problem.

Comment 11

[David :Bienvenu]

*** Bug 184571 has been marked as a duplicate of this bug. ***

Comment 12

[Jesse Ruderman]

Stefan Moebius figured out that the password returned is the first in the .s
file, not me.

Comment 13

[Darin Fisher]

*** Bug 184115 has been marked as a duplicate of this bug. ***

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

fwiw, that mail regression that was talked about is the bug 184115 that was
older than this and just marked a duplicate of this one.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 109150 [details] [diff] [review]
v1 patch

>+    // XXX how can this be right?!?
>+    LOG(("  returning first element in the signon list\n"));
>     return (si_SignonURLStruct *) (si_signon_list->ElementAt(0));

boy, I hate leaving this here. This is the second time this
exploit has come up so clearly this odd behavior is a trap
waiting to happen. But this behavior is used by SI_DeleteAll
and SI_RemoveAllSignonData() which pass NULL to si_RemoveUser()
which in turn calls si_GetURL. And knowing a little about this
convoluted code it might not be the only place that's taking
advantage of this magic behavior.

We need to fix that, but let's take the safe fix for now (trying
to get this into 1.3a) and try to clean this up later.

>   primaryUrl = si_GetURL(primaryRealm);
>-  legacyUrl = si_GetURL(legacyRealm);
>+
>+  if (legacyRealm)
>+    legacyUrl = si_GetURL(legacyRealm);
>+  else
>+    legacyUrl = nsnull;

I'm concerned primaryRealm might also be NULL. The code above this
specific exploit prevents this, but there are other paths into
si_GetCompositeURL that don't pre-flight this value. I'd be
more comfortable if you did both values

  primaryUrl = (primaryRealm) ? si_GetURL(primaryRealm) : nsnull;
  legacyUrl = (legacyRealm) ? si_GetURL(legacyRealm) : nsnull;

Comment 16

[Darin Fisher]

dveditz: i think any change like that is risky.  the old code (before
si_GetCompositeURL) allowed for a NULL passwordRealm.  i really don't think we
should risk such a change for 1.3 alpha even if it seems like a safe thing.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 109150 [details] [diff] [review]
v1 patch

sr=dveditz on this patch unchanged. Darin has convinced me of the virtues of
extreme conservatism in this code since it often breaks in unexpected ways due
to all the interrelations.

Comment 18

[Daniel (Leaf) Nunes]

patch landed on the 1_3a release tag, a=asa

Comment 19

[Daniel Veditz [:dveditz]]

Since this will make it into 1.3a (leaf is respinning) and didn't affect
previous milestones I guess this doesn't need to be security-sensitive any more.

Comment 20

[Darin Fisher]

fixed-on-trunk

Comment 21

[Darin Fisher]

i filed bug 185118 "si_GetURL returns first password entry if given a null
argument" as a followup to this bug.

Comment 22

[Ben Goodger (use ben at mozilla dot org for email)]

I'll verify this tomorrow. I can also reproduce. 

Comment 23

[grylchan]

using commercial
2002-12-13-08-trunk xp, linux 2.2
2002-12-13-07-trunk mac 10.1.5

the problem with the Mail password as described in bug 184571
has been fixed.

Comment 24

[David :Bienvenu]

*** Bug 185246 has been marked as a duplicate of this bug. ***

Comment 25

[Ben Goodger (use ben at mozilla dot org for email)]

I also see this as fixed on the trunk. 

Comment 26

[Jesse Ruderman]

*** Bug 185075 has been marked as a duplicate of this bug. ***

Comment 27

[Sander]

*** Bug 184485 has been marked as a duplicate of this bug. ***

Comment 28

[Sébastien Delahaye]

*** Bug 186072 has been marked as a duplicate of this bug. ***

Comment 29

[Jesse Ruderman]

*** Bug 187237 has been marked as a duplicate of this bug. ***

Comment 30

[ron adams]

How would i remove that password from my password manager in Phoenix 0.5? Since
password manager exists, but is inaccessable, what file should i delete to
remove all the stored passwords?

Comment 31

[Stefan Moebius]

You need to remove a file whose name consists of 8 numbers + '.s'.

Comment 32

[jacqui Owens]

Need more information

Comment 33

[Matthias Versen [:Matti]]

>Need more information
For what ?
This bug is fixed and there is no reason for more information.

Comment 34

[Jesse Ruderman]

*** Bug 192986 has been marked as a duplicate of this bug. ***

Comment 35

[Jesse Ruderman]

*** Bug 185075 has been marked as a duplicate of this bug. ***

Comment 36

[Jesse Ruderman]

*** Bug 196216 has been marked as a duplicate of this bug. ***

Comment 37

[jacqui Owens]

I dont quite understand what your e-mail is saying exactly. What is it I am 
supposed to do with the e-mail since it is new? Please understand I dont know 
computers  & I kept getting messages I had a bug so I looked you up & from 
there........ I was kinda lost. Can you please help me to understand what is 
was going on? & what exactly I am supposed to do with the e-mail?

Comment 38

[:Hb]

VERIFIED Windows NT 5.1; en-US; rv:1.9.1.5pre) Gecko/20091019