The net impact of this bug is that it breaks SecretDecoderRing on hardware tokens. PK11_FreeSymKey will generally call C_DestroyObject on a symmetric key. If it is a token object, it will get deleted, which is NOT what we want in the case of SecretDecoderRing keys. PK11_FreeSymKey is called on all SDR keys by PK11SDR_Encrypt. This means SDR is completely broken. Or it would be, except that the problem is masked by another bug. If symKey->session is a read-only session, the DestroyObject call will fail with CKR_SESSION_READ_ONLY. This return value is ignored by PK11_FreeSymKey. It turns out that keys generated on the internal token always have read-only sessions. Keys generated on hardware tokens might have read-write sessions. Thus, token symmetric keys generated on hardware tokens might get clobbered by PK11_FreeSymKey immediately after they are created. The distinction seems to be in pk11_getKeyFromList, which tries to reuse symmetric key objects. If it reuses a symmetric key, it may, in some cases, reuse the old session, which might by a read-write session. If it has to create a new session, it will always create a read-only session. It turns out that it is reusing a read-write session for my hardware token, but always creates a new, read-only session when run against the internal token. It seems to me that we need to differentiate between freeing the PK11SymKey data structure, and deleting the underlying key. PK11_FreeSymKey does both, which is not what we want, except when the bug is is masked by a read-only session.
The target milestone will be changed to 3.7.2 when Bugzilla has it.
My workaround for this is to call PK11_ReferenceSymKey() right after calling PK11_TokenKeyGen, before calling PK11_FreeSymKey(). This effectively leaks the key, so it will never get cleaned up. This only helps when we generate the key. What about the next time we run the program, when we lookup the key? Bob, should I call PK11_ReferenceSymKey() after looking up the key as well?
symkey->owner is supposed to be False for Token keys.
Created attachment 111878 [details] [diff] [review] Token keys don't own their Object Handles. Token keys don't own their object handles, those handles are shared by anyone looking up and using those keys. They should not be destroyed on when the key is freed. Good Catch Jamie.
I tested this on my original test program, and it fixed the problem. Next I decided to test by having one program generate an SDR key and use it to create an SDR-encrypted blob, then have another program try to decrypt the blob. This would ensure that the key was surviving across program intances. This second test is hanging in the nCipher driver. It looks like a deadlock in their code. It is probably unrelated to this issue. It happens when we try to read the value of the symmetric key in order to get its length. The stack trace is: C_GetAttributeValue (CKA_VALUE) PK11_ReadAttribute PK11_ExtractKeyValue PK11_GetKeyLength I think Bob's patch probably does fix this bug, but now we have another bug that is preventing us from completely testing it. I will open another bug for this issue.
See also bug 189546.
OK, I'll check in this patch. bob
Fix checked into both the branch and the tip.
I worked around bug 189546 and was able to test Bob's patch completely. It worked. That is, the key did not get clobbered within the running program, and it was still present on the token in a later program instantiation.