certutil cannot delete orphan private keys

NEW
Unassigned

Status

NSS
Tools
P4
enhancement
13 years ago
3 years ago

People

(Reporter: Julien Pierre, Unassigned)

Tracking

(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

25.24 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review-
Details | Diff | Splinter Review
7.18 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review-
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
certutil has the ability to create keys on a token during keygen or CSR creation.
Currently, there is no way to delete those keys. One has to obtain a cert for
the key first, and then delete that cert and key at the same time.

I propose to add an option to delete a key by giving the CSR as input. In that
case, certutil should check that there is no cert for that key and ask for
confirmation if there is one.

For keys that were only generated, without a CSR, or if the CSR was lost, I'm
not sure what the right UI is. Currently, certutil only prints a number, which
is the index in a for loop, without any information about each key. There is not
even a CKA_LABEL for standalone orphan private keys. We could print the CKA_ID
next to the index. Then certutil could take the index as input to a key delete
function. The CKA_ID would be displayed along with a confirmation prompt in that
case as well.
QA Contact: bishakhabanerjee → jason.m.reid
Assignee: wtchang → nobody
QA Contact: jason.m.reid → tools
Priority: -- → P4
(Reporter)

Updated

12 years ago
Assignee: nobody → biswatosh.chakraborty

Comment 1

11 years ago
(In reply to comment #0)
Julien,

Could you elaborate on the following lines in ur comment #0 ? I still am not very clear with CKA_ID and CKA_LABEL.

1)Why do you expect a CKA_LABEL for standalone orphan private key that is generated by certutil ? 

2)Could you elaborate a little on ur following lines? 
>We could print the CKA_ID next to the index. Then certutil could take the index >as input to a key delete function. The CKA_ID would be displayed along with a >confirmation prompt in that case as well.

My confusion is: Are we using PKCS11 while generating keys and certs through certutil? If not, then how is CKA_ID is coming to the picture? Do I have to compute it based on the hash of the modulus? 
I am sorry for asking this trivial question.
(Reporter)

Comment 2

11 years ago
Biswatosh,

CKA_ID is a PKCS#11 attribute on private kes and certificate objects. This is how these objects are associated.

CKA_LABEL is another PKCS#11 attribute typically used for naming objects.

1) It's possible the certificate was deleted, but not the private key. In this case, the orphan private key may have a CKA_LABEL - the same as the certificate; although I'm not sure we actually ever set CKA_LABEL on the key. In any case, we can't expect it to be present, since in some cases, if there was never a certificate, we know for sure it won't be set.

2) We are using PKCS#11 for generating keys and certs through certutil. 
IIRC, Once upon a time, long ago, certutil (or perhaps it was keyutil) had 
the ability to identify keys by their CKA_IDs in Hex.  certutil had a 
command line option whose argument was the CKA_ID.  IIRC, it was not 
necessary to enter the entire ID to identify a key.  You only had to enter 
enough to be unique.  This feature was removed from certutil/keyutil, and 
the present scheme of identifying keys indirectly through cert nicknames 
took its place.  If we're going to bring this feature back, IMO the feature
should use the same command line syntax as before.

Bob, do you concur?

Comment 4

11 years ago
yes.;)

bob

Comment 5

11 years ago
This is just to show the progress and thus not for review. But, any suggestions would be great!

I have added some new functions and now certutil with these finctions can display pvt keys with their CKA_IDs in hexa form(upper case). Similarly, certutil can find and delete pvt key based on CKA_ID in hexa form(upper case).
So, when u run certutil -K -d db, it will give:
<0>  E517EB630E7C140B508B3BCF61856EFEB62CB97C 
<1>  75D460340D4F46986B45FDC823A8F7DA204EB2D7 
<2>  6306667A7A688C51F131F23D53D3886E76B75188 
...
And, when u want to delete the 2nd key, with index <1>, run this:
certutil -F -J 75D460340D4F46986B45FDC823A8F7DA204EB2D7 -d db.
The key will get deleted. (-J) is a new option I added which if you 
give with -F, it deletes on the basis of CKA_ID but if u give all three,
that is -F -n -J, it will return error. So, the previous method still
exists where you delete a key based on nickname but then just give 
certutil -F -n. opt_KeyID is a new option added in certutil_options[] in certutil.c. (-J) corresponds to opt_KeyID.
 

The new functions are :
1)unsigned char *StrToHexStr(unsigned char *str,PRUint32 len)
2)PRUint32 converthexdigittodec(unsigned char c)
3)unsigned char *HexToStr(unsigned char *str,PRUint32 len)
4)static SECKEYPrivateKey * findkeybyid(PK11SlotInfo *slot,
                                        unsigned char *keyidinhex,
                                        int len,void *pwarg)
5)static SECKEYPrivateKey *FindKeybyID(PK11SlotInfo *slot, char *keyid,
                                       int len, secuPWData *pwdata)

StrToHexStr() converts any byte array of a given length to hexa(upper case).
HexToStr() converts any hexa string of a given length to a byte array.
converthexdigittodec() is simply a helpful function called by HexToStr().
This converts 'a' to 0xa, '9' to 0x9, 'f' to 0xf, '1' to 0x1, '0' to 0x0 and
so on.
findkeybyid() simply finds the pvt key based on the CKA_ID presented in the
hexa form. And, FindKeybyId() is a wrapper on findkeybyid(),just as ListKeys() is on listKeys().
And then, once pvt key is found, in certutil_main(), just call PK11_DeleteTokenPrivateKey(privKey, PR_TRUE) to delete the pvt key.
I am attaching a patch which will include these functions.
And, as mentioned before, in certutil_main(), all I am doing is this:
*********************************************
 /*Delete Key on the basis of CKA_ID: (-F  -J) */
    if(certutil.commands[cmd_DeleteKey].activated &&
       certutil.options[opt_KeyID].activated) {
    
    privKey = FindKeybyID(slot, certutil.options[opt_KeyID].arg,
                   strlen(certutil.options[opt_KeyID].arg),
                   &pwdata);
    if(privKey == NULL){
      fprintf(stderr,"\ncertutil : Key Not Found, exiting.\n");
      return 255;
    }
   rv = PK11_DeleteTokenPrivateKey(privKey, PR_TRUE);
   if(rv == SECSuccess)
     fprintf(stderr,"\nKey Deleted\n");
   else
     fprintf(stderr,"\nKey not deleted\n");

  }

**********************************************************

Comment 6

11 years ago
(In continuation to comment #5)

And, to list keys in hex form of CKA_IDs, the only change done was in 
secu_PrintKey() in certutil.c:

Earlier there was a line in that function.
fprintf(out, "<%d> %s\n", count, name);

And, now we remove it and add these two:

cka_id = PK11_GetKeyIDFromPrivateKey(key, wincx);
fprintf(out, "<%d> %s %s \n", count, name,StrToHexStr(cka_id->data,20));

This 20 is hardcoded. 20 is the length of the digest. We could also write strlen(cka_id->data).

Comment 7

11 years ago
Created attachment 247165 [details]
Preliminrary source code. not for review.

Comment 8

11 years ago
> cka_id = PK11_GetKeyIDFromPrivateKey(key, wincx);
> fprintf(out, "<%d> %s %s \n", count, name,StrToHexStr(cka_id->data,20));

> This 20 is hardcoded. 20 is the length of the digest. We could also write
> strlen(cka_id->data).

Neither is correct. It should be cka_id->len. For most of softoken, and orphaned
keys NSS creates the CKA_ID is 20 bytes long, but it doesn't have to be. strlen is wrong because cka_id->data is binary data and imbedded zeros are allowed.

bob

Comment 9

11 years ago
Created attachment 248237 [details] [diff] [review]
enable deletion of pvt key using certutil, v1

(In continuation to comment #7)

I have slightly modified the patch and would be nice if somebody could review this and suggest changes.
The new patch enables the pvt key to be deleted by certutil with -F -n option only. If a 40 byte hex string is passed with -n, it takes it as CKA_ID, otherwise as a nickname. The number 40 can be changed by changing the value of CKA_IDLENINHEX, defined in certutil.c. 
So, testifCKA_ID() takes as input a char string and judges whether it is a hex
string of CKA_IDLENINHEX bytes or not. This function is used by certutil_main()
to interpret the -n option, that is the value given with -n option means a nickname or a CKA_ID.

Comment 10

11 years ago
Biswatosh. 

Please resubmit the patch in uniform context(diff -U) and send some of us review request. Otherwise patch can disappear from radar.

I've glanced into the patch and found number of mem leaks. Please once again review your code and try to fix them before resubmitting the patch.

Comment on attachment 248237 [details] [diff] [review]
enable deletion of pvt key using certutil, v1

There are numerous problems with this patch, most of which can easily be fixed.  I will send email about the specifics to Biswatosh.
Attachment #248237 - Attachment description: The cvs diff of the files changed to enable deletion of pvt key using certutil. → enable deletion of pvt key using certutil, v1
Attachment #248237 - Flags: review-
Attachment #247165 - Attachment description: These contain 5 new functions to help certutil locate key based on CKA_ID as well to display keys with their CKA_IDs in hex. This is not for review but just to show the status. → Preliminrary source code. not for review.
Attachment #247165 - Attachment is patch: false
Created attachment 249749 [details] [diff] [review]
patch version 2

This is a modified patch, which removes memory leaks in the earlier patch(patch 1: 248237). Also, it tries to conform to the NSS coding style. Plus, the cvs diff is given in a unified context with the command cvs diff -Npu9.
Description of the functions are now added. But, functionality wise, it is the same as in patch1. The finding/deletion of the key based on CertReq is not done here. I have sent review request to Nelson. But, if anybody finds some mistake/has any suggestion/s, please tell.
Attachment #249749 - Flags: review?(nelson)
This comment is on finding and deleting a key based on cert req.
I found one way which worked in my sample code and I will be submitting
the source code very soon.

The idea is simple and is given below:

***********************
outFile = PR_Open(CERTREQ FILE,...);    
certReq = GetCertRequest(outFile,..);
pk = SECKEY_ExtractPublicKey(&(certReq->subjectPublicKeyInfo));
switch (pk->keyType) {
case rsaKey:
   pubKeyIndex = &(pk)->u.rsa.modulus;
break;
case dsaKey:
   pubKeyIndex = &(pk)->u.dsa.publicValue;
break;
case dhKey:
   pubKeyIndex = &(pk)->u.dh.publicValue;
break;
case ecKey:
   pubKeyIndex = &(pk)->u.ec.publicValue;
break;
}
CKA_IDItem = PK11_MakeIDFromPubKey(pubKeyIndex);
**********************

So, now we have the CKA_ID of the key to be deleted. Either we use this CKA_ID
to delete the key by using certutil -F -n command, based on the patch given in Comment #12 or let the code do that. That would be the details of the present idea, based on which I will be writing the patch now.
More on Comment #13

I tried on the idea in Comment #13 and could delete key based on cert req.
My patch could delete a key by executing "certutil -F -i certreqfile -d databasedir".

I wrote a function which takes two inputs,one a certreq file's descriptor and
another a boolean variable telling whether the file is ascii or not.
The function shall return the CKA_ID of the key based on the certreq file.
And, in certutil_main(), this function will be called and the key will be deleted based on the CKA_ID found. 

For deleting the key, I have two options. One would be just to repeat what
is being done in the patch in Comment #12 , that is write the same code again
for this case or somehow manage to use the same code,by not repeating the code again. I am choosing the second option.

For clarity(not for getting it reviewed right now),I attach the code containing
the function to find the CKA_ID based on the cert req and the small addition
in certutil_main() to delete the key. This attachment wont be the whole code. This would be just to show the idea of how the actual patch is intended to be.

Created attachment 250164 [details]
Preliminary code(not for review, this is just to give an idea).

This attachment is not complete and final.
It has a  function FindKeyIdfmCertReq(inFile,ascii), which takes the certreq file's descriptor and a boolean to declare whether the file is ascii ir not. It returns the CKA_ID of the key in SECItem form.

Then, in certutil_main(),it calls this and assigns it to the nickname variable
(name) and executes a portion of 249749: patch version 2 . This patch version 2
deletes a key based on nickname or or CKA_ID.
Created attachment 250832 [details] [diff] [review]
Patch 3, for review.

This is a complete patch. It can delete a key based on KeyID or on the basis of a CertReq file. So, a command like certutil -K -d db will print all the keys with their CKA_IDs in hex. certutil -F -n "A123F0..." -d db will delete the key having the CKA_ID as given,that is A123F0....And, certutil -F -i mycertreqfile -d db will delete the key by taking the certreq file and computing the CKA_ID from that.
The earlier patch simply deleted the key based upon CKA_ID in hex. This patch should do both.
Attachment #250832 - Flags: review?(nelson)
Comment on attachment 250832 [details] [diff] [review]
Patch 3, for review.

Previous patch (version 2) is now a part of this patch(version 3) and hence review of this one is sufficient.
Comment on attachment 249749 [details] [diff] [review]
patch version 2

This patch is obsoleted by v3.
Attachment #249749 - Attachment is obsolete: true
Attachment #249749 - Flags: review?(nelson)
Biswatosh, I have a lot of comments about your patch 3.  Here is the first one.
You added a new symbol in nss.def into the section for NSS 3.4.  
Once a release is made, we cannot go back and retroactively add a symbols to
an old version.  So, you cannot go back and add the symbols to NSS 3.4 now.
On the trunk, nss.def already has a section with new symbols for the 
forthcoming NSS 3.12 release.  Add your symbol there instead.
Target Milestone: --- → 3.12
Comment on attachment 250832 [details] [diff] [review]
Patch 3, for review.

Here are some of the review comments I gave to Biswatosh.

>+#define CKA_IDLENINHEX          40

Suggest renaming that to NUM_HEX_CHARS_IN_CKA_ID

Also, if this new feature is to work with PKCS#11 modules other
that NSS's softoken, it needs to be able to handle CKA_IDs of 
other lengths, and IDs that are ASCII strings, not binary.
It should print CKA_IDs in hex only when their bytes include
non-printable and/or non-ASCII characters.  
It should interpret CKA_IDs from the command line as HEX only
when they contain *only* valid hex characters.
That's a big change to the target specification.  
I'm sorry I didn't think of that sooner.

Instead of inventing new hex-to-binary and binary-to-hex functions
I suggested that he reuse the ones in fipstest.c (after dealing with 
bug 366390).  

I suggested he rename new function findkeybyid so that it won't be 
the same as new function indKeybyID except for capitalization.

New function findkeybyid leaks a SECKEYPrivateKeyList.  It shouldn't.
It should copy the target private key, destroy the list, and return 
the copy.

I suggested renaming FindKeyIdfmCertReq to FindKeyIdFromCertReq

The -F option should not invoke PK11_DeleteTokenPrivateKey with the
"force" option set to true by default.  The user should have to take
a separate action, such as setting yet another command line option,
to make the force argument true.

PK11_DeleteTokenPrivateKey(privkey, PR_TRUE) must be fixed to not 
leak the cert when the key is not an orphan.  That's bug 366405.
Attachment #250832 - Flags: review?(nelson) → review-
(In reply to comment #20)

I am going to attach the new certutil's cvs diff ,which implements Nelson's suggestions.
This is working and seems to do what Nelson have suggested. However,this is not for review now. I will self-review it further and test it again and then give it for review. 

Some basic changes are:

1)CKA_ID is now accepted as either as a hex string or as a string consisting of decimals(0-9)/English letters(a-z,A-Z). 
2)While printing the keys(certutil -K), if it finds that CKA_ID->data is itself
printable(0-9.a-z/A-Z),it will print that as it is, otherwise it will convert to hex form and then print.
3)certutil -F -n can work with these further options,-l and -e.
-l means "interpret the string given with -n as CKA_ID else as nickname".
-e will mean delete key by force. Means, even if a corresponding cert is there,
-e will enable certutil to delete the key,if found.
Not giving -e will mean if cert is there, it won't delete the key.

The earlier patch tried to guess whether the string with -n is nickname or CKA_ID by actually verifying whether each char is a hex or not. Now that, a CKA_ID can be given as any string with characters like z,G etc, unless and until
the user declares whether the string is a CKA_ID or a nickname, it can't determined what it is. Thus, is the requirement of another option(-l).



Created attachment 251518 [details] [diff] [review]
not for review. implements review suggestions to patch 3

Implements whatever was told in Comment #21 . This is just to tell how the code is going to look like. I will give it a relook and upload the patch for review ASAP. But, I dont expect the patch to be significantly different from this one unless the goal expands.
Testing if a character is printable is somewhat tricky.  We don't want to 
limit it to only ASCII letters and digits.  Spaces and punctuation are 
printable too.  A test that works for ASCII characters (only) is 
    is_printable_ASCII(x) ((x) >= 0x20 && (x) < 0x7f)
but this excludes many characters from other character sets.  
I think PKCS#11 does not specify any character set(s) that may be used 
in CKA_IDs, so NSS's usual assumption that character strings are UTF-8
probably doesn't hold.  Since we're willing to fall back on Hexadecimal,
it probably will suffice to use the test given above.

Comment 24

11 years ago
CKA_ID's are byte arrays, and are almost always treated as numbers.

PKCS #11 does define 'string' objects. 

1. It defines CK_CHARS, which are a restricted subset of ascii characters (upper/lower case, 0-9, space, 29 symbols) I haven't verified if there are in fact any missing symbols from the set of characters in CK_CHAR. This is the old CK_CHAR definition from the first PKCS #11 spec. I can't name any actual object that uses it CK_CHAR anymore, most use:

2. It also defines CK_UTF8CHAR, which, of course, is UTF8 Characters.

Most strings are now RFC2279, which are 'unpadded utf8 characters with no null-termination' (the length is given by the attribute length).

Anyway none of these applies to CKA_ID, which is a byte array, some type as a key exponent, or a modulus.

bob
Bob, the assumption that a CKA_ID is a modulus or other aspect of the key's
values is one made by NSS, but not by all third party PKCS#11 modules. 
But in fact, there are PKCS#11 modules that are known to set the CKA_ID and 
CKA_LABEL to the same value, essentially a string.  

I want to ensure that if certutil encounters objects in such a module, 
that it will list them as the strings that they are, and not list them 
in hexadecimal.  Likewise, I want certutil users to be able to specify 
them as strings when that's what they are.  

Comment 26

11 years ago
Nelson, you misunderstood. CKA_ID *TYPE* 'byte array'. This is the same *TYPE* as modulus, exponent, value, etc. That is it is specified to be a 'number' in PKCS #11.

My point is while a vendor could use a 'string' for CKA_ID, the spec says that string is a number. The vendor could use a number of meaningful values whose meaning would be not be appearant looking at the hex value (A DER encoded blob, for instance). I'm not sure it's worth trying to identify those, and may be confusing (would a CKA_ID of 'bob\0' display the same as a CKA_ID of 'bob')? 

In general, any string associated with an object is set to it's label, and is specified to be UTF8 with no NULL. Most vendors set the CKA_ID to one of 3 things:
1) the PKIX key id
2) the sha1 hash of some component of the public key (because they are following what NSS sets it to).
3) a key index (like 1,2,3...).

I don't thing trying to interpret CKA_ID as a string is worth the effort for those few tokens that may store CKA_ID as such. Printing it as hex should be less confusing because the hex value is *always* correct (no matter what, the CKA_ID is always a number).

bob
Created attachment 252756 [details] [diff] [review]
Same as the patch before(251518) except some corrections/changes. Not for review

Nelson,

As discussed in our telecon, this is the patch I wanted to give you 
for review. But, I have a small doubt on the way I produced the diff. I shall
write a separate mail to you. After that, I will be giving the patch for review.
This patch deals with CKA_IDS as printable or even if in hex, it can accept
CKA_IDs of other lengths and not merely of the default length. 
This has copied the hexstring to bytearray functions and it's converse functions from fipstoken.c to secutil.c. And, this patch uses these new functions. Patch 3 had created separate function for the same purpose, which are now not required.
Attachment #250164 - Attachment is obsolete: true
Attachment #250832 - Attachment is obsolete: true
Attachment #251518 - Attachment is obsolete: true
In the attachment in the previous Comment #27, please ignore the changes visible in the function LongUsage().
Created attachment 253744 [details] [diff] [review]
For review. This patch obsoletes earlier patches.

This patch enables interpretation of CKA_ID as an ascii string and also as a hex string whose length is more than the standard 40 hexadecimals. The default one(with 40 hexas) are of course dealt with as before.
The changes are the same as mentioned in comment #21 except once small change. In point 1 in comment #21 , ascii characters were confined to decimals(0-9)/English letters(a-z,A-Z). Now, it is expanded to all ascii chars between 0x20 and 0x7E, not inclusive of either.
Attachment #247165 - Attachment is obsolete: true
Attachment #248237 - Attachment is obsolete: true
Attachment #252756 - Attachment is obsolete: true
Attachment #253744 - Flags: review?(nelson)
Comment on attachment 253744 [details] [diff] [review]
For review. This patch obsoletes earlier patches.

r-  There are still many problems with this patch.
I will write them all up and email them to Biswatosh.
Attachment #253744 - Flags: review?(nelson) → review-

Updated

11 years ago
Assignee: biswatosh.chakraborty → neil.williams

Comment 31

11 years ago
Created attachment 282785 [details] [diff] [review]
draft of changes for IDing keys by CKA_ID and index

This patch requires two functions which were written for bug 291384: PK11_GetKeyIDFromPrivateKey() -- which just needs to be exported, and SECU_SECItemToHex(). Be forewarned I haven't been able to test this patch.
Assignee: neil.williams → nobody
Attachment #282785 - Flags: review?(nelson)
Comment on attachment 282785 [details] [diff] [review]
draft of changes for IDing keys by CKA_ID and index

One problem, one question/concern.

1. Function findKeyByIndex frees the list of private keys, and
then returns a value contained in the memory that was just freed.
So, r- for that.

2. It appears that this code intends to accept key IDs in the 
form <NN> which is the form of the index printed by certutil -K.  
The patch passes the string, starting with the second character,
to atoi, e.g. if the input string was "<12>" it would pass "12>"
to atoi.  

I wonder, is atoi guaranteed to return the number 12 in that case? 
or does it return an error indication because '>' is not numeric?
Attachment #282785 - Flags: review?(nelson) → review-

Updated

9 years ago
Blocks: 459298

Updated

9 years ago
No longer blocks: 459298

Comment 33

9 years ago
atoi parses the string up to the first non-digit number and return converted result. So atoi("12>") will return 12.

Comment 34

9 years ago
Created attachment 345569 [details] [diff] [review]
Patch v4 - looking for key by CKA_ID
Attachment #282785 - Attachment is obsolete: true
Attachment #345569 - Flags: review?(nelson)
The problem with using the index value formerly reported by certutil -K
is that it may change from run to run.  It does not uniquely identify 
a particular key in a way that does not change over time.  Now that
certutil -K really does output the keyID, we should use the real keyID
and not some temporal index value.

Comment 36

9 years ago
the patch allows both, temporal index and keyID,be used for key deletion. It is easy to remove the code that uses index.
(Reporter)

Comment 37

9 years ago
I agree with Nelson that we should not use an index which value can change. I thought that approach had already been previously rejected. The use of an index is even more of an issue now with the shared DB. Please delete it.
Comment on attachment 345569 [details] [diff] [review]
Patch v4 - looking for key by CKA_ID

r- 
two changes needed.
1. eliminate the code to "find key by index".
   Only find key by keyID.
2. There's a leak in findKeyByKeyID.  
   Each time through the loop, if the key's ID is too long for the 
   automatic buffer, it allocates a new buffer from the heap, and 
   leaks any buffer previously allocated from the heap.  
   Since this is command code and not library code, maybe the 
   simplest thing to do is to always allocate, and always free,
   all inside the loop. I think there may already be a function in 
   cmd/lib/ somewhere that allocates and does the toHex conversion.

3. We need to decode what to do with PK11_DeleteTokenCertAndKey.
Personally, I think the feature to delete private keys should delete
ONLY private keys, and not certs.
Attachment #345569 - Flags: review?(nelson) → review-
Unsetting target milestone in unresolved bugs whose targets have passed.
Target Milestone: 3.12 → ---
You need to log in before you can comment on or make changes to this bug.