396322 - Fix secutil's code and NSS tools that print public keys

Status: RESOLVED FIXED

(NSS :: Tools, defect, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

secutil.c, part of the library used by NSS commands, has a pair of functions:
SECU_PrintPrivateKey and SECU_PrintPublicKey. 

SECU_PrintPrivateKey expects as input an EncryptedPrivateKeyInfo.
It parses that and, when decrypted, handles any type of private key in it.

You might expect that SECU_PrintPublicKey would parse a SubjectPublicKeyInfo
and be able to print any kind of public key that it finds therein.  
But it doesn't.  

SECU_PrintPublicKey should be named SECU_PrintRSAPublicKey, because it 
parses and prints only the ASN.1 construct known as RSAPublicKey, a sequence
of two integers, the public modulus and exponent.

The NSS programs pp and signver each have a "-t" option by which you can
tell them to print a public-key or a private-key.  -t private-key causes
them to call SECU_PrintPrivateKey which prints a PrivateKeyInfo.
-t public-key causes them to call SECU_PrintPublicKey, which does NOT print
a PublicKeyInfo, so both programs only handle RSA public keys. 
They have no option to print any other kind of public key, and no option
to handle SubjectPublicKeyInfos.  

I dislike the asymmetry between the public-key and private-key choices.  
If one of them prints a KeyInfo, rather than a bare key, then so should the 
other. NSS's test scripts do not use this -t public-key feature in either 
program.  I suspect the feature is entirely unused.  But it would be useful
to have a tool to print SubjectPublicKeyInfos since Mozilla is now using 
them in its signature verification of browser addons.

I propose to make the following changes.
1) rename SECU_PrintPublicKey to SECU_PrintRSAPublicKey throughout NSS.
2) write a new function SECU_PrintSubjectPublicKeyInfo, that parses and 
prints a SubjectPublicKeyInfo,
3a) change pp and signver to call that new function instead of the old one
when they are invoked with -t public-key, and 
4a) (maybe) add a new -t option choice (e.g. -t RSA-public-key) to invoke
the old function, if anyone thinks that is still desirable.  

An alternative proposal is to do items 1 and 2 as above, but then do 
3b) have -t public-key continue to invoke the renamed SECU_PrintRSAPublicKey
function, and 
4b) add a new -t public-key-info option that calls the new function.  

I prefer to do the first of those two choices, because I think it is 
desirable for -t private-key and -t public-key to be symmetric, in that 
they should both print keyInfos, and I do not think there is much chance 
that any one in the world depends on the existing behavior of -t public-key.  

I invite discussion of these proposals.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Upon further examination, I see that signver is actually a copy of pp,
to which much other code was added.  Signver's primary purpose, to verify 
a PKCS7 signature, occurs only when the undocumented -t option is NOT given.  
So, I propose to remove the -t option entirely from signver.  

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1 - full patch including whitespace changes

This patch has the desired effects on pp and signver.
It is a major overhaul of signver, including changing the code to 
use tabstops at 8 characters instead of 4 characters.  So the patch
is large. 
I will also attach a patch in which whitespace has been ignored.
That will make it easier to see the functional changes in signver (I hope).

The signver overhaul fixes the -A command, so that it now works even
when the -V command is not specified.  The two commands are not mutually
exclusive.  If neither command is given, -V is assumed.  It appears to me
that signver did not work AT ALL before this patch.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1 - ignoring whitespace

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Binary Subject public key info file

with the above patch, the command 
    pp -t public-key -i mossop.der
will now print out the subject public key info in the attached file.

The attached file comes from 
http://wiki.mozilla.org/User:Mossop:Fx-Docs:AddonUpdateSignature

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 281112 [details] [diff] [review]
patch v1 - full patch including whitespace changes

Bob, please review.  Note that there are two copies of this patch attached, one with whitespace changes included (the full patch) and one ignoring whitespace.  Please feel free to review either one. 

This patch also removes the previously undocumented -t option from the signver command line, since that functionality completely duplicated the functionality in pp.

I discovered that signver can only verify detached signatures, not "opaque" signatures (where the PKCS7 data also contains the signed data).  That should be easy to fix, but I'll save that for another time.

Comment 6

[Robert Relyea]

Comment on attachment 281112 [details] [diff] [review]
patch v1 - full patch including whitespace changes

r+ with comments:

1) SECU_PrintSubjectPublicKeyInfo looks like it's missing a printing of a label. The char *m parameter is ignored and the level is incremented. I seems we should either a) pass m to secu_PrintSubjectPublicKeyInfo and pass the current level, or b) print out the label passed in to us at the current level.

2) Your renamed SECU_PrintPublicKey function (SECU_PrintRSAPublicKey) does not appear to have any callers (that may be intentional, ignore this comment if it is.

In signver.c:
3) The length of the static digest buffer is "magic" and seems to small for our largest hash cipher. This isn't a huge problem, since your digest function does check the buffer size to make sure the buffer fits. a) I would like to see this buffer size set to HASH_LENGTH_MAX from hasht.h. b) In an ideal world, it would also check to see if the buffer was to small and allocate a new buffer of the correct length, but I  won't say that that is necessary for this patch.

4) There is a curious mix of stdio and prio functions in this file. It would be nice to rationalize that, but that would entail determining why the PR_Open for the output file was changed to an fopen in the original code. Again that work is not necessary for this patch.

Anway I would really like to see 1 and 2a fixed, but neither are real bugs so the patch ir r+

bob

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Checking in lib/secutil.c      new revision: 1.81; previous revision: 1.80
Checking in lib/secutil.h      new revision: 1.23; previous revision: 1.22
Checking in pp/pp.c            new revision: 1.9;  previous revision: 1.8
Checking in signver/signver.c  new revision: 1.13; previous revision: 1.12

It was my intention that SECU_PrintRSAPublicKey would have no callers.
I did address comment 1 and comment 3.