Closed
Bug 265003
Opened 20 years ago
Closed 18 years ago
Add CRL generation to crlutil
Categories
(NSS :: Libraries, enhancement, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.11.3
People
(Reporter: julien.pierre, Assigned: alvolkov.bgs)
References
Details
Attachments
(7 files, 4 obsolete files)
81.75 KB,
patch
|
julien.pierre
:
review+
|
Details | Diff | Splinter Review |
48.66 KB,
text/plain
|
julien.pierre
:
review+
|
Details |
7.22 KB,
text/plain
|
julien.pierre
:
review+
|
Details |
43.94 KB,
text/plain
|
julien.pierre
:
review+
|
Details |
4.12 KB,
text/plain
|
julien.pierre
:
review+
|
Details |
59 bytes,
text/plain
|
Details | |
14.20 KB,
text/plain
|
Details |
Reporter | ||
Comment 1•20 years ago
|
||
Currently, we can't easily add CRL testing to our test suite because we don't have any tools to generate CRLs. The interface for generating a CRL from command-line is somewhat complicated, because NSS tools don't contain a full CA. We can decide to implement a limited number of CRL features for crlutil, and add them as we want to test them. Eg., start with options to put serial numbers/ranges of SNs without reason codes ... Or we can go all-out and design a text file format to feed crlutil as input to generate a binary ASN.1 CRL ...
Severity: normal → enhancement
Priority: -- → P2
Target Milestone: --- → 3.10
Reporter | ||
Updated•20 years ago
|
Keywords: sun-orion4
Reporter | ||
Updated•19 years ago
|
Assignee: wtchang → alexei.volkov.bugs
OS: SunOS → All
Hardware: Sun → All
Reporter | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 2•19 years ago
|
||
Alexei, Could you please attach your current code for review ? Thanks.
Assignee | ||
Comment 3•19 years ago
|
||
The tar file includes patch for existing nss files as well as additional files tared with the following relative path: mozilla/security/nss/cmd/crlutil/ cvs diff and tar are made from parent directory of mozilla.
Reporter | ||
Comment 4•19 years ago
|
||
Alexei, Here is the review feedback for the selfserv patch. More later. 1) in general, there are problems with the indentation related to tabs and spaces. Please don't use tabs in new code. Spaces are better. For existing blocks of code you are modifying, you can keep using the existing indentation 2) in reload_crl, use SECItem_FreeItem to free the old CrlDER, not PORT_Free . Otherwise, you only free the item structure, but you leak its content . 3) in the same function, you have too many return paths, but only one path releases the lock . Make sure you always release it before you return, or there will be deadlocks 4) also don't leak crlDER if there is a failure to uncache the last CRL or cache the new one. Again, having only one return path would be preferred 5) you are using protoEnd - fileName many times as the length of the URI . Please use that expression only once in a local variable . That will make the code much easier to read . 6) you have complex code to try to put the error message together. Please use a function like PR_snprintf to simplify the code . In general, please try not to attach tar files to bugzilla bugs. It's much harder to review with a tar file. Try to attach a set of diffs, grouped by logical pieces, perhaps by unit. You can do cvs diff -N after you do a cvs add of the new files on your local tree, and then the diff will include new files. Please attach the selfserv patch by itself, since it really doesn't depend on any of the other pieces. I'll continue to review the other parts of the new code in your tar file and add feedback, and you can post the changes as separate patch files as needed.
Reporter | ||
Comment 5•19 years ago
|
||
Alexei, Here are my review comments for secutil - header and source 1) I intend to fix the classic ASN.1 encoder bug (bug 280121) so it can encode CRLs. Therefore, the secutil hack in this patch - which duplicates CRL templates from crl.c to change the nextupdate field from an INLINE to a POINTER won't be necessary . Please provide a version of your code that doesn't have this hack, so I can work on the fix for the ASN.1 encoder and test it. 2) in SECU_StoreCRL, you are asserting that crl is NULL, but not actually checking it for NULL in the optimized case. Please add a check. 3) indentation is broken in SECU_StoreCRL 4) Please use a const PRBool for the ascii parameter in SECU_StoreCRL, rather than an int 5) It appears your function will either import the CRL to a slot or output to a file. Why not allow both ? 6) It should probably be an error if neither slot nor outfile is passed . In that case the whole function is a no-op 7) the comment + /* we only deal with cert v3 here */ is incorrect. That should say CRL v2 8) in SECU_SignAndEncodeCRL, if the final SEC_ASN1EncodeItem call fails or the PORT_ArenaZAlloc fails, your function should return a failure, but it currently does not 9) in general SECU_SignAndEncodeCRL has memory leaks in the CRL arena in failure cases. These can be avoided. I suggest you use a temporary arena for the many encoding steps instead, and if all the steps succeed, then copy the generated CRL DER to the CERTSignedCrl's arena with SECITEM_ArenaDupItem . Make sure to assign the derCrl field correctly also . 10) in SECU_CopyCRL, you have an assertion for srcCrl, but no check in optimized code. 11) You should also have checks for destCrl 12) SECU_StringToSignatureAlgTag should take a const char* instead of char*
Reporter | ||
Comment 6•19 years ago
|
||
Alexei, 1) Your certutil patch is fine. But of course the change is dependent upon the secutil changes, so hold off checking it in until secutil is OK. 2) In crlutil, GenerateCRL(), please delete the comment about DER_SetUInteger 3) Please use braces in this else statement to follow convention in the rest of this source file + hashAlgTag = SEC_OID_UNKNOWN; 4) The timing code (with starttime, endtime) probably doesn't belong in GenerateCRL 5) You are leaking the crlDER buffer from SECU_ReadDERFromFile in all failure cases with the goto loser 6) This comment below does not match the code. You are not actually checking for the CRL signing purpose. You should use CERT_FindUserCertByUsage, with KU_CRL_SIGN . That would dispense you from a separate usage check (which would be done with CERT_CheckCertUsage) . + /* If caCert is a v3 certificate, make sure that it can be used for + crl signing purpose */ + cert = CERT_FindCertByName(certHandle, &modCrl->crl.derName); 7) You are leaking the issuer cert if CERT_VerifySignedData fails due to the goto loser . 8) Your code tries to do the CA cert lookup by name in two places. If found the first time, a second lookup isn't necessary. 9) In the case of finding a cert by nickname, you must call CERT_CheckCertUsage to make sure it has the KU_CRL_SIGN usage 10) typo in this error msg + SECU_PrintError(progName, "bad sertificate usage\n"); 11) when computing outFileName, it would be easier to use PR_snprintf 12) when setting the CRL version, use the macro SEC_CRL_VERSION2, not the hardcoded value of 1 13) You need to check the result from SECITEM_CopyItem when copying the cert subject 14) several command-line options were changed, for example -p was used for partial decoding, and you changed it to -s . We can't change the existing syntax of our tools . Please use new options for your new features, and leave the existing options.
Assignee | ||
Comment 7•19 years ago
|
||
the patch has fixes for problems that was found during code review. Also reload_crl function is rewritten to cache new crl first and then uncache an old one.
Assignee | ||
Updated•19 years ago
|
Attachment #177639 -
Attachment is obsolete: true
Reporter | ||
Comment 8•19 years ago
|
||
Alexei, I reviewed your latest selfserv patch. 1) you are doing a late initialization of lastLoadedCrlLock . This isn't safe. 2 threads could try to create that lock at the same time. You need to do it at the beginning of the program. You should also destroy it at the end. 2) in the following, you should set iov_len to the return value of PR_snprintf. Otherwise, if the message is exactly of size sizeof(msgBuf), the string may not be NULL-terminated and PORT_Strlen could crash. PR_snprintf(msgBuf, sizeof(msgBuf), "%s%s ", crlCacheErr, errString); iovs[numIOVs].iov_base = msgBuf; iovs[numIOVs].iov_len = PORT_Strlen(msgBuf); Nits : 3) you can use the PR_MIN macro instead of the following : int protoLen = (protoEnd - fileName < sizeof(proto) - 1) ? protoEnd - fileName : sizeof(proto) - 1; 4) the indentation on the PR_TransmitFile line is incorrect 5) you can use PR_MIN there : if (errLen > sizeof msgBuf - 1) errLen = sizeof msgBuf - 1;
Assignee | ||
Comment 9•19 years ago
|
||
this patch includes new files.
Attachment #179084 -
Attachment is obsolete: true
Assignee | ||
Comment 10•19 years ago
|
||
Attachment #179342 -
Attachment is obsolete: true
Comment 11•19 years ago
|
||
Comment on attachment 180202 [details] [diff] [review] patch includes fixes for problems found during review I'm not going to do a real review of this code, but I have a request that applies to any new code, and any changes to exsting code, that allocates structs or arrays of type, with or without zeroing. Please change all lines of the form: ptr = (type *)PORT_ArenaAlloc(arena, sizeof(type)); to ptr = PORT_ArenaNew(arena type); Similarly, change ArenaZalloc to ArenaZNew. When no arenas are involved, change ptr = (type *)PORT_Alloc(sizeof(type)); to ptr = PORT_New(type); Similarly change PORT_Zalloc to PORT_ZNew. When arrays of some type are being allocated, change ptr = (type *)PORT_Alloc(nItems * sizeof(type)); to ptr = PORT_NewArray(type, nItems); Similary there are PORT_ZNewArray, PORT_ArenaNewArray and PORT_ArenaZNewArray. Note that type can include pointer types, e.g. SECItem ** foo = PORT_NewArray(SECItem *, 10); See the full set of these macros at http://lxr.mozilla.org/security/source/security/nss/lib/util/secport.h#158
Assignee | ||
Comment 12•19 years ago
|
||
Includes: secutil.c change to use CERT_IsUserCert function tests(cert.sh and ssl.sh) changes Use of proper allocation macros
Attachment #180202 -
Attachment is obsolete: true
Assignee | ||
Comment 13•19 years ago
|
||
Assignee | ||
Comment 14•19 years ago
|
||
Assignee | ||
Comment 15•19 years ago
|
||
Assignee | ||
Comment 16•19 years ago
|
||
Assignee | ||
Comment 17•19 years ago
|
||
Reporter | ||
Updated•19 years ago
|
Attachment #180442 -
Flags: review+
Reporter | ||
Updated•19 years ago
|
Attachment #180441 -
Flags: review+
Reporter | ||
Updated•19 years ago
|
Attachment #180438 -
Flags: review+
Reporter | ||
Updated•19 years ago
|
Attachment #180443 -
Flags: review+
Reporter | ||
Updated•19 years ago
|
Attachment #180444 -
Flags: review+
Comment 18•19 years ago
|
||
Alexei Just a note for the future, if you do a 'cvs add' for your new files, then do a 'cvs diff -uN', the patch will contain all your new files, and you can make a single attachment. It's not a big deal, I've only found out about '-N' recently myself;). bob
Comment 19•19 years ago
|
||
Comment on attachment 180438 [details] [diff] [review] last changes from review 04/11 >Index: cmd/lib/secutil.c >+ SECStatus >+ SECU_StoreCRL(PK11SlotInfo *slot, SECItem *derCrl, PRFileDesc *outFile, >+ const PRBool ascii, char *url) >+ { ... >+ } >Index: cmd/lib/secutil.h >+ extern SECStatus SECU_StoreCRL(PK11SlotInfo *slot, SECItem *derCrl, >+ PRFileDesc *outFile, int ascii, char *url); Note that the 'ascii' parameter has different types in secutil.c and secutil.h. The 'const' should definitely be removed because it is useless in that context. Between PRBool and int, PRBool is better but seems to require fixing the callers of this function as well, so it's okay if you simply change "const PRBool" to "int" in secutil.c.
Comment 20•19 years ago
|
||
Comment on attachment 180441 [details]
new file: crlgen.c
The rangeFrom member of the CRLGENGeneratorDataStr structure
is declared as PRUint64 in crlgen.h. But in crlgen.c, we
pass it to SEC_ASN1EncodeInteger, which takes a long:
SECItem * certIdItem =
SEC_ASN1EncodeInteger(arena, NULL,
crlGenData->rangeFrom + i);
On 32-bit platforms, crlGenData->rangeFrom is truncated to
32 bits, and this defeats the purpose of declaring rangeFrom
as PRUint64.
This is reported as a compiler warning, but it is a real design
problem. I don't know what the right solution is, but it is
definitely not adding a (long) cast.
Comment 21•19 years ago
|
||
Comment on attachment 180443 [details]
new file: crlgen_lex.c
This file uses isatty(). On Unix, isatty() is declared
in <unistd.h>, but on Windows it is declared in <io.h>.
So this file needs to include <io.h>, which now is done
only if __TURBOC__ is defined. Since this is a generated
file, the fix may need to be done on the original file
first.
Reporter | ||
Comment 22•19 years ago
|
||
Wan-Teh, I'm aware of the limitation and the current truncation of the serial number. I discussed this at length with Alexei during the code review last week. Our longer term goal would be to support 160 bit serial numbers, as required by RFC 3280. Unfortunately, we have no public interfaces at this time that support such big numbers, and we aren't allowed at this stage in the Sun 3.10 cycle to add new ones, so we had to settle for a shorter one, which is acceptable for CRLs used in the QA test suite. This is one of the many tracking bugs/RFE that Alexei is supposed to file in bugzilla that we found in the code review but that had no immediate solution. Alexei is on vacation and he will be back next monday.
Updated•19 years ago
|
QA Contact: bishakhabanerjee → jason.m.reid
Comment 23•19 years ago
|
||
I believe this bug was fixed in NSS 3.10. Alexei, Julien, could you confirm by marking the bug resolved/fixed?
Reporter | ||
Comment 24•19 years ago
|
||
Yes, this was fixed in 3.10 . Marking fixed.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 25•19 years ago
|
||
Reopening. Although the capability to generate CRLs was added to crlutil in NSS 3.10, the CRLs thus generated have been producing QA test failures ever since then. The CRLs thus generated have invalid DER encoding, IINM. IIRC, the cause of this problem is bug 244922, on which Alexei is working. I think there may be other related bugs about that problem. I don't think this bug can be considered truly resolved fixed until the CRLs generated all have valid DER encoding. Please correct me if I am mistaken about this.
Reporter | ||
Comment 26•19 years ago
|
||
Nelson, I believe bug 244922 is unrelated to CRL templates. I'm not aware of invalid encoding of CRLs in our QA tests. The reason for the intermittent failures is not because of invalid CRL encoding, but rather problems with selfserv and shutting it down. Sometimes the tests that expect certs to be revoked run against a selfserv instance which doesn't have CRLs loaded in its db and/or dynamically loaded with CERT_CacheCRL . Closing again as FIXED.
Status: REOPENED → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
Comment 27•19 years ago
|
||
Ever since the implementation of this RFE was checked in, I have been seeing the attached output every time I run all.sh. It reports invalid DER encoding of the newly generated CRLs. This is the subject of bug 292285.
Comment 28•19 years ago
|
||
reopening, and marking as blocked by bug 292285. I think (but am not certain) that bug 292285 in turn depends on bug 244922.
Updated•18 years ago
|
QA Contact: jason.m.reid → libraries
Reporter | ||
Comment 29•18 years ago
|
||
Alexei, Please review this bug to find out whether it has been fixed.
Target Milestone: 3.10 → 3.11.3
Assignee | ||
Comment 30•18 years ago
|
||
292285 will be fixed in 3.12. The side effect from integrated changes for this bug fix are not observed any more after the teporary fix for 292285 was integrated by Nelson. It does not block this particular rfe from been closed. Mark it as fixed and remove 292285 from dependency list.
Status: REOPENED → RESOLVED
Closed: 19 years ago → 18 years ago
No longer depends on: 292285
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•