99525 - reset master pwd disable view stored pwd when if they were encrypted

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

[Stephane Saux]

Start a new profile.
visit a log in page (e.g., bugzilla)
select to store the log in info.
select to encrypt the log in info.
quit.
start again.
reset the master pwd.
go to
prefs->priv&sec->web pwd.
"view stored passwords" fails to bring the window that allows you to view stored
web pwds.
Try to change from encrypted to obscure: get a error dialog that one failed to
convert the data.

Note that if you didn't choose to encrypt web pwds, then the feature works fine.

Comment 1

[Stephane Saux]

blocks 96018.

Comment 2

[Stephane Saux]

More testing.
encrypting web passwords and then deleting the cert and key dbs produces the
same results.

What I think is happening is that the master pwd is used to encrypt the web
pwds. When the master pwd is changed (either using the reset master pwd or
deleting the dbs), then whenever the code tries to access the encrypted file, it
fails to decrypt it, and the application doesn't know what to do in that case.

Comment 3

[Stephane Saux]

Ok,
After talking to relyea, my understanding is that each entry is encrypted using
a key and each entry has some kind of key identifier associated with the data.
Potentially we should be able to have each entry use a different key.

Thus for this bug, one of the keys would not be found and the data for that
entry should appear encrypted in the pwd manager.

Another options is to delete entries for which we don't find the key, but that
does  not take into consideration the case when the key was stored in a hardware
token which is inherently portable. The entry should not be deleted, so that a
subsequent request to view the web pwds when the key is available (the hardware
token is attached to the machine) does display/use the info.

Copy morse for comments.

Comment 4

[Robert Relyea]

I would recommend *NOT* deleting those entries just because you could not
decrypt them. You allow the user the option of deleting those entries
themselves. Mostly for the reasons Stephan has already meantioned, but also
because there may be other reasons why a particular key is not immediately
available, but may be available in the future.

Comment 5

[Stephen P. Morse]

This is working fine for me using a build from 9-11.  Let me know if this is a 
regression introduced in the past two days, in which case I'll pull a new tree 
and test it out, but I'd hate to do that unnecessarily.  In other words, can you 
reproduce this problem on an older build or only on todays build?

Comment 6

[(Don't use, old account) - Kai Engert]

Additional thoughts regarding whether to delete entries from the web password db
or not.

If we are going to call ResetToken, we do know which key IDs we will delete. We
can iterate over the token that we will reset, and find all the key IDs that we
will erase.

What kind of key is used to protect the web pwd entries? I guess it is not an
asymetric key pair? I suppose it is just a plain symetric key that lives in a
token, protected by the master password.

If it is a symetric key, the user is unlikely to have a backup of that key,
right? I guess this key is generated internally in Mozilla. I only see a GUI in
Mozilla to export/import certificates, but not for the key that is used to
encrypt the web passwords.

If we know that we are going to call ResetToken on the internal Software
Security Device, and we know that the user will not have a backup of the
symetric keys that we will delete, isn't it safe to delete all web passwords
encrypted with exactly that key?

Is it correct that the database of web passwords must have a key identifier for
each of its entries? It it has, then we know which ones will be unusable after
our call to ResetToken, and could delete just those, not touching other entries
with unknown key IDs.

Comment 7

[Robert Relyea]

You won't be able to do this, because you do know know what the keyID is. In
fact you cannot tell if a given entry is a symetric key or a private key, even
if there was a pkcs #11 interface that allowed you to even get an idea if there
are any keys without first providing the password. All that data is protected
behind your encrypted password.

bob

Comment 8

[(Don't use, old account) - Kai Engert]

morse: I just downloaded the Mozilla Linux build 2001-09-11-06. It does not
work, it behaves as Stephane described, the password manager does not open.

Did you really use "Reset Master Password"? This is a very new feature that was
added last week. It is accessed using Preferences / Privacy & Security / Master
Password / Reset Password.

When I try to open with a current debug build, I see an assertion failure: 
###!!! ASSERTION: NS_ENSURE_TRUE(NS_SUCCEEDED(EnsureScriptEnvironment()))
failed: '(!((EnsureScriptEnvironment()) & 0x80000000))', file
../../../mozilla/docshell/base/nsWebShell.cpp, line 287
###!!! Break: at file ../../../mozilla/docshell/base/nsWebShell.cpp, line 287

It is triggered whenever I try open the web passwords window.

Comment 9

[Stephen P. Morse]

No, I didn't do it from the preferences (I didn't know about that one) but used 
the original one from tasks->privacy->password-manager->reset-master-password.

So why do we have two different mechanisms for accomplishing the same thing?  If 
you want it in the preferences, then can't you call the same routine that the 
tasks-menu calls in order to accomplish this?

Comment 10

[(Don't use, old account) - Kai Engert]

Actually I didn't know the command "Clear Sensitive Information", but we are
talking about two different things.

We want more than just clearing the data in the wallet. We want to erase the
whole security database. Doing this will not erase the stored information in the
wallet, but will make it impossible to decrypt that data, because we erase the
key protecting the wallet.

We just had a meeting discussing what we should do with the wallet data when we
need to reset the security database.

Should we just erase everything contained in the wallet, basically calling both
the PK11_ResetToken and your existing function to clear the sensitive
information?

Or should we not call erase, but leave the wallet as it is, and live with the
fact that there are some entries now undecryptable?

The motivation gave us the motivation for the latter idea: Some entries in the
wallet might be protected with a hardware token, and a user might not want to
lose those entries. And as we can't decide which wallet entries might be
decryptable later, we better delete nothing.

To find the reason why the dialog is not displayed, I looked into
wallet/src/singsign.cpp, SINGSIGN_Enumerate. When si_Decrypt does not work, you
currently just return, not displaying the dialog. We could react differently
depending on the return code of si_Decrypt. If the user entered the wrong master
password, we'd see NS_ERROR_NOT_AVAILABLE.

We could restrict suppressing the dialog to the case of NS_ERROR_NOT_AVAILABLE.
But when we see NS_ERROR_FAILURE, we could just skip this entry. This would
result in the web password manager to contain nothing, even if there are entries
stored (which are now undecryptable).

Implementing "skipping" requires some changes to your enumeration logic, because
we don't know in advance how many entries can be enumerated (=displayed).

Looking at the current implementation I wonder whether there is a safe way to
add skipping as we go.

Do you think this makes sense? If yes, is it ok to change SINGSIGN_Enumerate, or
is changing it's behaviour not allowed? Should we add a new function and only
change nsPasswordManagerEnumerator?

Or do you think we should not bother and simply erase everything?

Comment 11

[Stephen P. Morse]

> Actually I didn't know the command "Clear Sensitive Information", but we are
> talking about two different things.

That's not the command I was talking about.  I was talking about 
tasks->privacy->password-manager->change-master-password.  It is does not erase 
any of the data.  It simply changes the master password and reencrypts all my 
saved data under the new password.  Why can't you do the same thing?

Actually if what you are doing is getting rid of the master password rather than 
changing it, what you want would be tasks->privacy->password-manager->obscure.  
That changes all my saved data from being encrypted (using the master password) 
to being obscured (using no master password).

I would not like to see you delete any of the saved data just because the user 
decided to change or get rid of the master password.

Comment 12

[(Don't use, old account) - Kai Engert]

We are talking about the case the user has *forgotten* the password.

Comment 13

[Stephen P. Morse]

OK, that's what tasks->privacy->clear-sensitive-information is about.  So you 
were correct in what you said above.

In that case we do throw away all the data that was saved.

Comment 14

[(Don't use, old account) - Kai Engert]

morse:

Ok, thanks. I can do that. With your hint that this erase functionality already
exists, I was able to create a patch which I will attach.

But I'd like to ask you: What is your opinion about the alternative
implementation we could use, that I explained above? I.e., not erasing the
wallet, but keeping it, in case it contains some entries the user still might be
able to access protected by a hardware token?

Comment 15

[(Don't use, old account) - Kai Engert]

Attachment: Patch / simple fix strategy / erasing everything

Comment 16

[Stephen P. Morse]

> not erasing the wallet, but keeping it, in case it contains some entries the
> user still might be able to access protected by a hardware token?

Currently that concept doesn't exist.  All the wallet entries are either 
protected by the master password or they are not protected.  We don't have the 
ability to protect some with a hardware token but not others.

As for the patch, r=morse

Comment 17

[Terry Hayes]

Comment on attachment 49296 [details] [diff] [review]
Patch / simple fix strategy / erasing everything

I think we should try to decouple the dependency that is added by
this patch.  This is easy in Javascript (since there are no header
files to include).  My suggested version also folds the getService
call with the QueryInterface.

Replace the "+" lines with:

var wallet = Components.classes['@mozilla.org/wallet/wallet-service;1'];
if (wallet) {
  wallet = wallet.getService(Components.interfaces.nsIWalletService);
  wallet.WALLET_DeleteAll();
}

>Index: mozilla/security/manager/pki/resources/content/resetpassword.js
>===================================================================
>RCS file: /cvsroot/mozilla/security/manager/pki/resources/content/resetpassword.js,v
>retrieving revision 1.1
>diff -u -d -r1.1 resetpassword.js
>--- mozilla/security/manager/pki/resources/content/resetpassword.js 2001/09/08 00:52:21 1.1
>+++ mozilla/security/manager/pki/resources/content/resetpassword.js 2001/09/14 01:51:05 
>@@ -53,6 +53,11 @@
>   var token = pk11db.findTokenByName(tokenName);
>   token.reset();
> 
>+  var wallet = Components.classes['@mozilla.org/wallet/wallet-service;1'];
>+  wallet = wallet.getService();
>+  wallet = wallet.QueryInterface(Components.interfaces.nsIWalletService);
>+  wallet.WALLET_DeleteAll();
>+
>   var bundle = document.getElementById("pippki_bundle");
>   var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService();
>   promptService = promptService.QueryInterface(Components.interfaces.nsIPromptService);

Comment 18

[Robert Relyea]

From an NSS perspective both Kai's and Terry's patches look fine. r=relyea

Comment 19

[David P. Drinan]

Along with Terry's suggestion the patch is good. r=ddrinan.

Comment 20

[Stephane Saux]

adding nsbranch+, review, patch keywords.

Comment 21

[Christopher Blizzard (:blizzard)]

Comment on attachment 49296 [details] [diff] [review]
Patch / simple fix strategy / erasing everything

sr=blizzard

Comment 22

[Marek Z. Jeziorek]

PDT+ per bug discussion

Comment 23

[(Don't use, old account) - Kai Engert]

Patch checked in to both trunk and 094 branch. Closing bug.

Comment 24

[(Don't use, old account) - Kai Engert]

Reopening. The bug is already fixed, but the fix is not optimal.

The above fix clears the wallet (form data & web passwords) in any case.

If the data was not encrypted, but obscured only, the master password is not
required to access the data. Therefore we don't need to clear the wallet.

I'll attach a new patch that clears the wallet only if it was encrypted. The
code is taken from walletTasksOverlay.xul.

Comment 25

[(Don't use, old account) - Kai Engert]

Attachment: New patch, adding check for encrypted wallet

Comment 26

[Stephane Saux]

target 2.2

Comment 27

[Stephane Saux]

remove nsbranch+, PDT+ which doesn't apply to the additional extra fix.

Comment 28

[(Don't use, old account) - Kai Engert]

Steve,

it was suggested that a call to WALLET_DeleteAll should keep the setting,
whether the wallet is obscured or encrypted.

What do you think about this suggestion?

Currently, when the wallet is encrypted, and we call your implementation of
nsIWalletService::WALLET_DeleteAll, the preference is switched to "obscure".

Comment 29

[Stephen P. Morse]

I suppose that would be OK.  In effect, the user is saying that he wants his 
data to be encrypted but no data has yet been saved and no master password 
established, so he'll get asked to create a master password when he next 
attempts to save some data.  Someone should verify that it indeed works that 
way.

Comment 30

[(Don't use, old account) - Kai Engert]

I tested, it does. When I call Wallet_DeleteAll, and enable wallet encryption
manually using the UI, go to a website, enter the login data, I indeed get
prompted for a new master password.

Comment 31

[David P. Drinan]

r=ddrinan.

Comment 32

[(Don't use, old account) - Kai Engert]

Steve, where do you suggest sould we add this modification, to keep the
obscure/encryption flag? Should this be done in the PSM client code or within
your wallet code?

Comment 33

[Stephane Saux]

I believe is should be in the wallet code. We call WALLET_DeleteAll, but so does
the password manager (from the UI). I assume that in both cases, the encrypt
preference would be reset to obscure, which is incorrect.

Comment 34

[(Don't use, old account) - Kai Engert]

I created bug 102709 for the WALLET_DeleteAll fix.

Comment 35

[Christopher Blizzard (:blizzard)]

Comment on attachment 49607 [details] [diff] [review]
New patch, adding check for encrypted wallet

sr=blizzard

Comment 36

[(Don't use, old account) - Kai Engert]

Changing my prefered e-mail address.

Comment 37

[Kai Engert [:KaiE:]]

Checked in to trunk.

Comment 38

[John Unruh]

Verified fixed on trunk Win98 build. On the branch, stored passwords are deleted 
even though they were not encrypted. Adding nsbranch keyword.