396999 - PK11_Authenticate, PK11_DoPassword fail on 3rd party slots if NSS softoken is in FIPS140-2 mode

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

The act of enabling FIPS140-2 on NSS softoken makes certs in other modules disappear, as shown by the sequence below.

The only fix is to disable FIPS140-2 on the softoken. The state of the NSS softoken shouldn't have any incidence on the availability of certs other modules. so I would say something is broken in pk11wrap land.

I reproduced this both on the trunk and NSS_3_11_BRANCH .

[jp96085@zino]/home/jp96085/nss/mozilla/dist/SunOS5.10_i86pc_DBG.OBJ/bin 175 % modutil -dbdir sca6000 -list
Using database directory sca6000...

Listing of PKCS #11 Modules
-----------------------------------------------------------
  1. NSS Internal PKCS #11 Module
         slots: 2 slots attached
        status: loaded

         slot: NSS Internal Cryptographic Services
        token: NSS Generic Crypto Services

         slot: NSS User Private Key and Certificate Services
        token: NSS Certificate DB

  2. SCF
        library name: /usr/lib/libpkcs11.so
         slots: 2 slots attached
        status: loaded

         slot: Sun Metaslot
        token: Sun Metaslot

         slot: Sun Crypto Softtoken
        token: Sun Software PKCS#11 softtoken
-----------------------------------------------------------
[jp96085@zino]/home/jp96085/nss/mozilla/dist/SunOS5.10_i86pc_DBG.OBJ/bin 176 % !160
certutil -d sca6000 -L -h all
Enter Password or Pin for "Sun Software PKCS#11 softtoken":
Enter Password or Pin for "Sun Metaslot":
Sun Metaslot:server-cert                                     u,u,u
[jp96085@zino]/home/jp96085/nss/mozilla/dist/SunOS5.10_i86pc_DBG.OBJ/bin 177 % modutil -dbdir sca6000 -fips true


WARNING: Performing this operation while the browser is running could cause
corruption of your security databases. If the browser is currently running,
you should exit browser before continuing this operation. Type
'q <enter>' to abort, or <enter> to continue:
Using database directory sca6000...
FIPS mode enabled.
[jp96085@zino]/home/jp96085/nss/mozilla/dist/SunOS5.10_i86pc_DBG.OBJ/bin 178 % certutil -d sca6000 -L -h all
Enter Password or Pin for "Sun Software PKCS#11 softtoken":
Enter Password or Pin for "Sun Metaslot":
[jp96085@zino]/home/jp96085/nss/mozilla/dist/SunOS5.10_i86pc_DBG.OBJ/bin 179 %

Comment 1

[glen beasley]

in Non-Fips mode if you enter the password incorrectly you do not get an invalid password. certutil simply returns to the command prompt. 

In Fips Mode:
certutil -d . -L -h "Sun Metaslot"
Enter Password or Pin for "Sun Metaslot":
certutil: problem printing certificate nicknames: An I/O error occurred during security authorization

Comment 2

[Julien Pierre]

Yes, but I was using the correct password in all cases, pasting it with the mouse.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Glen, Please file a bug about the erroneous certutil behavior you 
mentioned in comment 1.

Comment 4

[glen beasley]

Per comment # 2 Sorry Julien, I meant this as additional information, and as Nelson pointed out I created a separate bug 397077  for the non-fips error. you used -h all and I used -h "Sun Metaslot" in fips mode I think both cases could be dealt with in this bug?

Comment 5

[Robert Relyea]

Question: is the meta slot opening softoken as well? (I wonder if the meta slot is blowing up on softoken in FIPS mode in this case).

bob

Comment 6

[Julien Pierre]

The Meta slot only opens the Solaris softoken, but not the NSS softoken.

Comment 7

[Julien Pierre]

The failure occurs at the following stack, while authenticating the Sun MetaSlot :

=>[1] PK11_VerifyMechanism(slot = 0x80a9fc0, intern = 0x80a90c8, mech = 273U, data = 0xfee1a458, iv = 0xfee1a46c), line 1048 in "pk11slot.c"
  [2] PK11_VerifySlotMechanisms(slot = 0x80a9fc0), line 1162 in "pk11slot.c"
  [3] pk11_CheckVerifyTest(slot = 0x80a9fc0), line 1185 in "pk11slot.c"
  [4] PK11_DoPassword(slot = 0x80a9fc0, loadCerts = 0, wincx = 0x80466ec), line 620 in "pk11auth.c"
  [5] PK11_Authenticate(slot = 0x80a9fc0, loadCerts = 0, wincx = 0x80466ec), line 319 in "pk11auth.c"
  [6] pk11_AuthenticateUnfriendly(slot = 0x80a9fc0, loadCerts = 0, wincx = 0x80466ec), line 333 in "pk11auth.c"
  [7] pk11_TraverseAllSlots(callback = (nil), arg = (nil), forceLogin = 1, wincx = 0x80466ec), line 1699 in "pk11obj.c"
  [8] PK11_ListCerts(type = PK11CertListAll, pwarg = 0x80466ec), line 2407 in "pk11cert.c"
  [9] ListCerts(handle = 0x80abdf8, name = (nil), slot = (nil), raw = 0, ascii = 0, outfile = 0x808c0e0, pwdata = 0x80466ec), line 551 in "certutil.c"
  [10] certutil_main(argc = 8, argv = 0x8046794, initialize = 1), line 2057 in "certutil.c"
  [11] main(argc = 8, argv = 0x8046794), line 2446 in "certutil.c"

The error is specifically in this code :

 1046       /* load the keys and contexts */
 1047       symKey = PK11_KeyGen(intern,mech,NULL, key_size, NULL);
 1048       if (symKey == NULL) goto loser;

The resulting symKey is NULL . This is happening with mech = CKM_RC4 .
Apparently, NSS is trying to verify the implementation of the CKM_RC4 against the one of our internal token, to make sure it is correct.
This check is never executed unless the NSS softoken is in FIPS140-2 mode.
I think there is a bug in this code, and I also wonder why we are trying to do something different with 3rd party tokens based on the FIPS status of our own token here.

Comment 8

[Julien Pierre]

Actually, we are always executing this check. It just happens that the keygen fails when NSS softoken is in FIPS mode. So this check always fails. This would mean it's impossible to properly authenticate to any other token while NSS is in FIPS mode.

Actually, the C_Login has previously succeeded due to the proper password being fed, but PK11_Authenticate returns a failure due to this check failure, and so the 3rd party slot is not taken into account by PK11_ListCerts .

Comment 9

[Julien Pierre]

The C_GenerateKey fails because of CKR_USER_NOT_LOGGED_IN . NSS softoken requires a login to do a session keygen in FIPS mode.

certutil does not previously login to the NSS softoken when it is in FIPS mode, as it does when it is in non-FIPS mode.

The problem is that PK11_IsFriendly always returns PR_TRUE for our softoken slots. Perhaps we should mark NSS softoken unfriendly is in FIPS mode ?

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

It sounds like some code is using the Friendly bit to mean "can do crypto ops such as keygen without logging in".  By that definition of Friendly, no FIPS
mode device is Friendly.  

Is this bug a regression? And if so, in what release was it introduced?

Comment 11

[Julien Pierre]

I tried with 3.9 and the problem was already there. So it's been there a while, it's not a recent regression.

PK11_ListCerts has this code :

 2406       /* authenticate to the slots */
 2407       (void) pk11_TraverseAllSlots( NULL, NULL, PR_TRUE, pwarg);

And pk11_TraverseSlots is as follows :

 1693       list = PK11_GetAllTokens(CKM_INVALID_MECHANISM,PR_FALSE,PR_FALSE,wincx);
 1694       if (list == NULL) return SECFailure;
 1695
 1696       /* look at each slot and authenticate as necessary */
 1697       for (le = list->head ; le; le = le->next) {
 1698           if (forceLogin) {
 1699               rv = pk11_AuthenticateUnfriendly(le->slot, PR_FALSE, wincx);
 1700               if (rv != SECSuccess) {
 1701                   continue;
 1702               }
 1703           }
 1704           if (callback) {
 1705               (*callback)(le->slot,arg);
 1706           }
 1707       }

pk11_AuthenticateUnfriendly attempts to authenticate only the unfriendly tokens by calling PK11_Authenticate. So, it skips the NSS FIPS token. It tries only the third-party slots.

PK11_Authenticate ends up calling PK11_DoPassword, which has this at the end :

  619       if (rv == SECSuccess) {
  620           rv = pk11_CheckVerifyTest(slot);
  621           if (!PK11_IsFriendly(slot)) {
  622               nssTrustDomain_UpdateCachedTokenCerts(slot->nssToken->trustDomain,
  623                                                 slot->nssToken);
  624           }
  625       } else if (!attempt) PORT_SetError(SEC_ERROR_BAD_PASSWORD);
  626       return rv;
  627   }

pk11_CheckVerifyTest will always fail unless the NSS FIPS token has been previously authenticated.

Maybe the fix is to change PK11_VerifyMechanism to force authentication of the internal slot if it gets a CKR_USER_ERROR_NOT_LOGGED_IN . I will try that and attach a patch if it works.

Comment 12

[Julien Pierre]

Attachment: Authenticate the internal slot if needed when verifying a mechanism

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 296636 [details] [diff] [review]
Authenticate the internal slot if needed when verifying a mechanism

I would prefer Bob to review this patch.
This patch does what it intends to do, but perhaps another solution would be better.  I want Bob's opinion.

Comment 14

[Robert Relyea]

Comment on attachment 296636 [details] [diff] [review]
Authenticate the internal slot if needed when verifying a mechanism

While I think it's a good idea in general, the lack of a wincx is a serious problem.

Applications can choose to call NSS functions with NULL wincx (really password arg) because applications have control over what their password prompt callbacks do, but NSS itself can't make this assumption. It's possible some applications would crash if called with a NULL password arg in their callback.

bob

Comment 15

[Julien Pierre]

Bob,

The wincx is passed except when called from PK11_InitToken, since that's a public API that doesn't take a wincx argument. Do you think tokens will require authentication upon initialization ?

I could change pk11_CheckVerifyTest to take an additional PRBool to select whether to try this authentication or not, and just set that to PR_FALSE in the call to it from PK11_InitToken.

Comment 16

[Robert Relyea]

Yes, but they are only called from the PK11_InitToken path.

Traditionally we have not required tokens to authenticate at initialization time, but your patch changes that. You potentially authenticate to softoken when we initialize another pkcs #11 module. If your code is triggered in an application that requires wincx, you can cause the application to crash.

I suspect we may have to install a more complicated version of this patch.

We could:

1) perhaps remove the startup validation checks. These checks were put in under certain export regulations. The regulations have changed (certainly have gotten more complicated), but the change allowed us to have installable crypto without signing each module. I don't know if we still have to do it.

2) Simply disable 'broken' crypto. Instead of rejecting the whole module, we turn off the ability of the module to do the crypto we check for. (may have export ramifications).

3) If softoken requires login, but isn't yet logged in (the crypto engine of softoken), we can delay Init tokens that don't need login until softoken is logged in.

4) Combination of 2 and 3 (initialize the token, but don't enable any crypto until softoken has been initialized first).

Note that this case is technically a bit pathelogical. If you are truly running in FIPS mode, and you have other tokens loaded, those tokens must also be FIPS validiated. In that case they should also authenticate the user before continuing, which means the only issue is if you authenticate these tokens before you authenticate softoken, as PK11_InitToken isn't called until the token is authenticated.

BTW PK11_InitToken is not exported, but it doesn't matter because it is eventually called by exported functions that do not take a wincx value.

bob

Comment 17

[Julien Pierre]

Bob,

Re: comment 16,

1) That would be fine by me. How do we find out if these checks still apply ?

2) In order to disable "broken" crypto, we would still have to be able to perform this test. So this wouldn't be a solution to this problem.

3) How do you propose we implement that ?

4) I guess you meant until softoken has been authenticated, not initialized. That was the case here with certutil - the 3rd party token was in the process of being authenticated, and was the first token to be authenticated. See the stack  in comment 7 .

I don't see how we can prevent this case. There is nothing in our API that precludes it or dictates the order of token authentication. It should be OK. Our own apps already do it. And this order only causes issues if the softoken is in FIPS mode.

You are right, PK11_InitToken is not exported. But it is called from PK11_ResetToken which is exported. 

Comment 18

[Julien Pierre]

Attachment: Fix only PK11_DoPassword . No change for PK11_InitToken

This patch only forces the authentication to the FIPS token when the check happens from PK11_DoPassword. When it's being triggered from PK11_InitToken, the extra authentication does not take place. So we don't end up calling the password call back with a NULL wincx anymore.

Of course, this means there may still be failures if the checks are triggered from PK11_InitToken, whose callers are :

pk11_checkPassword
PK11_ResetToken
PK11_InitPin
PK11_ChangePW
PK11_InitSlot
pk11_isPresentCertLoad
PK11_ResetToken

This may be best handled in a separate patch to this bug, or in a separate bug.

Comment 19

[Julien Pierre]

Attachment: Fix only PK11_DoPassword . No change for PK11_InitToken

Add one important check which was missing from the previous patch.

Comment 20

[Robert Relyea]

> 1) That would be fine by me. How do we find out if these checks still apply ?

That would be an export legal question.

> 2) In order to disable "broken" crypto, we would still have to be able to
> perform this test. So this wouldn't be a solution to this problem.

the presumption of doing #2 is that use of that crypto mechanism in that token is not critical. Basically it says, if you fail the test, you only disable that algorithm, not the entire module. You still get issues with certain tokens, but you don't fail to load the token (which is usually what is being complained about here --- it's the loss of certs, not the loss of DES).

> 3) How do you propose we implement that ?

We already delay the check until the target token has been authenticated. We would include the requirement that the softoken crypto services is authenticated as well. It's not trivial code, but definately implementable.

> 4) I guess you meant until softoken has been authenticated, not initialized.

correct.

> That was the case here with certutil - the 3rd party token was in the process
> of being authenticated, and was the first token to be authenticated. See the
> stack  in comment 7 .

1. It's extremely simple for an app to guarrentee authenticating to softoken first -- you simply force an authentication to softoken before you do any of your other tokens. The issue is what about tokens that don't need authentication. -- but that's not the point.

The model for 4 is you don't do any crypto until you authenticate to softoken.
You can guarrentee that by simply authenticating to softoken in your startup. It doesn't matter if you authenticate to the other tokens first, you just have to make sure both are authenticated before you try to use crypto.

bob

Comment 21

[Robert Relyea]

Comment on attachment 297691 [details] [diff] [review]
Fix only PK11_DoPassword . No change for PK11_InitToken

I'm tempted to r- this patch for the simple reason it seems to only fix the case that can be trivially fixed without digging into the code.

You can always force authentication to the softoken before you try to authenticate to any of these other tokens.

Is it the case that we are only dealing with authenticated order, not unauthenticated tokens here?

bob

Comment 22

[Julien Pierre]

Bob,

We have many different groups at sun that write different applications. It is not trivial at all for us to change them all to confirm to this undocumented limitation.

Without my patch, someone who calls PK11_Authenticate first on a 3rd party token will always fail the authentication unless the FIPS token is previously authenticated. We may be able to partly hide the problem by having our token list functions (like PK11_GetAllTokens()) always return the FIPS slot first. But the truth is that we just can't predict what slot the application will try to authenticate first. So, I think the patch I wrote should go in.

Comment 23

[Robert Relyea]

But your patch just pastes over a wound. If, as you say, you have lots of different apps with different ways of doing startup and init, then your patch only 'fixes' a small number of them.

In that case we need to implement one of the other options I presented.

bob

Comment 24

[Julien Pierre]

Bob,

Re: comment 23, I have reduced the scope of this bug to only callers of PK11_Authenticate and PK11_DoPassword. I believe the patch fixes those problems.

I have been reading and re-reading your comment 16 and comment 20, but I'm still not completely clear exactly what you propose.

1) Let's assume the lawyers will nix removing this check altogether for now.

2) we don't fail to load the token. We fail in performing some other operations on the token that end up having the export check as a prerequisite today.

This export check chokes on DES is only failing because the FIPS module is not authenticated. It seems that we would have to maintain more complex state about the test, and perform the test again when it is fully authenticated. Even so, for the callers of PK11_InitToken that don't take wincx, they just aren't going to get the password and thus are not going to be able to authenticate and pass the test. The only option is to skip the test to make those callers succeed. We can "delay" the test until some other API has the wincx, but in the context of those callers, the only option is to skip or fail the test.

3) sure, I can just check if the softoken is in in FIPS mode and unauthenticated and then skip the test, for the case where wincx isn't available (PK11_InitToken). But when it is available (PK11_DoPassword) I think we should go ahead and authenticate since we are able to perform the test immediately.

4) that is only an option for select apps and if you can change them all. We can't fix it in the library and that isn't a good fix.

Comment 25

[Julien Pierre]

Attachment: Also delay the check in PK11_InitToken is we are in FIPS mode and not authenticated

Note that I preserved the PRBool fipsauth argument to pk11_CheckVerifyTest in case the FIPS token login changed from logged in to logged out between the caller and the callee in another thread, in order to avoid ever calling the password callback with a NULL wincx.

Comment 26

[Robert Relyea]

I think you misunderstood options 2-3:

Currently, if any of the cipher checks fail, we fail loading the entire token. Option 2 changes this to: if any of the cipher checks fail, we disable using that cipher for that token. This means the token still loads and the certs are still visible, but we don't use the failed cipher that we checked.

Option 3. We don't even initialize a token that needs verify checks until the softoken has been authenticated.

Option 4 is to implement option 2, but only disable the cipher temporarily until we later authenticate to the softoken. Once softoken is authenticated we retry the test and turn on any previously disabled ciphers.

We can't just not do the checks unless we have permission for option 1. As I look at it, Option 4 is probably our best bet, but Option 2 solves the immediate problem (None of the symmetric ciphers have any bearing on display of certs and keys). Option 3 has issues in that it there is no way to force the application to authenticate to softoken, so we may still have a problem.


Adding your patch after we have Option 2 taken care of would be an optimization that would allow tokens, under certain cases, to have their symmetric algorithms work.

bob

Comment 27

[Robert Relyea]

Comment on attachment 298632 [details] [diff] [review]
Also delay the check in PK11_InitToken is we are in FIPS mode and not authenticated

r- 

We need to have a patch for one of the options I outlined first.

bob

Comment 28

[Julien Pierre]

Bob,

I believe this problem needs to be split into 2 parts, that can actually be fixed independently of each other.

1) the case where it's possible to do the test, because we have a wincx on the stack. This is the case of PK11_DoPassword, fixed in attachment 297691 [details] [diff] [review] .
No crypto needs to be disabled, and no test needs to be delayed. All that needs to happen is to pass the wincx all the way from the top of the stack.

Your proposals do not adequately fix this case. Take the example of a server that starts by doing the sequence  :
1) NSS_Initialize
2) enumerate all tokens through PK11_GetAllTokens() and call PK11_Authenticate to authenticate the

The above is a real code sequence which I wrote in the web server years ago. It happens to be the same sequence that certutil uses.

Right now, if the first slot returned by PK11_GetAllTokens, is not the FIPS slot, PK11_Authenticate on the token fails, and the server will not be able to see the certs in the token.

Your proposals would cause PK11_Authenticate to suceed, because it doesn't use crypto, but at the same time disable the token crypto during the test, with no chance to get it enabled again. Thus, your proposal does not fix the PK11_Authenticate / PK11_DoPassword adequately for the server. It only fixes it for certutil, which will do nothing but list the cert, but not require the token crypto.

My proposal (specifically, in attachment 297691 [details] [diff] [review]) causes PK11_Authenticate to succeed, and the token crypto to also be available.

None of your proposals would actually cause PK11_Authenticate to work *and* the token to work properly after the authentication. Only my attachment 297691 [details] [diff] [review] does that. Please reconsider it in light of this info.

2) the case where the crypto test cannot pass, because we can't authenticate to the FIPS token due to the lack of wincx anywhere on the stack. This is the case of the callers of PK11_InitToken .

Your have made some proposals to fix that case. You have also claimed that implementing them would fix the other case of PK11_Authenticate . I have just shown that they wouldn't satisfactorily fix that case. My patch is still required to do that.

So, in summary, we need two independent patches to fix these 2 issues. I propose to split this bug into 2. I have changed the description of this bug to narrow it down to the first (PK11_DoPassword / PK11_Authenticate) case only . We can handle the case of PK11_InitToken in a separate bug. There is no reason not to fix the first case just because the second case is not yet fixed.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

In thursday's meeting, we agreed that the best fix for this is to remove the 
algorithm checks, IFF we can do so without impacting the export filings of 
products that use NSS.  The next step is to check with our respective 
export administration departments to confirm this.

Comment 30

[Julien Pierre]

Nelson,

As we well know, lawyers take a long time, months, sometimes years. There are several of them involved, and we don't even know if they will come to the same answer. In the meantime, is there any good reason not to fix the half of the problem that we can right now, ie. check in attachment 297691 [details] [diff] [review] ? We can always remove the check entirely afterwards, if and when the lawyers come through and agree that these checks are unnecessary. I agree that that would be the best solution. I just don't think our customers should have to wait for that if we can have a partial fix earlier.

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

I'm not sure any lawyers need to get involved.  
Let's talk to our own export administration department folks first.

Comment 32

[Julien Pierre]

Something is rotten on the trunk. None of the patches that I attached solve the problem anymore. But they do  on the branch.

Comment 33

[Julien Pierre]

I didn't mean to reassign this bug. Taking it back.

Comment 34

[Julien Pierre]

Please ignore comment 32. The patch still works.

My FIPS key DB was not initialized, so the call to PK11_DoPassword was failing (PK11_NeedUserInit failed).

Since we only have one slot in FIPS mode for both DB and crypto, it means the FIPS softoken DB has to be created first. Otherwise we can't authenticate to the FIPS token, and thus we can't run the export check on the third party tokens, and thus we can't use any 3rd party token. This is pretty messed up. Let's get rid of those stupid export checks ASAP.

Comment 35

[Nelson Bolyard (seldom reads bugmail)]

Bob, 
Were you going to attach a patch to "remove the startup validation checks"?
We're ok to do that.

Comment 36

[Julien Pierre]

Attachment: Remove checks

Comment 37

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 307629 [details] [diff] [review]
Remove  checks

This looks right to me, but let's ask Bob to concur.

Comment 38

[Robert Relyea]

Comment on attachment 307629 [details] [diff] [review]
Remove  checks

r+ 

bahbye...;)

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 297691 [details] [diff] [review]
Fix only PK11_DoPassword . No change for PK11_InitToken

Now that the other patch has gotten r+ and sr+, there is no further need for this patch. r-

Comment 40

[Julien Pierre]

Bob, Nelson,

Thanks for the reviews. I checked the patch in to the trunk :

Checking in lib/pk11wrap/pk11auth.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11auth.c,v  <--  pk11auth.c
new revision: 1.8; previous revision: 1.7
done
Checking in lib/pk11wrap/pk11priv.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11priv.h,v  <--  pk11priv.h
new revision: 1.9; previous revision: 1.8
done
Checking in lib/pk11wrap/pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.91; previous revision: 1.90
done

Comment 41

[Julien Pierre]

Also fixed on NSS_3_11_BRANCH :

Checking in pk11auth.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11auth.c,v  <--  pk11auth.c
new revision: 1.5.2.1; previous revision: 1.5
done
Checking in pk11priv.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11priv.h,v  <--  pk11priv.h
new revision: 1.5.2.2; previous revision: 1.5.2.1
done
Checking in pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.87.2.2; previous revision: 1.87.2.1
done