412468 - modify certutil, vfychain and vfyserv utilities to use CERT_PKIXVerifyCert function

Status: RESOLVED WONTFIX

(NSS :: Libraries, enhancement, P1)

Description

[Alexei Volkov]

Those two utilities suppose to become main tools of libpkix testing. They should be modifies to use new NSS API that validates certificate chains with pkix library.

Initially, both utilities should have command line options to enable validation with libpkix and also should be able to accept arbitrary oid of an explicit policy with which a user want to validate a chain.

Comment 1

[Alexei Volkov]

Attachment: Initial patch

Make use of CERT_PKIXVerifyCert for cert chain validation(option -p). Explicit policy can be set with -o <arg> option.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 297206 [details] [diff] [review]
Initial patch

Lots of comments.

1. This patch points out the fact that we don't have an "error log"
for CERT_PKIXVerifyCert.  We really need an error log for that API.

2. This patch has the effect that, if a cert fails to pass verification
when being checked with the old method, the string "Chain is bad," will
be printed AFTER the call to SECU_printCertProblemsOnDate, instead of 
before it.  Consequently, it will print a different error number than
it would have printed if called before.  That should be fixed, even if
it means that the line to print that string must be duplicated.
I'd prefer to see the "Chain is bad" line come before the error log output.

3. 99% of the time, or more, the policy OID will be unknown to NSS. 
Consequently, the value returned by this line
>+            oidTag = SECOID_FindOIDTag(&oid);
will almost always be SEC_OID_UNKNOWN.  The solution is to detect 
that case, and then register the OID with NSS as a dynamic OID,
then call SECOID_FindOIDTag(&oid) again.

Comment 3

[Alexei Volkov]

Attachment: Patch v2

Changes:
1. using cert_po_errorLog flag to make CERT_PKIXVerifyCert to reuse CERTVerifyLog  data to print validation log.
2. Changing the sequence of calls: print error information that happened during validation first and printing log right after that. BTW, SECU_printCertProblemsOnDate does not change error code since it saves and restores it after the function done printing the chain.
3. Adding use defined policy oids to the dynamic table.
4. Decrease the sequence of functions that have been called before CERTVerifyLog printing:
    * before CERT_VerifyCertificate was called without log argument and returned error as soon as it find it. Next CERT_VerifyCertificate was called with log argument to find as many problems with the chain. Later, the log was printed out.
    * this patch changes so that CERT_VerifyCertificate get called with the log  first time. Later, log is printed with all error codes that identify chain problems.
Yes, the returned error code will be different compare to what vfychain is returning today in case when a chain has more then one problem. But I think is inconsistence is acceptable in this program.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 297855 [details] [diff] [review]
Patch v2

Preliminary review comments. (More review comments will follow.)

1. Patch leaves dead function behind in certutil.c

>-		rv = printCertCB(the_cert, the_cert->trust);
>+		rv = SEC_PrintCertificateAndTrust(the_cert, the_cert->trust,
>+		                                  "Certificate");

This patch deletes the only invocation of function printCertCB, replacing
it with a call the the new function SEC_PrintCertificateAndTrust which is 
in secutil.[ch].  That makes function printCertCB be dead code. 

It appears that the intent was to move function printCertCB to secutil and 
rename it SEC_PrintCertificateAndTrust, but the old printCertCB function 
was left behind in certutil.c

2. r+ for the new function in secutil.[ch]

3. I am reviewing the change to vfychain.c now.  Please wait for those 
review comments before submitting another patch.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 297855 [details] [diff] [review]
Patch v2

Here are the rest of my review comments and questions.  
Please answer these questions before submitting another patch.

1. This patch changes the behavior of vfychain so that it always
prints out some information about the trust anchor, if the trust
anchor is available.  It adds an option to control whether this 
output is terse (only the subject name) or verbose (entire cert 
and trust info).  

I think it should not print out any info about the trust anchor
unless the user requests it.  Perhaps the option can take an 
argument telling how verbose to be.  Or perhaps it could be an 
argument that prints a little if it occurs once and a lot if it 
occurs twice, e.g. -l or -ll.

The help message for that new option is:
>+	"\t-l \t\t Print whole cert info\n"

Maybe it should include the word "root" or "trust anchor".

Also, it seems odd that the program would offer to print out 
info about the trust anchor and not about the leaf cert.  So I 
suggest you offer another option to print out the leaf cert.

Also, IIRC, one of the outputs of PKIX is the set of policies 
for which the cert is valid.  We should have an option to print
out those policy OIDs.

2. Question about this code:

>+    /* First, let's try to find the cert in existing DB. */
>+    cert = CERT_FindCertByNicknameOrEmailAddr(defaultDB, name);
>+    if (!cert) {
>+        cert = PK11_FindCertFromNickname(name, NULL);
>+    } 

Is it possible for PK11_FindCertFromNickname to return non-NULL
after CERT_FindCertByNicknameOrEmailAddr returns NULL ?
If so, Under what conditions could that happen?  (e.g. how?)

3. In this line:
>+    PRBool               usePkixEng   = PR_FALSE;

What does "Eng" mean in that variable?  
"English"?
"Engineering"?
Please spell it out or drop that suffix.

4. In the previous version of this patch, it looked up the OID in the 
SECOid table, and if it didn't find that oid, it would fail.  I suggested
that if can't find the oid, the OID should be added to the dynamic OID table.

This version of the patch does not look the OID up in the SECOid table,
and always tries to add the OID to the dynamic OID table.  IIRC, that will 
fail if the OID is already present in the SECOid table.  (Right?) 

Please test that using an OID that is known in the SECOid table.  
If I'm right that a known OID will fail to add, then I think you need to 
change the patch to first see if it's in the OID table, and then attempt
to add it if not.

>+	/* Have cert refs in the log only in case of failure.
>+	 * Destroy them. */
>+	for (node = log.head; node; node = node->next) {
>+	    if (node->cert)
>+	        CERT_DestroyCertificate(node->cert);
>+	}

Good catch.  That fixes a pre-existing leak, I think.

Comment 6

[Alexei Volkov]

> 1. This patch changes the behavior of vfychain so that it always
> prints out some information about the trust anchor, if the trust
> anchor is available.  It adds an option to control whether this 
> output is terse (only the subject name) or verbose (entire cert 
> and trust info).  
> 
> I think it should not print out any info about the trust anchor
> unless the user requests it.  Perhaps the option can take an 
> argument telling how verbose to be.  Or perhaps it could be an 
> argument that prints a little if it occurs once and a lot if it 
> occurs twice, e.g. -l or -ll.

Found out that the program already has the flag that can be used: it is -v(verbose).
I'll make it so if -v  is use, the program will print root cert subject name.
If -v repeated more then once, then the whole root cert info will get printed.  

> The help message for that new option is:
> >+	"\t-l \t\t Print whole cert info\n"
will change this. 

> Maybe it should include the word "root" or "trust anchor".
> 
> Also, it seems odd that the program would offer to print out 
> info about the trust anchor and not about the leaf cert.  So I 
> suggest you offer another option to print out the leaf cert.

Can not do this: CERT_PKIXVerifyCert does not currently return the whole chain.
API has this feature, but it is not implemented yet.

> Also, IIRC, one of the outputs of PKIX is the set of policies 
> for which the cert is valid.  We should have an option to print
> out those policy OIDs.
Neither this. 

> 
> 2. Question about this code:
> 
> >+    /* First, let's try to find the cert in existing DB. */
> >+    cert = CERT_FindCertByNicknameOrEmailAddr(defaultDB, name);
> >+    if (!cert) {
> >+        cert = PK11_FindCertFromNickname(name, NULL);
> >+    } 
> 
> Is it possible for PK11_FindCertFromNickname to return non-NULL
> after CERT_FindCertByNicknameOrEmailAddr returns NULL ?
> If so, Under what conditions could that happen?  (e.g. how?)
This block was copied from somewhere else(plenty of such function call sequences through out the code), but it seems that CERT_FindCertByNicknameOrEmailAddr does everything that PK11_FindCertFromNickname plus more in cases when last argument is NULL.
Will remove the "if" statement. 


> 3. In this line:
> >+    PRBool               usePkixEng   = PR_FALSE;
> 
> What does "Eng" mean in that variable?  
> "English"?
> "Engineering"?
> Please spell it out or drop that suffix.

Obvious, that in this case Eng stands for English :) . But I will remove it.

> 4. In the previous version of this patch, it looked up the OID in the 
> SECOid table, and if it didn't find that oid, it would fail.  I suggested
> that if can't find the oid, the OID should be added to the dynamic OID table.
> 
> This version of the patch does not look the OID up in the SECOid table,
> and always tries to add the OID to the dynamic OID table.  IIRC, that will 
> fail if the OID is already present in the SECOid table.  (Right?) 
IMO, the test is not needed, because the "add" function does the test, before adding the OID.

Comment 7

[Alexei Volkov]

Attachment: Patch v3

Fixes related to review comments.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 302662 [details] [diff] [review]
Patch v3

r=nelson

Comment 9

[Alexei Volkov]

/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.130; previous revision: 1.129
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.82; previous revision: 1.81
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.24; previous revision: 1.23
/cvsroot/mozilla/security/nss/cmd/vfychain/vfychain.c,v  <--  vfychain.c
new revision: 1.12; previous revision: 1.11

Keep the bug open. Need a patch for vfyserv.

Comment 10

[Julien Pierre]

We need a way to specify the NIST CRL policy for vfychain. This is required for running PKITS.

Also, I used vfychain -p and got a reference leak running NIST test 4.4.6 . See 
https://bugzilla.mozilla.org/show_bug.cgi?id=414561#c3 .

Comment 11

[Julien Pierre]

Comment on attachment 302662 [details] [diff] [review]
Patch v3

Alexei,

Your call to SEC_PrintCertificateAndTrust in certutil has arguments 2 and 3 reversed. This results in a compiler warning on Solaris :

"certutil.c", line 521: warning: argument #2 is incompatible with prototype:
        prototype: pointer to const char : "../../../../dist/private/nss/secutil.h", line 298
        argument : pointer to struct CERTCertTrustStr {unsigned int sslFlags, unsigned int emailFlags, unsigned int objectSigningFlags}

Also, why did you use a void* in the prototype for the trust instead of a CERTCertTrust* ?

Comment 12

[Julien Pierre]

I checked in the fix to reverse the arguments. See bug 417392 . I still wonder why you used a void*.

Comment 13

[Alexei Volkov]

Attachment: Remove unused function and rename argument.

Julien, I agree that void * is here for no reason. It's origin is from the function that is now called SEC_PrintCertificateAndTrust.

Comment 14

[Alexei Volkov]

patch 303301 is committed.

Comment 15

[Julien Pierre]

Alexei,

Now there is another compiler warning :

"secutil.c", line 3241: warning: argument #3 is incompatible with prototype:
        prototype: pointer to char : "secutil.c", line 3197
        argument : pointer to const char

I think a function cast may be needed.
Doesn't your compiler give you any warnings about these things M

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch - fix crash on windows (checked in)

Use of the -p -o OID option crashes every time on windows.
This patch fixes it.
Alexei, please review.

Comment 17

[Julien Pierre]

certutil -V also needs to have the ability to use PKIX to verify the cert, just like vfychain and vfyserv. I had mentioned that in one of our Thursday concalls.

Comment 18

[Julien Pierre]

It seems that the support was added, but I thought it was not in, because the usage page of certutil was unchanged. Please fix it.

Comment 19

[Julien Pierre]

OK, never mind. The support was never added to certutil. It needs to be. There were probably some attempts made in this bug since certutil.c was touched, but the PKIX support was not actually added to certutil.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 304290 [details] [diff] [review]
patch - fix crash on windows (checked in)

Checking in vfychain.c; new revision: 1.13; previous revision: 1.12

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Julien, are you aware of the environment variable NSS_ENABLE_PKIX_VERIFY ?
When set to any non-empty string, it causes the old CERT_VerifyCert* 
functions to use libPKIX.  works in all programs.

Comment 22

[Julien Pierre]

Nelson,

I am aware of it, but I want to use CERT_PKIXVerifyCert, not CERT_VerifyCert* . I thought had made it clear in our previous meetings that certutil -V should be one of the tools that should have explicit PKIX support added, and there was agreement. I am just not sure why it did not translate into this bug.

Comment 23

[Alexei Volkov]

Attachment: Patch v4

Change vfychain options: validate cert using libpkix by calling CERT_VerifyCertificate if -p option is specified once, and use CERT_PKIXVerifyCert if the option is specified twice and more times.

Also, the patch enables programmatic interface that allows to switch CERT_VerifyCert* to use libpkix.

Comment 24

[Alexei Volkov]

Attachment: Add -t argument to vfychain, v1

Modify vfychain to use cert_pi_certList option to supply trust anchors.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 307282 [details] [diff] [review]
Patch v4

r=nelson

Please change two things before you commit.

>+/* Makes old cert validation APIs(CERT_VerifyCert, CERT_VerifyCertificate)
>+ * to use libpkix validation engine. The function should be called ones at
>+ * application initialization time.
>+ * Function is not thread safe.*/
>+SECStatus CERT_SetUsePKIXForValidation(PRBool enable);
>+
>+/* The function return PR_TRUE if cert validation should use
>+ * libpkix cert validation engine. */
>+PRBool CERT_CheckUsePKIXForValidation();

1. Please change that name to CERT_GetUsePKIXForValidation, so we'll have 
Get and Set, instead of Check and Set.  Obviously you'll have to change 
it in many places.  Also, a question: shouldn't those declarations be 
preceded by the word "extern" ?

2. There's an obvious problem with alphabetical order in the following.
Please fix that.  

> CERT_FindNameConstraintsExten;
> CERT_GetValidDNSPatternsFromCert;
> CERT_SetOCSPTimeout;
> CERT_PKIXVerifyCert;
>+CERT_SetUsePKIXForValidation;
> HASH_GetType;

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 307625 [details] [diff] [review]
Add -t argument to vfychain, v1

Alexei, I gather that you intended that the -t parameter must be
supplied immediately before each trusted cert. e.g. 

 -t trustedcert1 -t trustedcert2 -t trustedcert3 

and that if -t is omitted from a cert, then that cert becomes
part of the chain to be verified, not the trusted certs, so that
one could intermix the two, e.g. 

 -t trustedcert1 chaincert1 -t trustedcert2 chaincert 2  (etc.)

And my question to you is: have you actually tested that last 
combination, where trusted and non-trusted certs are intermixed?

If that doesn't work, then I think we need to find another strategy
for handling trusted certs.  When I know the answer to that question,
I will give this patch r+ or r-.

I have only two review concerns about this patch. one is trivial.
1) the potential problem described above
2) the fact that this patch uses cert_pi_certList, which is the wrong
enumerated value.  That's trivial to fix, of course.

Comment 27

[Alexei Volkov]

(In reply to comment #26)
> 
>  -t trustedcert1 chaincert1 -t trustedcert2 chaincert 2  (etc.)
>
I've tested: this sequence leads to have two certs in the chain and two cert in the list of trusted roots.

> I have only two review concerns about this patch. one is trivial.
> 1) the potential problem described above
> 2) the fact that this patch uses cert_pi_certList, which is the wrong
> enumerated value.  That's trivial to fix, of course.  

Yes, I should use cert_pi_trustAnchors

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 307625 [details] [diff] [review]
Add -t argument to vfychain, v1

r+, provided that you change cert_pi_certList to cert_pi_trustAnchors

Comment 29

[Kai Engert (:KaiE:)]

We must update this code at the same time the changes in bug 294531 land (today!), because vfychain no longer builds, because we are removing the old CERT_REV_ flags.

Comment 30

[Kai Engert (:KaiE:)]

Attachment: Patch v5: Adjust vfychain to new revocation flags

Comment 31

[Kai Engert (:KaiE:)]

Comment on attachment 309492 [details] [diff] [review]
Patch v5: Adjust vfychain to new revocation flags

Sorry, this patch is buggy, another one coming up.

Comment 32

[Kai Engert (:KaiE:)]

Attachment: Patch v6: Adjust vfychain to new revocation flags

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 309499 [details] [diff] [review]
Patch v6: Adjust vfychain to new revocation flags

r=nelson

Comment 34

[Alexei Volkov]

Comment on attachment 309499 [details] [diff] [review]
Patch v6: Adjust vfychain to new revocation flags

>+    CERTRevocationFlags  rev;
>+    PRUint64             revFlags[1];

Please move there two lines in to the block that starts at line  377. Thx

Comment 35

[Kai Engert (:KaiE:)]

Attachment: Patch v6 plus variables moved [checked in]

This patch has the variables moved as requested by Alexei, and this is what I will check in.

Comment 36

[Kai Engert (:KaiE:)]

Comment on attachment 309551 [details] [diff] [review]
Patch v6 plus variables moved [checked in]

This patch checked in.

Checking in mozilla/security/nss/cmd/vfychain/vfychain.c;
/cvsroot/mozilla/security/nss/cmd/vfychain/vfychain.c,v  <--  vfychain.c
new revision: 1.17; previous revision: 1.16
done

Comment 37

[Alexei Volkov]

Attachment: Print out validated chain

the patch provides some help to debug bug 422859.

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 311690 [details] [diff] [review]
Print out validated chain

r=nelson
>     if (error != NULL) {
>         SECErrorCodes         nssErrorCode = 0;
> 
>         cert_PkixErrorToNssCode(error, &nssErrorCode, plContext);
>         PORT_SetError(nssErrorCode);
>         PKIX_PL_Object_DecRef((PKIX_PL_Object *)error, plContext);
>+        /* XXX Destroy output params in case of error */

What does that comment mean?  Is there a bug here?  a leak?

Comment 39

[Alexei Volkov]

(In reply to comment #38)
> (From update of attachment 311690 [details] [diff] [review])
> r=nelson
> >     if (error != NULL) {
> >         SECErrorCodes         nssErrorCode = 0;
> > 
> >         cert_PkixErrorToNssCode(error, &nssErrorCode, plContext);
> >         PORT_SetError(nssErrorCode);
> >         PKIX_PL_Object_DecRef((PKIX_PL_Object *)error, plContext);
> >+        /* XXX Destroy output params in case of error */
> 
> What does that comment mean?  Is there a bug here?  a leak?
> 

Yes, this is a possibility to a leak. Filed a bug 425516.

Comment 40

[Alexei Volkov]

attachment 311690 [details] [diff] [review] is integrated.

Comment 41

[Alexei Volkov]

Attachment: vfyserv.c patch v1 - modify vfyserv tool to use libpkix api

Moves some functions from vfychain.c into secutil.c to use them in vfyserv.c. Adds new options into vfyserv to use libpkix API.

Comment 42

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 363359 [details] [diff] [review]
vfyserv.c patch v1 - modify vfyserv tool to use libpkix api

r-
I reviewed this with Alexei in person last week.  The patch is mostly good, 
but has some structural issues.  Alexei has the full review details, but 
here I will mention a few.

The big one is that this patch moves code from one of the utility programs 
into the utility library (nss/cmd/lib), and that code does things that are
appropriate for a program but not for a library, things such as defining 
and exporting global variables, defining functions that store state 
information in static variables, etc.  

Neither vfyserv nor vfychain need to be multi-threaded.  The each have only
one worker thread, but they contain lots of code for managing threads. 
I would prefer that we remove the code that attempts to manage threads 
from them, rather than embellish it.

I agree that it's a good idea to try to have common capabilities and 
common command line options between those two programs.

Comment 43

[Alexei Volkov]

Attachment: patch v1 - add option to validate the same cert multiple times. (checked in 2009-Apr-01)

It is -i <num> option.  This option is needed to test libpkix caches.

Comment 44

[Nelson Bolyard (seldom reads bugmail)]

Attachment: the previous patch - ignoring whitespace

Comment 45

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 367045 [details] [diff] [review]
patch v1 - add option to validate the same cert multiple times. (checked in 2009-Apr-01)

r=nelson

Comment 46

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 367045 [details] [diff] [review]
patch v1 - add option to validate the same cert multiple times. (checked in 2009-Apr-01)

Wan-Teh, this patch was committed for 3.12.3.
Is this bug resolved fixed now?

Comment 47

[Nelson Bolyard (seldom reads bugmail)]

Sorry, that last question was meant for Alexei.  :(

Comment 48

[Alexei Volkov]

Not yet. Need to respin patch for vfyserv(attachment 363359 [details] [diff] [review]) and develop new patch for certutil.

Comment 49

[Slavomir Katuscak]

Today I added PKITS tests support to Tinderbox and vfychain produced some core files. I was looking which part of code is failing, and I found out that it's part of patch checked in for this bug.

Core analysis:
t@null (l@1) program terminated by signal SEGV (no mapping at the fault address)
0xbfaa2a5c: strlen+0x000c:      movl     (%eax),%edx
Current function is main
  733               fprintf(stderr, "Chain is bad, %d = %s\n", err, SECU_Strerror(err));
(dbx) where                                                                  
  [1] strlen(0x8066861, 0x8046a28, 0xbfb480a8, 0x0), at 0xbfaa2a5c 
  [2] _fprintf(0xbfb480a8, 0x806684c, 0x0, 0x0), at 0xbfafa8a7 
=>[3] main(argc = 6, argv = 0x8046bf8, envp = 0x8046c14), line 733 in "vfychain.c"

For now I have results only from Solaris where it fails, and from Linux where results are OK, Windows and Mac tests are stull running. 

Those tests are now running only on Tinderboxes (added today), and I plan to ask Christophe to add them also to nightly tests (there is required to set PKITS_DATA variable).

Comment 50

[Slavomir Katuscak]

Total 20 PKITS tests failed, all those tests were negative tests where was expected that vfychain would fail.

Examples of crashes:
---
Test case Invalid CA Signature Test2

certutil -d /export/tinderlight/data/dositups_32_DBG/mozilla/tests_results/security/dositups.1/pkits/PKITSdb -A -t ",," -n BadSignedCACert -i /export/tinderlight/PKITS_DATA/certs/BadSignedCACert.crt
crlutil -d /export/tinderlight/data/dositups_32_DBG/mozilla/tests_results/security/dositups.1/pkits/PKITSdb -I -i /export/tinderlight/PKITS_DATA/crls/BadSignedCACRL.crl
vfychain -d /export/tinderlight/data/dositups_32_DBG/mozilla/tests_results/security/dositups.1/pkits/PKITSdb -u 4 /export/tinderlight/PKITS_DATA/certs/InvalidCASignatureTest2EE.crt /export/tinderlight/PKITS_DATA/certs/BadSignedCACert.crt
./pkits.sh: line 212: 24582 Segmentation Fault      (core dumped) ${BINDIR}/vfychain -d $PKITSdb -u 4 $* >${PKITSDIR}/cmdout.txt 2>&1
Chain is bad, pkits.sh: #2604: Invalid CA Signature Test2 (139)  - Core file is detected - FAILED
pkits.sh SUCCESS: Invalid CA Signature Test2 returned as expected 139
---
Test case Invalid EE Signature Test3

certutil -d /export/tinderlight/data/dositups_32_DBG/mozilla/tests_results/security/dositups.1/pkits/PKITSdb -A -t ",," -n GoodCACert -i /export/tinderlight/PKITS_DATA/certs/GoodCACert.crt
crlutil -d /export/tinderlight/data/dositups_32_DBG/mozilla/tests_results/security/dositups.1/pkits/PKITSdb -I -i /export/tinderlight/PKITS_DATA/crls/GoodCACRL.crl
vfychain -d /export/tinderlight/data/dositups_32_DBG/mozilla/tests_results/security/dositups.1/pkits/PKITSdb -u 4 /export/tinderlight/PKITS_DATA/certs/InvalidEESignatureTest3EE.crt /export/tinderlight/PKITS_DATA/certs/GoodCACert.crt
./pkits.sh: line 212: 24999 Segmentation Fault      (core dumped) ${BINDIR}/vfychain -d $PKITSdb -u 4 $* >${PKITSDIR}/cmdout.txt 2>&1
Chain is bad, pkits.sh: #2605: Invalid EE Signature Test3 (139)  - Core file is detected - FAILED
pkits.sh SUCCESS: Invalid EE Signature Test3 returned as expected 139
---

Comment 51

[Nelson Bolyard (seldom reads bugmail)]

  fprintf(stderr, "Chain is bad, %d = %s\n", err, SECU_Strerror(err));

SECUStrerror has returned NULL.  
That is the correct behavior for SECU_Strerror when it is passed an 
invalid/unrecognized/unknown error number as an argument.  
I suspect that the error code may have been zero.

Comment 52

[Slavomir Katuscak]

  [2] _fprintf(0xbfb480a8, 0x806684c, 0x0, 0x0), at 0xbfafa8a7 

Error was zero.

Comment 53

[Julien Pierre]

So, most likely this is a bug in CERT_PKIXVerifyCert not setting the NSPR error code.  Many tinderboxes are green. It must be that it's OK on those platforms to fprintf("%s", NULL), but it's obviously not OK on Solaris since all the Solaris tinderboxes are crashing with this problem.

I'm a little reluctant to introduce a check for this NULL string that will completely mask the PKIX bug. On the other hand, we are in crunch mode and we can't really afford to have all the tinderboxes be orange as it holds our release candidate.

I propose the following :
1) file PKIX bug for the NSPR error issue (if there isn't one already - it may need to be reopened)
2) add a check to vfychain for a NULL error string before calling fprintf
3) add an assertion for a zero error code from PR_GetError(). This way, debug builds on Solaris will still fail the QA, but optimized builds will get past it. This will helps give us confidence that our other checkins don't break Solaris, until the PKIX bug is fixed (which should be ASAP ...).

Comment 54

[Julien Pierre]

Bug 412318 is related. I think bug 417024 may need to be reopened. Also see bug 420991.

Comment 55

[Julien Pierre]

Attachment: Change SECU_Strerror

Another proposal :

1) assert if errNum is NULL
2) assert if there is no matching string for error
3) return "Unknown error" instead of NULL if there is no matching string for error code.

This should get past this problem for optimized builds green again.
Debug builds will still fail, and this patch might actually find additional pre-existing problems.

Comment 56

[Julien Pierre]

Attachment: Change SECU_Strerror.

Builds better with this include.

Comment 57

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 371976 [details] [diff] [review]
Change SECU_Strerror.

I suppose this is a patch

Comment 58

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 371976 [details] [diff] [review]
Change SECU_Strerror.

Don't assert in this code.
It's not wrong to pass zero (or any other int) as input.  It's only wrong for callers to ignore a null return value.  

I think there may be some code that checks the return value and does something smart, such as display the error number.  If we change this function to return "Unknown error" instead of NULL, we'll defeat whatevr code detects that NULL, and possibly end up displaying "unknown error" instead of an error number.  

On the other hand, if examination of the callers of this function reveals that no callers do anything better with a NULL return value, then go ahead and submit a patch that merely does 
return "Unknown error";

Comment 59

[Julien Pierre]

Nelson,

Given that SECU_Strerror is an internal function which is statically linked and used only in our tools, we can change its semantics, and that's what I was proposing in attachment 371976 [details] [diff] [review]. The assertions will help detect bugs - IMO, nobody should call SECU_Strerror() unless some API failed, and if they pass zero, that would probably indicate that the API in question forgot to set an error code. Likewise, if they pass some unknown error code, it would indicate that we probably don't have an error string for that error in our table, which would also be a bug in need of fixing. Therefore, I think these assertions are helpful for debug code, and they don't hurt optimized code. Please reconsider your review.

Regarding the issue of returning "Unknown error", I inspected all callers of SECU_Strerror. You can check for yourself at http://mxr.mozilla.org/security/search?string=secu_strerror . I found very few instances that bothered to check the return value - about 7 out of about 50. All the others pass the return value directly to fprintf of PR_fprintf. So, I think it would be good to never have SECU_Strerror return NULL to make it easier on the vast majority of those callers.

Comment 60

[Julien Pierre]

Nelson,

FYI, now that Alexei has fixed bug 420991 again, neither of the assertions in attachment 371936 [details] [diff] [review] were hit when I ran all.sh on a new tree containing just with just this patch, on a debug build, and the QA passed. Therefore, you need not be concerned about tools about displaying "unknown error" instead of an error number as you stated in comment 58.

Comment 61

[Nelson Bolyard (seldom reads bugmail)]

Regarding the assertions, see
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/crlutil/crlutil.c&rev=1.32&mark=293-298#289
for correct code that would fail the proposed assertions.

returning "Unknown error" is OK.  Assertion failures are not.

Comment 62

[Julien Pierre]

Nelson,

1) Does SECU_Strerror ever returning a non-NULL pointer that points to a 0-length string ? I'm not aware that it does, and if so, the first clause in the if statement is never reached in code fragment you pointed out in comment 61 .

2) The purpose of these new assertions is to detect bugs in NSS. In this particular instance, the NSS API called immediately before SECU_Strerror is PK11_ImportCRL . Would you not say it's valuable to assert that PK11_ImportCRL always sets a non-zero NSPR error code, and that if it does, we always have a string for that error code ?

Comment 63

[Nelson Bolyard (seldom reads bugmail)]

I'm marking this resolved/fixed.

Comment 50 and following comments document a NEW and DIFFERENT problem 
that began when PKITS testing was started.  That problem belongs in a 
new bug.

Comment 64

[Julien Pierre]

Nelson,

Did you file a bug for that other problem ?

Comment 65

[Alexei Volkov]

No, we didn't. Still need to have another revision of a patch for vfyserv and new patch for certutil.

Comment 66

[Alexei Volkov]

Attachment: vfyserv.c patch v2 - modify vfyserv tool to use libpkix api

Main patch changes:
   * remove multi-threading from vfyserv.c
   * do not use unnecessary global variables
   * fix tool usage printout

Comment 67

[Julien Pierre]

Comment on attachment 390851 [details] [diff] [review]
 vfyserv.c patch v2 - modify vfyserv tool to use libpkix api 

1) This patch does not compile.
It contains a change to the prototype of
SECU_printCertProblemsOnDate in secutil.h, but no corresponding change in secutil.c .

2) I think the following does not belong in secutil.h if you want to eliminate globals :
extern RevMethods revMethodsData[];

3) in setupSSLSocket, why are you setting SSL_NO_CACHE ?

4) Without threads, vfyserv will now take longer when doing multiple connections. The amount of code deleted is not very large, especially compared with the rest of the patch. Was there any specific problem with the threading code ?

5) in vfyserv.c, main, th e indentation is wrong after
switch (usePkix)
specifically the default case , and the final switch brace. There should not be two closing braces at the same level.

6) the patch for certutil is missing

Comment 68

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last months and this bug has priority 'P1'.
:beurdouche, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 69

[John Schanck [:jschanck]]

Closing in favor of Bug 1648172