Last Comment Bug 337010 - OOM crash [@ NSC_DigestKey] Dereferencing possibly NULL "att"
: OOM crash [@ NSC_DigestKey] Dereferencing possibly NULL "att"
Status: RESOLVED FIXED
FIPS [CID 533]
: coverity, crash
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.11
: All Linux
: P2 critical (vote)
: 3.12
Assigned To: Alexei Volkov
:
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-07 07:55 PDT by timeless
Modified: 2011-06-13 10:01 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
return error core if att is null (3.20 KB, patch)
2006-05-17 12:46 PDT, Alexei Volkov
nelson: review+
wtc: review+
Details | Diff | Review
change CKR values returned by NSC_DigestKey (993 bytes, patch)
2006-05-18 20:52 PDT, Nelson Bolyard (seldom reads bugmail)
wtc: review+
nelson: superreview+
Details | Diff | Review
changes according wtc comment (1.05 KB, patch)
2006-05-20 14:31 PDT, Alexei Volkov
nelson: review+
wtc: superreview+
Details | Diff | Review

Description timeless 2006-05-07 07:55:54 PDT
Event returned_null: Function "sftk_FindAttribute" returned NULL value (checked 62 out of 65 times) [model]
Comment 1 Alexei Volkov 2006-05-17 12:46:23 PDT
Created attachment 222397 [details] [diff] [review]
return error core if att is null
Comment 2 Nelson Bolyard (seldom reads bugmail) 2006-05-17 12:57:44 PDT
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) 2006-05-17 13:01:37 PDT
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) 2006-05-17 13:03:50 PDT
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) 2006-05-17 13:05:30 PDT
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 2006-05-17 13:09:49 PDT
Thanks, Nelson. I had some other changes in the directory. Will try to keep them
separate next time.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-05-17 13:58:46 PDT
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 2006-05-18 12:12:42 PDT
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 2006-05-18 13:56:44 PDT
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 2006-05-18 14:25:44 PDT
reopen the bug to prevent make it go off a radar.
Comment 11 Nelson Bolyard (seldom reads bugmail) 2006-05-18 14:38:36 PDT
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) 2006-05-18 20:52:12 PDT
Created attachment 222578 [details] [diff] [review]
change CKR values returned by NSC_DigestKey

Wan-Teh, is this what you hand in mind?
Comment 13 Wan-Teh Chang 2006-05-19 00:44:27 PDT
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 2006-05-20 14:31:29 PDT
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:

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 2006-05-21 15:07:50 PDT
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) 2006-05-22 15:38:05 PDT
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) 2006-05-22 15:39:31 PDT
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) 2006-05-22 15:40:27 PDT
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) 2006-06-08 18:32:02 PDT
retargetted to 3.12
Comment 20 Nelson Bolyard (seldom reads bugmail) 2006-06-10 18:10:18 PDT
CID 533
Comment 21 Alexei Volkov 2006-06-12 14:43:17 PDT
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) 2006-06-12 17:55:03 PDT
removing "more work needed" from status whiteboard, since this bug is now marked resolved/fixed

Note You need to log in before you can comment on or make changes to this bug.