356309 - CertVerifyLog in CERT_VerifyCertificate terminates early on expired certs

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Kai Engert (:KaiE:)]

While working on improved certificate warning dialogs in PSM, in order to combine all cert problems into a single warning dialog, I began to use CertVerifyLog.

During my testing I ran into a failure where a certificate that was both untrusted and expired did not report both errors in the log.

I found that CERT_VerifyCertificate terminates too early when the cert is expired.

I'll attach a patch that fixes the problem for me.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 241972 [details] [diff] [review]
Patch v1

Here are some unsolicited observations and suggestions. :)

1. instead of patching this "notdef" code, please just eliminate it.

> #ifdef notdef 
>     /* check if this cert is in the Evil list */
>     rv = CERT_CheckForEvilCert(cert);
>     if ( rv != SECSuccess ) {
> 	PORT_SetError(SEC_ERROR_REVOKED_CERTIFICATE);
> 	LOG_ERROR(log,cert,0,0);
>-	return SECFailure;
>+        if (!log)
>+            return SECFailure;
>     }
> #endif

2. There is a macro named LOG_ERROR_OR_EXIT used elsewhere in this 
same file for this purpose.  I don't know if it will work as-is
in this place, but if so, I'd prefer that you use it for 
consistency.

>     if ( validity != secCertTimeValid ) {
>         LOG_ERROR(log,cert,0,validity);
>-	return SECFailure;
>+        if (!log)
>+            return SECFailure;
>     }

Comment 3

[Kai Engert (:KaiE:)]

Attachment: Patch v2

(In reply to comment #2)
> 1. instead of patching this "notdef" code, please just eliminate it.

Fine with me. I had assumed that code was left in for a reason.
Removed in new patch.


> 2. There is a macro named LOG_ERROR_OR_EXIT used elsewhere in this 
> same file for this purpose.  I don't know if it will work as-is
> in this place, but if so, I'd prefer that you use it for 
> consistency.

This macro requires the use of goto.
The function I'm changing is currently not using any goto or jump labels.
I'm personally not in favor of goto.
But if you prefer consistency, fine with me.
I introduced a jump label in the new patch.

Comment 4

[Kai Engert (:KaiE:)]

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

sigh, this new patch does not work for me...

Comment 5

[Kai Engert (:KaiE:)]

There are additional problems when using CERT_VerifyCertificate with logging.

The big loop over all usages uses this condition:

  for (i=1;
       i<=certificateUsageHighest && 
       !(SECFailure == valid && !returnedUsages) ;) {

I find this difficult to read.
I translated it into:

  for (i=1;
       i<=certificateUsageHighest && 
       (SECSuccess == valid || returnedUsages) ;) {

This means, the loop will run over all usages if the caller requested to obtain the usages in variable "returnedUsages".
If the caller did not request that, the loop will terminate early.

The problem is:

In my scenario, the caller does not passes null for "returnedUsages".
But the caller passed in a VerifyLog.

As a result, the function will abort the loop as soon as something "invalid" is found.

I think the function should run through all usages if either "returnedUsages" or a VerifyLog has been passed in.

As a solution I'm proposing this new loop condition:

Comment 6

[Kai Engert (:KaiE:)]

Attachment: Patch v3

(In reply to comment #5)
> As a solution I'm proposing this new loop condition:

    for (i=1; i<=certificateUsageHighest && 
              (SECSuccess == valid || returnedUsages || log) ; ) {


This patch works for me.

Comment 7

[Kai Engert (:KaiE:)]

(In reply to comment #5)
> In my scenario, the caller does not passes null for "returnedUsages".

Correction:

In my scenario, the caller passes null for "returnedUsages".

Comment 8

[Robert Relyea]

Kai:
for (i=1;
       i<=certificateUsageHighest && 
       !(SECFailure == valid && !returnedUsages) ; ) {

and

for (i=1;
       i<=certificateUsageHighest && 
       (SECSuccess == valid || returnedUsages) ; ) {

are not eqivalent. The second for statement should be:

for (i=1;
       i<=certificateUsageHighest && 
       (SECSuccess != valid || returnedUsages) ; ) {

If you want to preserve the same functionality. (!(A&&!B) == !A || B).

bob

Comment 9

[Robert Relyea]

My mistake, kaie changed SECFailure to SECSuccess, which is has the same effect as changing the == to a != (assuming we don't run into the case where it becomes SECWouldBlock. In that case I think kaie's code is more likely correct).

Comment 10

[Kai Engert (:KaiE:)]

(In reply to comment #9)
> assuming we don't run into the case where it
> becomes SECWouldBlock

I don't see a possibility for value SECWouldBlock.

Variable "valid" only gets assignment from within this function and from its own INVALID_USAGE macro. It never gets assigned the result code of other function calls.

Comment 11

[Robert Relyea]

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

r=relyea

Comment 12

[Kai Engert (:KaiE:)]

checked in to trunk, marking fixed

/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.50; previous revision: 1.49

Comment 13

[Kai Engert (:KaiE:)]

Let me know if you think we should land this correctness fix on 3.11 branch.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

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

r=nelson
Thanks for addressing all my previous comments.

Comment 15

[Kai Engert (:KaiE:)]

fixed on 3.11 branch

/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.44.10.5; previous revision: 1.44.10.4

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Kai, NSS bugs use the "target milestone" to record the NSS release in which
the bug fix first appears.  Please keep it current.

Also, we stopped fixing non-FIPS bugs on the 3.11 branch months ago, IINM.
We should decide as a group whether we are going to resume fixing bugs 
on the 3.11 branch now, and if so, what we are going to do about the MANY
many bug fixes that have been made only on the trunk, even though they 
were equally valid for the branch.

Comment 17

[Kai Engert (:KaiE:)]

Nelson, I apologize, but I had asked whether to check in to 3.11 branch, and you had responded with an r+ after my question - so I draw the conclusion you want it in.