Closed Bug 289530 Opened 15 years ago Closed 15 years ago
_Copy Object doesn't copy token objects
14.09 KB, patch
|Details | Diff | Splinter Review|
30.58 KB, patch
|Details | Diff | Splinter Review|
1.50 KB, patch
|Details | Diff | Splinter Review|
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: 184.108.40.206; previous revision: 220.127.116.11 pkcs11u.c; new revision: 18.104.22.168; 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.