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

RESOLVED FIXED in seamonkey2.12

Status

SeaMonkey
Passwords & Permissions
--
major
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: David E. Ross, Assigned: ewong)

Tracking

Trunk
seamonkey2.12
Bug Flags:
in-testsuite -

SeaMonkey Tracking Flags

(seamonkey2.9 wontfix, seamonkey2.10 fixed, seamonkey2.11 fixed)

Details

Attachments

(1 attachment, 7 obsolete attachments)

1.99 KB, patch
Robert Kaiser
: review+
neil@parkwaycc.co.uk
: superreview+
Robert Kaiser
: approval-comm-aurora+
Robert Kaiser
: approval-comm-beta+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
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

6 years ago
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
(Reporter)

Comment 3

6 years ago
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

6 years ago
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.
(Reporter)

Comment 5

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

This is still a problem.  Also see bug #571997.
(Assignee)

Comment 6

5 years ago
Created attachment 612122 [details] [diff] [review]
Require master password before allowing password to be copied.
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #612122 - Flags: review?(kairo)
(Assignee)

Comment 7

5 years ago
Created attachment 612138 [details] [diff] [review]
Require master password before allowing password to be copied. (v2)
Attachment #612122 - Attachment is obsolete: true
Attachment #612138 - Flags: review?(kairo)
Attachment #612122 - Flags: review?(kairo)

Comment 8

5 years ago
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.
Attachment #612138 - Flags: superreview?(neil)
Attachment #612138 - Flags: review?(kairo)
Attachment #612138 - Flags: review+

Comment 9

5 years ago
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 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
}
Attachment #612138 - Flags: superreview?(neil) → superreview-

Comment 11

5 years ago
(In reply to neil@parkwaycc.co.uk 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.
(Assignee)

Comment 12

5 years ago
Created attachment 613060 [details] [diff] [review]
Require master password before allowing password to be copied. (v3)
Attachment #612138 - Attachment is obsolete: true
Attachment #613060 - Flags: review?(kairo)

Comment 13

5 years ago
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.
Attachment #613060 - Flags: review?(kairo)
(Assignee)

Comment 14

5 years ago
Created attachment 613963 [details] [diff] [review]
Require master password before allowing password to be copied. (v4)
Attachment #613060 - Attachment is obsolete: true
Attachment #613963 - Flags: superreview?(neil)
Attachment #613963 - Flags: review?(kairo)
(Assignee)

Comment 15

5 years ago
Created attachment 613970 [details] [diff] [review]
Require master password before allowing password to be copied. (v4)
Attachment #613963 - Attachment is obsolete: true
Attachment #613970 - Flags: superreview?(neil)
Attachment #613970 - Flags: review?(kairo)
Attachment #613963 - Flags: superreview?(neil)
Attachment #613963 - Flags: review?(kairo)
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.
Attachment #613970 - Flags: superreview?(neil) → superreview+

Updated

5 years ago
Attachment #613970 - Flags: review?(kairo) → review+
(Assignee)

Comment 17

5 years ago
Created attachment 615031 [details] [diff] [review]
Require master password before allowing password to be copied. (v5)

Fixed nits.
Attachment #613970 - Attachment is obsolete: true
Attachment #615031 - Flags: superreview+
Attachment #615031 - Flags: review+
(Assignee)

Comment 18

5 years ago
Created attachment 615033 [details] [diff] [review]
Require master password before allowing password to be copied. (v6)

Minor changes.
Attachment #615031 - Attachment is obsolete: true
Attachment #615033 - Flags: superreview+
Attachment #615033 - Flags: review+
(Assignee)

Comment 19

5 years ago
Created attachment 615036 [details] [diff] [review]
Require master password before allowing password to be copied. (v7) [Checkin: Comments 21 and 22]
Attachment #615033 - Attachment is obsolete: true
Attachment #615036 - Flags: superreview?(neil)
Attachment #615036 - Flags: review?(kairo)

Updated

5 years ago
Attachment #615036 - Flags: superreview?(neil) → superreview+

Updated

5 years ago
Attachment #615036 - Flags: review?(kairo) → review+

Comment 20

5 years ago
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.
Attachment #615036 - Flags: approval-comm-beta+
Attachment #615036 - Flags: approval-comm-aurora+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/0338581fa753
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.12
Keywords: checkin-needed
Whiteboard: [c-n on c-a, c-b]
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
Attachment #615036 - Attachment description: Require master password before allowing password to be copied. (v7) → Require master password before allowing password to be copied. (v7) [Checkin: Comments 21 and 22]
status-seamonkey2.10: --- → fixed
status-seamonkey2.11: --- → fixed
status-seamonkey2.9: --- → wontfix
Keywords: checkin-needed
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: [c-n on c-a, c-b]
Version: SeaMonkey 2.1 Branch → Trunk
You need to log in before you can comment on or make changes to this bug.