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

RESOLVED FIXED in 3.9.3

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: Julien Pierre, Assigned: Julien Pierre)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

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

Comment 2

13 years ago
Created attachment 155318 [details] [diff] [review]
don't return a private key object if CK_OBJECT_HANDLE is invalid
(Assignee)

Updated

13 years ago
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+
(Assignee)

Comment 4

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

Comment 5

13 years ago
Created attachment 155323 [details] [diff] [review]
patch as checked in

Eliminate gotos
Attachment #155318 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #155318 - Flags: superreview?(bugz)
(Assignee)

Updated

13 years ago
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Updated

13 years ago
Priority: -- → P1
Target Milestone: --- → 3.9.3
You need to log in before you can comment on or make changes to this bug.