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



13 years ago
8 years ago


(Reporter: timeless, Assigned: alvolkov.bgs)


({coverity, crash})

coverity, crash

Firefox Tracking Flags

(Not tracked)


(Whiteboard: FIPS [CID 533], crash signature, URL)


(2 attachments, 1 obsolete attachment)



13 years ago
Event returned_null: Function "sftk_FindAttribute" returned NULL value (checked 62 out of 65 times) [model]
Hardware: PC → All
Target Milestone: --- → 3.11.2
Priority: -- → P2


13 years ago
Assignee: nobody → alexei.volkov.bugs


13 years ago
Whiteboard: FIPS

Comment 1

13 years ago
Created attachment 222397 [details] [diff] [review]
return error core if att is null
Attachment #222397 - Flags: review?(nelson)
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 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);
5830     crv = NSC_DigestUpdate(hSession,(CK_BYTE_PTR)att->attrib.pValue,
5831                            att->attrib.ulValueLen);
5832     sftk_FreeAttribute(att);
Attachment #222397 - Flags: review?(nelson) → review-
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
Attachment #222397 - Flags: review- → review+
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.
Attachment #222397 - Flags: superreview?(rrelyea)

Comment 6

13 years ago
Thanks, Nelson. I had some other changes in the directory. Will try to keep them
separate next time.
Changing the URL to a bonsai link, which will never "expire" (contrary to LXR links that may become useless if the source file changes).
Attachment #222397 - Flags: review?(wtchang)

Comment 8

13 years ago
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

This function returns CKR_KEY_TYPE_INCONSISTENT if
isn't a documented return value of C_DigestKey.  I think we
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).
Attachment #222397 - Flags: review?(wtchang) → review+

Comment 9

13 years ago
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. 

/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:; previous revision:
Last Resolved: 13 years ago
Resolution: --- → FIXED
Whiteboard: FIPS → FIPS "more work needed"

Comment 10

13 years ago
reopen the bug to prevent make it go off a radar.
Resolution: FIXED → ---
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. 
Created attachment 222578 [details] [diff] [review]
change CKR values returned by NSC_DigestKey

Wan-Teh, is this what you hand in mind?
Attachment #222578 - Flags: review?(wtchang)

Comment 13

13 years ago
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
key->objclass != CKO_SECRET_KEY case.
Attachment #222578 - Flags: review?(wtchang) → review+

Comment 14

13 years ago
Created attachment 222745 [details] [diff] [review]
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:


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:

Attachment #222397 - Attachment is obsolete: true
Attachment #222745 - Flags: superreview?(wtchang)
Attachment #222745 - Flags: review?(nelson)
Attachment #222397 - Flags: superreview?(rrelyea)

Comment 15

13 years ago
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);
     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

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
Attachment #222745 - Flags: superreview?(wtchang) → superreview+
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 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.
Attachment #222745 - Flags: review?(nelson) → review+
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)
Attachment #222578 - Flags: superreview+
retargetted to 3.12
Target Milestone: 3.11.2 → 3.12
CID 533
Whiteboard: FIPS "more work needed" → FIPS "more work needed" [CID 533]

Comment 21

13 years ago
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.
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
removing "more work needed" from status whiteboard, since this bug is now marked resolved/fixed
Whiteboard: FIPS "more work needed" [CID 533] → FIPS [CID 533]
Crash Signature: [@ NSC_DigestKey]
You need to log in before you can comment on or make changes to this bug.