Closed Bug 254393 Opened 20 years ago Closed 20 years ago

PK11_FindKeyByAnyCert always returns a key object even if there is no key

Categories

(NSS :: Libraries, defect, P1)

3.9.2
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julien.pierre, Assigned: julien.pierre)

Details

Attachments

(1 file, 1 obsolete file)

Our appserver won't start with NSS 3.9.2 RTM ; but it started sucessfully with
our 3.9.2 beta2 snapshot taken on 6/18/2004 .

I have narrowed down the regression to the following check-in in pk11cert.c,
which is unfortunately quite large. To make things harder, a single check-in was
made for two bugs.

revision 1.132
date: 2004/06/21 23:01:52;  author: relyea%netscape.com;  state: Exp;  lines:
+101 -79

Bug 244914, 244907 r=nelsonb
Add support for unprotected private keys without requiring authentication.
Add support to access application specific PKCS #11 objects through NSS.

The problem occurs with SEC_PKCS12AddCertAndKey failing.
I traced this and the failure was in adding the key object. The private key
object had a null key type . I will post more info with a detailed stack later.
 An NFS server just went down and I can't post it now ...
The error returned is SEC_ERROR_PKCS12_UNABLE_TO_EXPORT_KEY .

Here is the deepest stack where the error happens.

=>[1] pk11_private_key_encrypt_buffer_length(key = 0x629458), line 5084 in
"pk11skey.c"
  [2] PK11_ExportEncryptedPrivKeyInfo(slot = 0xdd128, algTag =
SEC_OID_PKCS12_V2_PBE_WITH_SHA1_AND_3KEY_TRIPLE_DES_CBC, pwitem = 0xffbfc544, pk
= 0x629458, iteration = 1, wincx = 0xffbfc704), line 5196 in "pk11skey.c"
  [3] PK11_ExportEncryptedPrivateKeyInfo(slot = 0xdd128, algTag =
SEC_OID_PKCS12_V2_PBE_WITH_SHA1_AND_3KEY_TRIPLE_DES_CBC, pwitem = 0xffbfc544,
cert = 0x609c40, iteration = 1, wincx = 0xffbfc704), line 5290 in "pk11skey.c"
  [4] SEC_PKCS12AddKeyForCert(p12ctxt = 0x627038, safe = 0x6270b0, nestedDest =
(nil), cert = 0x609c40, shroudKey = 1, algorithm =
SEC_OID_PKCS12_V2_PBE_WITH_SHA1_AND_3KEY_TRIPLE_DES_CBC, pwitem = 0x647500,
keyId = 0x211e3c, nickName = (nil)), line 1312 in "p12e.c"
  [5] SEC_PKCS12AddCertAndKey(p12ctxt = 0x627038, certSafe = 0x627108,
certNestedDest = (nil), cert = 0x609c40, certDb = 0xdb228, keySafe = 0x6270b0,
keyNestedDest = (nil), shroudKey = 1, pwitem = 0x647500, algorithm =
SEC_OID_PKCS12_V2_PBE_WITH_SHA1_AND_3KEY_TRIPLE_DES_CBC), line 1525 in "p12e.c"

At that point, we have :
(.../dist/share/forte_dev,v6.2/SUNWspro/bin/../WS6U2/bin/sparcv9/dbx) p *key 
*key = {
    arena        = 0x210a68
    keyType      = nullKey
    pkcs11Slot   = 0xdd128
    pkcs11ID     = 0
    pkcs11IsTemp = 1
    wincx        = 0xffbfc704

The pkcs11ID is invalid, and so is the keyType .

I found that the regression is in the function PK11_FindKeyByAnyCert . With
Bob's patch, it always returns a pointer to a SECKEYPrivateKey* object, even if
the CK_OBJECT_HANDLE is 0. This is incorrect. A NULL pointer should be returned.

This breaks the app server because at the beginning, it enumerates all the
private keys for all the certs. And later it tries to back them up to a PKCS#12
file, but this fails because the keys don't actually exist.

By fixing this function, the fictitious keys are no longer found, and the
application only backs up the keys that are actually there to the PKCS#12 file,
and thus succeeds.

Patch fortcoming.
Summary: Regression in pk11cert.c → PK11_FindKeyByAnyCert always returns a key object even if there is no key
Attachment #155318 - Flags: superreview?(nelson)
Attachment #155318 - Flags: review?(bugz)
Comment on attachment 155318 [details] [diff] [review]
don't return a private key object if CK_OBJECT_HANDLE is invalid

>+    if (keyHandle == CK_INVALID_HANDLE) { 
>+        goto loser;
>     }
>     privKey =  PK11_MakePrivKey(slot, nullKey, PR_TRUE, keyHandle, wincx);
> loser:

I think I'd prefer that to be:

      if (keyHandle != CK_INVALID_HANDLE) { 
	  privKey = PK11_MakePrivKey(slot, nullKey, PR_TRUE, keyHandle, wincx);
      }
loser:

But either way, r=nelson
Attachment #155318 - Flags: superreview?(nelson)
Attachment #155318 - Flags: superreview?(bugz)
Attachment #155318 - Flags: review?(bugz)
Attachment #155318 - Flags: review+
Nelson,

Thanks for the review and the suggestion. I have checked in a variant on it that
allows the elimination of the evil goto statements.

NSS_3_9_BRANCH :

Checking in pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.128.4.4; previous revision: 1.128.4.3
done

And tip :
Checking in pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.137; previous revision: 1.136
done
Eliminate gotos
Attachment #155318 - Attachment is obsolete: true
Attachment #155318 - Flags: superreview?(bugz)
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Priority: -- → P1
Target Milestone: --- → 3.9.3
You need to log in before you can comment on or make changes to this bug.