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

VERIFIED FIXED in psm2.2

Status

P1
major
VERIFIED FIXED
18 years ago
2 years ago

People

(Reporter: ssaux, Assigned: kaie)

Tracking

1.0 Branch
psm2.2
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

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

Comment 1

18 years ago
blocks 96018.
Blocks: 96018
Severity: normal → major
Priority: -- → P1
Target Milestone: --- → 2.1
Version: 1.01 → 2.1
(Reporter)

Comment 2

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

Comment 3

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

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

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

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

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

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

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

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

Comment 11

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

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

Comment 13

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

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

18 years ago
Created attachment 49296 [details] [diff] [review]
Patch / simple fix strategy / erasing everything

Comment 16

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

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

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

Comment 19

18 years ago
Along with Terry's suggestion the patch is good. r=ddrinan.
(Reporter)

Comment 20

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

Comment 22

18 years ago
PDT+ per bug discussion
Whiteboard: Want PDT+ → PDT+

Comment 23

18 years ago
Patch checked in to both trunk and 094 branch. Closing bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 24

18 years ago
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 → ---

Comment 25

18 years ago
Created attachment 49607 [details] [diff] [review]
New patch, adding check for encrypted wallet
(Reporter)

Comment 26

18 years ago
target 2.2
Target Milestone: 2.1 → 2.2
(Reporter)

Comment 27

18 years ago
remove nsbranch+, PDT+ which doesn't apply to the additional extra fix.
Keywords: nsbranch+
Whiteboard: PDT+

Updated

18 years ago
QA Contact: bsharma → junruh

Comment 28

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

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

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

17 years ago
r=ddrinan.

Comment 32

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

Comment 33

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

17 years ago
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+

Comment 36

17 years ago
Changing my prefered e-mail address.
Assignee: kai.engert → kaie
Status: REOPENED → NEW
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 37

17 years ago
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago17 years ago
Resolution: --- → FIXED

Comment 38

17 years ago
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
Keywords: nsbranch

Updated

14 years ago
Component: Security: UI → Security: UI
Product: PSM → Core

Updated

11 years ago
Version: psm2.1 → 1.0 Branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.