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

RESOLVED FIXED in 3.11.2

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Julien Pierre, Assigned: Julien Pierre)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: FIPS)

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

12 years ago
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.
(Assignee)

Updated

12 years ago
Priority: -- → P1
Whiteboard: FIPS
Target Milestone: --- → 3.11.2
(Assignee)

Updated

12 years ago
Summary: PK11_FindKeyByAnyCert does not work if softoke is in FIPS140-2 mode → PK11_FindKeyByAnyCert does not work if softoken is in FIPS140-2 mode
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

12 years ago
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.
Assignee: nobody → rrelyea
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

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

12 years ago
Created attachment 222078 [details] [diff] [review]
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?
(Assignee)

Comment 6

12 years ago
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.
(Assignee)

Comment 7

12 years ago
Created attachment 222079 [details] [diff] [review]
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.
Attachment #222079 - Flags: superreview?(wtchang)
Attachment #222079 - Flags: review?(christophe.ravel.bugs)
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."
Attachment #222079 - Flags: review+

Comment 9

12 years ago
Created attachment 222088 [details] [diff] [review]
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

12 years ago
Created attachment 222089 [details] [diff] [review]
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.

Updated

12 years ago
Attachment #222089 - Attachment is obsolete: true

Comment 11

12 years ago
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.
Attachment #222079 - Flags: superreview?(wtchang)
Attachment #222079 - Flags: superreview+
Attachment #222079 - Flags: review?(rrelyea)

Comment 12

12 years ago
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.
Attachment #222079 - Flags: review?(rrelyea) → review+
(Assignee)

Updated

12 years ago
Assignee: rrelyea → julien.pierre.bugs
(Assignee)

Comment 13

12 years ago
Created attachment 222119 [details] [diff] [review]
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
Attachment #222079 - Attachment is obsolete: true
Attachment #222079 - Flags: review?(christophe.ravel.bugs)
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 14

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

Comment 15

12 years ago
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.
(Assignee)

Comment 16

12 years ago
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 .
Summary: PK11_FindKeyByAnyCert does not work if softoken is in FIPS140-2 mode → PK11_ functions that find objects fail when user not logged in and softoken is in FIPS140 mode
You need to log in before you can comment on or make changes to this bug.