130699 - PK11_ListPublicKeysInSlot returns NULL key if slot contains token symmetric key

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Jamie Nicolson]

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.

Comment 1

[Jamie Nicolson]

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

Comment 2

[Robert Relyea]

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

Comment 3

[Jamie Nicolson]

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.

Comment 4

[Robert Relyea]

OK, this is probably a bug in the softoken, though we should also make the
PK11_ListPublicKeysInSlot() more robust as well.

bob

Comment 5

[Robert Relyea]

Attachment: 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.

Comment 6

[Jamie Nicolson]

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.

Comment 7

[Robert Relyea]

Did it work before the patch?

bob

Comment 8

[Jamie Nicolson]

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

[Wan-Teh Chang]

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

Comment 10

[Robert Relyea]

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.

Comment 11

[Jamie Nicolson]

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.

Comment 12

[Robert Relyea]

Jamie are you in anytime today, so I can sit down and see what's happening with
the symetric keys.

thanks,

bob

Comment 13

[Wan-Teh Chang]

Moved to 3.4.1.

Comment 14

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 15

[Wan-Teh Chang]

Moved to NSS 3.5.

Should we fix this bug?

Comment 16

[Jamie Nicolson]

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

[Wan-Teh Chang]

Target 3.6.

Comment 18

[Wan-Teh Chang]

This bug should be assigned to Bob.

Comment 19

[Robert Relyea]

Jamie,

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

bob

Comment 20

[Jamie Nicolson]

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.

Comment 21

[Robert Relyea]

Attachment: 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.

Comment 22

[Robert Relyea]

Can I get a review on this patch? Thanks,

bob

Comment 23

[Wan-Teh Chang]

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

Comment 24

[Jamie Nicolson]

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.

Comment 25

[Jamie Nicolson]

Attachment: 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).

Comment 26

[Jamie Nicolson]

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

Comment 27

[Robert Relyea]

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.

Comment 28

[Robert Relyea]

Attachment: 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

Comment 29

[Robert Relyea]

Attachment: Updated version of the pkcs11.c portion of patch 100657

Updated from our review.

Comment 30

[Robert Relyea]

checked in.

Comment 31

[Wan-Teh Chang]

Comment on attachment 100897 [details] [diff] [review]
Updated version of the pkcs11.c portion of patch 100657

r=wtc.