667327 - Data Manager Allows Passwords to Be Copied Without Input of Master Password

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[David E. Ross]

If I have already entered my master password during a session, I can reach the Passwords tab in Data Manager.  On selecting the Show Passwords button, I have to enter my master password again to view passwords.  

When I do get the Passwords tab after only one entry of my master password, however, I can select an entry in the right-hand pane, right-click to get the pull-down context menu, and select Copy Password without again entering my master password.  I can then paste the password to an input area, text editor, or any other writable space.  

This indicates a security vulnerability for Passwords & Permissions in Data Manager because the need to input the master password to view actual passwords can be bypassed.  

Note:  Using the Password Exporter 1.2.1 extension to access passwords as implemented in SeaMonkey 2.0.14 and previous versions, I see the same problem.  Thus, this is not something new, merely something newly discovered.

Comment 1

[David E. Ross]

Mozilla/5.0 (Windows NT 5.1; rv:2.0.1) Gecko/20110608 SeaMonkey/2.1

I reproduced this problem operating SeaMonkey in Safe Mode.

Comment 2

[therube]

Don't know when or how many times a Master Password should prompt, but I can confirm the OP's findings.

Mozilla/5.0 (Windows NT 5.1; rv:5.0) Gecko/20110626 Firefox/5.0 SeaMonkey/2.2

Comment 3

[David E. Ross]

Re comment #2:

I suggest the following:  

The master password should be required to see the list of domains for which logon passwords have been saved.  If the master password was already entered previously in the current session (the session ending if profiles are switched), that should suffice.  

Since a user might forget that the master password was previously entered, the master password should be required again to see passwords.  This is a safety measure.  However, having once selected "Show Passwords" and then entered the master password, the master password should not again be required during the current session of the Data Manager.  Merely viewing a different browser tab or a different Data Manager tab should not mean the end of the current session of the Data Manager.

Comment 4

[bugzilla.mozilla.org]

When entering into Password manager the user should be required to enter master password to either view passwords or copy passwords by right clicking & copying password.

Comment 5

[David E. Ross]

Mozilla/5.0 (Windows NT 5.1; rv:11.0) Gecko/20120312 SeaMonkey/2.8

This is still a problem.  Also see bug #571997.

Comment 6

[Edmund Wong (:ewong)]

Attachment: Require master password before allowing password to be copied.

Comment 7

[Edmund Wong (:ewong)]

Attachment: Require master password before allowing password to be copied. (v2)

Comment 8

[Robert Kaiser]

Comment on attachment 612138 [details] [diff] [review]
Require master password before allowing password to be copied. (v2)

Review of attachment 612138 [details] [diff] [review]:
-----------------------------------------------------------------

This basically looks good to me, thanks a lot. I'm a bit weak on how to use those services, though, so I want Neil to give it a look to see if you used this correctly.

Comment 9

[Robert Kaiser]

Comment on attachment 612138 [details] [diff] [review]
Require master password before allowing password to be copied. (v2)

Oh, actually, I have one nit: File style is to not have opening braces on their own line but on the same line as try, if, catch, etc. - so please do the same here.

Comment 10

[[e-mail]]

Comment on attachment 612138 [details] [diff] [review]
Require master password before allowing password to be copied. (v2)

I think it would be pointless to require the master password if the passwords have been made visible. Conveniently this is readily available via the this.showPasswords property. However if you think you can argue the case you should rerequest review.

Assuming KaiRo is going to update his extension, I also think it would be a good idea to keep the code in sync. However, this does mean having to work with older versions of Gecko in which I believe there was a bug in the master password code relating to token.needsUserInit which could be false even though the master password was blank. Our _confirmShowPasswords method works around this by simply calling token.checkPassword("") which will return true for a blank password.

I would therefore appreciate it if you only attempt to login if both of the above checks have failed. Unfortunately as you know the login function throws an exception instead of failing otherwise the code would have been written as:
if (this.showPasswords || token.checkPassword("") || token.login(true)) {
  // Copy selected signon's password to clipboard
}

Comment 11

[Robert Kaiser]

(In reply to [e-mail] from comment #10)
> I think it would be pointless to require the master password if the
> passwords have been made visible. 

Thanks, I knew why I wanted to involve you there. :)
That said, in most cases, people have entered the master password when showing those, will the current code always re-prompt or only if this has been too long out?
Anyhow, the proposed improvements seem like a good idea!

> Assuming KaiRo is going to update his extension

I just posted an update with the patch from here included, but I surely will update the add-on code with any further improvements.

Comment 12

[Edmund Wong (:ewong)]

Attachment: Require master password before allowing password to be copied. (v3)

Comment 13

[Robert Kaiser]

Comment on attachment 613060 [details] [diff] [review]
Require master password before allowing password to be copied. (v3)

Umm this is the same patch as the one before. Please request sr from Neil as well when you attach the correct patch.

Comment 14

[Edmund Wong (:ewong)]

Attachment: Require master password before allowing password to be copied. (v4)

Comment 15

[Edmund Wong (:ewong)]

Attachment: Require master password before allowing password to be copied. (v4)

Comment 16

[[e-mail]]

Comment on attachment 613970 [details] [diff] [review]
Require master password before allowing password to be copied. (v4)

>+    var row = this.tree.currentIndex;
>+    var password = gPasswords.displayedSignons[row].password;
...
>-    let row = this.tree.currentIndex;
>-    let password = gPasswords.displayedSignons[row].password;
>-    gLocSvc.clipboard.copyString(password);
>+    // Prompt for the master password upfront.
>+    let token = Components.classes["@mozilla.org/security/pk11tokendb;1"]
>+                          .getService(Components.interfaces.nsIPK11TokenDB)
>+                          .getInternalKeyToken();
I'm confused. You changed these to var, and then added a let...
Better to leave these using let, this should improve the diff too.

>+    try {
>+      if (this.showPasswords || token.checkPassword(""))
>+      {
>+        this.copySelPassword();
>+      }
>+      else 
>+      {
>+        token.login(true);
>+        this.copySelPassword();
>+      }  
>+    } catch (ex) {
>+      // If user cancels an exception is expected.
>+    }
Please put the try/catch in the else block of the if. Also, you don't need the {}s around the first part of the if.

Comment 17

[Edmund Wong (:ewong)]

Attachment: Require master password before allowing password to be copied. (v5)

Fixed nits.

Comment 18

[Edmund Wong (:ewong)]

Attachment: Require master password before allowing password to be copied. (v6)

Minor changes.

Comment 19

[Edmund Wong (:ewong)]

Attachment: Require master password before allowing password to be copied. (v7) [Checkin: Comments 21 and 22]

Comment 20

[Robert Kaiser]

Comment on attachment 615036 [details] [diff] [review]
Require master password before allowing password to be copied. (v7) [Checkin: Comments 21 and 22]

And given that this could be seen as a potential security problem, I'm approving this for aurora and beta.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/0338581fa753

Comment 22

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 615036 [details] [diff] [review]
Require master password before allowing password to be copied. (v7) [Checkin: Comments 21 and 22]

http://hg.mozilla.org/releases/comm-aurora/rev/9cb63cb40c87
http://hg.mozilla.org/releases/comm-beta/rev/950c8af11431