PK11_FreeSymKey clobbers token symmetric key



16 years ago
16 years ago


(Reporter: jamie-bugzilla, Assigned: rrelyea)


Firefox Tracking Flags

(Not tracked)



(1 attachment)



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

Comment 1

16 years ago
The target milestone will be changed to 3.7.2 when
Bugzilla has it.
Priority: -- → P1
Whiteboard: [3.7.2]
Target Milestone: --- → 3.8


16 years ago
Whiteboard: [3.7.2]
Target Milestone: 3.8 → 3.7.2

Comment 2

16 years ago
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?

Comment 3

16 years ago
symkey->owner is supposed to be False for Token keys.

Comment 4

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

Comment 5

16 years ago
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)

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.

Comment 6

16 years ago
See also bug 189546.

Comment 7

16 years ago
OK, I'll check in this patch.


Comment 8

16 years ago
Fix checked into both the branch and the tip.
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 9

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


16 years ago
Target Milestone: 3.7.2 → 3.7.1
You need to log in before you can comment on or make changes to this bug.