Closed Bug 289530 Opened 15 years ago Closed 15 years ago

sftk_CopyObject doesn't copy token objects

Categories

(NSS :: Libraries, defect, P2, major)

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nelson, Assigned: nelson)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
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.

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
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.
Attachment #180109 - Attachment is obsolete: true
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.
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.
Priority: -- → P2
Target Milestone: --- → 3.10
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
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.
Attachment #180578 - Flags: review?(rrelyea)
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.
Attachment #180578 - Flags: review?(rrelyea) → review+
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
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
Status: NEW → ASSIGNED
retargetting to 3.11
Target Milestone: 3.10 → 3.11
QA Contact: bishakhabanerjee → jason.m.reid
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. 
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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.
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.
Attachment #192571 - Flags: review?(nelson)
Comment on attachment 192571 [details] [diff] [review]
Fix ECC compile errors

r=nelson@bolyard
Attachment #192571 - Flags: review?(nelson) → review+
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.
You need to log in before you can comment on or make changes to this bug.