modify certutil, vfychain and vfyserv utilities to use CERT_PKIXVerifyCert function

REOPENED
Assigned to

Status

NSS
Libraries
P1
enhancement
REOPENED
10 years ago
8 years ago

People

(Reporter: Alexei Volkov, Assigned: Alexei Volkov)

Tracking

(Blocks: 2 bugs)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX)

Attachments

(12 attachments, 5 obsolete attachments)

20.29 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Julien Pierre
: superreview-
Details | Diff | Splinter Review
3.86 KB, patch
Julien Pierre
: review+
Details | Diff | Splinter Review
727 bytes, patch
Alexei Volkov
: review+
Details | Diff | Splinter Review
12.44 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
3.32 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
2.48 KB, patch
Alexei Volkov
: review+
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
2.47 KB, patch
Details | Diff | Splinter Review
4.36 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
14.82 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
3.15 KB, patch
Details | Diff | Splinter Review
914 bytes, patch
Nelson Bolyard (seldom reads bugmail)
: review-
Details | Diff | Splinter Review
53.97 KB, patch
Julien Pierre
: review-
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
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.
(Assignee)

Updated

10 years ago
Severity: normal → enhancement
Whiteboard: PKIX
(Assignee)

Comment 1

10 years ago
Created attachment 297206 [details] [diff] [review]
Initial patch

Make use of CERT_PKIXVerifyCert for cert chain validation(option -p). Explicit policy can be set with -o <arg> option.
Attachment #297206 - Flags: review?(nelson)
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.
Attachment #297206 - Flags: review?(nelson) → review-
(Assignee)

Comment 3

10 years ago
Created attachment 297855 [details] [diff] [review]
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.
Attachment #297206 - Attachment is obsolete: true
Attachment #297855 - Flags: review?(nelson)
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.
Attachment #297855 - Flags: review?(nelson) → review-
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.
(Assignee)

Comment 6

9 years ago
> 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.
(Assignee)

Comment 7

9 years ago
Created attachment 302662 [details] [diff] [review]
Patch v3

Fixes related to review comments.
Attachment #297855 - Attachment is obsolete: true
Attachment #302662 - Flags: review?(nelson)
Comment on attachment 302662 [details] [diff] [review]
Patch v3

r=nelson
Attachment #302662 - Flags: review?(nelson) → review+
(Assignee)

Comment 9

9 years ago
/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

9 years ago
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 .

Updated

9 years ago
Blocks: 414556

Updated

9 years ago
Blocks: 414563

Comment 11

9 years ago
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* ?
Attachment #302662 - Flags: superreview-

Comment 12

9 years ago
I checked in the fix to reverse the arguments. See bug 417392 . I still wonder why you used a void*.
(Assignee)

Comment 13

9 years ago
Created attachment 303301 [details] [diff] [review]
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.
Attachment #303301 - Flags: review?(julien.pierre.boogz)

Updated

9 years ago
Attachment #303301 - Flags: review?(julien.pierre.boogz) → review+
(Assignee)

Comment 14

9 years ago
patch 303301 is committed.

Comment 15

9 years ago
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
Created attachment 304290 [details] [diff] [review]
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.
Attachment #304290 - Flags: review?(alexei.volkov.bugs)
(Assignee)

Updated

9 years ago
Attachment #304290 - Flags: review?(alexei.volkov.bugs) → review+

Comment 17

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

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

9 years ago
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.
Summary: modify vfychain and vfyserv utilities to use CERT_PKIXVerifyCert function → modify certutil, vfychain and vfyserv utilities to use CERT_PKIXVerifyCert function
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
Attachment #304290 - Attachment description: patch - fix crash on windows → patch - fix crash on windows (checked in)
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

9 years ago
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.
(Assignee)

Comment 23

9 years ago
Created attachment 307282 [details] [diff] [review]
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.
Attachment #307282 - Flags: review?(nelson)
(Assignee)

Updated

9 years ago
Priority: -- → P1
(Assignee)

Comment 24

9 years ago
Created attachment 307625 [details] [diff] [review]
Add -t argument to vfychain, v1

Modify vfychain to use cert_pi_certList option to supply trust anchors.
Attachment #307625 - Flags: review?(nelson)
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;
Attachment #307282 - Flags: review?(nelson) → review+
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.
Attachment #307625 - Attachment description: Patch v5 → Add -t argument to vfychain, v1
(Assignee)

Comment 27

9 years ago
(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 on attachment 307625 [details] [diff] [review]
Add -t argument to vfychain, v1

r+, provided that you change cert_pi_certList to cert_pi_trustAnchors
Attachment #307625 - Flags: review?(nelson) → review+

Comment 29

9 years ago
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.

Updated

9 years ago
Blocks: 294531

Comment 30

9 years ago
Created attachment 309492 [details] [diff] [review]
Patch v5: Adjust vfychain to new revocation flags
Attachment #309492 - Flags: review?(alexei.volkov.bugs)

Comment 31

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

Sorry, this patch is buggy, another one coming up.
Attachment #309492 - Attachment is obsolete: true
Attachment #309492 - Flags: review?(alexei.volkov.bugs)

Comment 32

9 years ago
Created attachment 309499 [details] [diff] [review]
Patch v6: Adjust vfychain to new revocation flags
Attachment #309499 - Flags: review?(alexei.volkov.bugs)
Comment on attachment 309499 [details] [diff] [review]
Patch v6: Adjust vfychain to new revocation flags

r=nelson
Attachment #309499 - Flags: review+
(Assignee)

Comment 34

9 years ago
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
Attachment #309499 - Flags: review?(alexei.volkov.bugs) → review+

Comment 35

9 years ago
Created attachment 309551 [details] [diff] [review]
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

9 years ago
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
Attachment #309551 - Attachment description: Patch v6 plus variables moved → Patch v6 plus variables moved [checked in]
(Assignee)

Comment 37

9 years ago
Created attachment 311690 [details] [diff] [review]
Print out validated chain

the patch provides some help to debug bug 422859.
Attachment #311690 - Flags: review?(nelson)
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?
Attachment #311690 - Flags: review?(nelson) → review+
(Assignee)

Comment 39

9 years ago
(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.
Status: NEW → ASSIGNED
(Assignee)

Comment 40

9 years ago
attachment 311690 [details] [diff] [review] is integrated.
(Assignee)

Updated

9 years ago
Target Milestone: 3.12 → 3.12.1
Assignee: alexei.volkov.bugs → julien.pierre.boogz
Status: ASSIGNED → NEW
(Assignee)

Comment 41

8 years ago
Created attachment 363359 [details] [diff] [review]
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.
Attachment #363359 - Flags: review?(nelson)
Assignee: julien.pierre.boogz → alexei.volkov.bugs
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.
Attachment #363359 - Flags: review?(nelson) → review-
(Assignee)

Comment 43

8 years ago
Created attachment 367045 [details] [diff] [review]
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.
Attachment #367045 - Flags: review?(nelson)
Created attachment 367314 [details] [diff] [review]
the previous patch - ignoring whitespace
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
Attachment #367045 - Flags: review?(nelson) → review+
Target Milestone: 3.12.1 → 3.12.3
Version: 3.12 → trunk
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?
Attachment #367045 - Attachment description: patch v1 - add option to validate the same cert multiple times. → patch v1 - add option to validate the same cert multiple times. (checked in 2009-Apr-01)
Sorry, that last question was meant for Alexei.  :(
(Assignee)

Comment 48

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

Comment 49

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

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

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

Error was zero.

Comment 53

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

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

Comment 55

8 years ago
Created attachment 371975 [details] [diff] [review]
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.
Attachment #371975 - Flags: review?(nelson)

Comment 56

8 years ago
Created attachment 371976 [details] [diff] [review]
Change SECU_Strerror.

Builds better with this include.
Attachment #371975 - Attachment is obsolete: true
Attachment #371976 - Flags: review?(nelson)
Attachment #371975 - Flags: review?(nelson)
Comment on attachment 371976 [details] [diff] [review]
Change SECU_Strerror.

I suppose this is a patch
Attachment #371976 - Attachment is patch: true
Attachment #371976 - Attachment mime type: application/octet-stream → text/plain
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";
Attachment #371976 - Flags: review?(nelson) → review-

Comment 59

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

8 years ago
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.
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

8 years ago
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 ?
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.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 64

8 years ago
Nelson,

Did you file a bug for that other problem ?
(Assignee)

Comment 65

8 years ago
No, we didn't. Still need to have another revision of a patch for vfyserv and new patch for certutil.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 66

8 years ago
Created attachment 390851 [details] [diff] [review]
 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
Attachment #363359 - Attachment is obsolete: true
Attachment #390851 - Flags: review?
Attachment #390851 - Flags: review? → review?(julien.pierre.boogz)

Comment 67

8 years ago
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
Attachment #390851 - Flags: review?(julien.pierre.boogz) → review-
You need to log in before you can comment on or make changes to this bug.