Closed Bug 265003 Opened 20 years ago Closed 18 years ago

Add CRL generation to crlutil

Categories

(NSS :: Libraries, enhancement, P2)

3.9.2
enhancement

Tracking

(Not tracked)

RESOLVED FIXED
3.11.3

People

(Reporter: julien.pierre, Assigned: alvolkov.bgs)

References

Details

Attachments

(7 files, 4 obsolete files)

 
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
Keywords: sun-orion4
Assignee: wtchang → alexei.volkov.bugs
OS: SunOS → All
Hardware: Sun → All
Status: NEW → ASSIGNED
Depends on: 285233
Depends on: 280121
Alexei,

Could you please attach your current code for review ?

Thanks.
Attached file crl genetation patch (obsolete) —
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.
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.
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*
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.

Attached patch selfserv patch. (obsolete) — Splinter Review
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.
Attachment #177639 - Attachment is obsolete: true
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;
Attached patch crl generation latest changes (obsolete) — Splinter Review
this patch includes new files.
Attachment #179084 - Attachment is obsolete: true
Attachment #179342 - Attachment is obsolete: true
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
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
Attached file new file: crlgen.c
Attached file new file: crlgen.h
Attached file new file: crlgen_lex.c
Attachment #180442 - Flags: review+
Attachment #180441 - Flags: review+
Attachment #180438 - Flags: review+
Attachment #180443 - Flags: review+
Attachment #180444 - Flags: review+
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 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 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 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.
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.
QA Contact: bishakhabanerjee → jason.m.reid
I believe this bug was fixed in NSS 3.10.  Alexei, Julien,
could you confirm by marking the bug resolved/fixed?
Yes, this was fixed in 3.10 . Marking fixed.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
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.  
Status: RESOLVED → REOPENED
Depends on: 244922
Resolution: FIXED → ---
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 ago19 years ago
Resolution: --- → FIXED
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.
reopening, and marking as blocked by bug 292285.
I think (but am not certain) that bug 292285 in turn depends on bug 244922.
Status: RESOLVED → REOPENED
Depends on: 292285
Resolution: FIXED → ---
QA Contact: jason.m.reid → libraries
Alexei,

Please review this bug to find out whether it has been fixed.
Target Milestone: 3.10 → 3.11.3
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 ago18 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.