291384 - certutil -K behavior doesn't match usage

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Julien Pierre]

At this time, the certutil -K command says that it lists keys, but in fact it
lists only private keys.

When doing a keygen, one of our users expected to see 2 keys - the public and
private keys.

NSS softoken public key objects are only available if an NSS softoken private
key object also exists. In other word, there isn't truly way to manage the
public keys in NSS softoken separataely. So, it makes sense that certutil
doesn't need to manage public keys in softoken. But it may make sense for other
token implementations.

The resolution to this bug might be as simple as correcting the usage in
certutil to state "private keys".

I think we should also discuss the potential need for managing public keys at
the same time. Perhaps we should have certutil manage keypairs, rather than just
private keys, so it would work better with other tokens which may have separate
storage for public and private key objects.

Comment 1

[Robert Relyea]

Does -K list Symetric keys as well? It probably should add that too.

bob

Comment 2

[Julien Pierre]

Bob,

No, certutil uses PK11_ListPrivateKeysInSlot for its key list, so it doesn't
show symetric keys. Perhaps we should further qualify the keys with the word
"asymetric" if we choose to show keypairs.

Currently symetric keys are only managed by symkeyutil. If we add support for
them in certutil, we should probably also merge all of the symkeyutil
functionality into certutil, IMO.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

certutil -K lists ALL the private keys in the indicated slot (or in 
all slots if no slot is selected).

It "lists" each key by printing the key's CKA_LABEL attribute (nickname) 
or "< orphaned >" if the key has no nickname (as is typical, IINM).

certutil's usage message shows the syntax of the -K command to be:

        certutil -K [-n key-name] [-h token-name] [-k dsa|ec|rsa|all]
                 [-f pwfile] [-X] [-d certdir] [-P dbprefix]

which suggests that the user can choose to filter the output by key nickname
or by key type.  But in fact the function ignores the key nickname and key
type arguments completely.  

The usage and actual behavior should agree.  Either the function shuold not
ignore the key nickname and key type options, or the usage should be changed
to stop showing that those options are relevant to the -K command.

Comment 4

[Biswatosh Chakraborty]

So, we have several braches of this discussion.
(1)The usage of certutil(from certutil -h) is not fully implemented(see comment #3). 
(2)What should certutil -K give as output? Should it display pub keys(see the "description" for this bug)?

Now, before giving a patch, we should resolve (2). 
Mozilla's page on certutil (http://www.mozilla.org/projects/security/pki/nss/tools/certutil.html), says on certutil -K this :"List the keyID of keys in the key database. A keyID is the modulus of the RSA key or the publicValue of the DSA key. IDs are displayed in hexadecimal ("0x" is not shown)."
Currently. certutil -K does not seem to give modulus in hex. So, either the
description in mozilla needs to be corrected or the code for certutil. 
Comment #3 states that it prints key's  CKA_LABEL or "<orphaned>".
So, do we stick to this behaviour of "certutil -K "? 

Hence, let us first decide what do we want certutil -K to do. Of course, whatever we decide, we have to see that certutil -h states what is true
and implemented. But, first the decision and then the patch.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

The specification on the cited web page is correct. 
-K lists the KeyIDs, the CKA_ID attribute values.

By convention, key IDs are generally a function of some component of the
public key value itself.  But this is not essential.  
It is essential that -K print the keyID, whether that keyID is derived
from the public key value, or not.

Comment 6

[Neil Williams]

Biswatosh, part of bug #291383 includes adding the hexadecimal representation of key IDs to -K, so the usage message should reflect that.

Re: comments #1, #2: symkeyutil already deals with symmetric keys; certutil as it stands is something of a mess (look at the output of -H), I suggest not adding any symmetric key facilities to certutil. It would be good to change certutil's usage message to indicate -K lists private keys only, though.

Comment 7

[Biswatosh Chakraborty]

So, are we saying that 

1)certutil -K should give ouput like:

<0>  E517EB630E7C140B508B3BCF61856EFEB62CB97C 
<1>  75D460340D4F46986B45FDC823A8F7DA204EB2D7 
<2>  6306667A7A688C51F131F23D53D3886E76B75188 
...
where, the hexadecimal representation is the the hex form
of CKA_ID ?

and 
2)The usage message of certutil should reflect that(point 1).

Comment 8

[Neil Williams]

(In reply to comment #7)
> So, are we saying that 
> 
> 1)certutil -K should give ouput like:
> 

this is part of bug #291383.

> and 
> 2)The usage message of certutil should reflect that(point 1).
> 
Julien's suggestion to change the usage message to say "-K lists private keys" is the simple way to fix this. But he brought up the possibility of managing key pairs instead of just private keys. Perhaps we can get him to weigh in on this. Although I can't see how that would affect the -K part of the usage message. Julien?

Comment 9

[Julien Pierre]

Neil,

Just adding the word "private" to the usage for -K would be a satisfactory resolution as far as -K is concerned. We should also fix the issue Nelson brought up in comment 3 and make -n and -k work with -K, or delete them from the usage.

Perhaps the management of public keys or keypairs should be done in another command as an RFE, and the discussions around that from this bug could be copied over to it.

Comment 10

[Biswatosh Chakraborty]

Hence, are we coming to the following conclusions?

1)certutil -K should give the CKA_IDs, as in
<0>  E517EB630E7C140B508B3BCF61856EFEB62CB97C 
<1>  75D460340D4F46986B45FDC823A8F7DA204EB2D7 
...

2)The options like -n,-k and other options do not work with certutil -K,
hence correct them. That is, make them work. Or, don't mention them
in the usage. Preferably, make them work.

3)The usage of certutil -K should mention "private". We are not going
to deal with public keys here.

Yes, point(1) is part of https://bugzilla.mozilla.org/show_bug.cgi?id=291383
It means, roughly the same code will go here with some changes, if required.

Comment 11

[Biswatosh Chakraborty]

Attachment: For review.

Some code from patch from bug 291383 has been reused.

Salient points of this patch:

1)certutil -K can now use the options -k, -h ,-n for keytype,
token name and nickname respectively.

2)certutil -H will shows this now:
********************************
-K              List all private keys
   -h token-name     Name of token in which to look for keys
                     (default is internal, use "all" to list keys on all tokens.
                     Note:Wrong token name is treated same as "all")
   -k key-type       Type of key pair to list ("all", "dsa", "rsa" (default))
                     Note:If the keytype is wrong, it is treated same as "all"
   -f password-file  Specify the password file
   -d keydir         Key database directory (default is ~/.netscape)
   -P dbprefix       Cert & Key database prefix
   -X                force the database to open R/W
****************************

3)This has copied the hexstring to bytearray functions and it's converse
functions from fipstoken.c to secutil.c.

4)This patch enables interpretation of CKA_ID as an ascii string and also as a
hex string whose length could be more than the standard 40 hexadecimals. The default one is with 40 hexas. 
Ascii characters are expanded to all ascii chars
between 0x20 and 0x7E, not inclusive of either.

Comment 12

[Neil Williams]

Comment on attachment 274259 [details] [diff] [review]
For review.

Biswatosh, this bug is about the usage message. Please make a patch with just the changes to the usage message message. The rest of the patch is addressed by bug 291383 and should be fixed there.

Comment 13

[Biswatosh Chakraborty]

Neil,

I thought we also need to implement what the web page on certutil
(http://www.mozilla.org/projects/security/pki/nss/tools/certutil.html)
says. That is, certutil -K should give hexadecimal output of the key.
Anyways, my patch now would be a subset of the patch given for review
and would address points 1 and 2 only of comment #11. 

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Neil,  IINM certutil -K is NOT presently printing out keyids in hex.
It should do so.  Is this bug not the right place to make it do so?
bug 291383 is about the ability to enter keyids in a command to act on
keys, which seems to me to be separate from the act of displaying keyids.

Comment 15

[Biswatosh Chakraborty]

Attachment: Not for review.

This patch addresses Comment #12. That is, it is a part of
the earlier patch(274259), where now it caters to points 1 and 2 
only of comment #11. 

If we stick to the idea of certutil -K displaying keys in hex,
then I propose the earlier patch(274259) otherwise this current patch
would be given for review.

Comment 16

[Neil Williams]

Comment on attachment 274259 [details] [diff] [review]
For review.

I am retracting my review while I reconsider this patch.

Biswatosh, it's not clear to me what the output of the -K option will look like. Please attach a sample.

One general observation: this patch is quite a bit more complicated than I had envisioned.

Comment 17

[Biswatosh Chakraborty]

Neil,

The output of certutil -K with this patch would be like what was mentioned 
in comment #7 and comment #10. 
That is, each line would contain
the counter number, nickname and hexadecimal representation of the
key. There, in that sample output, nickname was blank
but in a more general case, it would look like the following:
(one key with the name "mytestcert" is also listed at counter no. 59)

*****************************
bash-2.05# certutil -K -d db
Enter Password or Pin for "NSS Certificate DB":

<0>  4ACF763D0BB734D2C98E4C1450F26A38BC7A436C 

<1>  6760850AFC2FFFCF5E4EFE75C3DB897126C2EC9E 

<2>  7C406D3DE6B923C175117724B5465926DEF4614D 

<3>  C366DFBF1430FE9E985FA20358572BB3A12ED7A9 
...
...
...
<56>  994689B0774765E4F769104569AF74CA60E95FA2 

<57>  81588BA2D538F7E3CB84709E9E1F370886BF6E0C 

<58>  A6E76521F9FFD585F948D824F171AD188EFCA252 

<59> mytestcert 7E4FACD1AED162CD45D7795113FBB23FD454179E 

<60>  2E58B20A5BE8F2151D915AF8B2B7CC5CF3B2D905 

<61>  EB2A6F62B9320C5564552DF857A7EAFC8E739FA6 

<62>  A8E597C84F2BE26EADC72503D1607017E041F2C5 

<63>  F93909D102218C3C987C0F687F6641859159D675 

<64>  9913334C40286134C15C6DABB67BFCC84F1D94BD 

<65>  0E052143635BDDEBB94C9E32427CF4E2EE0BDD05 

<66>  00CF92814E28C626748B03DFDEC491C708B05086 
...
...


<101>  B550C427C04E27D96605A74ED02509EA0C568CAD 

<102>  3DA8AD26EFE9FB94C8BA0FE6B2851AA37AEFDFDC 
bash-2.05# 

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

Comment 18

[Biswatosh Chakraborty]

Comment # 23 in https://bugzilla.mozilla.org/show_bug.cgi?id=291383 is also relevant for this bug.

Comment 19

[Neil Williams]

Comment on attachment 274259 [details] [diff] [review]
For review.

This is mostly OK. Some comments:

1) NUM_HEX_CHARS_IN_CKA_ID            - this name is wrong; see  comment #8 in bug 291383.

2) secu_PrintKey() - The count argument doesn't seem to be useful and should be deleted. If you are going to list CKA_ID in hex or ASCII depending on printablity you should indicate which mode you've used for a particular key. Perhaps enclosing ASCII strings in double quotes.

3) It complicates the code to have a fixed length buffer for the hex string. I'd just allocate the buffer for every one.

4) When a CKA_LABEL is printed it messes up the output alignment. I suggest printing it after the CKA_ID. You can make the field 40 chars wide so any labels always appear in a separate column.

5) The SECU functions should be cleaned up. There is already a byte-to-hexchar function in cmd/lib/secutil.c, look at SECU_PrintAsHex for a more elegant solution. Then carve out the part that does the bytearray-to-hexstring work, put it in a new SECU_ function and have both functions use it.

Comment 20

[Biswatosh Chakraborty]

Neil,

Thanks for your suggestions. I have some doubts.

Point 1 of comment #19:
You mentioned comment# 8 in bug 291383. It speaks about the mistake
of hardcoding the length of the digest(as 20).
Please see comment #20 in the same bug(#291383). It infacts suggests
this name. Anyways, if I am going to allocate the buffer everytime
(as you suggested in point 3), I would not need this value.

Point 2:
Are you saying we don't use "count" at all? But, this at least
gives the information on the number of keys in the database
plus sometimes to look for the key we are interested in, this
can be of help. I don't mean for a program but for view purposes.

Point 3:
Yes, the code becomes more complicated but the advantage is,
if the number of keys is very large, it has to go on allocing 
everytime if we go by the suggestion.
But, in the patch, it will hardly do an alloc, for
all practical purposes. Whenever, it will require it will do.
If by chance,we change the underlying digest function, we will
just change the value of NUM_HEX_CHARS_IN_CKA_ID and again alloc
will hardly be required.

Point 5: I tried today with SECU_PrintAsHex() and this works
as fine. I can directly use this function. Does the folliwing
output look OK?

$ certutil -K 

Count: 89 Nickname: mycert
Key:61:fd:18:3c:3c:27:d7:20:a0:94:5e:3a:c6:43:9e:51:
6a:99:f1:66

Count: 90 Nickname: hellocert
Key:f8:b4:10:fb:f6:13:3f:ac:c6:69:88:52:f9:0d:91:ff:
9d:68:74:00

This displays "count" but if we decide we don't want "count", 
this will look like:

Nickname: hellocert
Key:f8:b4:10:fb:f6:13:3f:ac:c6:69:88:52:f9:0d:91:ff:
9d:68:74:00

Comment 21

[Neil Williams]

(In reply to comment #20)
> Neil,
> 
> Thanks for your suggestions. I have some doubts.
> 
> Point 1 of comment #19:
I saw that. The very next paragraph in that comment makes it clear that CKA_ID is fixed onyl for the NSS token.

> 
> Point 2:
Count isn't useful because it is not a fixed index for the key, ie. it can change depending on key DB activity. The main consideration with this enhancement is to make it possible to identify keys for later operations (like deleting them). Since count doesn't help do that let's take it out. If you like you can list the number of keys found as the first or last line of the output--maybe with column headers, too--but I think large key DBs are probably not common.

> Point 3:
That's true but certutil is a CLI utility. I can't imagine any current model computer where you'd notice the run time difference.

 
> Point 5: I tried today with SECU_PrintAsHex() and this works...
It looks OK. I'm thinking ahead to when we try to find keys for other operations. For those commands we'll need a byte-to-hexchar function. Whatever we do the output format of this command should match the input format of those. I'd suggest something like this:

$ certutil -K

4ACF763D0BB734D2C98E4C1450F26A38BC7A436C  <mytestcert>

6760850AFC2FFFCF5E4EFE75C3DB897126C2EC9E 

"Brand X Token Key"                       <ForeignCert>

More from point 2. The last line would be for CKA_IDs that contain all printable characters.

Comment 22

[Neil Williams]

(In reply to comment #21)
> > Point 3:
> That's true but certutil is a CLI utility. I can't imagine any current model
> computer where you'd notice the run time difference.

Maybe better still leave the code as is but use PORT_Realloc instead of writing the code out.

Comment 23

[Biswatosh Chakraborty]

Attachment: For review. Implements suggestions made on the earlier patch.

Please refer to the suggestions made in Comment #19, 
described below as "Points".

Point 1: The name is still mainitained as NUM_HEX_CHARS_IN_CKA_ID as
I mentioned in Comment #20. The code is flexible enough to handle cases
where the size of the digest changes and accordingly reallocs. Is it OK?

Point 2:"Count" is removed now and ascii strings are kept within quotes("").

Point 3:I am now reallocing as suggested in Comment #22.

Point 4:nickname is now printed after CKA_ID and CKA_ID is 40 chars wide.

Point 5: For converting a SECItem to a hex array, I am using the idea
from SECU_PrintAsHex() and thus have added one new function 
SECU_ConvertByteArrayToHex() kept in cmd/lib/secutil.c. 
I did not write a function which can be used by SECU_PrintAsHex() as well,
as this will entail testing of other functions using SECU_PrintAsHex() as well.
Writing SECU_ConvertByteArrayToHex() and currently being used only in the proposed certutil.c patch simplies the testing. 

On the last point mentioned in Comment #21: Now, certutil will print
the hexadecimals output first and then the printable CKA_IDs.

Comment 24

[Biswatosh Chakraborty]

Attachment: Not for review. But, is fundamentally same as the patch(277230 ) given for review.

I am attaching this patch to help the review process. The main changes done
were in two functions in certutil.c, namely in listKeys() and secu_PrintKey().
I kept the two functions under "#ifdef 0" in this current attachment so that in the cvs diff output, the patch contains the two newly written functions fully, without getting mixed up with the older versions. 
The patch given for review is mixed(with - and +s) for the two functions mentioned and this patch is not and hence could also be used for reviewing the "official" patch. 
If this does not help, please ignore this "assistant" patch. :)

Comment 25

[Biswatosh Chakraborty]

Comment on attachment 277230 [details] [diff] [review]
For review.  Implements suggestions made on the earlier patch.

Neil, 

I had thought that the name NUM_HEX_CHARS_IN_CKA_ID
was now acceptable to you 
but after a later thought, I felt you probably still wanted a change in the name. Hence, I am removing this patch and reattaching the modified patch for your review.

Comment 26

[Biswatosh Chakraborty]

Attachment: For review. Earlier patch removed.

All points made in Comment #23 applicable to this patch, except Point 1.

Comment 27

[Biswatosh Chakraborty]

Attachment: Not for review. Is for the same purpose as mentioned in Comment # 24.

This is thus to help review the official patch (#277356).

Comment 28

[Neil Williams]

Comment on attachment 277356 [details] [diff] [review]
For review. Earlier patch removed. 

Biswatosh, sorry this took so long to review. I see you made fixes for everything I brought up in comment #19 but we're still not finished.

1) NUM_HEX_CHARS_IN_CKA_ID_FOR_NSS_TOKEN is a rather long name. I think you can do without this definition. More below.

2) secu_PrintKey: Why allocate NUM_HEX_CHARS_IN_CKA_ID_FOR_NSS_TOKEN bytes first then check to see if it's long enough? You have the correct length. Use it and there will be no need for reallocating. Notice that would get reduce your if-else printing blocks to one.

3) listKeys: Your loop is difficult to proofread partly because you use 2 if statements when if-else would be more appropriate. When the key has no nickname you allocate "< orphaned >" to use, then if keyType doesn't match you continue the loop which will leak the string. 

4) listKeys: You go through the list twice listing all non-printable CKA_IDs first. Why? Also, you repeat the loop by goto'ing a label before the loop. A nested loop would be much clearer.

5) SECU_ConvertByteArrayToHex: Last line "out = out - (2 * data->len);" has no effect since out is a local variable.

Comment 29

[Biswatosh Chakraborty]

Attachment: For review

This contains the current versions of secu_PrintKey(), listKeys() and ListKeys()
under #ifdef 0 - #endif block, just to make the review process easier. However, I am attaching 
another patch with the old versions of the above 3 functions removed. Otherwise,
both patches are same.

Salient points of this patch:(The points below map to the points in Comment #28)

1)NUM_HEX_CHARS_IN_CKA_ID_FOR_NSS_TOKEN  is removed now.

2)Yes, cka_id->len is known and allocation is done for that length and thus
if-else block is reduced now.

3)The two if statements are now in if-else and leakage is removed now.

4)I had thought initially that I need to print non-printable ones first but
the confusion is removed now. I am going to print as supplied
by the for loop.

5)Now, a double pointer is being passed to SECU_ConvertByteArrayToHex().

Comment 30

[Biswatosh Chakraborty]

Attachment: To assist review of the patch(279433)

This patch is basically same as 279433. See comment #29 for
further details.

Comment 31

[Biswatosh Chakraborty]

A sample output:

$certutil -K -d db:
*****************************

4acf763d0bb734d2c98e4c1450f26a38bc7a436c

6760850afc2fffcf5e4efe75c3db897126c2ec9e

7c406d3de6b923c175117724b5465926def4614d

"small data"                             

3ea3df4ca540e942e90022104bfc8c704799b0a5

"another small data"                      <name of data>

e69b33beb628261e23dffcbb43f6d0090067ecb5

"small data"                             

"another small data"                      <name of data>

f42b4e2c468ae68e521fdab7e69054eae6459956

c997e8951b3b24444d0263ed2c682861abc77c78

e70da2d2107e23a5d7444eee33494a276a8a43e2

7daaf9021eea1c6f8656eaa1188f0e9a2c0675f3
...
...
7e4facd1aed162cd45d7795113fbb23fd454179e  <mytestcert> 

2e58b20a5be8f2151d915af8b2b7cc5cf3b2d905

eb2a6f62b9320c5564552df857a7eafc8e739fa6

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

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

Attachment: alternative patch

This patch comes closer, but doesn't print the nickname for all types 
of keys with associated certs.

Biswatosh, try this with your test DB.  Be sure to specify -k all.  
Attach the results as a text attachment to this bug (not as an inline comment).

Comment 33

[Biswatosh Chakraborty]

Attachment: Output of Nelson's patch

Nelson,

I tried with some combination of commands and all of them are in the attached
output file. 

The immediate differences I see with the output from my patch
and your's are that I don't display the count number whereas your's one does.
And, your patch displays which type of key it is. Mine does not.
Although, certutil -k keytype will display keytype keys only in my patch.

Comment 34

[Nelson Bolyard (seldom reads bugmail)]

Thanks for the output.  Unlike the output shown in comment 31, I didn't see 
any "small data" in the output attached to comment 33, nor did I see any 
CKA_IDs that are obviously ASCII characters encoded in hex.  Do the keys that 
produced those strings in comment 31 still exist in your DB?  If so, how did
keys with non-ASCII CKA_IDs produce those strings?

The significant differences between the two patches are not in the output 
they produce, but rather are in the code that produces them.  

Comment 35

[Biswatosh Chakraborty]

Nelson,

The output shown in Comment #31 is not based entirely on my db but I had to
tweak my code in a line or two to force such an output. 

Basically, Neil as my reviewer, wanted to see how ascii characters as CKA_IDs would showup and I told him that to display such an output, I would forcefully make some CKA_IDs as
ascii strings, before actually printing them. 

In showing the output, I had Neil in the mind and now realize that it can create
confusion to others. Hence, I should have written the background in comment #31
only. Sorry for that.

Comment 36

[Biswatosh Chakraborty]

Nelson,

I see that in your alternative patch, the following is being
done in PrintKey():
**********************
 else {
    	/* print ckaid in hex */
	SECItem idItem = *ckaID;
	if (idItem.len > 20)
	    idItem.len = 20;
        SECU_SECItemToHex(&idItem, ckaIDbuf);
     }
**********************
If idItem.len > 20, why is it being forced to become 20?

Comment 37

[Neil Williams]

I didn't get around to reviewing the latest patch until after the request was cancelled. Sorry for the delay. It looks like the last patch is real close. I'll refrain from dithering over the details and let you and Nelson hash it out. Biswatosh, if this languishes too long please bring it to my attention.

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Attachment: updated patch for trunk only

This patch Adds the changes to the long usage message which were missing 
from my previous patch.  It also changes certutil to use 
PK11_ListPrivKeysInSlot rather than PK11_ListPrivateKeysInSlot when a 
nickname has been given.  PK11_ListPrivKeysInSlot is supposed to return
ONLY the keys that match the nickname.  

This patch is for the trunk only.  with this patch, it is easy to prove 
(that is, to confirm) bug 353714.

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for NSS 3.11 branch

This patch is equivalent to the preceding patch for the trunk.

Comment 40

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 284272 [details] [diff] [review]
patch for NSS 3.11 branch

requesting review, for 3.11 branch.

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 284272 [details] [diff] [review]
patch for NSS 3.11 branch

Please review this patch for the branch.

Comment 42

[Robert Relyea]

Comment on attachment 284272 [details] [diff] [review]
patch for NSS 3.11 branch

r+ as long as you make the following changes...


1) Change the call from PK11_GetKeyIDFromPrivatekey to PK11_GetLowlevelKeyIDForPrivateKey() and remove the export from the list (I would also quickly r+ the removal of the dead code in PK11_GetKeyIDFromPrivateKey and PK11_GetKeyIDFromCert as well).

don't export the PK11_GetKeyIDFromPrivateKey function.

bob

Comment 43

[Nelson Bolyard (seldom reads bugmail)]

I'm going to need to take some time to study the code to understand Bob's
comment 42.  

Bob, I think you're saying that PK11_GetKeyIDFromPrivateKey and PK11_GetKeyIDFromCert are deprecated and dead, and one of them has been
superseded by PK11_GetLowlevelKeyIDForPrivateKey().  Yes? 
 
The term "low level" sounds like a private interface, internal to softoken.
If that is correct, then we don't want certutil to use it.
If that is incorrect, then one wonders why it has that name...

I'll need to understand the differences between  PK11_GetKeyIDFromPrivateKey 
and PK11_GetLowlevelKeyIDForPrivateKey.  I may write a new patch.

Comment 44

[Robert Relyea]

the code id identical between the 2 is effectively identical (PK11_GetLowLevelKeyIDFor* uses common code to get the key id).

I find no other users of PK11_GetKeyIDFromPrivateKey, it's not exported and does exactly what PK11_GetLowLevelKeyIDForPrivateKey does().

I know now this because was looking for the header file changes, assuming PK11_GetKeyIDFromPrivateKey should move from pk11priv.h to pk11pub.h. In searching for it's use, I found that we already exported a function which does the same thing. BTW PK11_GetKeyIDFromCert has the same issues.

bob

Comment 45

[Julien Pierre]

Comment on attachment 283644 [details] [diff] [review]
updated patch for trunk only

1) In PrintKey, why is the size of ckaIDBuf 44 ? There should probably a macro and a comment.

strcpy and sprintf are used. There are better calls with built-in boundary checking available, such as PR_snprintf that can be used.

SECU_SECItemToHex has no way of reporting an error and thus is not being checked for one. That seems like a problem. I think it should return SECStatus and take the size of the target buffer as one of its arguments in order to be able to do boundary checking.

2) In ListKeysInSlot :
There is a command about a bug with empty key nicknames. Could the bugzilla # be referenced there ?

There is a PORT_Free((void*)keyName); This could be freeing a NULL pointer in that path. This may not be illegal, but still seems wrong.

The use of const and the casts are confusing. The value returned by PK11_GetPrivateKeyNickname is not a const string, yet it gets cast to a const char* .
It appears the only reason for keyname to be declared const char * is that sometimes it gets assigned the const orphan string. I think it's preferably to have keyName be a char*, and PORT_Strdup the orphan string into it, even it's less efficient, so that the code can be simplified, less error-prone, and more maintainable.

I don't agree with the assertion for the case where the module returns unwanted keys. Modules are user-installable and the bug may be in a third party module, not in NSS. We are not just talking about the NSS PKCS#11 modules. Also, we want to detect that error in optimized builds too. I think the assertion should be removed, and there should be a serious error message.

Comment 46

[Nelson Bolyard (seldom reads bugmail)]

One of the other bugzilla bugs that the above patch works around is bug 353714 

Comment 47

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 284272 [details] [diff] [review]
patch for NSS 3.11 branch

I'm changing Bob's r+ to an r- to remind myself not to commit this patch.  
I intepret Bob's comment 44 to mean that
- PK11_GetKeyIDFromPrivateKey is dead code and should be deleted, perhaps replaced with a comment pointing readers to function PK11_GetLowLevelKeyIDForPrivateKey, rather than being declared in a public header file and exported. 
- Likewise, PK11_GetKeyIDFromCert is dead code, and should be deleted, perhaps replaced with a comment pointing readers to PK11_GetLowLevelKeyIDForCert (which is equivalent when called with a NULL slot pointer argument).
My next patch will address those issues also.

Comment 48

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for 3.11 branch, addresses review comments (checked in on trunk)

This patch is for the branch.  
I will produce an equivalent patch for the trunk.

Comment 49

[Robert Relyea]

Comment on attachment 292726 [details] [diff] [review]
patch for 3.11 branch, addresses review comments (checked in on trunk)

r+ rrelyea

Comment 50

[Alexei Volkov]

r+ alexei

Comment 51

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 292726 [details] [diff] [review]
patch for 3.11 branch, addresses review comments (checked in on trunk)

This patch has been committed on both trunk and branch.

Comment 52

[Nelson Bolyard (seldom reads bugmail)]

I think this is fixed now.

Comment 53

[Nelson Bolyard (seldom reads bugmail)]

Reopening.  I will back out that patch.  
Somehow it broke a simple certutil -K command (with no arguments) in FIPS mode.  
:(

Comment 54

[Nelson Bolyard (seldom reads bugmail)]

Something is wrong with the trunk.
The patch above is now checked in on the branch and working FINE.  No problems.
The same patch, on the trunk seems to cause big breakage in FIPS mode.

Once again, Tinderbox is useless.  Backing out the flawed checkin does not 
turn the tree green again.  

Bob, please figure out why this patch doesn't work on the trunk in FIPS mode.
I doubt that the problem is with this patch.

Comment 55

[Robert Relyea]

I've looked into it.

The patch is failing because certutil is defaulting to -k rsa rather than -k all.
FIPS tests are using DSA keys, so certutil -K is not finding any keys.

Before the patch certutil was incorrectly ignoring the keytype.

Possible fixes:
- restore -k all as the default keytype for listing keys (NOTE: this is tricky since we still want -k rsa to be the default in other cases).
- add a -k dsa or -k all to the FIPS test line (fix to fips.sh)


I tried to debug why the branch was working, but I was unable to fine the patch on the 3.11 branch.

bob

Comment 56

[Nelson Bolyard (seldom reads bugmail)]

Yes, thanks, I agree.  The help message has been wrong all along.  It says
that the default is "-k rsa", but in fact the default depended on the 
operation being performed, and was actually "-k all" for the list operation.
I will submit a new patch.

Comment 57

[Nelson Bolyard (seldom reads bugmail)]

Attachment: supplemental patch (checked in on trunk)

This patch is an additional patch, to be applied after the previous patch.
It corrects the following things:
1. change the default for certutil -K to be -k all
2. correct the usage message for certutil -K to show that the default is -k all
3. change the behavior of certutil -K when a specific slot is specified, so 
that the command reports failure if no such slot is found, or no keys are found
in the named slot, but the command succeeds if any keys are found in the named
slot, even if other slots exist that have no keys by that name.

Comment 58

[Julien Pierre]

Comment on attachment 301815 [details] [diff] [review]
supplemental patch (checked in on trunk)

This code will make ListKeys return SECFailure only if all the ListKeysInSlot calls in the loop have failed. If a single call succeeds, the function returns SECSuccess. I'm not sure if this was your intention, but I don't think this is correct. I would make it succeed only if all the calls succeeded, and fail otherwise.

Also, there is a pre-existing memory leak, a missing call to 
PK11_FreeSlotList after PK11_GetAllTokens.

Comment 59

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 301815 [details] [diff] [review]
supplemental patch (checked in on trunk)

Julien, 
I'm not sure why, but certutil -K is supposed to report failure if it cannot
find ANY keys that qualify (e.g. with the specified key type(s) in the 
specified slot(s)), and success if it finds any that qualify.  But that's not what the code does prior to this patch.  This patch attempts to make it work as intended.

ListKeysInSlot returns SECFailure if it does not find any keys in the indicated
slot, and returns SECSuccess if it finds any keys that qualify.  Consequently,
to achieve the results stated above, we want function ListKeys to return SECSuccess if ANY call to ListKeysInSlot returns SECSuccess.   

Prior to this patch, when not slot is specified by name, the following 
problems occurred:
1. If NO slots were found, the program indicated success
2. The result reported by the program was the value returned by ListKeysInSlot for the last slot examined.  If the last slot had no keys, but a previously
examined slot had keys, this program reported failure. 

This patch corrects those things.  Please review it in that light.

Comment 60

[Robert Relyea]

Comment on attachment 301815 [details] [diff] [review]
supplemental patch (checked in on trunk)

r+

I believe the semantic is right. If you successfully list certs in at least one key slot, you should succeed. This is because ListKeysInSlot fail if it finds no keys. If you are asking to list all the keys in all the slots, it should be OK to have slots with no keys in them.

Other types of failures are more questionable (do you want to fail because one of your tokens failed to initialize?).

changing to default to all is reasonable. The code correctly changes that default only for listKeys.

bob

Comment 61

[Nelson Bolyard (seldom reads bugmail)]

On the trunk, I committed certutil.c revision: 1.127 which is the union
of the last two reviewed patches shown above.
Am awaiting second review for branch.

Comment 62

[Julien Pierre]

Comment on attachment 301815 [details] [diff] [review]
supplemental patch (checked in on trunk)

Nelson,

Thanks for the explanation. I agree that the patch works as intended. But I think some more explanation of the intended behavior is needed, either in the usage for -K, in the code, or perhaps both. 

Also a SECstatus is an enum, not a bitflag, and I think using binary AND arithmetic with it is unadvisable, and unless you know that SECStatus has the value zero (I had to double-check it for the review), is confusing. So, please change 

-		rv = ListKeysInSlot(le->slot,nickName,keyType,pwdata);
+		rv &= ListKeysInSlot(le->slot,nickName,keyType,pwdata);

to something like :

if (SECSuccess == ListKeysInSlot(le->slot,nickName,keyType,pwdata)) rv = SECSuccess;

This is probably a good place for the comment also, something like.

/* ignore failure. We need at least one successful call */

Also, this patch still has a memory leak of the token list, which should be fixed.

Comment 63

[Julien Pierre]

In the previous comment, I meant, unless you know that SECSuccess has the value zero.

Comment 64

[Nelson Bolyard (seldom reads bugmail)]

Julien,
I'm willing to add a comment to the code, and I'm willing to plug the 
pre-existing leak.  What's the right function to destroy the slot list?

Comment 65

[Nelson Bolyard (seldom reads bugmail)]

I've known that SECSuccess is zero and SECFailure is -1 for over 10 years.
I'm surprised you didn't.

Comment 66

[Nelson Bolyard (seldom reads bugmail)]

Another idea, 
Change ListKeysInSlot to that it outputs a count of found keys.
Have it return SECSuccess unless it actually fails (some fatal error).
In other words, simply finding zero keys will not return SECFailure.
Then have the listkeys function add up the count of keys returned by 
each call to ListKeysInSlot, and report failure if either
a) any invocation of ListKeysInSlot returns SECFailure, or
b) the total count of found keys is zero.  

Comment 67

[Nelson Bolyard (seldom reads bugmail)]

Attachment: suplemental patch #2, plug leak (checked in on trunk)

Address preceding review comments

Comment 68

[Alexei Volkov]

Comment on attachment 302080 [details] [diff] [review]
suplemental patch #2, plug leak (checked in on trunk)

r=alexei

Comment 69

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 302080 [details] [diff] [review]
suplemental patch #2, plug leak (checked in on trunk)

Committed on trunk.
Checking in certutil.c; new revision: 1.129; previous revision: 1.128

awaiting review for branch.

Comment 70

[Nelson Bolyard (seldom reads bugmail)]

Julien, how does bug 416067 depend on this bug?

Comment 71

[Julien Pierre]

Only because of a merge conflit. See https://bugzilla.mozilla.org/show_bug.cgi?id=416067#c4 .

Comment 72

[Robert Relyea]

Comment on attachment 302080 [details] [diff] [review]
suplemental patch #2, plug leak (checked in on trunk)

r+ rrelyea

Comment 73

[Nelson Bolyard (seldom reads bugmail)]

I checked in the 3 patches above together in a single checkin on the branch.
Checking in certutil.c; new revision: 1.97.2.12; previous revision: 1.97.2.11