Last Comment Bug 356073 - C_GetTokenInfo should return CKR_CRYPTOKI_NOT_INITIALIZED if not initialized
: C_GetTokenInfo should return CKR_CRYPTOKI_NOT_INITIALIZED if not initialized
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: unspecified
: All All
: -- normal (vote)
: 3.11.4
Assigned To: glen beasley
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-10-09 15:33 PDT by glen beasley
Modified: 2006-10-12 18:13 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
added checks for not initialized, sftk_fatalError, and isLoggedIn (716 bytes, patch)
2006-10-11 09:38 PDT, glen beasley
wtc: review-
Details | Diff | Splinter Review
when the softoken is not initialize C_GetTokenInfo returns CKR_CRYPTOKI_NOT_INITIALIZED (1.85 KB, patch)
2006-10-11 19:29 PDT, glen beasley
no flags Details | Diff | Splinter Review
modified GetTokenInfo per nelson and wan-teh (2.03 KB, patch)
2006-10-12 10:00 PDT, glen beasley
nelson: review+
neil.williams: superreview+
Details | Diff | Splinter Review

Description glen beasley 2006-10-09 15:33:06 PDT
 
Comment 1 Nelson Bolyard (seldom reads bugmail) 2006-10-10 01:04:24 PDT
Glen, Please offer some clues about what additional values are desired.
Comment 2 glen beasley 2006-10-11 09:38:15 PDT
Created attachment 241958 [details] [diff] [review]
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 2006-10-11 13:34:15 PDT
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 2006-10-11 19:29:23 PDT
Created attachment 242020 [details] [diff] [review]
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 2006-10-11 19:31:37 PDT
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) 2006-10-12 00:34:32 PDT
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 2006-10-12 07:05:41 PDT
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 2006-10-12 10:00:08 PDT
Created attachment 242077 [details] [diff] [review]
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) 2006-10-12 14:19:51 PDT
Comment on attachment 242077 [details] [diff] [review]
modified GetTokenInfo per nelson and wan-teh

r=nelson
Comment 10 Nelson Bolyard (seldom reads bugmail) 2006-10-12 14:20:34 PDT
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 2006-10-12 14:44:56 PDT
Comment on attachment 242077 [details] [diff] [review]
modified GetTokenInfo per nelson and wan-teh

Looks good.
Comment 12 Julien Pierre 2006-10-12 14:52:52 PDT
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 2006-10-12 15:39:09 PDT
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

Note You need to log in before you can comment on or make changes to this bug.