269581 - NSS calls C_GetAttributeValue unnecessarily when token is logged in

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

Some changes were implemented recently in NSS 3.9.2 to allow using smartcards
that had RSA private keys without the CKA_PRIVATE attribute, ie. they didn't
need the token to be logged in . The checks look like :

    if (!PK11_HasAttributeSet(slot,wrappingKey->pkcs11ID,CKA_PRIVATE)) {
	PK11_HandlePasswordCheck(slot,wrappingKey->wincx);
    }


This has a significant performance impact in the SSL server-side codepath,
because C_GetAttributeValue is now being called on the RSA private key for each
full SSL handshake to check if we need to login to the token to use the key,
even though the token is already logged in in most cases.

The optimization is to check PK11_IsLoggedIn() first, and if true, skip the
password check. This needs to be done in a number of places.

Comment 1

[Julien Pierre]

Attachment: patch for the tip

Comment 2

[Julien Pierre]

Attachment: patch for NSS_3_9_BRANCH

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

The logic of these patches appears OK to me, but they seem not to follow 
the usual NSS coding style conventions, e.g. lines much longer than 80 
columns and if statements with the opening brace on the following line.
So, please fix the coding style of the patches.  
And then please ask Bob Relyea to review them.  Thanks.

Comment 4

[Robert Relyea]

Comment on attachment 165767 [details] [diff] [review]
patch for the tip

The change to pk11akey.c is fine, but the changes in pk11obj.c and pk11skey.c
change the semantics of NSS. PK11_HandlePasswordCheck may require a new
authentication of the password has 'timed out'. 

A better idea might be to cache the value of CKA_PRIVATE in the private key
structure in PK11_MakePrivKey and only checked that cached value.

bob

Comment 5

[Robert Relyea]

OK, now that I think about it, servers never allow reauthentication anyway
(typically all the tokens are authenticated on startup with no provision for
reauthentication later). We could really improve things by setting an NSS
'attribute' called 'no reauthentication'. In that case we would never check
'isLoggedIn' in NSS, never call PK11_Authenticate, (and won't need to check
CKA_PRIVATE in the places identified in this patch).

bob

Comment 6

[Julien Pierre]

Bob,

Actually, it may still be that a logout & reauthentication happens in the server
case, accidentally, say, through an NSAPI plug-in, or java servlet running
inside the server logging out the token ... But it isn't a common case and I
don't know if we want to support it.
I agree the optimization you suggested would be good to have if we could
guarantee that logins never need to be done again, and that the state of the
cached information in the private key structure never changes, which would
require additional locking for the object.

Nelson : would we have a way at the SSL API level to know this upfront so we
could set that bit without somewhere needing the user to call a new API to tell us ?

Another possible solution would be to just try using the key, and only login if
we get CKR_USER_NOT_LOGGED_IN, rather than trying to check if we are logged in
upfront. But that may break compatibility for modules that return the wrong
error code ...

Comment 7

[Julien Pierre]

Answering my own question originalyl for Nelson here :

I noticed that SSL_ConfigSecureServer is making a copy of the private key during
setup. One optimization is to add a bit to that copy of SECKEYPrivateKey, which
can then be looked up later at use time to skip the check. I'm going to add a
patch that uses this optimization.

FYI, the cost of a C_GetAttributeValue with the NSS softoken is insignificant,
but with another softoken I am testing with, which is multiprocess safe for
read/write, the cost of C_GetAttributeValue is very significant due to disk I/O,
and it makes that token completely unusable for SSL, so we need to avoid this
call in the SSL server codepath.
 
However, I found the call to PK11_IsLoggedIn is also not free and has negative
impact when using the NSS softoken of about 3%, so some sort of caching would be
better.

Comment 8

[Julien Pierre]

Attachment: new optimization for tip . Relies on SSL_ConfigSecureServer

This patch could still be improved to login in case we get
CKR_USER_NOT_LOGGED_IN from the following operation.

Comment 9

[Robert Relyea]

> FYI, the cost of a C_GetAttributeValue with the NSS softoken is insignificant,
> but with another softoken ... the cost of C_GetAttributeValue is very
> significant...

 I expected that was the impedious behind the change. That is way I thought caching

> However, I found the call to PK11_IsLoggedIn is also not free and has negative
> impact when using the NSS softoken of about 3%

I was also afraid of that. In general I would expect IsLoggedIn (though
IsLoggedIn caches it's last known state for about 1-3 seconds just like
IsPresent does) to be as expensive or more expensive than a GetAttributeValue
call. In the server case, the HasAttributeSet call will always return 'true', so
we then drop into PK11_HandlePasswordCheck which, among other things, calls
IsLoggedIn again. Your current patch is probably the simplest and most efficient
way for servers to operate.

I need to double check that we've handled all the required paths for adding a
new member to PK11PrivateKey before I give it a r+. Currently my only nit would
be I'd like the name of the bool to be something like "never reauthenticates"
which we can then later add a call to set or clear the value on the key from the
application (In case we have client apps which never reauthenticates, or in case
we have server apps that need to).

bob

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 166072 [details] [diff] [review]
new optimization for tip . Relies on SSL_ConfigSecureServer

This patch changes the behavior of libSSL for ALL programs that use it to act
as
an SSL server.	It changes those products so that they will no longer attempt
to (re)authenticate to the token if the token decides it is no longer in a 
logged-in state at any time after SSL_ConfigSecureServer returns.

That may be an acceptable change for commercial https server products running 
with commercial-grade RSA accelerators.  But consider a program that acts as a
low-volume server, and uses an inexpensive personal crypto token that happens
to require re-authentication to the token every 5 minutes.  Such a program
would work OK with NSS as it is today, and would stop working with this patch.

So, I think this is an unacceptable semantic change to libSSL.

Comment 11

[Julien Pierre]

Nelson,

Unfortunately, continuing to support the smartcard SSL server case is quite
complicated. There are 2 approaches :

1) have a configurable NSS setting for this optimization

This would have the disadvantage that it's one more application call to make.
And what's the right default ? Prior to 3.9.2, none of this code that we are
trying to optimize here existed .

The setting could be a global one (for all of pk11wrap), or for individual SSL
server sockets.

2) first try the operation without login, and then if it fails with 

CKR_USER_NOT_LOGGED_IN, login and try again .

Looking at the current code, most functions involved here do not return a CK_RV
. There is a lot of work needed to propagate the need to look for this error all
the way from the top layer of pk11wrap to the bottom function . Then we have to
do this twice, once for the tip and another for the 3.9 branch, which have
already diverged because of the 3 files vs 1 . I wish we hadn't put the
smartcard changes into the 3.9 branch in the first place, but I was trying to be
nice to everybody and did not veto them .

This is not a foolproof solution, as it requires the PKCS#11 vendor modules to
give us the specific error we expect, which may not always be the case. If a
different error is returned by some vendors for this case, we wouldn't try
again, and so the SSL server on a smartcard could still fail if a token requires
relogin.

Comment 12

[Robert Relyea]

Juliens plan for trying first then logging won't work without a lot of futzing
because:

1) The functions we are calling not only check and authenticate if necessary,
they also force authentication in the case of timeouts, or 'ask every'
semantics. These functions need to be called before we attempt the operation on
the token.

2) In some of the cases we can't tell the difference between failure and
non-existant keys. C_FindObjects* does not return CKR_USER_NOT_LOGGED_IN if you
call it when you aren't logged in and looking for private keys. Private keys are
just not visible if you aren't logged in (the PKCS #11 module may not even know
if the private keys exist until you log into the token).

---------------------------------

We can alieviate the performance issue that prompted this bug by caching the
value of CKA_PRIVATE in the private key. On servers the private key object is a
long-lived object, so we'll get the full benefit from doing the caching.

Longer term, I still see benefit from identifying that the given application
does not do reauthentication and cutting out all our authentication checks. New
servers can access this performance improvement by calling a new function to set
this option or adding a new flag to NSS_Init (I think I prefer the latter).

bob

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

If we try to "remember" that a token is authenticated in the private key
structure, and the token spontaneously enters an unauthenticated state,
without removal and insertion, then the "cached" information becomes wrong
and leads to failures in cases that today recover transparently and 
automatically.  Remember that not all servers are "enterprise" servers.
Some are AIM clients.

Comment 14

[Julien Pierre]

Nelson, Bob, we need to come to an agreement on a resolution for this problem.
At the very least we need to agree on the default behavior.
The problem is technically a side effect of the changes that went in 3.9.2 . The
module was written before those changes and worked properly then. 
If it requires a configuration option to select the proper behavior, this will
require changes to existing applications, including possible calls to new APIs,
which is a big no-no at Sun in patch releases. So, I would advocate that the
default behavior should be the pre-3.9.2 one, and the re-login should be enabled
explicitly in SSL servers, only for those few applications that truly need it.

Comment 15

[Julien Pierre]

I spoke to Nelson about this. The issue is that we don't want to always cache
login state as many tokens could log themselves out, even in SSL servers . So, I
marked the last patch as obsolete. I think it should still be possible to do
this still, but not by default. This could perhaps be a new SSL socket option,
as it avoids the heavy cost of trying to login again, for those tokens that the
user knows won't log themselves out . I will open an RFE on this .

Regarding the issue of C_GetAttributeValue, we should be able to cache the value
of the CKA_PRIVATE attribute of the server's private key, so that we don't call
C_GetAttributeValue again during the SSL code path .

However, it seems that we are copying the private key for each server socket in
SSL_ImportFD, and thus get different PKCS#11 key ojects, and would need to call
C_GetAttributeValue again on this new key object once per socket, so we wouldn't
have won anything. This is a much more serious problem that's related to bug
274518 . More investigation on this issue is needed .

Comment 16

[Robert Relyea]

Nelson, actually I was talking about caching the CKA_PRIVATE value (as Julien
suggested), not the is logged-in. The CKA_PRIVATE value almost certainly lives
as long as the key, and is independent of token removal and reinsertion.

The copy is only an issue if we actually copy the underlying keys (which doesn't
happen for token keys). For session keys, it would be an excellent idea to avoid
the C_CopyObject, which I guarrentee is much slower than the C_GetAttribute call;).


bob

Comment 17

[Julien Pierre]

Attachment: cache the CKA_PRIVATE attribute on private keys

I am now implementing Bob's suggestion to cache the value of CKA_PRIVATE . I
have verified that it works with the token in question .

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 174553 [details] [diff] [review]
cache the CKA_PRIVATE attribute on private keys

Here are some preliminary comments on the patch to just one of the 
source files:

>Index: cryptohi/keythi.h

>+typedef enum {
>+    KeyUndetermined,
>+    KeyNotPrivate,
>+    KeyPrivate
>+} PrivateFlagInfo;
>+

Since this is a public enum in a public header, all the 
constants should have numeric values affixed.  
Also comments would be good.  , e.g. 

>+    KeyUndetermined = 0,
>+    KeyNotPrivate   = 1,  /* CKA_PRIVATE not CK_TRUE */
>+    KeyPrivate      = 2   /* CKA_PRIVATE is CK_TRUE */


> /*
> ** A generic key structure
> */ 
> struct SECKEYPrivateKeyStr {
>     PLArenaPool *arena;
>     KeyType keyType;
>     PK11SlotInfo *pkcs11Slot;	/* pkcs11 slot this key lives in */
>     CK_OBJECT_HANDLE pkcs11ID;  /* ID of pkcs11 object */
>     PRBool pkcs11IsTemp;	/* temp pkcs11 object, delete it when done */
>     void *wincx;		/* context for errors and pw prompts */
>+    PrivateFlagInfo privateflag;  

Suggested comment:  /* cached value of CKA_PRIVATE */

While we're at it ... we can anticipate the need to know the value
of the CKA_SENSITIVE attribute also.  Should we cache it too at 
this time?  Instead of an enum for each new boolean, should we 
perhaps have a PRUint32 that contains individual bits that 
show the state of sensitive, private, and a bit that means 
"this cache of attributes is valid"?

Comment 19

[Julien Pierre]

Nelson,

Thanks for the comments. I agree with your proposed changes about the enum
explicit values and the comment descriptions of the fields and values.

On the subject of caching other attribute values, we can do it with a bit field,
as long as we use two bits for each attribute. We really need 1 bit to specify
whether there is a cache, and another bit to specify the value . Thus, with a 32
bit integer, we could cache 16 attributes. That's probably enough for the
foreseeable future. I will produce a new patch that uses a bit field with only
the first 2 bits specified, instead of the enum with 3 defined values.

We can add the other attributes, such as CKA_SENSITIVE, at the time we find that
we need to cache them, by defining additional bits. Currently, with SSL3 and the
RC4 with SHA1 cipher suite, my patch causes all C_GetAttributeValue calls to be
eliminated, so it has met its purpose.

Comment 20

[Julien Pierre]

Attachment: more generic patch, to allow caching more attributes in the future. Uses bitfield

Comment 21

[Julien Pierre]

Comment on attachment 174553 [details] [diff] [review]
cache the CKA_PRIVATE attribute on private keys

Canceling obsolete review requests

Comment 22

[Julien Pierre]

Comment on attachment 174636 [details] [diff] [review]
more generic patch, to allow caching more attributes in the future. Uses bitfield

Bob, Nelson, please review.

While writing this patch, I preserved the logic in pk11skey.c which calls
PK11_PassWordCheck if CKA_PRIVATE is not set.

However, I can't help but think that the logic is reversed, and we should do
the opposite, ie. call the password check function if it's CK_TRUE.

Comment 23

[Robert Relyea]

Comment on attachment 174636 [details] [diff] [review]
more generic patch, to allow caching more attributes in the future. Uses bitfield

In general, I like the idea of the patch. I have the following concerns:

Must fix: 

1) Around lines 195 in keythi.h.

2^x returns a bit-wise XOR, not a power of 2. (1<<(x)) will do want you want.

Please Fix:

1) around line 666 and similiar lines in pk11obj.c.

It would be nice to colapse all the if statements into a single function or
macro. Obviously to colapse it to a single function would require extensively
changing the patch, so a macro would be fine.

Thing about:

We really only need one "isCached" bit. We should read all anticipated
attributes at once (making a single PKCS #11 call is more effecient than
multiples).

bob

Comment 24

[Robert Relyea]

RE comment 22.
CKA_PRIVATE is a required attribute. If it doesn't exist should we prompt or
not? I think I agree with Julien that the answer is we should prompt.

bob

Comment 25

[Wan-Teh Chang]

Comment on attachment 174636 [details] [diff] [review]
more generic patch, to allow caching more attributes in the future. Uses bitfield

The CachedAttribute and CacheAttribute macros are
only used once.  Why do you define them?

The way you use ## in the four macros is unusual.
This may become a maintenance issue.

The four macros have names that do not look like
macros in NSS code (whose names usually use all
capital letters).  People may be surprised when
they step through the code in a debugger or browse
the code in LXR.

>+#define IsAttributeCached(key,attribute) (0 == (key->staticflags & attribute##_Cached) ? \
>+                                        PR_FALSE : PR_TRUE)
>+
>+#define IsAttributeSet(key,attribute) (0 == (key->staticflags & attribute##_Set) ? \
>+                                        PR_FALSE : PR_TRUE)

You can define these two macros without using the "? :"
operator.

Comment 26

[Robert Relyea]

re comment 25

Wan-Teh, I think Julien is right making these macros for two reasons: 1) making
them macros makes the code self-documenting, and 2) he's looking to the (near)
future when we may want to add additional bits.

I agree with Wan-Teh's comment about the macro names (Convention in NSS is
supposed to be ALL_CAPS for macros, though I bet we could find exceptions ;).

bob

Comment 27

[Julien Pierre]

Bob,

Re: comment 23

Thanks, I will fix the bitmask macro. I will also use only one cache bit.
I will also add a third macro to simplify the code in pk11obj and pk11skey.

Re: comment 24 

My comment about the logic wasn't about the case where CKA_PRIVATE is missing,
but case where it is set and CK_TRUE . There are really three cases:
1) CKA_PRIVATE not set
2) CKA_PRIVATE set to CK_FALSE
3) CKA_PRIVATE set to CK_TRUE

The existing function PK11_HasAttributeSet returns CK_FALSE for the first two
cases, and CK_TRUE for the third case .

The existing tests, however, are written as :

    if ( !PK11_HasAttributeSet(slot, key->pkcs11ID,CKA_PRIVATE) {
	PK11_HandlePasswordCheck(slot, key->wincx);
    }

Ie., we try to login only if CKA_PRIVATE is missing or unset.
Doesn't the presence of CKA_PRIVATE with a value of CK_TRUE mean that we need to
login in order to use the key ? And its presence with a value of CK_FALSE that
we shouldn't login ?

Please clarify so I can create the next patch.

Wan-Teh.

Re: comment 25

As Bob already answered, I defined the macros in anticipation of caching more
attributes.

How else would you suggest I use ## ? I believe only pre-ANSI C compilers will
have a problem with this (they require /**/ ).

I will make the macros uppercase to distinguish them from functions.

I know I could define them without the ? operator. However, I meant for them to
return a value which can be tested against PRBool values (PR_TRUE, PR_FALSE), so
I didn't want to just mask the bit and test for non-zero. Perhaps I should use
CK_TRUE and CK_FALSE instead, though.

Comment 28

[Robert Relyea]

re: sense of PK11_HasAttributeSet

You are quite right. The existing code is wrong.

bob

Comment 29

[Wan-Teh Chang]

Julien,

I'm not suggesting that you use ## in a different
way.  I'm suggesting that you not use ##.  The
reason is not that some compilers may not support
##.  The reason is that average C programmers may
not know what ## does, so it makes our code harder
to understand.  But this is just a my personal
preference.

PR_TRUE and PR_FALSE are macros with values 1 and
0.  A comparison with 0 ("0 == x" or "0 != x")
evalutes to 1 or 0.  So no type conversion is
needed.  (This does depend on how PR_TRUE and
PR_FALSE are defined, but their definitions will
not change.)

Comment 30

[Julien Pierre]

Attachment: fixes integrating Bob and Wan-Teh's review comments. Also fix bogus logic

Comment 31

[Robert Relyea]

Comment on attachment 175370 [details] [diff] [review]
fixes integrating Bob and Wan-Teh's review comments. Also fix bogus logic

looks good!

bob

Comment 32

[Julien Pierre]

Thanks, Bob. I checked in this patch on the tip.

Checking in sslsecur.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsecur.c,v  <--  sslsecur.c
new revision: 1.26; previous revision: 1.25

Checking in pk11obj.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11obj.c,v  <--  pk11obj.c
new revision: 1.4; previous revision: 1.3
done
Checking in pk11skey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v  <--  pk11skey.c
new revision: 1.90; previous revision: 1.89
done

Checking in nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.141; previous revision: 1.140

Checking in keyhi.h;
/cvsroot/mozilla/security/nss/lib/cryptohi/keyhi.h,v  <--  keyhi.h
new revision: 1.12; previous revision: 1.11
done
Checking in keythi.h;
/cvsroot/mozilla/security/nss/lib/cryptohi/keythi.h,v  <--  keythi.h
new revision: 1.8; previous revision: 1.7
done
Checking in seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.34; previous revision: 1.33