244907 - NSS needs to handle unprotected keys in tokens.

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Robert Relyea]

Currently NSS believes all private keys in all tokens are protected if the 
token itself has a password. It is possible, however, for tokens to 
contain 'public' private keys, where the private key is not access controlled.
 
NSS needs to be able to find and use these keys without require the user (or 
application) to provide a pin. These keys are already marked in PKCS #11 with 
the CKA_PRIVATE attribute set to FALSE.

Comment 1

[Robert Relyea]

Attachment: Allow use of "Public" private keys

Patch information:
   There are 2 different types of changes in this patch.

1) Changes to authentication before looking up Private keys.
   These changes are those made in pk11cert.c. They delay the PK11Authenticate
call if the token is 'Friendly' even when looking for private keys. Most tokens
hide private objects until the token is authenticated, therefore if we do not
find any private keys, we need to authenticate and try again.

   NOTE: if there is no private key on the token, this code takes now takes 2
tries to determine this, even in the case were the token was authenticated
before hand. It might be worth adding the extra complexity to read the current
authentication state and not do the second find if the token was already
authenticated.

2) Check the CKA_PRIVATE flag before forcing Authentication operations when
doing private key operations.

    These changes are in pk11skey.c Just before we do some operation on the
private key, we make a call 'PK11_HandlePasswordCheck'. This function differs
from PK11_Authenticate in that it handles the application configured password
options. The application can configure NSS to require the password/pin be
reentered every time the key is used, or reentered after a certain timeout
time. PK11_Authenticate() only caused a authentication on a token that needs
authentication, but has not been authenticated yet. PK11_HandlePasswordCheck()
also enforces the application's password requirements. These changes make will
make the applications configured options a no-op for keys that are considered
public.

bob

Comment 2

[Robert Relyea]

I'm submitting these for review now.

One issue to consider is the way CKA_PRIVATE is checked. If CKA_PRIVATE is not 
available (the PKCS #11 module is non-standard), then the code currently treats 
this as 'FALSE'. In this case the "ask every" or "timeout" options will 
erroneously not be active for this key. If the key is also visible without a 
password, NSS may not automatically authenticate before using the key.

This could be fixed by adding a new function "PK11_HasAttributeClear()" which 
returns TRUE if the Boolean is false, and false if the Boolean is true or does 
not exist. The calls PK11_HasAttributeSet() tests would become !
PK11_HasAttributeClear().

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 149448 [details] [diff] [review]
Allow use of "Public" private keys

Several comments, but no r+ or r- yet.	More comments forthcoming.
Please answer my questions below (see comments 4 and later)

Re: pk11cert.c:

1. Lots of new duplicated code that should be a new function instead.

There are about 5 places where a single function call is replaced 
with a new larger block of code, which is duplicated in each of the
places.  The 5 places are in 
   PK11_findPrivKeyFromCert
   PK11_FindCertAndKeyByRecipientList
   PK11_FindCertAndKeyByRecipientListNew
   PK11_FindCertInSlot
   PK11_GetLowLevelKeyIDForCert

Example:

>-    rv = PK11_Authenticate(slot, PR_TRUE, wincx);
>-    if (rv != SECSuccess) {
>-	return NULL;
>+    if (!PK11_IsFriendly(slot)) {
>+	rv = PK11_Authenticate(slot, PR_TRUE, wincx);
>+	if (rv != SECSuccess) {
>+	    return NULL;
>+	}

It would be MUCH better, IMO, to create a new function named
PK11_AuthenticateIfNotFriendly
which takes the same arguemnts as PK11_Authenticate, and which
does the logic duplicated in all 5 places.  Then in those 5
places, you can simply replace one function name with the 
other.	This avoids source code bloat, and, in the event that
you later decide to further tweak this logic, you only have to
tweak it in one place, not in 5.


2. This patch adds 4 places where an operation is attempted, and 
if that operation fails FOR ANY REASON, the code then attempts to
(re)authenticate and try again.  Those 4 places are in 
   PK11_FindPrivKeyFromCert
   PK11_KeyForCertExists
   PK11_FindKeyByAnyCert
   PK11_FindKeyObjectByDERCert

At the very least, those places should check the ERROR CODE returned
by the previous failed call to see if it indicates an authentication
failure (e.g. token not logged in, etc).  We shouldn't be retrying
with authentication for errors that clearly have nothing to do with
authentication.  We shouldn't do an expensive test to see if the 
failure was authentication related.  The answer should be handed to us. 
We shouldn't have to go dig it up.  

This may mean that functions pk11_FindPrivateKeyFromCertID  and 
PK11_MatchItem need to be changed to return meaningful error codes.

3. I'm wondering why we have both PK11_FindPrivKeyFromCert AND
PK11_FindKeyByAnyCert.	I need to look at those functions more 
carefully.  Maybe one should call the other.  (This is not really
a comment about this patch)

4. It appears to me that the changes to functions PK11_FindCertInSlot
and PK11_GetLowLevelKeyIDForCert are solving a DIFFERENT problem than
the one being solved elsewhere in this patch.  This bug is about
access to private keys without authenticating.	But those two 
functions provide access to certs, not priv keys.  So, it looks like
these two changes are intended to eliminate unnecessary authentication
for cert access, which is a separate issue.  (But I agree with these
changes.)


Re pk11skey.c:

5. Question:  In PK11_MakePrivKey, a new call is added to PK11_Authenticate.
Should this be calling PK11_HandlePasswordCheck instead?

The rest of the changes to pk11skey.c look fine.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 149448 [details] [diff] [review]
Allow use of "Public" private keys

Of my 5 comments above, the only one that is serious enough to warrant
an r- is number 2.  I'd like to see #1 changed also, but if that was
my only comment, I wouldn't hold up r+ for it.	But I think comment 2
warrants a change.

Comment 5

[Robert Relyea]

Thanks Nelson, I'll look into all the comments and definately will change 2) 
and probably 1).

bob

Comment 6

[Robert Relyea]

Attachment: Incremental patch on top of the previous patch.

This patch replaces the pk11cert.c portion of the previous patch. It also addes
changes to slot.cpp and secmodi.h.

Differences between this patch and the previous patch:

1) new pk11_AuthenticateUnfriendly() function only authenticates new tokens. 
Note: this patch changes everywhere PK11_Authenticate is called conditional on
the friendly flag, not just the 5 new places. This should address area 1.

2)The conditional retry case now happens checks 3 conditions:
  1) was the search unsuccessful (key was INVALID). -- if we found it we're
done.
  2) was it invalid be cause it wasn't found (PORT_Error() set to
SSL_NO_CERTICATE). -- if token is private an we aren't logged in, the object
will appear to be missing. This short cuts other reasons like problems with the
cert object on the token, device problems, memory allocation, etc.
  3) does the token need to be logged in (because the token is one that
requires loggin and  it is not already logged in) pk11_LoginStillRequired().
This test is somewhat common within pk11wrap, so I've lumpped it into a single
call and replaced the other occurances with this new call. -- this tells us
whether or not we are already logged in. If we are, then there is no point in
Authenticating and trying again (we have already determined that the object is
not present, even when authenticated).

Note that this 3rd check is necessary. There is no way to tell from PKCS #11 if
a private object is present if the token is not logged in. This is true because
many tokens hide the private objects at the token level (they aren't even
visible) until the token is authenticated. 

These changes (proportedly) address nelson's primary concern (#2).

Addressing area 3..

PK11_FindPrivKeyFromCert is a short cut if the application already knows which
token it is dealing with. It only searches the specified token.
PK11_FindKeyByAnyCert "searches" for the Cert in all the tokens. It then uses
the token it found the cert on to look for the key.

Addressing area 4.

Yes, these locations are searching for certs. They are missing the traditional
"isFriendly" check.

Addressing area 5.

This code is really paranoia. With a standard compliant PKCS #11 module, we
should never trip here. Basically private keys, be default, are authenticated
before the get to this point. If a PKCS #11 module had 'private' private keys
that were still 'visible' without authenticating, it would be possible to get
here unauthenticated. The paranoia part is if some code were expecting these
keys to be authenticated (because they always are), that could would break.
Anyway I wouldn't have a problem removing it (though this current patch does
not do so).

Comment 7

[Robert Relyea]

Comment on attachment 150985 [details] [diff] [review]
Incremental patch on top of the previous patch.

Wan-Teh, please also review pk11skey.c from the previous patch.

Thanks, bob

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Bob, please provide a new patch showing diffs against the trunk.
I'd prefer that to the incremental patch.  Thanks.
I will make reviewing that my top priority.
One tiny bit of review feedback:
There are several errors in the block comment preceeding the new function
pk11_LoginStillRequired in pk11slot.c

Comment 9

[Robert Relyea]

Attachment: This is the same as the previous patch plus pk11skey.c from the first patch (plus a comment fix)

Here is the whole patch. By Incremental in the previous comment I meant "here
are the files that had changes", not "here are the diffs relative to the
previous patch". My word choice was poor.

Anyway it's simple enough to give you the combined patch. This patch does vary
from he previous 2 patches in that I adjusted the comment in pk11slot.c nelson
meantioned.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 151129 [details] [diff] [review]
This is the same as the previous patch plus pk11skey.c from the first patch (plus a comment fix)

I'm going to mark this patch r+, with the proviso that the following things
be changed.

>@@ -2345,7 +2371,7 @@
> 	return NULL;
>     }
> 
>-    rv = PK11_Authenticate(*slotPtr,PR_TRUE,wincx);
>+    rv = pk11_AuthenticateUnfriendly(*slotPtr, PR_TRUE, wincx);
>     if (rv != SECSuccess) {
> 	goto loser;
>     }

This call to pk11_Authenticate seems to be unnecessary, since the function
called immediately thereafter, pk11_FinKeyByAnyCert, seems to do the 
authentication and retry logic.


>@@ -2418,10 +2444,10 @@
>     /* at this point, rl->slot is set */
> 
>     /* authenticate to the token */
>-    if (PK11_Authenticate(rl->slot, PR_TRUE, wincx) != SECSuccess) {
>-	goto loser;
>+    rv = pk11_AuthenticateUnfriendly(rl->slot, PR_TRUE, wincx);
>+    if (rv != SECSuccess) {
>+        goto loser;
>     }
>-
>     rl->privkey = PK11_FindKeyByAnyCert(cert, wincx);
>     if (rl->privkey == NULL) {
> 	goto loser;

Same comment here.  This call to pk11_authenticate seems to be unnecessary.



>Index: pk11slot.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v
>retrieving revision 1.78
>diff -u -r1.78 pk11slot.c
>--- pk11slot.c	25 Apr 2004 15:03:12 -0000	1.78
>+++ pk11slot.c	18 Jun 2004 16:17:34 -0000
>@@ -906,11 +906,23 @@
> }
> 
> /*
>+ * Returns true if the token is needs login and isn't logged in.

I think that wants to say "token needs login", or "token is needLogin".

Comment 11

[Robert Relyea]

Checking Log for NSS 3.9 Branch..
Checking in lib/pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.128.4.2; previous revision: 1.128.4.1
done
Checking in lib/pk11wrap/pk11skey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v  <--  pk11skey.c
new revision: 1.84.2.3; previous revision: 1.84.2.2
done
Checking in lib/pk11wrap/pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.77.4.1; previous revision: 1.77
done
Checking in lib/pk11wrap/secmodi.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmodi.h,v  <--  secmodi.h
new revision: 1.14.16.1; previous revision: 1.14
done

Comment 12

[Robert Relyea]

Check-in log for tip:
Checking in pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.132; previous revision: 1.131
done
Checking in pk11wrap/pk11skey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v  <--  pk11skey.c
new revision: 1.88; previous revision: 1.87
done
Checking in pk11wrap/pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.79; previous revision: 1.78
done
Checking in pk11wrap/secmodi.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmodi.h,v  <--  secmodi.h
new revision: 1.16; previous revision: 1.15
done

Comment 13

[Robert Relyea]

Checkin log for Tip:
Checking in nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.132; previous revision: 1.131
done
Checking in nss/nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.67; previous revision: 1.66
done
Checking in pk11wrap/debug_module.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/debug_module.c,v  <--  debug_module.c
new revision: 1.10; previous revision: 1.9
done
Checking in pk11wrap/pk11func.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11func.h,v  <--  pk11func.h
new revision: 1.53; previous revision: 1.52
done
Checking in pk11wrap/pk11skey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v  <--  pk11skey.c
new revision: 1.88; previous revision: 1.87
done
Checking in pk11wrap/secmodt.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmodt.h,v  <--  secmodt.h
new revision: 1.22; previous revision: 1.21
done
Checking in pk11wrap/secmodti.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmodti.h,v  <--  secmodti.h
new revision: 1.18; previous revision: 1.17
done
Checking in util/secerr.h;
/cvsroot/mozilla/security/nss/lib/util/secerr.h,v  <--  secerr.h
new revision: 1.15; previous revision: 1.14
done

Comment 14

[Robert Relyea]

checkin log from comment 13 applies to bug 244914 
Comments 12 and 11 are correct for this bug.

Comment 15

[Wan-Teh Chang]

Sun is also using the NSS 3.9 branch, so anything
we check in on the NSS 3.9 branch must be approved
by a Sun representative.  Let's continue this
discussion in email.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 149448 [details] [diff] [review]
Allow use of "Public" private keys

cancelling review requests on obsolete patches

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 149448 [details] [diff] [review]
Allow use of "Public" private keys

cancelling review requests on obsolete patches (really this time)

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 150985 [details] [diff] [review]
Incremental patch on top of the previous patch.

cancelling review requests on obsolete patches.