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 | Splinter 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 | Splinter Review
changes according wtc comment (1.05 KB, patch)
2006-05-20 14:31 PDT, Alexei Volkov
nelson: review+
wtc: superreview+
Details | Diff | Splinter Review

Description User image 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 User image Alexei Volkov 2006-05-17 12:46:23 PDT
Created attachment 222397 [details] [diff] [review]
return error core if att is null
Comment 2 User image 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 User image 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 User image 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 User image 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 User image 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 User image :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 User image 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 User image 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 User image Alexei Volkov 2006-05-18 14:25:44 PDT
reopen the bug to prevent make it go off a radar.
Comment 11 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Nelson Bolyard (seldom reads bugmail) 2006-06-08 18:32:02 PDT
retargetted to 3.12
Comment 20 User image Nelson Bolyard (seldom reads bugmail) 2006-06-10 18:10:18 PDT
CID 533
Comment 21 User image 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 User image 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.