Closed Bug 953875 Opened 6 years ago Closed 6 years ago

Use toolkit password manager

Categories

(Instantbird :: Other, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: florian)

References

(Blocks 1 open bug)

Details

(Whiteboard: [wanted])

Attachments

(2 files, 1 obsolete file)

*** Original post on bio 434 by John Doe <buggy AT mailinator.com> at 2010-07-03 13:36:00 UTC ***

In the roadmap[1], it says:

> Handle passwords with the Mozilla password manager. Passwords shouldn't be in clear text anymore. 

This is in the roadmap for v0.3, so it should be implemented by now. But passwords are still stored in clear text, and it's trivial to retrieve them (about:config, prefs.js).

Passwords should never be stored unencrypted, and there should be an option to set a master password.

And the wiki should be changed to clarify that this hasn't been implemented yet. If you have such a gaping security hole, at least tell people so they can decide if they want to store their passwords. There should also be a warning "Don't do this if you're on a shared computer or unencrypted partition".


[1] https://wiki.instantbird.org/Instantbird:Roadmap#0.3
*** Original post on bio 434 by John Doe <buggy AT mailinator.com> at 2010-07-03 13:38:56 UTC ***

Oops - I got mixed up with the version numbers. Of course v0.3 is a future version. I apologize.

But I still think there should be a warning.
*** Original post on bio 434 at 2010-07-03 20:04:59 UTC ***

Just wanted to point out that most IM programs store passwords in plain text, Pidgin certainly does. Other ones try to "hide" it somehow. But regardless this will be implemented in 0.3 using the toolkit password manager. So it'll happen. No other program gives a warning, I'm not sure its necessary for Instantbird to.
*** Original post on bio 434 at 2010-09-25 17:49:48 UTC ***

I changed this bug into a request to use the password manager, setting all fields as necessary.
Severity: major → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
Summary: Passwords still in the clear → Use toolkit password manager
Version: 0.2 → trunk
Blocks: 953929
*** Original post on bio 434 at 2010-09-25 17:59:11 UTC ***

Set this bug to block bug 953929 (bio 492) (Adapt Firefox Sync for Instantbird) as we need passwords being handled by the password manager to be able to reuse the existing password syncing code from Firefox.
*** Original post on bio 434 at 2010-09-25 22:50:36 UTC ***

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/public/nsILoginManager.idl

I guess we will need:
addLogin
 when creating an account or when the user sets a password for an account that didn't have one previously.
removeLogin
 when removing an account or when the user sets an empty password for the account
modifyLogin
 when the user changes the password
searchLogins
 when a protocol plugin is about to connect an account
 We want to match both on the account name and on the protocol name.

There needs to be some migration code for the password that are currently stored in the preferences with the account settings.
If we make this a one-time migration (ie. just move the passwords from the preferences to the password manager), we remove the ability for users to switch back and forth between nightlies and the 0.2 release.
If we just copy the password to the new storage and never remove the passwords stored in the preferences, old users will still have the security hole of password stored in plain text.

The best plan is probably to start by maintaining both storages: when loading the list of accounts at the application startup, we copy all the passwords found in the preferences to the password manager, and if the user edits a password, we apply the change in the preferences too.
Once we decide it's ok to no longer have a backward compatible profile with 0.2, we should change this to remove the password from the preferences after it has been added in the password manager.

Possible time to drop the old storage:
- just *before* the next stable release.
Pro: This means people who try pre-release versions will be able to revert to the latest stable release until we release a new stable version.
Cons: Once a profile is used with the new stable version, it's no longer usable with the previous version: the major update can't be rolled back.

- just *after* the next stable release.
Pro: This mean nightly users will always be able to go back to the previous stable version.
Con: Mis-leading impression of security given to users of the new release. Especially if they use the master password feature.


(maybe we should remove the plain-text pref-based storage as soon as the master password is used, even if it's before we remove the plain-text storage read/write support, because by activating the master password feature, the user clearly indicates a concern about password privacy?)
*** Original post on bio 434 at 2010-09-25 22:59:16 UTC ***

Should we work at the same time on the support for not storing the password at all for some/all accounts, and prompt the user at connection time?
(We should not re-ask for the password during automatic reconnections after connection errors.)

What about reworking the new account wizard so that the password is asked only at the last step, which would have "Connect now" and "Connect later" buttons, and a "don't remember password" (unchecked by default) checkbox?
Then we could display the connection progress in the wizard, and close the wizard only once the account is connected. The detail I really like about this is that it allows checking the password is right before storing it. The current UI is a pain when the user doesn't remember for sure the password of the account and has to try a couple of them before finding the right one.

Well, that may be increasing the scope of this bug a bit too much...
*** Original post on bio 434 at 2010-09-25 23:08:58 UTC ***

(In reply to comment #6)
> What about reworking the new account wizard so that the password is asked only
> at the last step, which would have "Connect now" and "Connect later" buttons,
> and a "don't remember password" (unchecked by default) checkbox?
> Then we could display the connection progress in the wizard, and close the
> wizard only once the account is connected. The detail I really like about this
> is that it allows checking the password is right before storing it. The current
> UI is a pain when the user doesn't remember for sure the password of the
> account and has to try a couple of them before finding the right one.
I like this method, and it also allows the code to be simpler (I think) in that the account wizard doesn't really know anything at all about the password. Its all handled by the log in logic (and can be saved on successful login, otherwise its ignored and reprompts for a password until a successful one is given).

I believe this is the method previously used by Thunderbird by the way (before they switched to the new account wizard).
*** Original post on bio 434 at 2010-09-25 23:13:13 UTC ***

Bug 953702 (bio 257) is requesting behaviour as described in comment (In reply to comment #6)
> Should we work at the same time on the support for not storing the password at
> all for some/all accounts, and prompt the user at connection time?
> (We should not re-ask for the password during automatic reconnections after
> connection errors.)

Bug 953702 (bio 257) requests this behaviour.
*** Original post on bio 434 at 2010-09-25 23:30:43 UTC ***

Looking at this again, the API at
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/public/nsILoginManagerCrypto.idl
is probably enough.

We could continue to store the passwords in the preferences (in
.passwordEncrypted instead of .password). It may be better than abusing the
password manager database we don't really need with protocol names instead of
URLs.
*** Original post on bio 434 at 2010-09-26 09:38:29 UTC ***

> There needs to be some migration code for the password that are currently
> stored in the preferences with the account settings.
> If we make this a one-time migration (ie. just move the passwords from the
> preferences to the password manager), we remove the ability for users to switch
> back and forth between nightlies and the 0.2 release.

If there was another release (flo/clokep suggested this in IRC) including secured password storage which would be _otherwise not different from 0.2_, then people would have no reason to change back (or forth and back between) these versions as everything else is what they know from 0.2.
So moving the setting instead of copying and keeping it in sync could actually work.
Blocks: 953702
Whiteboard: [0.3-wanted]
Whiteboard: [0.3-wanted] → [0.3-wanted-beta]
*** Original post on bio 434 at 2011-05-29 23:10:39 UTC ***

Too late to start working on this for 0.3 now.
Whiteboard: [0.3-wanted-beta] → [wanted]
Attached patch Patch v1 (obsolete) — Splinter Review
*** Original post on bio 434 as attmnt 1130 at 2012-01-24 17:31:00 UTC ***

This patch fixes both this bug and bug 953702 (bio 257).

- Passwords are stored in the toolkit password manager (not using the nsILoginManagerCrypto.idl API I described in comment 9 because we now have compatibility requirements with Thunderbird, which already uses the full password manager).
- Passwords stored in the preferences are migrated once. New passwords are not stored in the preferences.
- Master passwords are handled. The user shouldn't be prompted several times in a row if the master password prompt is canceled.
- Users can decide to not store their password (bug 953702 (bio 257)). When an account without any password in the password manager is connected, the user is prompted for the password.
- Consequently, a missing password is no longer an error (the "Error: A password is required" string is replaced with "Entering a password is required" in the account manager). I think this part of the UI still needs to be polished, but it can be handled separately (bug 954651 (bio 1219)).
Attachment #8352875 - Flags: review?(clokep)
Assignee: nobody → florian
Status: NEW → ASSIGNED
*** Original post on bio 434 at 2012-01-25 01:29:45 UTC ***

After applying this patch, when I try to create an account without a password I end up with:
> "No account 8 but there is some data in the buddy list for an account with this > number. Your profile may be corrupted."

After restarting Instantbird I had THREE of that account created, however. Similar results when using a fully clean profile. Any idea what's going on here flo?
Attached patch Patch v2Splinter Review
*** Original post on bio 434 as attmnt 1133 at 2012-01-25 11:13:00 UTC ***

As pointed out in comment 13, there was a problem when creating an account without password. The problem is that the password manager doesn't like saving empty passwords.
Attachment #8352878 - Flags: review?(clokep)
Comment on attachment 8352875 [details] [diff] [review]
Patch v1

*** Original change on bio 434 attmnt 1130 at 2012-01-25 11:13:39 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352875 - Attachment is obsolete: true
Attachment #8352875 - Flags: review?(clokep)
Attached patch InterdiffSplinter Review
*** Original post on bio 434 as attmnt 1134 at 2012-01-25 11:18:00 UTC ***

The change in imAccounts.js was the only change really required to fix the problem with empty passwords. However, I also fixed the account wizard so that it doesn't show empty passwords in the summary, and doesn't attempt to save them (note that even if the account wizard no longer saves empty passwords, fixing handling of empty passwords in imAccounts.js was still required because the account properties dialog can let the user remove an account's password).
*** Original post on bio 434 at 2012-01-25 11:22:27 UTC ***

(In reply to comment #13)
> After applying this patch, when I try to create an account without a password I
> end up with:
> > "No account 8 but there is some data in the buddy list for an account with this > number. Your profile may be corrupted."
> 
> After restarting Instantbird I had THREE of that account created, however.
> Similar results when using a fully clean profile. Any idea what's going on here
> flo?

When I tried to reproduce, the first click on "Finish" in the account wizard failed silently (too bad the wizard binding eats errors... :(), the subsequent clicks on "Finish" caused an error to appear in the console saying I was attempting to create a duplicate account.
After a restart I had only one account created.

Maybe you tested with a protocol plugin that doesn't implement correctly the accountExists method? (jsProtoHelper currently implements it as |accountExists: function() false, //FIXME|)

That would explain the 3 accounts, but not the first error message you pasted. It's possible you do have some profile corruption though :).
Comment on attachment 8352878 [details] [diff] [review]
Patch v2

*** Original change on bio 434 attmnt 1133 at 2012-01-25 12:41:36 UTC ***

With the additional comments sent over IRC:
 - Defaulting JS Protos to requiring a password.
 - Removing the FIXME comment in imIAccount.idl
Attachment #8352878 - Flags: review?(clokep) → review+
*** Original post on bio 434 at 2012-01-26 00:44:46 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/6dbe0da3ffc4 (along with a fix for bug 953702 (bio 257)). Thanks flo!

(In reply to comment #16)
> Maybe you tested with a protocol plugin that doesn't implement correctly the
> accountExists method? (jsProtoHelper currently implements it as |accountExists:
> function() false, //FIXME|)
> 
> That would explain the 3 accounts, but not the first error message you pasted.
> It's possible you do have some profile corruption though :).

We need to file a bug about this and fix it.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
Duplicate of this bug: 953702
Blocks: 954676
You need to log in before you can comment on or make changes to this bug.