301554 - SSL/TLS Client Authentication with 3rd party PKCS#11 module failes in case there is an unrecognized token in addition to the token that ought to be used.

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Markus Heintel]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.9) Gecko/20050711 

System setup:
- 3rd party PKCS#11 module successfully installed in Firefox
- 2 smart card readers attached to the system
- reader 1 holding a smart card which is not supported by the 3rd party PKCS#11 
module
- reader 2 holding a smart card that works with the 3rd party PKCS#11 module


The following happens:
-- After calling C_GetSlotList() Firefox calls C_GetSlotInfo(). Both slots 
report CKF_TOKEN_PRESENT (which is OK from our point of view, since 
CKF_TOKEN_PRESENT does not indicate whether the token is supported or not)
-- Firefox calls C_GetTokenInfo() for both readers:
---> For reader 1 the call returns with CKR_TOKEN_NOT_RECOGNIZED
---> For reader 2 the call returns successfully
-- Nevertheless Firefox continues playing around with reader 1 (e.g. calls to 
C_FindObjectsInit(hSession = 0) which naturally return with CKR_OBJECT_HANDLE 
invalid)
-- Firefox also continues going on with the client authentication using reader 
2, but at some point it stops.

It is especially interesting that in case we change the 3rd party PKCS#11 
module in a way that there is a short delay (about 2 secs.) before returning 
CKR_TOKEN_NOT_RECOGNIZED the client authentication succeeds.

In case it would help I can send the complete PKCS#11 transcript of both cases 
working and non working (i.e. with and without the delay).

Reproducible: Always

Steps to Reproduce:
please see above.

Comment 1

[Frank Wein [:mcsmurf]]

->NSS i think...

Comment 2

[Wan-Teh Chang]

Markus: if you have the PKCS #11 transcripts handy, please
attach them to this bug report.

Comment 3

[Wan-Teh Chang]

NSS has a function called within_token_delay_period:

http://lxr.mozilla.org/security/ident?i=within_token_delay_period

which the nssSlot_IsTokenPresent function uses to prevent
polling for token presence too often.

http://lxr.mozilla.org/security/ident?i=nssSlot_IsTokenPresent

The current token presence polling delay time is 1 second.
This is why your 2 second delay makes a difference.  So the
investigation of this bug should begin with these two functions.

Comment 4

[Markus Heintel]

Attachment: PKCS#11 transcript of failed SSL/TLS Client Authentication

PKCS#11 transcript for two tokens. One is not recognized which seems to prevent
Firefox from a successfull SSL/TLS client auth with the other token.

Comment 5

[Markus Heintel]

Attachment: PKCS#11 transcript of successfull SSL/TLS client authentication.

Transcript for a successfull SSL/TLS client authentication with two tokens. One
of the tokens is not recognized. The only difference in the PKCS#11 library
compared two the other log (pkcs11_firefox2_m2.log) is that there is now a
sleep(2 sec.) before CKR_TOKEN_NOT_RECOGNIZED is returned. However, firefox
seems two behave quite different now.

Comment 6

[Robert Relyea]

I think there are 2 issues here.

The first is whether or not the 3rd party token should show 'present' for an
unrecognized token (I believe the answer is no). The weirdness related to this I
attribute to the token itself (the 2 second delay etc). The problem here is
without the ability to create a session object, NSS has no way of knowing when a
token change has happenned.

The second is that NSS is failing the second client auth. Other then a token out
and out crashing, a misbehaving token should not interfere with the normal
operation of other tokens.

Comment 7

[Robert Relyea]

I'll take a look at the symptoms anyway. There is probably some very bad
interaction with the stan code that is causing this. Note the large number of
calls with invalid sessions.

Comment 8

[Markus Heintel]

I think it is OK for PKCS#11 library to report a token as present even if it is 
not recognized. The PKCS#11 standard is not very explicit about this, it just 
states that CKF_TOKEN_PRESENT is set in case a token is present.

At least an application could tell from that information that a specific slot 
is occupied by an unsupported token. The return code CKR_TOKEN_NOT_RECOGNIZED 
for C_GetTokenInfo() would not make sense to me in case unrecognized tokens 
would never be shown by C_GetSlotInfo() and C_GetSlotList(). Should 
C_GetTokenInfo() return CKR_TOKEN_NOT_PRESENT then (even if the slot is 
occupied by an unrecognized token)? Unfortunately I do not remember whether 
this has been discussed on the Cryptoki mailing list before.

Therefore, I think an application should be prepared to handle 
CKR_TOKEN_NOT_RECOGNIZED.

Comment 9

[Robert Relyea]

Attachment: Clear the 'Present flag if slot fails to refresh'

OK, I don't have any tokens the the described behavior to test. This patch was
found by code inspection. It basically detects unfriendly modules who say the
slot is 'present' but can't do anything about the token in the slot anyway.
This patch will cause IsPresent to return false in this case. It needs to be
tested.

If everything works, I would still like to reproduce the problem where a bad
token is causing a good token to fail in client auth. It's probably some loop
looking up the certs which gets a nasty failure from one slot and aborts the
loop, but I'd like to track down where it's at.

In the meantime, Markus, would you like to try this patch out and see if it
solves your problem? If you can't build firefox yourself, I can supply you with
new NSS libraries.

bob

Comment 10

[Markus Heintel]

Of course I'd like to try your fix. I would prefer you sending me the new NSS 
libraries. 

Comment 11

[Markus Heintel]

Of course I'd like to try your fix. I would prefer you sending me the new NSS 
libraries. 

Comment 12

[Markus Heintel]

Fix works in our environment. The problem did not occur any longer on a system 
where we could reliably reproduce the problem without the fix applied.

Comment 13

[Robert Relyea]

Comment on attachment 193881 [details] [diff] [review]
Clear the 'Present flag if slot fails to refresh'

please review. needed for mozilla, so definately for 3.11, probably for 3.10.2
as well.

thanks.

Comment 14

[Robert Relyea]

adding nelson to cc list since I asked for a review... setting target to 3.10.2

Comment 15

[Robert Relyea]

BTW I believe wtc reopened this because the patch is not yet: 1) in the NSS
tree, nor 2) in the mozilla tree. Thanks for the test Markus.

bob

Comment 16

[Wan-Teh Chang]

Comment on attachment 193881 [details] [diff] [review]
Clear the 'Present flag if slot fails to refresh'

r=wtc.	I have some suggested changes to the new comment.

>+	/* the token has been removed, and reinserted, (or the slot contains
>+         * a token it doesn't recognize. invalidate all the old
>+	 * information we had on this token, if we can't refresh, set
>+	 * the present state to off */

Please do something about the opening parenthesis that doesn't
have a matching closing parenthesis.  You can just remove the '('.

I think "clear the 'present' flag" is easier to understand than
"set the present state to off".

Do you know what the pre-existing "XXX" comment means?

Earlier in the function, we have:

#ifdef PURE_STAN_CODE
    } else if (!slot->token) {
	/* token was not present at boot time, is now */
	slot->token = nssToken_Create(slot->slotID, slot);
	return (slot->token != NULL);
#endif

Should we also clear the 'present' flag if we return PR_FALSE
there (i.e., if the nssToken_Create call failed)?  (I know
PURE_STAN_CODE is dead code.)

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 193881 [details] [diff] [review]
Clear the 'Present flag if slot fails to refresh'

r=nelson
Without a lot more study of this stan code, I am unable to judge whether this
tiny change will fix the problems described in this bug, or not, but it seems
like a correct change. I do not believe it will be detrimental.

Comment 18

[Wan-Teh Chang]

Comment on attachment 193881 [details] [diff] [review]
Clear the 'Present flag if slot fails to refresh'

Bob, I am not sure if this bug fix is critical enough for
NSS 3.10.2, but I am confident that it has a very low risk.
For inclusion in NSS 3.10.2, please check this patch in today
because we will build NSS 3.10.2 Beta 1 tomorrow morning.

Comment 19

[Wan-Teh Chang]

Attachment: Patch as checked in

I checked in the fix on the NSS_3_10_BRANCH (NSS 3.10.2) and
NSS trunk (NSS 3.11) for Bob.  I took the liberty of making
the two comment changes I suggested.  Bob, feel free to edit
the comment as you see fit.

Comment 20

[Robert Relyea]

Thanks Wan-Teh... I wondered why I have having such a hard time finding this
bug. so I could get the fix checked in;).

bob

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment 21 was evidently attached to the wrong bug.  I marked it private to 
reduce confusion.