87334 - "Ask every time" for password not enforced for SDR operation

Status: VERIFIED FIXED

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

Description

[Terry Hayes]

The SDR operation (secret decoder ring, for saving passwords etc) does not
enforce the "ask every time" option for passwords.

Currently, SDR operations will succeed without asking the user for the password.
 It should put up the password dialog box.

To reproduce:
1) Set the password options to "ask everytime"
2) Go to a web site where a password field has been saved using SDR (the value
is saved, and encryption is turned on for saved values).  The password prompt
should appear prior to the value appearing in the form input box.

Another way to test is to connect to a mail server where the mail account
password has been saved using SDR.

Comment 1

[Terry Hayes]

Attachment: Call correct PK11 function - what about auth failure?

Comment 2

[Terry Hayes]

The attachment has the basic changes necessary to call the correct PK11 function
for enforcing "ask every time". However, the patch also removes the check on the
status of the authentication.  What happens if the user cancels out of the
password confirmation dialog?  Will the operation succeed?  Or will in fail in
the following attempts to encrypt or decrypt.

More testing of this case is necessary.

Comment 3

[Ian McGreer]

I tried this patch.  It asked me for my password every time, but twice each 
time.  That is, I went to a site with a login.  It asked me for my master 
password.  I typed it in.  It filled in my login.  It asked me for my password 
again.  I filled it in again.  It filled in my password (for the site).

Having it ask me for my password twice (once for the web login and once for the 
web password) is completely reproducible with the patch.  Happens every time.  
So this one isn't quite right ;)

Oh, and if I hit cancel, it goes into an infinite loop of asking me for my 
password, even if I type it in.

Comment 4

[Terry Hayes]

Except for the cancel thing, this sounds like the expected behavior.  The wallet
code is asking for two separate decryption operations, one for the login name,
and one for the password.  Each operation requires the user to type in the
master password (according to the pref setting).

To change this, we would need some sort of "transaction", where we verify the
password once for a larger operation, and issue some kind of token to allow
continued access until the high level operation is complete.  PSM can't do this
on its own, it needs wallet to understand this as well.

For fun (!!) you should try saving more than one login/password for a web site,
and see how many password prompts you can get.

---

For the cancel problem, we should track this down.  It may be due to the lack of
a check on the return code (as indicated in the patch), or it may be a different
bug.

Comment 5

[Ian McGreer]

I retract my statement about the cancel behavior.  I must have actually typed 
"foo" in incorrectly, because when I tried it again I was able to make the 
window go away by typing in the correct password.  Nonetheless, if you keep 
hitting "Cancel", it won't go away.

Comment 6

[Bob Lord]

Does this need to get fixed for RTM?

Comment 7

[Stephane Saux]

Looks like the kind of issue that require careful design especially since I 
agree with Terry that a "transaction" model would be the best solution.
It should definitely be fixed for the next release.
So my vote is not for RTM.

Comment 8

[Bob Lord]

Target 2.0 -> 2.1

Comment 9

[Robert Relyea]

Sorry, I should be commenting on this bug:

When you set 'ask every time' there is already serveral cases where you may get
a double password prompt before the operation. This is a known issue with
Communicator, PSM, and NSS. NSS has a transaction based code, but it has never
been used. Anyway the double password prompt is completely familiar to anyone
who actually uses ask every time, so shouldn't preclude being fixed.

BTW Ian, when you get the 'double password', that should be the first time you
authenticate. You should get a single password prompt on all subsequent calls.
(until you explicitly log out again).

bob

Comment 10

[Ian McGreer]

Bob-

I noticed that with client-auth.  The first time I do client-auth, I'm prompted 
twice.  Subsequently, I'm only prompted once.  I stepped through the debugger 
and decided that was okay, since like you said, that's how it's always been.

But Terry's patch makes it so that *every* time you use the Password Manager 
feature, you're prompted twice, once for the login and once for the password.  I 
personally found it rather annoying (why use something that auto-fills passwords 
when you have to do the same amount of typing with it on?).

I actually thought of a hack to work around this.  Since PSM has the slot 
pointer, it could do the following before an SDR operation:

if (slot->askpw == "every time") {
     PK11_Logout(slot);
}

That would force a single authentication for every operation.

Comment 11

[Ian McGreer]

Nevermind.  That would have the same effect as Terry's patch, unless we have 
some way of knowing whether the "login" field or the "password" field is being 
asked for.

Comment 12

[David P. Drinan]

-> P1

Comment 13

[David P. Drinan]

Really P1 this time.

Comment 14

[John Unruh]

Mass assigning QA to ckritzer.

Comment 15

[David P. Drinan]

Setting to P3 after yesterdays bug council.

Comment 16

[Bob Lord]

Feedback from a user:

Netscape 6.1 Problem
 
Primary Browser: 	ntsc61  
Operating System: 	Win2K  
Language: 		English  
Issue Summary: 		Master Password is only required once
Component: 		Security
Doing What: 		Changing preferences  
Severity: 		SomethingDidNotWorkRight  
Can Reproduce: 		Always
Try this URL: 		http://
Issue Detail: 
When my Master Password is set to require the
password every time it is needed, it still only
asks for the password the initial time.  In order
to be prompted for the password again, I must
close all my Netscape windows.  This does not seem
to match the configured settings and presents a
major security vulnerability, as the password is
appears to be cached even though it is configured
not to do so.

Comment 17

[Ian McGreer]

I thought the final decision was to have release notes and/or help notifying the
user of this bug, and also to say that a workaround for "ask every time" is to
set "ask timeout" with a timeout of 1 minute.  Is that not the case?

As a side note, the password is not cached anywhere, it is just not asked for. 
Functionally, that makes no difference, but at least the plaintext password
isn't being stored somewhere.

Comment 18

[Stephane Saux]

Move to future. Won't have time to fix these for 2.1

Comment 19

[Stephen P. Morse]

I wasn't aware of this bug (or even that PSM had this feature) until today.  See 
bug 101637 where I already implemented this feature and checked it in.  And I do 
not have the problem of asking for password multiple times on the same page 
because I log out of the master password each time the page is changed.

So now we have two separate features that accomplish the same thing and we need 
to merge them together.

Comment 20

[Stephen P. Morse]

Attaching patch that integrates this feature with the equivalent one used for 
password-manager.  See bug 101637 for details.

The patch does the following:

1. Removes the equivalent checkbox from the password-manager pref panel
2. Sets the pref that password-manager's checkbox used to set

The pref mentioned in step 2 above is already being tested for on each page load 
and expires the master password at that time.  This is already checked in and 
working.  Unfortunately it is prompting for the master password several times 
per pageload instead of once, but that will be rectified as soon as the latest 
patch in bug 101637 is checked in.

Comment 21

[Stephen P. Morse]

Attachment: Integrates this feature with equivalent one in password manager pref panel

Comment 22

[Stephen P. Morse]

POSSIBLE SECURITY ATTACK: IS THIS OF CONCERN TO US?

If I understand the rationale for expiring the master password after each time 
it is used, it is the following: a user in a shared environment can walk away 
from his machine without having to remember to log out of his master password 
and still be sure that no one can walk up to his machine and use his protected 
data.

But then consider the following scenario:

1. user has master password set to expire after each use
2. user walks away from his machine
3. intruder walks up to machine, changes pref so that master password
   expires once per session, and then walks away
4. user returns, visits a site that requests master password and he provides it
5. user walks away again
6. intruder returns and has full access to all of user's protected data

Note that the equivalent checkbox in the password manager pref panel had code to 
prevent such an attack.   Specifically, it requested the master password 
before allowing the "expire after every use" pref to be unset.  This prevented 
the intruder from performing step 3.

Is this not something that we need to be concerned about?

Comment 23

[Stephen P. Morse]

ddrinan and/or ssaux, please review my patch above
cc'ing alecf for sr

Comment 24

[Stephen P. Morse]

Attachment: forgot to remove item from elementIDs in my previous patch

Comment 25

[Alec Flett]

Comment on attachment 52713 [details] [diff] [review]
forgot to remove item from elementIDs in my previous patch

in the first _elementIDs, aren't you forgetting the elements that are already prefs, such as passwordAskTimes?

Comment 26

[Stephen P. Morse]

I didn't change anything there -- that's the way it always was.  It appears as 
though the security group is using some other mechanism for setting that pref.

Comment 27

[Stephen P. Morse]

still waiting for reviews from alecf and from ddrinan and/or ssaux.

Comment 28

[Alec Flett]

Sorry I didn't respond earlier.. this is the way the prefpanel works: if
_elementIDs is not set, then it walks the whole dom looking for elements with
pref="true" set - which means if you define _elementIDs where it wasn't
previously defined, you're overriding the search, which means the other elements
cannot be found. (the advantage of _elementIDs is speed - it's faster than
walking the DOM)

If in fact you are right that they are using another mechanism, can you at least
verify the mechanism - i.e. explain it in a few words here so we know its safe.

Comment 29

[Stephen P. Morse]

According to ssaux, none of the security things are stored in preferences but 
rather they are put into the security database so they will be secure.  I've 
asked him to add some comments to this bug report as well giving more details 
about it.

Comment 30

[Javier Delgadillo]

Here's a case where we have to keep the application prefs in sync with the prefs
stored in NSS.  So the pref value is stored in 2 places, in the prefs.js files
and the secmod.db file used by NSS.  Whenever the value changes, the Javascript
calls the nsIPK11Token interface to set the apropriate value.  I'm not sure why,
but the value used in the preferences form don't map directly to the values used
internally by NSS.  NSS uses values -1,0, -1 for its 3 states, where the xul/JS
use the values 0,1,2.  I don't know why this decision was made, but that's how
the current implementation works.

It appears that the password pref in question here is a special case due to the
fact that it needs to be synchronized with the app and NSS.

Comment 31

[Rangan Sen]

This patch seems to work ok - r=rangansen for the psm part of it...

Comment 32

[Stephen P. Morse]

cc'ing jag for a review of the non-psm parts
alecf, please sr.  thanks.

Comment 33

[Stephen P. Morse]

really cc'ing jag this time.  please review.  thanks.

Comment 34

[Alec Flett]

Comment on attachment 52713 [details] [diff] [review]
forgot to remove item from elementIDs in my previous patch

Ok, I looked at the current pref-masterpass.xul. The problem here is that there
are 4 other existing prefs in this panel that you're
breaking by /adding/ the _elementIDs:
changePasswordButton
passwordAskTimes
passwordTimeout
resetPasswordButton

You either have to include them in _elementIDs, or not add _elementIDs at 
all. I think jag can back me up here

Comment 35

[Stephen P. Morse]

Attaching a new patch to correct a name conflict (there already was an element 
with an id of "askEveryTime".

Alecf, regarding your list:

   changePasswordButton
   resetPasswordButton
      Regardless of what attributes they have here, these are buttons and not
      prefs.  They never were prefs and have no meaning as a prefs.

   passwordAskTimes
   passwordTimeout
      These indeed could be prefs and it would make a lot of sense.  But the
      security team has chosen not make them prefs but rather make them entries
      in the security database.  The fact that they have the pref attribute has
      no meaning

I agree with you that this module is confusing and should not use the pref 
attributes.  But that's a different issue and should be filed in a new bug 
report if you object strongly.

Furthermore, if I remove the _elementIDs completely as was one of your 
suggestions, then my pref (the only true pref in this module) does *not* get 
remembered.

Comment 36

[Stephen P. Morse]

Attachment: fix up name conflict

Comment 37

[Alec Flett]

but MCD uses these to determine which UI elements to lock... that's why they
have pref="true" and so forth. I'm not sure why yours didn't work without
_elementIDs...  _elementIDs is supposed to speed up the pref setting, not enable it.

Comment 38

[jag (Peter Annema)]

alecf: the conflicting ID could explain that one.

Morse, can you try remove _elementIDs again now that you've fixed the ID?

Comment 39

[Stephen P. Morse]

jag, of course I tried that before I made my last posting.  Fixing up the name 
conflict made no difference as far as the behavior I reported above.

Comment 40

[Stephen P. Morse]

Attachment: Adding all prefs to the elementIDs array per alecf's request.

Comment 41

[Alec Flett]

Comment on attachment 53954 [details] [diff] [review]
Adding all prefs to the elementIDs array per alecf's request.

ok, I'm satisfied :)

Comment 42

[jag (Peter Annema)]

Comment on attachment 53954 [details] [diff] [review]
Adding all prefs to the elementIDs array per alecf's request.

r=jag

Comment 43

[Stephen P. Morse]

reassigning

Comment 44

[Stephen P. Morse]

fix checked in

Comment 45

[John Unruh]

Verified on 10/29 Win2000 trunk.

Comment 46

[Stephen P. Morse]

This has stopped working.  Looks like the pref is getting stuck in the 
ask-every-time state.  See bug 114895.

Comment 47

[Stephen P. Morse]

See bug 148582