337010 - OOM crash [@ NSC_DigestKey] Dereferencing possibly NULL "att"

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[timeless]

Event returned_null: Function "sftk_FindAttribute" returned NULL value (checked 62 out of 65 times) [model]

Comment 1

[Alexei Volkov]

Attachment: return error core if att is null

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

The original URL given for this bug was for a different function, 
in a different source file, than the function named in the bug "summary".
I guess we can go forward by relying on the bug summary alone.  :/

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 222397 [details] [diff] [review]
return error core if att is null

Alexei, I think you attached the wrong patch to this bug.

This bug complains about the following code in NSC_DigestKey:

5826     /* get the key value */
5827     att = sftk_FindAttribute(key,CKA_VALUE);

complaint is that att could be NULL here.

5828     sftk_FreeObject(key);
5829 
5830     crv = NSC_DigestUpdate(hSession,(CK_BYTE_PTR)att->attrib.pValue,
5831                            att->attrib.ulValueLen);
5832     sftk_FreeAttribute(att);

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 222397 [details] [diff] [review]
return error core if att is null

Oh, wait, I see this patch has two parts, one of which is irrelevant to 
this bug, and a second part that is relevant.
r=nelson for the part in lib/softoken/pkcs11c.c

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 222397 [details] [diff] [review]
return error core if att is null

Asking Bob Relyea for second review on this softoken change.
Bob, this is a coverity bug.

Comment 6

[Alexei Volkov]

Thanks, Nelson. I had some other changes in the directory. Will try to keep them
separate next time.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Changing the URL to a bonsai link, which will never "expire" (contrary to LXR links that may become useless if the source file changes).

Comment 8

[Wan-Teh Chang]

Comment on attachment 222397 [details] [diff] [review]
return error core if att is null

r=wtc for the change to lib/softoken/pkcs11c.c in this patch.

I suggest that you return the error code CKR_KEY_INDIGESTIBLE
instead of CKR_KEY_HANDLE_INVALID.

This function returns CKR_KEY_TYPE_INCONSISTENT if
key->objclass != CKO_SECRET_KEY.  But CKR_KEY_TYPE_INCONSISTENT
isn't a documented return value of C_DigestKey.  I think we
should also return CKR_KEY_INDIGESTIBLE or CKR_KEY_HANDLE_INVALID
in that case.  PKCS #11 v2.20 says:

  CKR_KEY_HANDLE_INVALID: The specified key handle is not valid.
  It may be the case that the specifie handle is a valid handle
  for an object which is not a key.  We reiterate here that 0 is
  never a valid key handle.

  CKR_KEY_INDIGESTIBLE: This error code can only be returned by
  C_DigestKey.  It indicates that the value of the specified key
  cannot be digested for some reason (perhaps the key isn't a
  secret key, or perhaps the token simply can't digest this kind
  of key).

Comment 9

[Alexei Volkov]

Patch is modified to return CKR_KEY_INDIGESTIBLE. Didn't replace 
CKR_KEY_TYPE_INCONSISTENT as it requires multiple function modification
in the file. Updated the whiteboard with "more work needed" keyword for future refs. 

trunk:
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.83; previous revision: 1.82

3.11 branch:
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.68.2.13; previous revision: 1.68.2.12

Comment 10

[Alexei Volkov]

reopen the bug to prevent make it go off a radar.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Alexei,
your comment 9 above says "Patch is modified to return CKR_KEY_INDIGESTIBLE.", 
but your checkin to lib/softoken/pkcs11c.c still returns CKR_KEY_HANDLE_INVALID. 

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Attachment: change CKR values returned by NSC_DigestKey

Wan-Teh, is this what you hand in mind?

Comment 13

[Wan-Teh Chang]

Comment on attachment 222578 [details] [diff] [review]
change CKR values returned by NSC_DigestKey

r=wtc.  Yes, this is what I had in mind, except
that I'm not sure if we should return
CKR_KEY_HANDLE_INVALID instead in the
key->objclass != CKO_SECRET_KEY case.

Comment 14

[Alexei Volkov]

Attachment: changes according wtc comment

I'd like to get your suggestion on what to do with the rest of places in pkcs11c.c where we return CKR_KEY_TYPE_INCONSISTENT. For example:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/pkcs11c.c&rev=1.81&mark=3987,3957,3958,3959,3960#3945

Nelson wrote:
We should make sure that CKR_KEY_INDIGESTIBLE gets transdlated into a reasonable error code.

Well, we don't translate CKR_KEY_INDIGESTIBLE. And I guess we should not do so,
as it is the defined API error code. But CKR_KEY_TYPE_INCONSISTENT does get translated:

pk11err.c:91: MAPERROR(CKR_KEY_TYPE_INCONSISTENT, SEC_ERROR_INVALID_KEY)

Comment 15

[Wan-Teh Chang]

Comment on attachment 222745 [details] [diff] [review]
changes according wtc comment

r=wtc.  By the way, I'm having second thoughts
about this change:

     /* get the key value */
     att = sftk_FindAttribute(key,CKA_VALUE);
     sftk_FreeObject(key);
     if (!att) {
-        return CKR_KEY_HANDLE_INVALID;        
+        return CKR_KEY_INDIGESTIBLE;
     }

It seems that CKR_KEY_HANDLE_INVALID is also a
reasonable error code there.  Bob, what do you
think?

I think we should map CKR_KEY_INDIGESTIBLE.  If
we don't have a good error code, let's just map
it to the closest error code for now. Even
SEC_ERROR_INVALID_KEY would be much better than
the default error code SEC_ERROR_IO.

You don't need to review the other places where
we return CKR_KEY_TYPE_INCONSISTENT.  They may
not be wrong.  The error codes that may be returned
by each PKCS #11 C_xxx function are documented in
PKCS #11 v2.20, so you can consult it to see if
CKR_KEY_TYPE_INCONSISTENT is a possible error code
for each of those functions.

You asked about NSC_WrapKey.  PKCS #11 v2.20 says
C_WrapKey does not return CKR_KEY_TYPE_INCONSISTENT.
So perhaps we should return CKR_KEY_NOT_WRAPPABLE
or CKR_KEY_HANDLE_INVALID instead.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 15, 
CKR_KEY_HANDLE_INVALID means the handle is not a valid object handle
for any key type object in the module.  

If a program gets an object handle from C_FindObjects, with a template that 
specifies a key object, then that application should not subsequently get 
CKR_KEY_HANDLE_INVALID when it attempts to use the handle that was givien to 
it by C_FindObjects.  

A key object without a CKA_VALUE attribute is a malformed object, but that
does not make the handle invalid.   

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 222745 [details] [diff] [review]
changes according wtc comment

I can live with either of the two patches to pkcs11c.c that are attached to this bug.  I would prefer the other one to this one.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 222578 [details] [diff] [review]
change CKR values returned by NSC_DigestKey

Since this patch was suggested by Wan-Teh, I will give it r+ (even though I attached it)

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

retargetted to 3.12

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

CID 533

Comment 21

[Alexei Volkov]

Closing the bug as fixed. I've with pkcs11 2.20:

KEY_TYPE_INCONSISTENT can be returned by C_WrapKey:

CKR_WRAPPING_KEY_TYPE_INCONSISTENT: This value can only be returned
by C_WrapKey. It indicates that the type of the key specified to wrap another key is
not consistent with the mechanism specified for wrapping.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

removing "more work needed" from status whiteboard, since this bug is now marked resolved/fixed