324878 - crlutil -L outputs false CRL names

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

crlutil -L -d <dir>               outputs a list of CRL names, one per CRL.
crlutil -L -d <dir> -n <nickname> outputs the pretty printed content of one CRL.

The output of the first command should include nicknames that can be used 
in the second (-n) form of the command.   
But the names output by the first command are not CRL nicknames.  That is,
those output names cannot be used as the nickname argument in the -n form
of the command shown above.  So, the output of the first command is pretty
useless.  I can get a list of CRL names, but cannot use those names to see
the CRLs themselves.  

The nickname desired by the -n form of the command is the nickname of the
cert of the CA that issued the CRL.  But there's no good way to find that
nickname from the output of the first crlutil command.  IMO, the first 
command should find and output the nickname of the CA cert, so that the 
user can get the nickname for the second form of the command directly from
the output of the first command.

Example:

+ crlutil -L -d server
CRL names                                CRL Type
CN=NSS Test CA,O=BOGUS NSS,L=Mountain View,ST=California,C=US CRL

+ certutil -L -d server
BIJOU.red.iplanet.com                                        u,u,u
TestCA                                                       CT,C,C

+ certutil -L -d server -n "CN=NSS Test CA,O=BOGUS NSS,L=Mountain View,ST=California,C=US CRL"
certutil: Could not find: CN=NSS Test CA,O=BOGUS NSS,L=Mountain View,ST=Californ
ia,C=US CRL: security library: bad database.

+ crlutil -L -d server -n TestCA
CRL Info:
    Version: 2 (0x1)
    Signature Algorithm: PKCS #1 MD5 With RSA Encryption
    Issuer: "CN=NSS Test CA,O=BOGUS NSS,L=Mountain View,ST=California,C=US"
    This Update: Thu Jan 26 15:50:31 2006
    Entry (1):
        Serial Number: 40 (0x28)
        Revocation Date: Thu Jan 26 15:50:10 2006
        Entry Extensions:
            Name: CRL reason code

    Entry (2):
        Serial Number: 42 (0x2a)
        Revocation Date: Thu Jan 26 15:50:34 2006
    CRL Extensions:
        Name: Certificate Issuer Alt Name
        RFC822 Name: "[e-mail]"
        DNS name: "ca.com"
        Directory Name: "CN=NSS Test CA,O=BOGUS NSS,L=Mountain View,ST=Califo
            rnia,C=US"
        URI: "http://ca.com"
        IP Address:
            87:0b:31:39:32:2e:31:36:38:2e:30:2e:31

That first command would be much more useful if it output the string "TestCA"
instead of "CN=NSS Test CA,O=BOGUS NSS,L=Mountain View,ST=California,C=US CRL"

Comment 1

[Alexei Volkov]

Attachment: fix for reporter crl names

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 209901 [details] [diff] [review]
fix for reporter crl names

>+	    if (cert) {
>+	        asciiname = PORT_Alloc(strlen(cert->nickname) + 1);

what if cert->nickname is NULL? (it can happen), or points to an 
empty string (first character is \0 )?

>+	        sprintf(asciiname, "%s", cert->nickname);
>+	        CERT_DestroyCertificate(cert);
>+	    } else {
>+	        name = &crlNode->crl->crl.name;
>+	        if (!name){
>+	            fprintf(stderr, "%s: fail to get the CRL issuer name (%s)\n", progName,
>+	                    SECU_Strerror(PORT_GetError()));

That fprintf is equivalent to  a call to SECU_PrintError.  
Please use SECU_PrintError instead. 

>+	            break;

This terminates the loop, meaning that any CRLs that follow this one will not
be printed.  I'm pretty sure that in this case, we want to continue the loop
with the next CRL in the list (if any).

Comment 3

[Julien Pierre]

Comment on attachment 209901 [details] [diff] [review]
fix for reporter crl names

SECU_FindCrlIssuer was written for crlgen to find a user cert to sign a CRL . In this case we are only listing and we don't need a user cert. So, you can just use CERT_FindCertByName to look up by subject.

The 3 bugs that Nelson found are all pre-existing, but since you are working on this area of the code, you should fix them.

Comment 4

[Alexei Volkov]

Attachment: bug fix

Fixes problems that Nelson and Julien have found. Also has some modification to 
FindCRL function to be able to display CRLs using email address as well as AVA

Comment 5

[Julien Pierre]

Comment on attachment 210190 [details] [diff] [review]
bug fix

- In FindCrl, you can't access the CERT_NameTemplate directly when encoding. This will break the build on windows because global variables don't work in DLLs. You need to use SEC_ASN1_GET(CERT_NameTemplate) .

- The following expression is dangerous and might dereference NULL. Some more pointer checking is needed.
SECItem* subject = &crlNode->crl->crl.derName;

- nit: you can use PORT_Strdup instead of PORT_Alloc and sprintf to duplicate certName to asciiname.

Comment 6

[Julien Pierre]

Comment on attachment 210190 [details] [diff] [review]
bug fix

See previous comments.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 210190 [details] [diff] [review]
bug fix

I have nothing to add to Julien's review comments for this patch.

Comment 8

[Alexei Volkov]

Attachment: patch

changes according review comments

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Setting target to 3.12 while we decide if we want this fix in 3.11.1

Comment 10

[Alexei Volkov]

Checking in crlutil.c;
/cvsroot/mozilla/security/nss/cmd/crlutil/crlutil.c,v  <--  crlutil.c
new revision: 1.27; previous revision: 1.26

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Alexei, Is this bug fixed now?
Is there anything left to be checked in?
Any part left unfixed?

Comment 12

[Alexei Volkov]

Lost the track of it. It is not closed because it is not in 3.11 branch yet. Do we want it in 3.12 only? If yes, then the bug status should be set to "resolved".

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Please drive this bug to completion on the 3.11 branch as well.
That means getting a second review.  

Comment 14

[Alexei Volkov]

Comment on attachment 210400 [details] [diff] [review]
patch

the patch is integrated into the 3.11 branch.

Comment 15

[Alexei Volkov]

3.11 branch: 
crlutil.c
new revision: 1.26.24.1