67132 - certutil -C command crashes (and fails to use -a)

Status: RESOLVED FIXED

(NSS :: Tools, enhancement, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

certutil -C command crashes on HPUX, and probably other platforms.
Looks like it's been broken since before 3.0.

The problem is this.  in main(), privkey is never generated for 
the -C command, but is passed in to CreateCert, which passes it
in to SignCert, which crashes on the null pointer.

I think this patch should fix it.

diff -u -r1.20 certutil.c
--- certutil.c  2001/01/25 04:14:22     1.20
+++ certutil.c  2001/01/31 05:11:17
@@ -2540,6 +2540,7 @@
     /*  These commands require keygen.  */
     if (certutil.commands[cmd_CertReq].activated ||
         certutil.commands[cmd_CreateAndAddCert].activated ||
+        certutil.commands[cmd_CreateNewCert].activated ||
        certutil.commands[cmd_GenKeyPair].activated) {
        /*  XXX Give it a nickname.  */
        privkey =

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

That patch is _not_ the solution.  Rather than generating
a new keypair, certutil should be reusing a previously
generated one, probably one identified via the public key 
in the binary cert request.  

Comment 2

[Ian McGreer]

summarizing user comments from email:

User has a token with a key pair on it.  User has used that key pair to generate 
a certificate request.  User then wishes to use certutil to self-sign the 
request.

Based on the usage/documentation for certutil, this should be a possibility.  
However, it is not something I have ever tried or something that has been 
tested.  Our QA uses the following certutil semantics:

generate a self-signed cert with certutil -S -x
generate cert requests with certutil -R
sign the requests using the self-signed cert and certutil -C

Using certutil -C -x is simply a combination that has been overlooked.

Actually, I believe I know what the problem is.  I think using the -x option has 
always meant "generate a key pair".  However, at one time there was a "-k 
shortkeyid" option for finding keys by the first few bytes of their modulus.  
This option floated around in a different version of certutil, and relyea said 
that it should not be included because we wanted to move away from referring to 
keys by their short key id.

What certutil needs here is a way to locate the key.  In this case, that 
involves a combination of a token name (using -h, which works) and then some way 
to identify the key, which is lacking.  However, this is an even more special 
case, since the cert request contains the public key, so it would simply be a 
matter of using PKCS#11 to find the matching private key on the token (which 
shouldn't be difficult).

Bob, do you have any suggestions?  I'm going to look into this.  I think (in the 
course of writing this) that certutil -C -x should mean "sign this request using 
the private key that matches the public key in the request."  I don't think 
certutil was ever able to do that, but it seems reasonable enough.

Comment 3

[Ian McGreer]

pk11func.h has the following functions that return a private key:

SECKEYPrivateKey *PK11_GenerateKeyPair(PK11SlotInfo *slot,
   CK_MECHANISM_TYPE type, void *param, SECKEYPublicKey **pubk,
		 	    PRBool isPerm, PRBool isSensitive, void *wincx);
SECKEYPrivateKey *PK11_MakePrivKey(PK11SlotInfo *slot, KeyType keyType,
                        PRBool isTemp, CK_OBJECT_HANDLE privID, void *wincx);
SECKEYPrivateKey * PK11_FindPrivateKeyFromCert(PK11SlotInfo *slot,
				 	CERTCertificate *cert, void *wincx);
SECKEYPrivateKey * PK11_FindKeyByAnyCert(CERTCertificate *cert, void *wincx);
SECKEYPrivateKey * PK11_FindKeyByKeyID(PK11SlotInfo *slot, SECItem *keyID,
				       void *wincx);
SECKEYPrivateKey *PK11_FindKeyByDERCert(PK11SlotInfo *slot, 
					CERTCertificate *cert, void *wincx);
SECKEYPrivateKey * PK11_FindPrivateKeyFromNickname(char *nickname, void *wincx);

Since there is no certificate yet, it looks like the only one that will work is 
PK11_FindKeyByKeyID (also assuming that the key does not have a "nickname" 
attribute set).  But this gets back to referring to keys using the short key id.  
Bob, a more specific question is, is there any way to use a public key to find a 
private key in a slot?  Or do we need to use the short key id?

Comment 4

[Robert Relyea]

Yes, the solution is to look up the private key based on the public key. This is
very doable because that's how we find a private key based on the cert (public
key is extracted from the cert and used to look up the private key.

The correct low level function is PK11_FindKeybyKeyID(). There is another
function (I'd have to search the source) which makes a keyID from a public key.
What we probably need, though, is to define a new PK11 functions which takes a
public key and finds the private key that uses PK11_FindPrivateKeyByKeyID().

bob

Comment 5

[Ian McGreer]

I tried the following patch:

Index: certutil.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v
retrieving revision 1.20
diff -c -r1.20 certutil.c
*** certutil.c	2001/01/25 04:14:22	1.20
--- certutil.c	2001/01/31 19:02:54
***************
*** 1901,1906 ****
--- 1901,1907 ----
  static SECStatus
  CreateCert(
  	CERTCertDBHandle *handle, 
+ 	PK11SlotInfo *slot,
  	char *  issuerNickName, 
  	PRFileDesc *inFile,
  	PRFileDesc *outFile, 
***************
*** 1993,1998 ****
--- 1994,2011 ----
  
  	CERT_FinishExtensions(extHandle);
  
+ 	/* self-signing the cert request, find priv key on token */
+ 	if (selfsign && selfsignprivkey == NULL) {
+ 	    /*SECItem *keyId = PK11_GetKeyIDFromCert(subjectCert, NULL);*/
+ 	    SECItem *keyId = PK11_MakeIDFromPubKey(&subjectCert->derPublicKey);
+ 	    selfsignprivkey = PK11_FindKeyByKeyID(slot, keyId, NULL);
+ 	    if (selfsignprivkey == NULL) {
+ 		fprintf(stderr, "Failed to find private key to sign with.\n");
+ 		rv = SECFailure;
+ 		break;
+ 	    }
+ 	}
+ 
  	certDER = SignCert (handle, subjectCert, selfsign, selfsignprivkey, 
issuerNickName,pwarg);
  
  	if (certDER)
***************
*** 2152,2158 ****
      char        commandToRun    = '\0';
      secuPWData  pwdata          = { PW_NONE, 0 };
  
!     SECKEYPrivateKey *privkey;
      SECKEYPublicKey *pubkey = NULL;
  
      int i;
--- 2165,2171 ----
      char        commandToRun    = '\0';
      secuPWData  pwdata          = { PW_NONE, 0 };
  
!     SECKEYPrivateKey *privkey = NULL;
      SECKEYPublicKey *pubkey = NULL;
  
      int i;
***************
*** 2602,2608 ****
      /*  Create a certificate (-C or -S).  */
      if (certutil.commands[cmd_CreateAndAddCert].activated ||
           certutil.commands[cmd_CreateNewCert].activated) {
! 	rv = CreateCert(certHandle, 
  	                certutil.options[opt_IssuerName].arg,
  	                inFile, outFile, privkey, &pwdata,
  	                serialNumber, warpmonths, validitylength,
--- 2615,2621 ----
      /*  Create a certificate (-C or -S).  */
      if (certutil.commands[cmd_CreateAndAddCert].activated ||
           certutil.commands[cmd_CreateNewCert].activated) {
! 	rv = CreateCert(certHandle, slot,
  	                certutil.options[opt_IssuerName].arg,
  	                inFile, outFile, privkey, &pwdata,
  	                serialNumber, warpmonths, validitylength,

I generated a key using certutil and then attempted to sign a cert request with 
it using certutil -C -x.  I thought I could "fake" a token by using the internal 
slot, but I don't believe that will work (I don't think it will find keys by id 
on the internal token).  So I think the above code *should* work for a real 
token, but I'm not sure.  Does it look correct to you bob?  Julien, do you want 
to try this patch?

Comment 6

[Robert Relyea]

The only way to find keys in any token is by ID. Either the ID is generated from
the public key, or it is matched by looking up the cert and matching the cert's ID.

In any case this code should work for both internal and external tokens. If it
doesn't work for the internal token there's still a bug somewhere.

bob

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

The cert request contains the public key.  The code should extract
the public key from the cert request and lookup the private key
by that.  

Does "subjectCert->derPublicKey point to the public key extracted
from the cert request??

Comment 8

[Ian McGreer]

Whilst we debate the validity of using the -C -x combination, I'm renaming this 
bug.  The fact that -C disregards -a (ascii input) is clearly a bug, and I will 
endeavor to have that fixed by 3.2.  As far as the original intent of the bug, I  
believe the goal is to have NSS report an error claiming that -C -x is invalid, 
and remove that combination from the usage.  Kirk, I believe you volunteered to 
do that?

Comment 9

[Ian McGreer]

okay, I have a patch that fixed -C -a and reports an error on -C -x (I figured I 
was in there already so I would just do it, to avoid merging problems).  Anyone 
care to review?

Comment 10

[Ian McGreer]

Attachment: aforementioned patch

Comment 11

[Ian McGreer]

Attachment: ignore the last one, it was the wrong one (has missing ;)

Comment 12

[Wan-Teh Chang]

I reviewed Ian's patch.  It looks fine.

Comment 13

[Ian McGreer]

patch checked in.

Not marking this bug as fixed, since I consider the -C -x combination to be an 
open issue still.  But at this point I think it's fair to call it an 
enhancement.

-C -a now works as expected.

Comment 14

[Ian McGreer]

Per wtc's request, marking this as fixed so it shows up in NSS 3.2's bugs fixed.  
Note the crash and missing feature described in the subject are both fixed.  The 
continued review of -C -x will occur in bug 67890.