134095 - nssckfw PKCS#11 framework has no way of freeing attributes

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

If a PKCS#11 module is built on top of lib/ckfw and has non-const objects, there 
is a problem with getting attributes . nssckmdt.h specifies that a const NSSItem 
is returned by the GetAttribute function of the module :

   /*
    * This routine returns the specified attribute.  It can return
    * NULL upon error.  The pointer in the item will not be freed;
    * any host memory required should come from the object's arena
    * (which is likely the Framework's token or session arena).
    * It may return NULL on error.
    */
   const NSSItem *(PR_CALLBACK *GetAttribute)(
     NSSCKMDObject *mdObject,
     NSSCKFWObject *fwObject,
     NSSCKMDSession *mdSession,
     NSSCKMDInstance *mdInstance,
     NSSCKFWInstance *fwInstance,
     CK_ATTRIBUTE_TYPE attribute,
     CK_RV *pError
   );

However, the suggestion in the comment to use an arena in the object isn't 
workable for token objects, because there is no locking around a token object, 
and the module can't lock around its arena itself because it returns the object 
and doesn't get called again after CKFW uses the value and makes its own copy 
of the value.

We don't want to have an unnecessary arena lock either. What's needed is a way 
to tell the framework whether the attribute returned is const or not, and if 
not, call back into the module to free the attribute.

Comment 1

[Julien Pierre]

Attachment: Proposed patch

Includes patches for ckfw and for the builtins to conform to the new signatures

Comment 2

[Wan-Teh Chang]

Fixes for NSS 3.4 must be checked into both the
trunk and the NSS_3_4_BRANCH.

Please have Bob review your patch.  Thanks.

Comment 3

[Wan-Teh Chang]

Comment on attachment 76673 [details] [diff] [review]
Proposed patch

I have one stylistic suggestion.  The existing code
in these nssckfw files seems to indent by two spaces.
It would be a good idea to emulate that style so that
we have a consistent style within each file (even
though there are multiple styles in NSS sources, sigh).

1. nssckmdt.h

>+typedef struct {
>+    PRBool needsFreeing;
>+    NSSItem item;
>+} NSSCKFWItem ;

2. object.c

>-  if( (NSSItem *)NULL == mdItem ) {
>-    if( CKR_OK == *pError ) {
>-      *pError = CKR_GENERAL_ERROR;
>-    }
>+  if (CKR_OK != *pError) {
>+      goto done;
>+  }

Comment 4

[Julien Pierre]

Bob also voiced some concerns to me about the semantics being slightly changed 
since I can't check for a NULL NSSItem* anymore as I have it as part of my 
structure.

Here is a much lower impact patch, which will preserve the old behavior (and 
also the indentation ;).

Comment 5

[Julien Pierre]

Attachment: better patch

Comment 6

[Robert Relyea]

Comment on attachment 76681 [details] [diff] [review]
better patch

OK, the patch looks good. In the future I think we probably should replace the
'needs freeing' with a MD defined structure, but for now this looks good.

bob

Comment 7

[Wan-Teh Chang]

Comment on attachment 76681 [details] [diff] [review]
better patch

In object.c, around line 667

>+  if (PR_TRUE == mdItem.needsFreeing) {
>+    if (fwObject->mdObject->FreeAttribute) {
>+      *pError = fwObject->mdObject->FreeAttribute(&mdItem);
>+    }
>+  }

What do you do if mdItem.needsFreeing is true but
fwObject->mdObject->FreeAttribute is null?  It seems
that you should return a failure status.

Comment 8

[Wan-Teh Chang]

Comment on attachment 76681 [details] [diff] [review]
better patch

In nssckmdt.h

>   /*
>-   * This routine returns the specified attribute.  It can return
>-   * NULL upon error.  The pointer in the item will not be freed;
>-   * any host memory required should come from the object's arena
>-   * (which is likely the Framework's token or session arena).
>-   * It may return NULL on error.
>+   * This routine returns an NSSCKFWItem structure.
>+   * The NSSItem contains the attribute value, and the needsFreeing bit
>+   * tells the framework whether to call the FreeAttribute function .
>    */

It might be clearer to say "The item pointer points to an
NSSItem containing the attribute value".

You should describe what will be returned on error (an
NSSCKFWItem structure with a NULL item pointer).

Comment 9

[Julien Pierre]

Wan-Teh,

>What do you do if mdItem.needsFreeing is true but
>fwObject->mdObject->FreeAttribute is null?  It seems
>that you should return a failure status.

I'd like to, but I don't see how.
There is no CKR_LEAKED_MEMORY in pkcs11t.h :-) Should I return CKR_HOST_MEMORY 
in *pError?

But really there is no way to recover from this. We don't know ahead of time if 
needsFreeing is going to be PR_TRUE . We only know after the value is returned 
to us. Then all we can do is call the free function if the module implements it. 
I can add an assert in an else statement for the case where the module does not 
implement it.
I'm not sure what else to do. There is a copy of the item's value in rv. Do you 
think we should free that and return NULL ? That still won't prevent the leak 
from the copy that was allocated in the module's GetAttribute.
I think it is up to the module to implement a FreeAttribute if it sets 
needsFreeing = PR_TRUE . The built-ins don't do either, which is fine.

Comment 10

[Julien Pierre]

OK, the new comment says : 

  /*
   * This routine returns an NSSCKFWItem structure.
   * The item pointer points to an NSSItem containing the attribute value.
   * The needsFreeing bit tells the framework whether to call the
   * FreeAttribute function . Upon error, an NSSCKFWItem structure
   * with a NULL NSSItem item pointer will be returned
   */

As far as the object.c code, this is the way I have it now.

  if (PR_TRUE == mdItem.needsFreeing) {
    PR_ASSERT(fwObject->mdObject->FreeAttribute);
    if (fwObject->mdObject->FreeAttribute) {
      *pError = fwObject->mdObject->FreeAttribute(&mdItem);
    } else {
        /* bad, bad module : it allocated an attribute
        but does not know how to free it. a memory leak will result
        There is no way to prevent the leak, though . But report an
        error so it doesn't go unnoticed on opt builds. On dbg we
        assert above for the FreeAttribute function */
        if (rv) {
            nss_ZFreeIf(rv);
        }
        *pError = CKR_HOST_MEMORY;
    }

Comment 11

[Julien Pierre]

Attachment: Patch as checked in

Comment 12

[Julien Pierre]

Checked in before the midnight cron job to make it for the next nightly. I hope 
the clock is not ahead.

Checking in nssckmdt.h;
/cvsroot/mozilla/security/nss/lib/ckfw/nssckmdt.h,v  <--  nssckmdt.h
new revision: 1.3; previous revision: 1.2
done
Checking in object.c;
/cvsroot/mozilla/security/nss/lib/ckfw/object.c,v  <--  object.c
new revision: 1.8; previous revision: 1.7
done
Checking in builtins/bobject.c;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/bobject.c,v  <--  bobject.c
new revision: 1.2; previous revision: 1.1
done

A copy of the patch I ended up checking in is also attached.

Comment 13

[Julien Pierre]

Marking fixed.

Comment 14

[Wan-Teh Chang]

Comment on attachment 76709 [details] [diff] [review]
Patch as checked in

Seems that we don't need NSSCKFWItem.  All we need is:

1. change GetAttribute to return a non-const NSSItem *;

>-  const NSSItem *(PR_CALLBACK *GetAttribute)(
>+  NSSItem *(PR_CALLBACK *GetAttribute)(

2. add FreeAttribute;

>+  /*
>+   * This routine returns CKR_OK if the attribute could be freed.
>+   */
>+  CK_RV (PR_CALLBACK *FreeAttribute)(
>+    NSSItem *item
>   );

Such "free" functions usually return void.  I don't know
if a NSSCKFW function can return void.	Bob?

3. and invoke FreeAttribute if it is not NULL.

>+  if (fwObject->mdObject->FreeAttribute) {
>+    *pError = fwObject->mdObject->FreeAttribute(mdItem);
>+  }

Comment 15

[Robert Relyea]

The module wants to return both static and alocated data on a data element
basis, not on a module basis.

Otherwise, you are correct, it would be simpiler just to free the data if a free
function was supplied and the the MD code determine what it wants/needs to do.

bob

Comment 16

[Wan-Teh Chang]

Comment on attachment 76709 [details] [diff] [review]
Patch as checked in

>+      /* bad, bad module : it allocated an attribute
>+      but does not know how to free it. a memory leak will result
>+      There is no way to prevent the leak, though . But report an
>+      error so it doesn't go unnoticed on opt builds. On dbg we
>+      assert above for the FreeAttribute function */
>+      if (rv) {
>+        nss_ZFreeIf(rv);
>+      }
>+      *pError = CKR_HOST_MEMORY;

This should say

	if( (NSSItem *)NULL == itemOpt ) {
	  nss_ZFreeIf(rv);
	}
	rv = (NSSItem *)NULL;
	*pError = <some error code>;

where <some error code> probably should be CKR_GENERAL_ERROR,
which is the error code the same function uses if
fwObject->mdObject->GetAttribute is NULL.

In any case, I think the assertion you added is sufficient,
so I am going to delete this whole block of code.

Comment 17

[Wan-Teh Chang]

Attachment: Patch as checked in

I checked in this patch on the NSS_3_4_BRANCH and made the
equivalent change to the tip.

Comment 18

[Kai Engert [:KaiE:]]

Any chance this has caused a regression?

I suspect that, because I don't see other checkins to NSS recently.

I see a problem that all certs in the Mozilla CA tab are missing, and in fact
various crypto functionality complains about missing trust.

Ian just told me he sees a similar problem with certutil. The certs show up
there, but without trust.

Comment 19

[Wan-Teh Chang]

Kai,

I just installed the 20020402 trunk build of Netscape
6.2.1+ and it shows all the root CA certs correctly.
I can also log into my Etrade and Wells Fargo accounts
with HTTPS with no warnings about missing trust.

I suspect that you may be bitten by the lack of header
file dependencies in NSS and your problem will go away
after you do a clean rebuild in mozilla/security/nss.
Could you try that?

Comment 20

[Julien Pierre]

Closing this as it was resolved and was determined not to regress either with 
the server or the latest client build today. Kai, please reopen if you find that 
not to be the case.

Comment 21

[Julien Pierre]

Verified fixed