Last Comment Bug 356309 - CertVerifyLog in CERT_VerifyCertificate terminates early on expired certs
: CertVerifyLog in CERT_VerifyCertificate terminates early on expired certs
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.11.3
: All All
: P2 normal (vote)
: 3.11.4
Assigned To: Kai Engert (:kaie)
:
Mentors:
Depends on:
Blocks: bad-cert-ui
  Show dependency treegraph
 
Reported: 2006-10-11 11:46 PDT by Kai Engert (:kaie)
Modified: 2006-11-08 12:59 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 (1.72 KB, patch)
2006-10-11 11:51 PDT, Kai Engert (:kaie)
no flags Details | Diff | Review
Patch v2 (2.41 KB, patch)
2006-10-12 10:41 PDT, Kai Engert (:kaie)
no flags Details | Diff | Review
Patch v3 (2.75 KB, patch)
2006-10-12 12:17 PDT, Kai Engert (:kaie)
rrelyea: review+
nelson: review+
Details | Diff | Review

Description Kai Engert (:kaie) 2006-10-11 11:46:44 PDT
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) 2006-10-11 11:51:33 PDT
Created attachment 241972 [details] [diff] [review]
Patch v1
Comment 2 Nelson Bolyard (seldom reads bugmail) 2006-10-11 12:00:34 PDT
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) 2006-10-12 10:41:57 PDT
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.
Comment 4 Kai Engert (:kaie) 2006-10-12 10:45:14 PDT
Comment on attachment 242081 [details] [diff] [review]
Patch v2

sigh, this new patch does not work for me...
Comment 5 Kai Engert (:kaie) 2006-10-12 11:15:13 PDT
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) 2006-10-12 12:17:34 PDT
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.
Comment 7 Kai Engert (:kaie) 2006-10-12 13:21:22 PDT
(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 2006-10-12 14:21:45 PDT
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 2006-10-12 14:39:07 PDT
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) 2006-10-12 14:49:02 PDT
(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 2006-11-02 17:44:18 PST
Comment on attachment 242088 [details] [diff] [review]
Patch v3

r=relyea
Comment 12 Kai Engert (:kaie) 2006-11-08 10:42:17 PST
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) 2006-11-08 10:43:01 PST
Let me know if you think we should land this correctness fix on 3.11 branch.
Comment 14 Nelson Bolyard (seldom reads bugmail) 2006-11-08 11:24:49 PST
Comment on attachment 242088 [details] [diff] [review]
Patch v3

r=nelson
Thanks for addressing all my previous comments.
Comment 15 Kai Engert (:kaie) 2006-11-08 11:49:19 PST
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) 2006-11-08 12:29:28 PST
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) 2006-11-08 12:59:36 PST
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.

Note You need to log in before you can comment on or make changes to this bug.