Closed
Bug 244914
Opened 20 years ago
Closed 20 years ago
NSS needs to provide applications access to "application private" objects.
Categories
(NSS :: Libraries, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
3.9.2
People
(Reporter: rrelyea, Assigned: rrelyea)
References
Details
Attachments
(3 files, 2 obsolete files)
10.76 KB,
patch
|
nelson
:
review-
|
Details | Diff | Splinter Review |
8.64 KB,
patch
|
nelson
:
review-
|
Details | Diff | Splinter Review |
8.46 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
Applications may need to access information in PKCS #11 modules that are private between the application and the module (not standard PKCS #11 objects or attributes). NSS needs to private some calls to access these objects and their attributes.
Assignee | ||
Comment 1•20 years ago
|
||
Functions are 1) PK11_FindGenericObject() takes a slot and a class and returns 1 object of the given class. 2) PK11_ReadRawAttribute() takes a type selector and a pointer to one of 4 object types (PK11SymKey, PK11GenericObject, SECKEYPublicKey, SECKKEYPrivateKey), CK_ATTRIBUTE_TYPE to read and a secItem to store the result. The caller is responsible to free the data in SECItem. 3) PK11_DestroyGenericObject() destroys the object returned from PK11_FindGenericObject().
Assignee | ||
Comment 2•20 years ago
|
||
Add functions to nss.def and error messages to secerr.h NOTE: this patch also contains changes which allow applications to determine if NSS is already initialized. This patch needs to be reviewed and approved (or not) separately.
Assignee | ||
Comment 3•20 years ago
|
||
Comment on attachment 149455 [details] [diff] [review] Add functions to access PKCS #11 object attributes directly patch 1 of 2 Please review .
Attachment #149455 -
Flags: superreview?(wchang0222)
Attachment #149455 -
Flags: review?(MisterSSL)
Assignee | ||
Comment 4•20 years ago
|
||
Comment on attachment 149456 [details] [diff] [review] Add functions to access PKCS #11 object attributes directly patch 2 of 2 please review
Attachment #149456 -
Flags: superreview?(wchang0222)
Attachment #149456 -
Flags: review?(MisterSSL)
Assignee | ||
Comment 5•20 years ago
|
||
bug 245915 is about telling if NSS is initialized or not. Which has a fix in this patch.
Target Milestone: --- → 3.9.2
Comment 6•20 years ago
|
||
Comment on attachment 149455 [details] [diff] [review] Add functions to access PKCS #11 object attributes directly patch 1 of 2 I have two problems with this patch, specifically with the new function PK11_FindGenericObject. Everything else about this patch is fine. The issues are: 1) (a small thing): the type of the objClass argument should be CK_OBJECT_CLASS, not CK_ULONG. 2) (the major thing): This API can only return the FIRST instance of an object of the specified class. NSS has made the mistake many times of introducing a new find function that can only return one instance of the found thing; then later a second function was added that can return multiple instances of that type of thing. An example of this is pk11_FindObjectByTemplate, which was followed by pk11_FindObjectsByTemplate. Let's not repeat this mistake. Let's define the new interface so that it is capable of returning all instances from day 1. If we do that, then we only need one function, not two.
Attachment #149455 -
Flags: review?(MisterSSL) → review-
Comment 7•20 years ago
|
||
Comment on attachment 149456 [details] [diff] [review] Add functions to access PKCS #11 object attributes directly patch 2 of 2 The changes to secerr.h and nssinit.c are fine. I have two issues with the changes to nss.def: 1) I think you're going to want to change the name PK11_FindGenericObject to PK11_FindGenericObjects. 2) This bug has a target fix version of 3.9.2, but you're adding the new function names to the NSS 3.10 section. So, either the bug needs to be retargetted to 3.10, or you need to be creating an NSS 3.9.2 section in which to add these names.
Attachment #149456 -
Flags: review?(MisterSSL) → review-
Comment 8•20 years ago
|
||
Comment on attachment 149455 [details] [diff] [review] Add functions to access PKCS #11 object attributes directly patch 1 of 2 One other comment related to this patch. Please put these new functions in some file other than pk11skey.c. These are not symmetric key crypto functions. Maybe we need a pk11misc.c file.
Assignee | ||
Comment 9•20 years ago
|
||
This combined patch addresses the issues Nelson brought up except 1: The new functions added to pk11skey.c. Since pk11skey.c has several 'other object' functions in it, I intend to address this problem with major surgery to pk11skey.c as part of bug 246130. The new file will have a someone meaningful name (probabley pk11obj.c) and will contain a whole lot more than just these functions. I'll attach a patch for this to bug 246130 next week. This patch has some additional helper functions to access the additional objects returned from PK11_FindGenericObjects(). bob
Assignee | ||
Updated•20 years ago
|
Attachment #149455 -
Attachment is obsolete: true
Attachment #149456 -
Attachment is obsolete: true
Assignee | ||
Comment 10•20 years ago
|
||
Comment on attachment 150496 [details] [diff] [review] Add functions to access PKCS #11 objects attributes directly, combined patch Thanks for your review of the previous patch. This patch obviously changes the previous API, and thus warrents another look. Thanks, bob
Attachment #150496 -
Flags: superreview?(wchang0222)
Attachment #150496 -
Flags: review?(MisterSSL)
Assignee | ||
Comment 11•20 years ago
|
||
Comment on attachment 149455 [details] [diff] [review] Add functions to access PKCS #11 object attributes directly patch 1 of 2 remove review request from an obsolute patch.
Attachment #149455 -
Flags: superreview?(wchang0222)
Assignee | ||
Updated•20 years ago
|
Attachment #149456 -
Flags: superreview?(wchang0222)
Comment 12•20 years ago
|
||
Comment on attachment 150496 [details] [diff] [review] Add functions to access PKCS #11 objects attributes directly, combined patch Part of this patch are r+ and parts are r-. The patches to files nssinit.c, debug_module.c, secmodt.h and secerr.h are all r+ and don't need to be re-reviewed along with the other parts of this patch. There's a problem with the new structure declaration in secmodti.h: >+struct PK11GenericObjectStr { >+ PK11GenericObject *next; >+ PK11SlotInfo *slot; >+ CK_OBJECT_HANDLE objectID; >+}; This new declaration will prevent this header from compiling unless the including .c file includes secmodt.h first. The fix is to change the declaration of the "next" pointer to NOT use the typedef. e.g. >+ struct PK11GenericObjectStr *next; Likewise, the declaration of the new slot pointer has the same problem. This source file doesn't define type PK11SlotInfo, and doesn't include the header file that does define it. Likewise, the CK_OBJECT_HANDLE isn't defined herein, nor included. I believe that every NSS header file should be able to be included all by itself. If a header needs symbols from other headers, it must include those other headers. So, the options are to a) add more #includes to secmodti.h, or b) move this struct declaration elsewhere, perhaps into the .c file where the methods that use it are found. However, my MAJOR comment about this patch concerns the new API for generic objects. I have several issues with it. 1. The "objects" in the list don't stand alone as objects. The caller can't keep some of the returned objects and discard others. The caller must copy any objects it wants to keep. 2. FindGenericObjects should FAIL if it experiences any errors, such as a memory failure, not silently ignore them. 3. CopyGenericObject needs to call PK11_ReferenceSlot to dup the slot reference. 4. DestroyGenericObject should delink the object from the list, but it cannot (because the list is singly-linked), so it should at least ASSERT that obj->next is NULL. I'd recommend the following changes to this API: a) use a doubly-linked list, rather than a singly-linked one. b) have DestroyGenericObject delink the object and then destroy it. c) make public functions to link and delink these objects, as well as aceessors for next and prev. d) eliminate the copy object function. Alteratively, don't use a linked list, but rather return an array of object pointers that are truely independent of each other.
Attachment #150496 -
Flags: review?(MisterSSL) → review-
Assignee | ||
Comment 13•20 years ago
|
||
The major change is to change the link list to a doubly linked list. As part of the change the following changes were made. New accessor function for the prev link. PK11_DestroyGenericObjects() destroys all the objects in a list that a given object lives in (that is you can destroy a list of objects by passing any object in the list, not just the head object). PK11_DestroyGenericObject() is now exported and will destroy a single object in the list. (NOTE: for an object that is not in a list, these two functions will be identical. Replaced Copy with and Unlink function. The goal is to keep the common case (a single object) as simple to use as possible without precluding using the interface to manage multiple objects (which is why I've avoided using arrays). I've also added the extra includes in secmodti.h. It's more of an existing code cleanup, as the header file would never have compilied without these two headers included before it anyway (actually secmodti.h is usually including implicitly by secmodi.h which includes these two headers itself -- actually only pk11wrap/dev3hack.c breaks this rule). Anyway I prefer this fix to changing to the 'struct XXXStr' syntax as it fits with current predominate style in NSS.
Comment 14•20 years ago
|
||
Comment on attachment 150994 [details] [diff] [review] Incremental patch, replaces nss.def, pk11func.h pk11skey.c and secmodti.h This patch is MUCH better, but there are still a few minor points. >+ * return a linked, circular list of generic objects. It's not circular. It's doubly-linked, but has two distinct ends. The code and the comment should agree about that. >+ * If you are only interested >+ * in one object, just use the first object in the list. To find the >+ * rest of the list use PK11_GetNextObject() to return the next object. No function exists by that name. It's PK11_GetNextGenericObject. >+ * >+ * You can walk the list with the following code: >+ * firstObj = PK11_FindGenericObjects(slot, objClass); >+ * for (thisObj=firstObj; thisObj; thisObj=PK11_GetNextObject(thisObj)) { Same issue: It's PK11_GetNextGenericObject >+ * /* operate on thisObj */ >+/* } >+ * >+ * If you want a particular object form the list... form -> from >+PK11GenericObject * >+PK11_FindGenericObjects(PK11SlotInfo *slot, CK_OBJECT_CLASS objClass) >+{ >+ CK_ATTRIBUTE template[1]; >+ CK_ATTRIBUTE *attrs = template; >+ CK_OBJECT_HANDLE *objectIDs = NULL; >+ PK11GenericObject *obj,*firstObj = NULL; >+ PK11GenericObject **linkObj, *prevObj; >+ int i, count = 0; [snip] >+ /* where we connect our object once we've created it.. */ >+ linkObj = &firstObj; >+ prevObj = NULL; >+ >+ for (i=0; i < count; i++) { >+ obj = PORT_New(PK11GenericObject); >+ if ( !obj ) { >+ PK11_DestroyGenericObjects(firstObj); >+ PORT_Free(objectIDs); >+ return NULL; >+ } >+ /* link it in */ >+ *linkObj = obj; >+ obj->prev = prevObj; >+ linkObj = &obj->next; >+ prevObj = obj; >+ >+ /* initialize it */ >+ obj->slot = PK11_ReferenceSlot(slot); >+ obj->objectID = objectIDs[i]; >+ obj->next = NULL; >+ } >+ PORT_Free(objectIDs); >+ return firstObj; >+} This code uses 4 pointers to build the linked list. I think you could do it with two pointers ("first" and "current") if the list was circular and you had a separate function to do the linking, or with 3 pointers (first, last, current) or (head, tail, current) if it isn't circular and you have a separate linking function. Please create a separate function that links a GenericObject into a chain, and then call it in the above Create Generic Objects function. The function should then be a simple loop, { build new object, link it in } Please export that new link insertion function. Might as well allow the user to keep his own list of these objects. >+ * This function removes a single object from the list and destroys it. >+ * For an already unlinked object there is no difference between >+ * PK11_DestroyGenericObject and PK11_DestroyGenericObjects It's nice and clean and easy to review because the unlinking is separate. >+ * walk down a link list of generic objects destroying them. >+ * This will destroy all objects in a list that the object is linked into. >+ * (the list is traversed in both directions). It looks right. It would be simpler (I think) if the list was circular. Then you'd only need to traverse in one direction. But that's not necessary. >Index: pk11wrap/secmodti.h >+#include "secmodt.h" >+#include "pkcs11t.h" As long as this doesn't create any circular include lists, it's fine.
Attachment #150994 -
Flags: review-
Assignee | ||
Comment 15•20 years ago
|
||
Implemented the comments from nelson.
Comment 16•20 years ago
|
||
Comment on attachment 151066 [details] [diff] [review] Latest incremental, replaces nss.def, pk11func.h and pk11skey.c r=nelson, or nelsonb, or misterssl, or whoever i am this week. :-)
Attachment #151066 -
Flags: review+
Assignee | ||
Comment 17•20 years ago
|
||
Check in log for the 3.9 Branch. Checking in lib/nss/nss.def; /cvsroot/mozilla/security/nss/lib/nss/nss.def,v <-- nss.def new revision: 1.128.4.1; previous revision: 1.128 done Checking in lib/nss/nssinit.c; /cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v <-- nssinit.c new revision: 1.64.16.1; previous revision: 1.64 done Checking in lib/pk11wrap/debug_module.c; /cvsroot/mozilla/security/nss/lib/pk11wrap/debug_module.c,v <-- debug_module. new revision: 1.8.38.1; previous revision: 1.8 done Checking in lib/pk11wrap/pk11func.h; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11func.h,v <-- pk11func.h new revision: 1.51.4.1; previous revision: 1.51 done Checking in lib/pk11wrap/pk11skey.c; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v <-- pk11skey.c new revision: 1.84.2.3; previous revision: 1.84.2.2 done Checking in lib/pk11wrap/secmodt.h; /cvsroot/mozilla/security/nss/lib/pk11wrap/secmodt.h,v <-- secmodt.h new revision: 1.20.16.1; previous revision: 1.20 done Checking in lib/pk11wrap/secmodti.h; /cvsroot/mozilla/security/nss/lib/pk11wrap/secmodti.h,v <-- secmodti.h new revision: 1.16.16.1; previous revision: 1.16 done Checking in lib/util/secerr.h; /cvsroot/mozilla/security/nss/lib/util/secerr.h,v <-- secerr.h new revision: 1.12.16.1; previous revision: 1.12 done
Assignee | ||
Comment 18•20 years ago
|
||
Checkin log for Tip: Checking in nss/nss.def; /cvsroot/mozilla/security/nss/lib/nss/nss.def,v <-- nss.def new revision: 1.132; previous revision: 1.131 done Checking in nss/nssinit.c; /cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v <-- nssinit.c new revision: 1.67; previous revision: 1.66 done Checking in pk11wrap/debug_module.c; /cvsroot/mozilla/security/nss/lib/pk11wrap/debug_module.c,v <-- debug_module.c new revision: 1.10; previous revision: 1.9 done Checking in pk11wrap/pk11func.h; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11func.h,v <-- pk11func.h new revision: 1.53; previous revision: 1.52 done Checking in pk11wrap/pk11skey.c; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v <-- pk11skey.c new revision: 1.88; previous revision: 1.87 done Checking in pk11wrap/secmodt.h; /cvsroot/mozilla/security/nss/lib/pk11wrap/secmodt.h,v <-- secmodt.h new revision: 1.22; previous revision: 1.21 done Checking in pk11wrap/secmodti.h; /cvsroot/mozilla/security/nss/lib/pk11wrap/secmodti.h,v <-- secmodti.h new revision: 1.18; previous revision: 1.17 done Checking in util/secerr.h; /cvsroot/mozilla/security/nss/lib/util/secerr.h,v <-- secerr.h new revision: 1.15; previous revision: 1.14 done
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 19•20 years ago
|
||
Adding Julien to bugs targetted for NSS 3.9 (even after the fact). bob
Comment 20•20 years ago
|
||
This commit have added a warning on brad TBox: pk11skey.c:5782: warning: `lastObj' might be used uninitialized in this function
Assignee | ||
Updated•20 years ago
|
Attachment #150496 -
Flags: superreview?(wchang0222)
Assignee | ||
Comment 21•18 years ago
|
||
*** Bug 142161 has been marked as a duplicate of this bug. ***
You need to log in
before you can comment on or make changes to this bug.
Description
•