crlutil -L outputs false CRL names

RESOLVED FIXED in 3.12

Status

NSS
Tools
P2
normal
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Alexei Volkov)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

4.39 KB, patch
Julien Pierre
: review+
Details | Diff | Splinter Review
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: "caemail@ca.com"
        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"
(Assignee)

Comment 1

12 years ago
Created attachment 209901 [details] [diff] [review]
fix for reporter crl names
Attachment #209901 - Flags: review?(julien.pierre.bugs)
(Reporter)

Comment 2

12 years ago
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).
Attachment #209901 - Flags: review-

Comment 3

12 years ago
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.
Attachment #209901 - Flags: review?(julien.pierre.bugs) → review-
(Assignee)

Comment 4

12 years ago
Created attachment 210190 [details] [diff] [review]
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
Attachment #209901 - Attachment is obsolete: true
Attachment #210190 - Flags: superreview?(nelson)
Attachment #210190 - Flags: review?(julien.pierre.bugs)

Comment 5

12 years ago
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

12 years ago
Comment on attachment 210190 [details] [diff] [review]
bug fix

See previous comments.
Attachment #210190 - Flags: review?(julien.pierre.bugs) → review-
(Reporter)

Comment 7

12 years ago
Comment on attachment 210190 [details] [diff] [review]
bug fix

I have nothing to add to Julien's review comments for this patch.
Attachment #210190 - Flags: superreview?(nelson)
(Assignee)

Comment 8

12 years ago
Created attachment 210400 [details] [diff] [review]
patch

changes according review comments
Attachment #210190 - Attachment is obsolete: true
Attachment #210400 - Flags: review?(julien.pierre.bugs)

Updated

12 years ago
Attachment #210400 - Flags: review?(julien.pierre.bugs) → review+
(Reporter)

Comment 9

12 years ago
Setting target to 3.12 while we decide if we want this fix in 3.11.1
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → 3.12
(Assignee)

Comment 10

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

(Reporter)

Updated

12 years ago
QA Contact: jason.m.reid → tools
(Reporter)

Comment 11

12 years ago
Alexei, Is this bug fixed now?
Is there anything left to be checked in?
Any part left unfixed?
(Assignee)

Comment 12

12 years ago
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".
(Reporter)

Comment 13

12 years ago
Please drive this bug to completion on the 3.11 branch as well.
That means getting a second review.  
(Assignee)

Updated

12 years ago
Attachment #210400 - Flags: superreview?(nelson)
(Assignee)

Updated

12 years ago
Attachment #210400 - Flags: review?(neil.williams)
(Assignee)

Comment 14

12 years ago
Comment on attachment 210400 [details] [diff] [review]
patch

the patch is integrated into the 3.11 branch.
Attachment #210400 - Flags: superreview?(nelson)
Attachment #210400 - Flags: review?(neil.williams)
(Assignee)

Comment 15

12 years ago
3.11 branch: 
crlutil.c
new revision: 1.26.24.1
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.