CertVerifyLog in CERT_VerifyCertificate terminates early on expired certs

RESOLVED FIXED in 3.11.4

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

3.11.3
3.11.4

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

2.75 KB, patch
Robert Relyea
: review+
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

11 years ago
Created attachment 241972 [details] [diff] [review]
Patch v1
Attachment #241972 - Flags: superreview?(julien.pierre.bugs)
Attachment #241972 - Flags: review?(rrelyea)
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;
>     }
(Assignee)

Comment 3

11 years ago
Created attachment 242081 [details] [diff] [review]
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.
Assignee: nobody → kengert
Attachment #241972 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #242081 - Flags: superreview?(julien.pierre.bugs)
Attachment #242081 - Flags: review?(rrelyea)
Attachment #241972 - Flags: superreview?(julien.pierre.bugs)
Attachment #241972 - Flags: review?(rrelyea)
(Assignee)

Comment 4

11 years ago
Comment on attachment 242081 [details] [diff] [review]
Patch v2

sigh, this new patch does not work for me...
Attachment #242081 - Attachment is obsolete: true
Attachment #242081 - Flags: superreview?(julien.pierre.bugs)
Attachment #242081 - Flags: review?(rrelyea)
(Assignee)

Comment 5

11 years ago
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:

(Assignee)

Comment 6

11 years ago
Created attachment 242088 [details] [diff] [review]
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.
Attachment #242088 - Flags: superreview?(julien.pierre.bugs)
Attachment #242088 - Flags: review?(rrelyea)
(Assignee)

Comment 7

11 years ago
(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

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

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

Comment 10

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

Updated

11 years ago
Blocks: 345007

Comment 11

11 years ago
Comment on attachment 242088 [details] [diff] [review]
Patch v3

r=relyea
Attachment #242088 - Flags: review?(rrelyea) → review+
(Assignee)

Updated

11 years ago
Attachment #242088 - Flags: superreview?(bugzilla)
(Assignee)

Comment 12

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

11 years ago
Let me know if you think we should land this correctness fix on 3.11 branch.
Priority: -- → P2
Target Milestone: --- → 3.12
Comment on attachment 242088 [details] [diff] [review]
Patch v3

r=nelson
Thanks for addressing all my previous comments.
Attachment #242088 - Flags: review+
(Assignee)

Comment 15

11 years ago
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
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.
Target Milestone: 3.12 → 3.11.4
(Assignee)

Comment 17

11 years ago
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.
You need to log in before you can comment on or make changes to this bug.