Closed
Bug 130699
Opened 22 years ago
Closed 22 years ago
PK11_ListPublicKeysInSlot returns NULL key if slot contains token symmetric key
Categories
(NSS :: Libraries, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.6
People
(Reporter: jamie-bugzilla, Assigned: rrelyea)
References
Details
Attachments
(5 files)
2.33 KB,
patch
|
Details | Diff | Splinter Review | |
2.53 KB,
patch
|
Details | Diff | Splinter Review | |
6.00 KB,
application/octet-stream
|
Details | |
6.04 KB,
patch
|
Details | Diff | Splinter Review | |
2.79 KB,
patch
|
wtc
:
review+
|
Details | Diff | Splinter Review |
The internal token stores token symmetric keys by creating a dummy public key in the database. PK11_ListPublicKeysInSlot will come across this key, try to extract it with PK11_ExtractPublicKey, get back NULL from this function because it's not a real public key, and then store NULL in the public key list. (pk11cert.c:3622) The function should check for the NULL from ExtractPublicKey and should not enter it in the list in that case. I can work around this by checking for NULL public keys in the list and skipping them.
Reporter | ||
Comment 1•22 years ago
|
||
Actually I can't workaround so easily because SECKEY_RemovePublicKeyListNode (the destructor) asserts if node->key==NULL.
Assignee | ||
Comment 2•22 years ago
|
||
If you authenticate to the token, does everything work then? (not that that invalidates the bug, but it might be a work around for you). bob
Reporter | ||
Comment 3•22 years ago
|
||
I already am authenticating to the token. Note: this problem also occurs if I setup SecretDecoderRing on an otherwise clean database, then try to list the public keys.
Assignee | ||
Comment 4•22 years ago
|
||
OK, this is probably a bug in the softoken, though we should also make the PK11_ListPublicKeysInSlot() more robust as well. bob
Assignee | ||
Comment 5•22 years ago
|
||
On the first half is needed for this bug, however Jamie is running into a second problem with priv keys. The second half fixed the problem where we get false positives for pubkeys, but only if we've logged in, so the first patch is still needed.
Reporter | ||
Comment 6•22 years ago
|
||
I believe this patch has the effect of making all the token symmetric keys invisible. Specifically, I setup secret decoder ring by doing an SDR_Encrypt. Then, using the same databases, I tried to list the token objects (after logging in to the token). PK11_ListFixedKeysInSlot returned NULL.
Assignee | ||
Comment 7•22 years ago
|
||
Did it work before the patch? bob
Reporter | ||
Comment 8•22 years ago
|
||
OK, what was happening before the patch was that the symmetric key was being found, but it was found by ListPrivateKeys instead of ListFixedKeys. This patch prevented the key as being found by ListPrivateKeys, so now it isn't found at all. So there is an additional problem that PK11_ListFixedKeys is not finding the symmetric key.
Comment 9•22 years ago
|
||
Comment on attachment 74008 [details] [diff] [review] don't include 'failed' pubkeys in out list, don't include secret keys in priv or pub lists >- if (classFlags & NSC_KEY) { >+ if ((classFlags & NSC_KEY) && isSecretKey(privKey)) { Should there be a ! before isSecretKey(privKey)?
Assignee | ||
Comment 10•22 years ago
|
||
No, NSC_KEY flag means we are looking for a secret key. So if we are looking for a secret key, and it is a secret key, then add it to the list. The other if statements are looking for Public and Private keys, so if we are looking for one of them, and it's not a secret key, add it to the list.
Reporter | ||
Comment 11•22 years ago
|
||
Where we are now: 1. The first patch worked to keep the token symmetric key from being listed as a private or public key. That's good. 2. The token symmetric key is not, and never was, showing up in PK11_ListFixedKeysInSlot. We still need a fix for the second problem. This bug blocks 128172: "function to convert a session key to a token key", because the symmetric key is no longer accessible after turning it into a token key.
No longer blocks: 128172
Target Milestone: --- → 3.4
Assignee | ||
Comment 12•22 years ago
|
||
Jamie are you in anytime today, so I can sit down and see what's happening with the symetric keys. thanks, bob
Comment 14•22 years ago
|
||
Changed the QA contact to Bishakha.
QA Contact: sonja.mirtitsch → bishakhabanerjee
Comment 15•22 years ago
|
||
Moved to NSS 3.5. Should we fix this bug?
Priority: -- → P1
Target Milestone: 3.4.1 → 3.5
Reporter | ||
Comment 16•22 years ago
|
||
Yes we still need to fix this bug. It blocks 128172, which is important for JSS to properly implement the JCA KeyStore class.
Assignee | ||
Comment 19•22 years ago
|
||
Jamie, Do you have a simple test case, or do I need to run JCA to debug this. bob
Reporter | ||
Comment 20•22 years ago
|
||
You should be able to reproduce this with the following steps: 1. Apply the above patch and rebuild NSS. 2. Setup SDR with PK11_SDREncrypt(). 3. Call PK11_ListFixedKeysInSlot(). It won't list the symmetric key that was just created for SDR. The problem is that token symmetric keys aren't getting listed by PK11_ListFixedKeysInSlot. I'll try to write up a little test program and post it as an attachment.
Assignee | ||
Comment 21•22 years ago
|
||
NOTE: This patch also has a patch for a memory leak on shutdown. (The NSC_CloseAllSessions()) call.
Assignee | ||
Comment 22•22 years ago
|
||
Can I get a review on this patch? Thanks, bob
Comment 23•22 years ago
|
||
Comment on attachment 100023 [details] [diff] [review] Be able to look up SDR keys generally. Bob, what is the relation of this patch to the previous patch? The previous patch also modifies pk11cert.c. 1. There is already a for loop in nscFreeAllSlots. Can you merge this new for loop into the old for loop (by adding the line (void) NSC_CloseAllSessions(slotID); to the old for loop? 2. The other changes seem fine to me but you should ask Jamie or Terry to review them. The int variable 'found' should be declared as a PRBool, initialized to PR_FALSE, and 'found++' replaced by 'found = PR_TRUE'.
Reporter | ||
Comment 24•22 years ago
|
||
I'm trying out this patch, and now I can list the SDR key, but I can't list another token symmetric key that I imported (using PK11_ConvertSessionSymKeyToTokenSymKey). I think the key got imported successfully because it fails to import another key with the same name (the DBM "put" call fails the second time). I'll try and figure out a little better what's going on. Or maybe I can write a pure-C test case that you can debug.
Reporter | ||
Comment 25•22 years ago
|
||
Here is a test case, in this tar file. You have to change the first line of the makefile to point at your own dist directory. You may also have to change the PLATFORM variable. This test case generates a symmetric key, converts it to a token key, and prints out a list of all symmetric token keys before and after. I expect it to list the newly-stored key, but it does not. It does, however, print out the SDR key. In order to run this patch, you will have to apply the latest patch for bug 128172 (http://bugzilla.mozilla.org/attachment.cgi?id=100344&action=view), and also enable CKM_AES_KEY_GEN on the database token (as in bug 170423).
Reporter | ||
Comment 26•22 years ago
|
||
If I run the testcase once on an empty database, and then do "strings key3.db", I can see the nickname of the key (e.g. "symkey #1282825267"). If I run the testcase again, the original nickname is there, but the second key nickname does not show up in strings. Similarly, if I continue running the test case, only the very first key's nickname shows up with "strings key3.db".
Assignee | ||
Comment 27•22 years ago
|
||
I'm getting multiple changes in the same file trying to clear out some of the bugs. The memory leak is a fix for another problem. My original patch was to use the existing for look, however, the sessions need to be cleared out before we clobber the global slot_id table, so it has to happen as a separate call before we clear out the table. I'll look at Jamie's test case. perhaps the convert session object to token object isn't working correctly.
Assignee | ||
Comment 28•22 years ago
|
||
With this patch, Jamie's test program now works. The problem was the new keys did not have a CKA_ID, so they could not be extracted (the code was generating bad DER data on storage because it was trying to store a zero length value in a non-optional field). This patch was hand editted to remove other changes that are not related to this bug. If we can get a review on this, I can clean up my tree and get these changes checked in;). bob
Assignee | ||
Comment 29•22 years ago
|
||
Updated from our review.
Assignee | ||
Comment 30•22 years ago
|
||
checked in.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 31•22 years ago
|
||
Comment on attachment 100897 [details] [diff] [review] Updated version of the pkcs11.c portion of patch 100657 r=wtc.
Attachment #100897 -
Flags: review+
You need to log in
before you can comment on or make changes to this bug.
Description
•