189364 - PK11_FreeSymKey clobbers token symmetric key

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Jamie Nicolson]

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

[Wan-Teh Chang]

The target milestone will be changed to 3.7.2 when
Bugzilla has it.

Comment 2

[Jamie Nicolson]

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

[Robert Relyea]

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

Comment 4

[Robert Relyea]

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

[Jamie Nicolson]

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.

Comment 6

[Jamie Nicolson]

See also bug 189546.

Comment 7

[Robert Relyea]

OK, I'll check in this patch.

bob

Comment 8

[Robert Relyea]

Fix checked into both the branch and the tip.

Comment 9

[Jamie Nicolson]

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.