55898 - Certutil - print name of cert causing validation to fail

Status: RESOLVED FIXED

(NSS :: Tools, defect, P3)

Description

[Steve Parkinson]

In Certutil, the ValidateCert() call:
http://lxr.mozilla.org/mozilla/source/security/nss/cmd/certutil/certutil.c#649

.. should print out the name of the certificate causing the validation to
fail, (as well as the error) (line 733).

Also, I don't think we should break the loop if there's an error. Just
keep printing all the errors, so long as each error has a corresponding
certificate name.

Comment 1

[Fabian Guisset]

Steve Parkinson, is this still a problem? If yes could you confirm the bug?
Thanks,
Fabian.

Comment 2

[Wan-Teh Chang]

Ian, if this bug does not need to be fixed in NSS 3.2,
please set the target milestone to 3.3.  Thanks.

Comment 3

[Ian McGreer]

moving to 3.3

Comment 4

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 5

[Wan-Teh Chang]

Set target milestone to NSS 3.5.

Comment 6

[Wan-Teh Chang]

Assigned the bug to Julien.

Comment 7

[Wan-Teh Chang]

Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Remove target milestone of 3.8, since these bugs didn't get into that release.

Comment 9

[Julien Pierre]

The information that Steve is requesting - the cert that caused the verification
failure - is not currently returned by CERT_VerifyCert or CERT_VerifyCertificate
functions.

There are many possible cases of errors, and they don't always apply to a
particular cert, sometimes it is the absence of a cert (eg. missing issuer) that
causes the failure. I suppose you could treat this as the subject cert.

Another common error case is the absence of trust. The trust could be present on
any cert in the chain - which one do we decide is "missing" the trust ? We could
arbitrarily pick the root.

Solving this problem requires a new cert verification API, not just a patch to
certutil.

Comment 10

[Julien Pierre]

Bob, Nelson, do you know if the log from CERT_VerifyCert detail which
certificate might be the culprit ? If so we might be able to print the cert
causing the failure in certutil.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

The NSS programs vfychain and vfyserv already do this, IINM.  
So certutil -V should also be able to do this.
Look at the sources to those programs to see how.

Maybe the code to do that should move into libsecutil, rather than being
duplicated in several programs, as it is now.

Comment 12

[Julien Pierre]

Thanks, Nelson !
I wasn't aware they already did this.
It looks like each program had its own copy of the printCertProblems function.
I will generate a patch that moves that function to cmd/lib so all 3 programs
(certutil, vfyserv, vfychain) can call it without duplicating the code.

Comment 13

[Julien Pierre]

Attachment: move printCertProblems to library and use it in certutil

Also needed to change the code to use CERT_VerifyCertificate, since certutil
used that function.

Comment 14

[Wan-Teh Chang]

Comment on attachment 132612 [details] [diff] [review]
move printCertProblems to library and use it in certutil

This patch looks good.	Glad that you caught the
"NSS_Shutdown != SECSuccess" bug in vfychain/vfychain.c.
Two comments.

1. The printCertProblems function should be renamed
SECU_PrintCertProblems, with the SECU_ prefix.

I suggest that you move the SECU_PrintCertProblems
function to cmd/lib/secutil.c.	It seems to fit better
in that file.

2. In vfychain/vfychain.c, this change either is incorrect or breaks
backward compatibility:

>-	case 'u' : certUsage = (SECCertUsage)PORT_Atoi(optstate->value); break;
>+	case 'u' : certUsage = (SECCertificateUsage)PORT_Atoi(optstate->value); break;

It seems that we should either add a new command-line option for
SECCertificateUsage, or convert SECCertUsage to SECCertificateUsage
like this:

	case 'u' : certUsage = (SECCertificateUsage)(1 <<
PORT_Atoi(optstate->value)); break;

Comment 15

[Julien Pierre]

Attachment: incorporate Wan-Teh's feedback

Comment 16

[Wan-Teh Chang]

Comment on attachment 132847 [details] [diff] [review]
incorporate Wan-Teh's feedback

r=wtc.

It may be better to say:

   (SECCertificateUsage)1 << PORT_Atoi(optstate->value)

that is, first cast 1 to SECCertificateUsage (a 64-bit type)
and then left-shift it.  I am worried that if we ever have
more than 32 cert usages, 1 << 32 will break.

Comment 17

[Julien Pierre]

Checking in lib/secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.50; previous revision: 1.49
done
Checking in lib/secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.13; previous revision: 1.12
done
Checking in vfychain/vfychain.c;
/cvsroot/mozilla/security/nss/cmd/vfychain/vfychain.c,v  <--  vfychain.c
new revision: 1.4; previous revision: 1.3
done
Checking in vfyserv/vfyserv.h;
/cvsroot/mozilla/security/nss/cmd/vfyserv/vfyserv.h,v  <--  vfyserv.h
new revision: 1.4; previous revision: 1.3
done
Checking in vfyserv/vfyutil.c;
/cvsroot/mozilla/security/nss/cmd/vfyserv/vfyutil.c,v  <--  vfyutil.c
new revision: 1.6; previous revision: 1.5
done