All users were logged out of Bugzilla on October 13th, 2018

PK11_ListPublicKeysInSlot returns NULL key if slot contains token symmetric key

RESOLVED FIXED in 3.6

Status

P1
normal
RESOLVED FIXED
17 years ago
16 years ago

People

(Reporter: jamie-bugzilla, Assigned: rrelyea)

Tracking

x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Reporter)

Description

17 years ago
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

17 years ago
Actually I can't workaround so easily because SECKEY_RemovePublicKeyListNode
(the destructor) asserts if node->key==NULL.
(Assignee)

Comment 2

17 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

17 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

17 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

17 years ago
Created attachment 74008 [details] [diff] [review]
don't include 'failed' pubkeys in out list, don't include secret keys in priv or pub lists

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

17 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

17 years ago
Did it work before the patch?

bob
(Reporter)

Comment 8

17 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

17 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

17 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)

Updated

17 years ago
Blocks: 128172
(Reporter)

Comment 11

17 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
(Reporter)

Updated

17 years ago
Blocks: 128172
(Assignee)

Comment 12

17 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 13

17 years ago
Moved to 3.4.1.
Target Milestone: 3.4 → 3.4.1

Comment 14

17 years ago
Changed the QA contact to Bishakha.
QA Contact: sonja.mirtitsch → bishakhabanerjee

Comment 15

17 years ago
Moved to NSS 3.5.

Should we fix this bug?
Priority: -- → P1
Target Milestone: 3.4.1 → 3.5
(Reporter)

Comment 16

17 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.

Comment 17

17 years ago
Target 3.6.
Target Milestone: 3.5 → 3.6

Comment 18

16 years ago
This bug should be assigned to Bob.
Assignee: nicolson → relyea
(Assignee)

Comment 19

16 years ago
Jamie,

Do you have a simple test case, or do I need to run JCA to debug this.

bob
(Reporter)

Comment 20

16 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

16 years ago
Created attachment 100023 [details] [diff] [review]
Be able to look up SDR keys generally.

NOTE: This patch also has a patch for a memory leak on shutdown. (The
NSC_CloseAllSessions()) call.
(Assignee)

Comment 22

16 years ago
Can I get a review on this patch? Thanks,

bob

Comment 23

16 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

16 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

16 years ago
Created attachment 100467 [details]
test case

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

16 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

16 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

16 years ago
Created attachment 100657 [details] [diff] [review]
Successfully convert and display Token secret Keys.

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

16 years ago
Created attachment 100897 [details] [diff] [review]
Updated version of the pkcs11.c portion of patch 100657

Updated from our review.
(Assignee)

Comment 30

16 years ago
checked in.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 31

16 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.