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.
Severity: normal → major
Priority: -- → P1
Target Milestone: --- → 2.1
Version: 1.01 → 2.1
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.
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.
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.
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?
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.
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
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.
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?
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?
Status: NEW → ASSIGNED
> 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.
We are talking about the case the user has *forgotten* the password.
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.
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?
> 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
From an NSS perspective both Kai's and Terry's patches look fine. r=relyea
Along with Terry's suggestion the patch is good. r=ddrinan.
adding nsbranch+, review, patch keywords.
Keywords: nsbranch+, patch, review
Whiteboard: Want PDT+
Comment on attachment 49296 [details] [diff] [review] Patch / simple fix strategy / erasing everything sr=blizzard
PDT+ per bug discussion
Whiteboard: Want PDT+ → PDT+
Patch checked in to both trunk and 094 branch. Closing bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 2.1 → 2.2
remove nsbranch+, PDT+ which doesn't apply to the additional extra fix.
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".
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.
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.
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?
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.
I created bug 102709 for the WALLET_DeleteAll fix.
Comment on attachment 49607 [details] [diff] [review] New patch, adding check for encrypted wallet sr=blizzard
Attachment #49607 - Flags: superreview+
Changing my prefered e-mail address.
Assignee: kai.engert → kaie
Status: REOPENED → NEW
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago → 17 years ago
Resolution: --- → FIXED
Verified fixed on trunk Win98 build. On the branch, stored passwords are deleted even though they were not encrypted. Adding nsbranch keyword.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.