356073 - C_GetTokenInfo should return CKR_CRYPTOKI_NOT_INITIALIZED if not initialized

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[glen beasley]

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Glen, Please offer some clues about what additional values are desired.

Comment 2

[glen beasley]

Attachment: added checks for not initialized, sftk_fatalError, and isLoggedIn

another patch would be to add 
    if (!nsf_init) {
        return CKR_CRYPTOKI_NOT_INITIALIZED;
    }
to static CK_RV sftk_fipsCheck(void)

Comment 3

[Wan-Teh Chang]

Comment on attachment 241958 [details] [diff] [review]
added checks for not initialized, sftk_fatalError, and isLoggedIn

We should not call SFTK_FIPSCHECK() here.
The indentation of the closing brace } is off by one.

I hope we can do the check in NSC_GetTokenInfo instead
so that we fix the non-FIPS mode, too, but it's not clear
what's the right test to perform.

Comment 4

[glen beasley]

Attachment: when the softoken is not initialize C_GetTokenInfo returns CKR_CRYPTOKI_NOT_INITIALIZED

if the softoken is not initialize C_GetTokenInfo will return CKR_CRYPTOKI_NOT_INITIALIZED in either fips or non-fips mode. 
In sftk_SlotFromId added check to ensure the nscSlotHashTable is 
intialized.

Comment 5

[glen beasley]

Changed Summary of bug. The description of the bug is C_GetTokenInfo should return CKR_CRYPTOKI_NOT_INITIALIZED if C_Initialize has not been called. 

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 242020 [details] [diff] [review]
when the softoken is not initialize C_GetTokenInfo returns CKR_CRYPTOKI_NOT_INITIALIZED

This is not my final review comment, but is a question:

>Index: pkcs11.c

> CK_RV NSC_GetTokenInfo(CK_SLOT_ID slotID,CK_TOKEN_INFO_PTR pInfo)
> {
>     SFTKSlot *slot = sftk_SlotFromID(slotID, PR_FALSE);
>     NSSLOWKEYDBHandle *handle;
> 
>+    if (!nsc_init && !nsf_init) return CKR_CRYPTOKI_NOT_INITIALIZED;

Is slot a reference?  If we return here, do we leak that reference?
Should we call sftk_SlotFromID only after we're sure that it's initialized?

>     if (slot == NULL) return CKR_SLOT_ID_INVALID;

Comment 7

[Wan-Teh Chang]

Comment on attachment 242020 [details] [diff] [review]
when the softoken is not initialize C_GetTokenInfo returns CKR_CRYPTOKI_NOT_INITIALIZED

I also recommend writing the beginning of NSC_GetTokenInfo like this:

     SFTKSlot *slot;
     NSSLOWKEYDBHandle *handle;
 
     if (!nsc_init && !nsf_init) return CKR_CRYPTOKI_NOT_INITIALIZED;
     slot = sftk_SlotFromID(slotID, PR_FALSE);
     if (slot == NULL) return CKR_SLOT_ID_INVALID;

This will make the change to sftk_SlotFromID unnecessary.

Comment 8

[glen beasley]

Attachment: modified GetTokenInfo per nelson and wan-teh

I left the change in sftk_SlotFromID because In debugging the problem I also caused my test program to crash by initializing correctly and then passing in an incorrect slotID. With the change to sftk_SlotFromID, the user would recieve  
CKR_SLOT_ID_INVALID instead of crashing.

I figure since we want to prevent users from crashing if they call GetTokenInfo 
incorrectly before they initialize, then we also should try to prevent user error after they have initialized.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 242077 [details] [diff] [review]
modified GetTokenInfo per nelson and wan-teh

r=nelson

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 242020 [details] [diff] [review]
when the softoken is not initialize C_GetTokenInfo returns CKR_CRYPTOKI_NOT_INITIALIZED

Marking this patch obsolete, since another patch has now apparently superseded it.

Comment 11

[Neil Williams]

Comment on attachment 242077 [details] [diff] [review]
modified GetTokenInfo per nelson and wan-teh

Looks good.

Comment 12

[Julien Pierre]

Comment on attachment 242077 [details] [diff] [review]
modified GetTokenInfo per nelson and wan-teh

Testing nsc_init or nsf_init outside of acquiring any lock is not safe for non-TSO platforms.
On those platforms, another thread may have done a C_Initialize and set the flag to PR_TRUE, but the thread doing C_GetTokenInfo will see the value of the flag as being PR_FALSE unless the cache is synchronized, by acquiring a lock.

Similarly, a C_Finalize could have completed in another thread and set the flag to PR_FALSE, but the thread doing C_GetTokenInfo will see the flag as being PR_TRUE unless the cache is synchronized by acquiring a lock.

Even for platforms that have total store order, there could still be a race where another thread does a C_Finalize after C_GetTokenInfo has tested the flag. If the intent of this patch is to fix a crash by preventing access to some structures that are only available after initialization, then this patch doesn't completely fix it, merely reduce the probability of crash.

Comment 13

[glen beasley]

julien, per comment #12 wan-teh and I had a previous conversation and decided not to add
the lock protection, as this issue is really protecting against user error, 
and is generally a non-issue. 

Checking in pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.135; previous revision: 1.134
done

Checking in fipstokn.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstokn.c,v  <--  fipstokn.c
new revision: 1.24; previous revision: 1.23
done

Checking in pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.112.2.20; previous revision: 1.112.2.19
done
Checking in fipstokn.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstokn.c,v  <--  fipstokn.c
new revision: 1.11.2.13; previous revision: 1.11.2.12
done