289530 - sftk_CopyObject doesn't copy token objects

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

sftk_CopyObject doesn't copy token objects.  Instead it returns CKR_DEVICE_ERROR.
This makes it impossible to copy token objects to session objects.  
Copying token objects to device objects is part of our proposed workaround
for bug 274538, so this is somewhat urgent.

Comment 1

[Robert Relyea]

Hmmm... I see some potential another issue with sftk_CopyObject, if the source
object is a session private key, then the attributes have been nulled out.

The solution to this is either 1) don't null out the attributes at line 1361 of
pkcs11.c, or 2) add code to sft_CopyObject to reconstruct these attributes from
the private key stored in objectInfo.

Comment 2

[Robert Relyea]

Attachment: Implement copy Object, 'unhide' RSA private key attributes that are sensitive

This is an untested patch.

It implements copy token object for all known objects that we store in the
database.
It fixes a presumed bug where RSA session private keys won't get copied
properly (I wonder have we ever tried this?).
It exposes the RSA sensitive key data to the internal routines. They should
still be opaque to applications since C_GetAttributes enforces the sensitive
check (clearing them out was just paranoia).

I don't know if nelson is working on a fix, since this was assigned to him, so
if the patch isn't useful, just discard it.

It has not been tested, it may have bugs. I attatch it in hopes of possibly
accellerating your developement.

bob

Comment 3

[Robert Relyea]

Attachment: OK a patch that compiles.

OK, I did build a version of the patch before I attached it, I just didn't
include the code with exposes the private key attributes. This patch builds.
everything else is pretty much the same.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Thanks Bob.  I spent the afternoon writing essentially the same patch, 
instead of reading my email.  :/  I stopped and read my email when I had 
handled only private key objects. :) Our two patches look quite different. 

One BIG difference is that my patch changes 
    sftk_FindRSAPrivateKeyAttribute, 
    sftk_FindDSAPrivateKeyAttribute, 
    sftk_FindDHPrivateKeyAttribute and  
    sftk_FindECPrivateKeyAttribute 
so that they all return the values for all their attributes.  
Previously they all returned sftk_StaticNullAttr for certain attributes
such as CKA_VALUE, which contain the private key's private value.  
Your patch changes sftk_FindRSAPrivateKeyAttribute but not the others.  
If that was an intentional omission, please tell me!

Your patch handled all the types of token objects, with explicit lists
of attributes for each type of key (e.g. RSA private, DSA public).  
I made a single list of all the possible key attributes, and only copied
the ones that were present.  This wastes some cycles, but I don't think
this will be a high frequency operation.  Your attribute copy loop in 
sftk_copyTokenAttributes aborts if any attribute is missing.  Where your
code returns CKR_ATTRIBUTE_INVALID, mine does "continue".

Your list of dsaPrivKeyAttrs only includes CKA_SUBPRIME.  I wonder if
you intended to have case CKK_DSA in sftk_CopyTokenPrivateKey fall through
into case CKK_DH since all of DH's attributes are common to DH and DSA. ??
Clearly the DSA priv key needs to get those other attributes, too.

Bob, you've done more work on this patch than I have.  Please feel free to
take this bug if you want it.  :)  Otherwise I'll merge our patches on 
Monday and attach that result here for your review.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Since this is just a bug fix (albeit a big one), we can get this into 3.10
if it gets done next week.  So, targetting 3.10.

Comment 6

[Robert Relyea]

My  comments:

1. Making the rest of the private key objects return their sensitive values was
just an over site. Keep your version of these.

2. As far as missing attributes from the list of attributes to copy, it probably
should assert when the attribute isn't found, since the list and the attributes
are internally generated... it really means there is something wrong with the
software if we wind up with a missing attribute.

For 'critical' attributes (those that must be supplied like modulus and
exponents), the current effect of our respective patches would be the same
failure, since the last step of the creation of the new object is to validate
that enough information has been set for us to have a valid object.

3. The DSA issue is probably from my copying the attributes from a switch
statement that didn't have a break in it, so I didn't copy enough.

Go ahead and merge your changes. 

bob

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch that succesfully completes the copy operation

Bob, This patch is the merger of your previous patch and mine, PLUS
a lot of code that essentially eliminates the use of the sftk_StaticXXX
attributes.  I'll explain that below.  

With this patch, I am able to complete a C_CopyObject call that copies a 
private key from token to session object.  But The session key is created
on the same RWsession used to access the token key, and when that session
is ended, the newly created session key vanishes with it.  I tried using
a different session number than the RWSession in the CopyObject call and
that failed, reporting some kind of invalid handle.  So, I need to figure
out how to copy the token key into a session object that is associated 
with a different session than the RWsession used for the token key 
(I think).

I had to add CKA_CLASS to the list of common attributes, otherwise the 
session object had no class attribute, and handleObject would barf.

The static attribute objects appear to always have type 0, that is the 
attribute type is 0, which is CKA_CLASS, so the act of copying the 
attributes from the token object to the session object caused the 
session object to get MANY CKA_CLASS attributes added to it.  
The solution was to create a new attribute with the right attribute type.
Then the session objects were complete and handleObject was happy.

Oh, one more thing:  Your list of commonKeyAttrs included 3 attributes that
were not found in the token private key objects.  They were 
CKA_START_DATE, CKA_END_DATE, CKA_LOCAL,
Rather than remove them from commonKeyAttrs or try to add them to private
keys, I simply made them optional.  If they are missing, C_CopyObject just
goes on and doesn't report an error.  We can later determine what is the
right thing to do with those attributes.

Comment 8

[Robert Relyea]

Comment on attachment 180578 [details] [diff] [review]
patch that succesfully completes the copy operation

r+ looks good.

Things to think about: We should identify those attributes that may be looked
up frequently either in softoken or pk11wrap and restore those to full static
objects to avoid the allocate/free (I suspect Token and maybe sensitive are hit
common enough to warrent special treatment, but we should probably measure to
see.).

CKA_START_DATE, CKA_END_DATE, and CKA_LOCAL should all be returned (even if the
return null objects for the first 2). Since the spec says every key object must
have these attributes. We can then return an error again in copy object.

Comment 9

[Robert Relyea]

RE sessions:

I already told nelson via email, but I'll repeat it in this bug.

For session private and public keys, use the targetSlot->session in the copy. Be
sure to get the SlotMonitor (which protects against multiple concurrent calls on
the same session). Symetric keys use their own session (and their own key monitor).

bob

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

I checked the above reviewed patch in on the performance branch.
I will check it into 3.10 trunk if I can get local approval.

pkcs11.c;  new revision: 1.99.2.2; previous revision: 1.99.2.1
pkcs11u.c; new revision: 1.66.2.1; previous revision: 1.66

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

retargetting to 3.11

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Checking in pkcs11.c;   new revision: 1.109; previous revision: 1.108
Checking in pkcs11u.c;  new revision: 1.67; previous revision: 1.66

Checked in on trunk. 

Comment 13

[Wan-Teh Chang]

Comment on attachment 180578 [details] [diff] [review]
patch that succesfully completes the copy operation

"CKK_ECC" in this patch should be "CKK_EC".  I will fix it.

Comment 14

[Wan-Teh Chang]

Attachment: Fix ECC compile errors

The compilation errors for NSS_ENABLE_ECC were all
due to an extra C ("ecc" and "ECC" as opposed to
"ec" and "EC").

I also fixed two unrelated signed/unsigned comparison
compiler warnings.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 192571 [details] [diff] [review]
Fix ECC compile errors

r=nelson@bolyard

Comment 16

[Wan-Teh Chang]

Comment on attachment 192571 [details] [diff] [review]
Fix ECC compile errors

Checking in pkcs11u.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11u.c,v	<--  pkcs11u.c
new revision: 1.68; previous revision: 1.67
done

Checked in on trunk.