Last Comment Bug 189364 - PK11_FreeSymKey clobbers token symmetric key
: PK11_FreeSymKey clobbers token symmetric key
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.7
: All All
P1 blocker (vote)
: 3.7.1
Assigned To: Robert Relyea
: Bishakha Banerjee
Depends on:
  Show dependency treegraph
Reported: 2003-01-16 14:09 PST by Jamie Nicolson
Modified: 2003-01-17 17:59 PST (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Token keys don't own their Object Handles. (590 bytes, patch)
2003-01-17 17:00 PST, Robert Relyea
no flags Details | Diff | Splinter Review

Description User image Jamie Nicolson 2003-01-16 14:09:51 PST
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 User image Wan-Teh Chang 2003-01-16 17:09:50 PST
The target milestone will be changed to 3.7.2 when
Bugzilla has it.
Comment 2 User image Jamie Nicolson 2003-01-17 13:27:33 PST
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 User image Robert Relyea 2003-01-17 16:48:19 PST
symkey->owner is supposed to be False for Token keys.
Comment 4 User image Robert Relyea 2003-01-17 17:00:53 PST
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 User image Jamie Nicolson 2003-01-17 17:39:06 PST
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 User image Jamie Nicolson 2003-01-17 17:45:01 PST
See also bug 189546.
Comment 7 User image Robert Relyea 2003-01-17 17:48:29 PST
OK, I'll check in this patch.

Comment 8 User image Robert Relyea 2003-01-17 17:57:02 PST
Fix checked into both the branch and the tip.
Comment 9 User image Jamie Nicolson 2003-01-17 17:58:55 PST
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.

Note You need to log in before you can comment on or make changes to this bug.