337789 - PK11_ functions that find objects fail when user not logged in and softoken is in FIPS140 mode

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

This bug is another regression introduced in NSS 3.9.2 by the fix for bug 244907 - "NSS needs to handle unprotected keys in token".

The following logic is repeated in 4 places in that bug :

 1591       keyHandle = PK11_MatchItem(slot,certHandle,CKO_PRIVATE_KEY);
 1592       if ((keyHandle == CK_INVALID_HANDLE) &&
 1593                           (PORT_GetError() == SSL_ERROR_NO_CERTIFICATE) &&
 1594                           needLogin) {
 1595           /* authenticate and try again */
 1596           rv = PK11_Authenticate(slot, PR_TRUE, wincx);
 1597           if (rv == SECSuccess) {
 1598               keyHandle = PK11_MatchItem(slot,certHandle,CKO_PRIVATE_KEY);
 1599           }
 1600       }

The above is from pk11cert.c on the tip.
The problem is that with the FIPS token, even though it is not logged in, the error returned is SEC_ERROR_LIBRARY_FAILURE rather than SSL_ERROR_NO_CERTIFICATE . So, the function does not attempt to login, and just fails.

Here is a sample stack from selfserv demonstrating the problem :

=>[1] pk11_FindObjectByTemplate(slot = 0x81560d8, theTemplate = 0x8046ab4, tsize = 2), line 1366 in "pk11obj.c"
  [2] PK11_MatchItem(slot = 0x81560d8, searchID = 4218554470U, matchclass = 3U), line 1495 in "pk11obj.c"
  [3] PK11_FindKeyByAnyCert(cert = 0x815c9c8, wincx = 0x8082ea8), line 1591 in "pk11cert.c"
  [4] main(argc = 16, argv = 0x8046c24), line 1979 in "selfserv.c"

First, C_FindObjectsInit returns CKR_USER_NOT_LOGGED_IN . Then, it gets mapped to SEC_ERROR_LIBRARY_FAILURE, and pk11_FindObjectsByTemplate returns CK_INVALID_HANDLE with that error. Then, PK11_MatchItem returns CK_INVALID_HANDLE without changing this error.

I verified in the debugger that just remapping the CKR_USER_NOT_LOGGED_IN error to SSL_ERROR_NO_CERTIFICATE was enough to allow the private key to be found and the server to come up.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

I'm amazed that we have no SEC_ERROR that means simply "user not logged in"
or "user not authenticated".  Rather than using SSL_ERROR_NO_CERTIFICATE for 
this (? oy!) may I suggest that we use SEC_ERROR_RETRY_OLD_PASSWORD.  

Comment 2

[Wan-Teh Chang]

Let's add a new NSS error code for "user not authenticated".
SEC_ERROR_RETRY_OLD_PASSWORD isn't being used, but it seems
to mean that one entered the wrong old password when one
tries to change the password.  This is very different from
"user not authenticated".

A few other functions in pk11cert.c have the same test
for PORT_GetError() == SSL_ERROR_NO_CERTIFICATE; they
should also be reviewed.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

SEC_ERROR_RETRY_OLD_PASSWORD is a leftover from Netscape Communicator.
It was used as part of a dialog for changing from an old to a new password.
It meant "you have not yet succesfully authenticated with your old password"
which was a prerequisite to changing to a new password.  But the message 
"you have not yet succesfully authenticated with your (old) password" 
is simply another way of saying "you are not (yet) successfully logged in"
so I think the two are logically equivalent.  I'd suggest we might add 
another #define symbol with the same number as SEC_ERROR_RETRY_OLD_PASSWORD.  

Comment 4

[Wan-Teh Chang]

SEC_ERROR_RETRY_OLD_PASSWORD meant "you have not yet succesfully
re-authenticated with your old password".  Note the word
"re-authenticated".  This error is different from
"you are not (yet) successfully logged in".

Another reason to not use SEC_ERROR_RETRY_OLD_PASSWORD is
that some NSS-based products may have copied
"Old password entered incorrectly.  Please try again."
from NSS source files or the SSL Reference as the
error message for this error code.  This error message
does not describe the error condition that one can't
use a FIPS token without logging in.

Comment 5

[Wan-Teh Chang]

Attachment: Patch that shows where the bug is

This patch shows where the bug is.  You can use this patch
to allow testing of selfserv in FIPS mode.

We should add a new NSS error code for CKR_USER_NOT_LOGGED_IN
and change PK11_MapError to map CKR_USER_NOT_LOGGED_IN to the
new error code.  But I'm not sure how to fix PK11_FindKeyByAnyCert.

Should it check for SSL_ERROR_NO_CERTIFICATE and the new error code?

Should it not check for any error code?

Comment 6

[Julien Pierre]

Wan-Teh,

I'm working on a patch now that will fix all occurrences of this problem. I'm testing it now and will attach it soon. The patch will check both the old and new error code, which I decided to call SEC_ERROR_TOKEN_NOT_LOGGED_IN . Ideally, I would prefer to get rid of the SSL_ERROR_NO_CERTIFICATE, but I'm not confident about doing that right now.

Comment 7

[Julien Pierre]

Attachment: login patch

This patch allows PK11_FindKeyByAnyCert to find the key in FIPS mode, and selfserv to start. It also fixes all the other places that checked for SSL_ERROR_NO_CERTIFICATE . This patch also contains some changes from the fix bug 335036, in which I had defined a new error code.

Christophe, please test this patch and see if you can run all of the ssl.sh test in FIPS mode now.

Wan-Teh, please review.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 222079 [details] [diff] [review]
login patch

This patch includes another patch, a fix for another bug, 
to which I have previously given r+.  
Here's r+ for this patch too, except for this one new string.

>+ER3(SEC_ERROR_TOKEN_NOT_LOGGED_IN,  		(SEC_ERROR_BASE + 155),
>+"A PKCS#11 token is not logged in.")

That message can be interpreted as saying the false statements that 
the user must be logged in on all tokens, and if any token is not 
logged in, the operation cannot succeed.

I think this should say 
"operation failed because the token is not logged in." 
or
"operation requires user to be logged in on the token."
or
"The token requires the user to be logged in for this operation."

Comment 9

[Wan-Teh Chang]

Attachment: Alternate patch: continue to use SSL_ERROR_NO_CERTIFICATE

This is another way to fix the bug.  I don't know why pk11cert.c specifically tests
for SSL_ERROR_NO_CERTIFICATE, but the SSL Reference seems to document this
usage of SSL_ERROR_NO_CERTIFICATE (the "wrong password for key database"
case):

  SSL_ERROR_NO_CERTIFICATE

  "Unable to find the certificate or key necessary for authentication." 

  This error has many potential causes; for example: 

  Certificate or key not found in database. 

  Certificate not marked trusted in database and Certificate's issuer not marked trusted in database. 

  Wrong password for key database. 

  Missing database.

At least this error code has been used for several errors.

Comment 10

[Wan-Teh Chang]

Attachment: Alternate patch: continue to use SSL_ERROR_NO_CERTIFICATE

This is another way to fix the bug.  I don't know why pk11cert.c specifically tests
for SSL_ERROR_NO_CERTIFICATE, but the SSL Reference seems to document this
usage of SSL_ERROR_NO_CERTIFICATE (the "wrong password for key database"
case):

  SSL_ERROR_NO_CERTIFICATE

  "Unable to find the certificate or key necessary for authentication." 

  This error has many potential causes; for example: 

  Certificate or key not found in database. 

  Certificate not marked trusted in database and Certificate's issuer not marked trusted in database. 

  Wrong password for key database. 

  Missing database.

At least this error code has been used for several errors.

Comment 11

[Wan-Teh Chang]

Comment on attachment 222079 [details] [diff] [review]
login patch

r=wtc.  Ideally this patch should be reviewed by Bob
Relyea because his patch introduced this bug.

PRErrorCode should be changed to 'int' because PORT_GetError
returns 'int'.  I would use 'err' or 'error' instead of 'rc'
because 'rc' is often used for the return value, as opposed
to error code, of a function.

+ER3(SEC_ERROR_NOT_INITIALIZED,      		(SEC_ERROR_BASE + 154),
+"NSS not initialized.")

I suggest changing the error message to
"NSS is not initialized." because most of the
error messages in that file are full sentences.

Comment 12

[Robert Relyea]

Comment on attachment 222079 [details] [diff] [review]
login patch

r+=relyea

wan-teh's suggesting of removing the conditional error code test is also acceptable. The test is only an optimization. An optimization that fails should be removed;).

I prefer one one nelson's string descriptions for the new error code.

Even if we remove the check, the new error code should stay.

Comment 13

[Julien Pierre]

Attachment: patch as checked in

To the tip :

Checking in lib/pk11wrap/pk11err.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11err.c,v  <--  pk11err.c
new revision: 1.7; previous revision: 1.6
done
Checking in lib/pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.148; previous revision: 1.147
done
Checking in cmd/lib/SECerrs.h;
/cvsroot/mozilla/security/nss/cmd/lib/SECerrs.h,v  <--  SECerrs.h
new revision: 1.12; previous revision: 1.11
done
Checking in lib/util/secerr.h;
/cvsroot/mozilla/security/nss/lib/util/secerr.h,v  <--  secerr.h
new revision: 1.19; previous revision: 1.18
done

And to NSS_3_11_BRANCH :

Checking in lib/pk11wrap/pk11err.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11err.c,v  <--  pk11err.c
new revision: 1.6.2.1; previous revision: 1.6
done
Checking in lib/pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.143.2.4; previous revision: 1.143.2.3
done
Checking in cmd/lib/SECerrs.h;
/cvsroot/mozilla/security/nss/cmd/lib/SECerrs.h,v  <--  SECerrs.h
new revision: 1.11.24.1; previous revision: 1.11
done
Checking in lib/util/secerr.h;
/cvsroot/mozilla/security/nss/lib/util/secerr.h,v  <--  secerr.h
new revision: 1.18.28.1; previous revision: 1.18
done

Comment 14

[Wan-Teh Chang]

Let's remember to add these error codes to the SSL Reference.

Comment 15

[Wan-Teh Chang]

I added six new error codes to the documentation.

http://www.mozilla.org/projects/security/pki/nss/ref/ssl/sslerr.html

 SEC_ERROR_UNKNOWN_OBJECT_TYPE
 SEC_ERROR_INCOMPATIBLE_PKCS11
 SEC_ERROR_NO_EVENT
 SEC_ERROR_CRL_ALREADY_EXISTS
 SEC_ERROR_NOT_INITIALIZED
 SEC_ERROR_TOKEN_NOT_LOGGED_IN

I don't understand the error message for SEC_ERROR_INCOMPATIBLE_PKCS11.
I don't know what "in an incompatible way" means.

I don't understand the error message for SEC_ERROR_UNKNOWN_OBJECT_TYPE.
Without a context, I don't know what the "object type" is.

Comment 16

[Julien Pierre]

I suggest the doc issues for the error codes be discussed outside this bug. We could have a doc bug if necessary. I will email Wan-Teh about SEC_ERROR_INCOMPATIBLE_PKCS11 .