78754 - Password manager should have mode to display saved password

Status: VERIFIED FIXED

(SeaMonkey :: Passwords & Permissions, enhancement)

Description

[Terry Hayes]

From Raymond Toy (via n.p.m.crypto newsgroup):

I've been using Mozilla for quite some time, and I really like the password
manager feature to remember my logins and passwords.  However, now that it
remembers my passwords for me, I never remember them so I promptly forget what
passwords I use.  When I use the Password Manager it shows me the sites and the
login ids I used for those sites. However, I can't seem to find a way to get at
the passwords themselves. How do I do this?

Comment 1

[Stephen P. Morse]

There would be much too much opposition to having this because of the 
potential for a privacy breach.

Comment 2

[Andreas Otte]

I once asked for this myself ;-) 

It was the same answer then, but I think the request is still valid. It's really
hard to remember all those passwords with every site you have a login. You would
be lost if you had no such feature as the remembering of passwords. So all is
fine? But what happens if you want to change the browser? Of course who wants to
change the browser once you started to use mozilla?

But I kill my .mozilla directory very often on purpose during testing and start
very fresh, so all the stored data is lost. I would find it very helpful to be
able to access those passwords, maybe protected through an extra password, and
export them somehow, so I can enter them again. Just a thought.

Comment 3

[Terry Hayes]

I think it's too early to "resolve" this request.  If there are privacy
concerns, we should understand what they are, and design the feature to avoid them.

Comment 4

[André Schild]

Why not only let them display when the user uses Encryption to store the
sensitive data.
Since he any way has to enter the "master password" before he gets access to
them, then we can assume that he is the "correct" user and should be allowed to
see them. (Perhaps reask the "master password" before displaying them would be a
good idea....

Comment 5

[Stephen P. Morse]

For the same reason that most programs display asterisks whenever you type in a 
password.  Specifically, there could be someone looking over your shoulder and 
seeing what is being displayed.

Comment 6

[toy]

If security were really important, you wouldn't even show the asterisks.
By doing so, you've reduced the search space many orders of magnitude.

If someone is looking over my shoulder and I'm trying to open a combination
lock, I ask him to go away, don't open the lock, or let him watch.  How is this
any different?

I am at least grateful that this bug isn't resolved or wontfix.  There's some
hope for me!

Comment 7

[Jesse Ruderman]

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

Comment 8

[Jesse Ruderman]

You can get out one password at a time using this bookmarklet:
http://www.squarefree.com/bookmarklets/pagedata.html#show_passwords

Comment 9

[Olivier Calle]

Attachment: Hack patch to see passwords in cleartext

A very simple patch that will show the passwords in the password manager in
cleartext.  I've built mozilla with this change for quite a while and the 2
files involved have been quiescent for a while.

Comment 10

[Stephen P. Morse]

I have several objections to this patch.

1. It is more of a quick hack than a patch.  It is relying on the fact that when 
encryption is used there is the added phrase "(encrypted)" following the 
username, and the  patch is putting the password in place of the parenthesized 
word.  Besides being a hack, this won't work if the password is not encrypted, 
which is the more common case when you are not concerned about security.

2. And if you are concerned about security you would not want someone to see 
your passwords.  You might innocently go to password manager while someone is in 
your office, not realizing that your passwords are going to be shown in clear 
text.  Or you might step away from your machine for a moment without logging out 
of your master password.  With this patch, you've made it easy for an intruder 
to sneak in for an instant and harvest all your passwords.

As module owner, I cannot approve of this patch.

Comment 11

[Olivier Calle]

> 1. It is more of a quick hack than a patch.

I now realize that I did not describe what I submitted clearly enough and I
perhaps should not have attached it as a patch.  As you said, it is just a hack.
 A hack that I made to be able to see my passwords without the harder work of
building some UI to show the cleartext.

> 2. ... Or you might step away from your machine for a moment without
> logging out of your master password. ...

Right, I just wanted to share the hack for those who build mozilla from source,
want to see their passwords in clear, and don't care about / are aware of the
ramifications.
Maybe now I will see if I can learn enough about the UI to see about adding a
"cleartext" button that prompts for the master password regardless of when you
last typed it in...

Comment 12

[Stephen P. Morse]

Oh, that's different.  I thought you were submitting this patch for 
consideration to be checked into the tree.

> Maybe now I will see if I can learn enough about the UI to see about adding a
> "cleartext" button that prompts for the master password regardless of when you
> last typed it in...

A feature like that would probably be acceptable.  In avoids the problem that 
your cleartext passwords will accidentally appear when you didn't want them to.  
Rather than calling it a clear-text button (which implies that your passwords 
are encrypted), have it be a display-passwords button which works whether you 
have encrypted passwords or obscured passwords.

Comment 13

[Martin Sch]

I have written a javascript tool which reports your passwords from
the password manager. But this only works if the passwords are not 
encrypted.

Download http://de.geocities.com/jens_schlatter/test/moztools.jar.
Then for security reasons go offline.
Unpack passwd.html and passwd2.html, open passwd.html in mozilla.

Comment 14

[Brendan]

This is a note to say I think this is a valuable enhancement. 

From my point of view the vast majority of username/password combinations are
there for the benefit of the web site operator, *not* for my benefit. Mozilla
should let *me* decide on what is an appropriate level of privacy protection
rather than telling me the level of protection that's good for me (I don't
dispute that a high level of protection as a default is a good idea).

The way I use passwords I rate as negligible the downside/privacy risk from
having (most if not all of) my passwords visible.  It's more likely that I'll
forget my password and something will go wrong with Mozilla so I can't access
the account or I'll try to let a 3rd party get in but can't get the password or
I'll want to access the account from a different computer (at work or from an
internet cafe while on holidays?). 

At the moment I write my password/username combinations down in a separate book.
 If Mozilla is going to replace that book filling this enhancement request is
essential. Conversely if it's not filled Mozilla has the potential to do far
more harm than good when a user discovers they can no longer access any of their
passwords (hard drive crash? accidental deletion of data file? intentional
transfer of system but forgetting this configuration file).  I think that
without visibility of passwords, this feature of Mozilla is a disaster waiting
to happen. 

Cheers

Brendan 

Comment 15

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

Reassigning to new module owner.

Comment 16

[Marcel Mol]

Anmy progress on this? 
I'm strugling wiht this to. I want to take some of the passwords to a different
system but can't remember then. Most of the times I jsut reregistered at to  
specific website but that is not optimal.
I like the idea to just reask for the master password when a I want to see the
'encrypted' password. Did Oliver (comment 11) manage to build somthing yet?

Comment 17

[Martin Sch]

This does not really belong to the original bug, but: It would be nice ot have
an option (not necessarily in the GUI) to display password field contents as
plain text. Why shouldn't it be possible that passwords show up as plain text in
a password entry field instead of asterisks if I want that? 
If I sit at my PC alone at home, who says that it is forbidden to see my own
passwords while typing them or while they are typed by the password manager??

Comment 18

[larrybird]

Beyond the need to decrypt user/pw info if moving to a differnt browser, it also
is not apparent that a backup of the existing data file can be restored to a
fresh mozilla install using the same master pw.  

IMHO, most people that would like this feature really only want a method
of failsafe backup.  Could not a patch be added to export the user/pw file to
a seperate plain text file then encrypted by blowfish (or other) with the current
master pw?  Were things perfect there would also be an 'import encrypted pw' 
method as well to allow cross use between various mozilla based browsers.  But
at least the file would be a viable backup and usable outside of mozilla

Comment 19

[paxunix]

I've been working on this because it was one of the "things I wish Mozilla did"
most of all.  I have code completed that changes the signon viewer's UI to
toggle the display of saved passwords.

The last thing it needs to be fully functional is to prompt for the master
password if your signons are encrypted.  I haven't been able to figure out how
to prompt for and verify the master password.

Once I have this information, I can submit a patch for review.

(Incidentally, how does one set a master password in Firebird?  Also (and
slightly off-topic), does anyone know why Firebird has no built-in facility on
the Options|Privacy panel to manage certificates?)

Comment 20

[paxunix]

Attachment: Add UI to toggle display of passwords

My first crack at fixing it.  This patch adds a new Password column to the
display as well as a button to toggle password visibility.  If your passwords
are encrypted (there is a master password), you are ALWAYS prompted for the
master password every time you toggle the passwords to visible.  If your
passwords are not encrypted, you will still ALWAYS have to answer Yes/No that
you wish to make them visible.

NOTE: if a master password exists you may be prompted to do so when you click
the "View Saved Passwords" button in the Privacy preferences (depending on how
often your pref is set to ask for your master password).  _This was the
behavior before this patch_.  As such, you will be prompted again when you
toggle the password visibility on.  If you think this is annoying, first
consider the situation where you are only prompted once per session for your
master password.  You enter it in early in the day and later that afternoon you
and a coworker are at your workstation.  Wouldn't you be unhappy if you clicked
"Show Passwords" and suddenly they all appeared?

Comment 21

[paxunix]

It occurs to me that if a site's signon was saved and the site URI at the time
included a username and/or password (e.g. 
http://someuser:[e-mail]/path) then the password will be visible in
the Site column.  My patch doesn't attempt to address that, nor was it addressed
before.  It's probably an unlikely scenario nowadays, but I mention it for the
sake of completeness.  If it should be addressed, I can file a new bug for it.

Comment 22

[paxunix]

Attachment: Login to Software Security Device rather than manually prompt and verify password

I think this is cleaner and more correct.  Instead of prompting for the master
password explicitly then verifying it, let the existing interface for tokens do
both by logging us into the Software Security Device.  Everything else is as
before.

Comment 23

[Vaclav Dvorak]

paxunix: Thanks, this is great! Works like a charm.

One note, though: users without a master password might be confused by the
dialog "You have not set a master password.\n\nAre you sure you wish to show
your passwords?", because the relevance of the fact that no master password is
set is not obvious. I would recommend simply deleting the first sentence.

Comment 24

[Andrew]

Just wanted to report that this works great for me with Firebird cvs.

Comment 25

[paxunix]

Attachment: Fix lack of confirmation prompt when master password is empty

This updated patch fixes the problem where if the user's signons are encrypted
but the master password is empty, there is no login-authentication prompt when
logging into the Software Security Device.  So we have to detect this empty
password case and prompt for confirmation to prevent the surprise of passwords
suddenly becoming visible.  Everything else works as before.

This should work with Firebird and Mozilla since the signon viewer code is the
same for both.

Comment 26

[Vaclav Dvorak]

The patch is great, I'd really love to see it go in. Any particular reason not
to ask someone for a review?

Comment 27

[paxunix]

Comment on attachment 126804 [details] [diff] [review]
Fix lack of confirmation prompt when master password is empty

I agree--I think this patch is ready for review and I'm sure more people would
like to see it go in.

Who to ask for sr=?

Comment 28

[Vaclav Dvorak]

Comment on attachment 126804 [details] [diff] [review]
Fix lack of confirmation prompt when master password is empty

Let's try asking brendan for superreview. Or even an r/sr? Thanks.

Comment 29

[Brendan Eich [:brendan]]

Comment on attachment 126804 [details] [diff] [review]
Fix lack of confirmation prompt when master password is empty

I'm deflecting to bryner, who has been near this code recently, I believe.

/be

Comment 30

[[e-mail]]

Comment on attachment 126804 [details] [diff] [review]
Fix lack of confirmation prompt when master password is empty

taking for review...

Comment 31

[[e-mail]]

Comment on attachment 126804 [details] [diff] [review]
Fix lack of confirmation prompt when master password is empty

>+  document.getElementById("togglePasswords").label = kSignonBundle.getString((showingPasswords ? "hide" : "show") + "Passwords");
Why not getString(showingPasswords ? "hidePasswords" : "showPasswords")

>+      rv = showingPasswords ? signons[row].password : kSignonBundle.getString("hidden");
This is done at paint time so kSignonBundle.getString("hidden") should be
cached in a global.

>+  var commonBundle = document.getElementById("commonBundle");
Don't need this see below.

>+  var prompter = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService();
>+  prompter = prompter.QueryInterface(Components.interfaces.nsIPromptService);
Should use .getService(Components.interfaces.nsIPromptService);

>+  var result = { value: "" };
checkValue is an inout boolean... also this is a dummy parameter, not a result.

>+          commonBundle.getString("Confirm"),
This is the default, just pass in null.

>+                    .createInstance().QueryInterface(Components.interfaces.nsIPK11TokenDB);
.createInstance(Components.interfaces.nsIPK11TokenDB);

>+  <stringbundle id="pipnssBundle"
>+                src="chrome://pipnss/locale/pipnss.properties"/>
You don't seem to use this.

>+  <stringbundle id="commonBundle"
>+                src="chrome://global/locale/commonDialogs.properties"/>
Don't need this see above.

>+                           onclick="SignonColumnSort('password');"/>
Is this wise?

>+                          label=""
Unnecessary. BTW, why don't these buttons have access keys?

Comment 32

[Vaclav Dvorak]

Attachment: patch updated for bitrot and Neil's comments

Here's an updated patch. I made changes according to Neil's comments. (Note: I
am not the original author, I just did the update.) Unless I screwed something
up, it should be against current CVS head. It's made with "cvs diff -u".

The filenames in this new patch are somewhat different than those in the
original: e.g.,
extensions/wallet/signonviewer/resources/content/SignonViewer.js instead of
extensions/wallet/signonviewer/SignonViewer.js. I have no idea why this is, but
it seems correct - at least, I have those files in these locations in a source
tree that I downloaded as 1.6a tar.bz2 and updated with something like "cvs co
mozilla/client.mk; make -f client.mk MOZ_CO_FLAGS=-PA checkout". I have nearly
no experience with Mozilla's CVS and making patches, so I may very well have
screwed up. Handle with care. :-) Any comments are, of course, welcome.

Comment 33

[[e-mail]]

>The filenames in this new patch are somewhat different than those in the original

right, because i moved the files in cvs. their new locations are more sensible ;)

Comment 34

[[e-mail]]

Comment on attachment 135681 [details] [diff] [review]
patch updated for bitrot and Neil's comments

There's no need to try to extract the password from the URI - the whole point
of the password manager is to save the password. And I still don't think that
the password column should be sortable.

Having tried it for real I'm not convinced with the <hidden> text, I think the
button should show/hide the column instead.

Comment 35

[Vaclav Dvorak]

Attachment: modified patch with column hiding

Okay, this version hides the whole column istead of substituting the "<hidden>"
text. Also, I fix some whitespace that I don't like. Otherwise, no
modification.

Any particular reason for not allowing the password column to be sorted? It's
certainly not a very important feature, but then again why take it away unless
it raises a security concern? Does it?

As for extracting the password from the URI: to be honest, I don't really
understand the purpose of this part of the code, but are you sure? The original
code extracts the username. If that's necessary, I'd expect the password to be
necessary, too. But perhaps extracting the username isn't needed either...

Comment 36

[Michiel van Leeuwen (email: mvl+moz@)]

In my password file, i have an entry for imap://user@mailhost, and no seperate
username field, but it has a password field.
But as the purpose of the password manager is to store password, it can't hva an
entry for imap://user:password@mailhost, because that means the caller
(mailnews) already knows the password. Why would it ask wallet then?
So the code to extract the username is needed, but you don't need to add the
password part.

Comment 37

[Vaclav Dvorak]

Attachment: yet another update of the patch: removed the extraction of password from URL

Ah, now I understand. So here's another version, with the password extraction
from URL removed. I still modified that part of the code though, made it a bit
shorter and removed the unnecessary "username" variable. I also removed a
variable called "unused" that was - well, unused. I have no idea why it was
there.

Comment 38

[Vaclav Dvorak]

Attachment: a corresponding patch for the help file

I realized that I should also change the help file accordingly, so here's my
attempt at that. Coincidentally, Neil is also a peer of the help system so he
could review this, too - thanks, Neil. The patch makes identical changes to
both identical files passwords_help.html and passwords_help.xhtml - I don't
know how I'm supposed to handle these. Note that I'm not a native speaker so
feel free to improve the wording.

Comment 39

[[e-mail]]

Comment on attachment 136128 [details] [diff] [review]
yet another update of the patch: removed the extraction of password from URL

Only a couple of minor issues with this patch:

>+  document.getElementById("passwordCol").setAttribute("hidden", showingPasswords ? "false" : "true");
You can probably use .hidden = !showingPasswords; here.

>@@ -335,24 +394,24 @@
>@@ -432,24 +491,24 @@
>@@ -537,24 +596,24 @@
You shouldn't have bothered with these irrelevant whitespace changes; the ones
above are OK because they're near code you're patching.

Comment 40

[Vaclav Dvorak]

Attachment: final patch (hopefully)

Neil's comments addressed. Thanks, Neil.

Comment 41

[[e-mail]]

Comment on attachment 136191 [details] [diff] [review]
final patch (hopefully)

>-      var username;
>       try {
>-        username = ioService.newURI(host, null, null).username;
>+        user = ioService.newURI(host, null, null).username;
>+        if (user == "") {
>+          user = "<>";
>+        }
>       } catch(e) {
>-        username = "";
>-      }
>-      if (username != "") {
>-        user = username;
>-      } else {
>         user = "<>";
>       }
>     }
Oh, I forgot to mention that I didn't think this change was necessary, but it's
no worse your way, so I'll let it slide...

Comment 42

[Vaclav Dvorak]

Comment on attachment 136191 [details] [diff] [review]
final patch (hopefully)

Funny, these mid-air collisions. I hope I won't clear Neil's plus by clearing
my question mark on the review flag.

I confirm that the change mentioned in comment 41 is, indeed, unnecessary: it
does the same thing as the original, it just spares a few cycles in a loop.

Comment 43

[[e-mail]]

i'd recommend bryner or dveditz to superreview this. hyatt is likely to be a
black hole for getting superreview on this patch...

Comment 44

[Vaclav Dvorak]

Comment on attachment 136191 [details] [diff] [review]
final patch (hopefully)

I'm choosing dveditz as there wasn't any response from bryner since comment 26.
Thanks for the advice, Dan.

Comment 45

[Vaclav Dvorak]

Sorry, I meant comment 29.

Comment 46

[Phil Stracchino]

This is a feature I've wanted for a long time.  Yes, absolutely, protect it,
require entry of my master password before allowing viewing of actual passwords,
but for heaven's sake give me the capability to view them in case I need to use
them in some other fashion than through Mozilla.  Any write-only storage is bad,
mm'kay?  Whose passwords are they, after all?

I recently had a hell of a headache with Dotster, because my password was
memorized for their front page *only*, and they put some complex piece of
Javascript on their front page to detect whether you have Flash installed which
crashes Mozilla on Linux.  So I couldn't load the front page to log in there,
and Mozilla wouldn't fill in the password anywhere else.  I had no choice in the
end but to request a password change.

Comment 47

[Vaclav Dvorak]

Asking for 1.7b blocking and hoping that it will perhaps motivate bryner (or
someone else) to have a look at the patch, especially if the blocking flag
request will be granted.

It's really frustrating to have a patch and be unable to get it reviewed. And it
doesn't exactly provide motivation for a developer to start hacking Mozilla. :-(
Any suggestions on how to proceed? Thanks.

Comment 48

[Vaclav Dvorak]

Cool - thanks, Brian!
Now, I don't have checkin privileges, so anyone of you who read this, if you do,
could you check this in? Thanks!

Comment 49

[Vaclav Dvorak]

Aaargh, don't attempt to check in. It seems that the checkin of attachment
133756 [details] [diff] [review] of bug 186237 broke this - I assume the patch won't apply. Will try to
provide updated patch till tomorrow.

Comment 50

[Martin Sch]

The following addon displays a new password column in the password manager and
works very fine:
http://hskupin.info/mozilla/listpasswords.php

Comment 51

[Vaclav Dvorak]

Attachment: updated final patch

Final patch updated for current CVS head.
I assume that I don't need a new review or superreview as this is really just
an update, the patch is essentially the same except for line numbers and some
whitespace.
My Mozilla is building right now, after I test everything I will let you know.

Comment 52

[Vaclav Dvorak]

I can't seem to get encrypted passwords working with today's build, so I
couldn't really properly test the patch, but the part that I did test works.
Someone, could you please check it in? Thanks.

Comment 53

[Asa Dotzler [:asa]]

we're not going to block this release for a feature request. If this doesn't
land before the freeze, feel free to request driver approval for landing during
the freeze.

Comment 54

[Michiel van Leeuwen (email: mvl+moz@)]

Took the libery of checking this in.

Comment 55

[Michiel van Leeuwen (email: mvl+moz@)]

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

Comment 56

[Vaclav Dvorak]

Michiel: thanks for the checkin.

Comment 57

[Jeremy M. Dolan]

Is there a bug for porting this UI to firefox?

Comment 58

[Vaclav Dvorak]

(In reply to comment #57)
> Is there a bug for porting this UI to firefox?

I'm not aware of that. Feel free to file one. (On the other hand, I know little
about Firefox or its development, so don't trust blindly what I say.)

Comment 59

[Jeremy M. Dolan]

If mozilla now simply displays stored passwords automatically, for users without
a master password, what is the point in storing them in obscured format in the file?

Comment 60

[Ricardo Almeida]

Attachment: Passwords won't show

I'm using Mozilla 1.7b, and as can been seen in the attachment, the passwords
don't appear :-(

Comment 61

[Patrick]

(In reply to comment #60)
> Created an attachment (id=145165)
> Passwords won't show

w/o being aware of this comment i already filed a separate bug for this issue,
see bug 241110.