24.52 KB, patch
|Details | Diff | Splinter Review|
21.32 KB, patch
|Details | Diff | Splinter Review|
162 bytes, application/octet-stream
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.
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.
Created attachment 281112 [details] [diff] [review] 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.
Created attachment 281114 [details] 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
Attachment #281113 - Attachment description: patch v1 - ignorign whitespace → patch v1 - ignoring whitespace
Priority: -- → P3
Target Milestone: --- → 3.12
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.
Attachment #281112 - Flags: review?(rrelyea)
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
Attachment #281112 - Flags: review?(rrelyea) → review+
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.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.