The default bug view has changed. See this FAQ.

C_GetTokenInfo should return CKR_CRYPTOKI_NOT_INITIALIZED if not initialized

RESOLVED FIXED in 3.11.4

Status

NSS
Libraries
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: glen beasley, Assigned: glen beasley)

Tracking

unspecified
3.11.4

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

11 years ago
 
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Assignee: nobody → glen.beasley
Status: ASSIGNED → NEW
Glen, Please offer some clues about what additional values are desired.
(Assignee)

Comment 2

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

Updated

11 years ago
Attachment #241958 - Flags: review?(wtchang)

Comment 3

11 years ago
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.
Attachment #241958 - Flags: review?(wtchang) → review-
(Assignee)

Comment 4

11 years ago
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.
Attachment #241958 - Attachment is obsolete: true
Attachment #242020 - Flags: superreview?(neil.williams)
Attachment #242020 - Flags: review?(nelson)
(Assignee)

Comment 5

11 years ago
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. 

Status: NEW → ASSIGNED
Summary: FC_GetTokenInfo needs to support more return values → C_GetTokenInfo should return CKR_CRYPTOKI_NOT_INITIALIZED if not initialized
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

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

Comment 8

11 years ago
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.
Attachment #242077 - Flags: superreview?
Attachment #242077 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #242077 - Flags: superreview?(neil.williams)
Attachment #242077 - Flags: superreview?
Attachment #242077 - Flags: review?(nelson)
Attachment #242077 - Flags: review?
Comment on attachment 242077 [details] [diff] [review]
modified GetTokenInfo per nelson and wan-teh

r=nelson
Attachment #242077 - Flags: review?(nelson) → review+
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.
Attachment #242020 - Attachment is obsolete: true
Attachment #242020 - Flags: superreview?(neil.williams)
Attachment #242020 - Flags: review?(nelson)

Comment 11

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

Looks good.
Attachment #242077 - Flags: superreview?(neil.williams) → superreview+

Comment 12

11 years ago
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.
Attachment #242077 - Flags: superreview+ → superreview?(neil.williams)

Updated

11 years ago
OS: Solaris → All
Hardware: Sun → All

Updated

11 years ago
Attachment #242077 - Flags: superreview?(neil.williams) → superreview+
(Assignee)

Comment 13

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Target Milestone: --- → 3.11.4
You need to log in before you can comment on or make changes to this bug.