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)

3.9.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rrelyea, Assigned: rrelyea)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
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().
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.
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)
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)
bug 245915 is about telling if NSS is initialized or not. Which has a fix in 
this patch.
Target Milestone: --- → 3.9.2
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 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 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.
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
Attachment #149455 - Attachment is obsolete: true
Attachment #149456 - Attachment is obsolete: true
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)
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)
Attachment #149456 - Flags: superreview?(wchang0222)
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-
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 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-
Implemented the comments from nelson.
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+
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
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
Adding Julien to bugs targetted for NSS 3.9 (even after the fact).

bob
This commit have added a warning on brad TBox:

pk11skey.c:5782: warning: `lastObj' might be used uninitialized in this function
Attachment #150496 - Flags: superreview?(wchang0222)
*** 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.