108021 - validation against CRL fails if next update is in past

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Rangan Sen]

By default, validation against crl should not fail even if 'next-update' is in
the past. We should have some kind of an option/flag , and only when it is set,
we should get validation error for 'next-update' being in the past.

cc-ing Bob Relyea on this bug, for we had a discussion on this issue...

Comment 1

[Robert Relyea]

This needs to go into NSS 3.4. Assigning to me and setting the Priority and targets.

Comment 2

[Robert Relyea]

Fixed in 3.4

Comment 3

[Rangan Sen]

Bob, how do I make use this feature in PSM? Do I have to enable some kind of a 
flag, or so? Shall greatly appreciate any advice on this...

Comment 4

[Robert Relyea]

Just use 3.4. Vallidate a cert which is certified by a CA which a loaded CRL. It
should no longer look at the 'next update' time on  CRL.

bob

Comment 5

[Rangan Sen]

With the present fix, we do not have that flag/option [mentioned in the opening 
description, ie, above comment# 1 of this bug] to indicate whether the 
'next-update' field should be taken into consideration. With this fix, 
validation against a crl does not at all look into the 'next-update' field. 

I believe we really should have this flag - it would be extremely useful. For 
example, a user has enabled autoupdate for a CRL and the auto-update fails for 
some reason[eg, for networking issues]. With this capability and the user having 
enabled strict validation [ie, always consider the 'next-update' date] he is 
still assured that he will never be using an old crl for validation.

We might want to re-open this...

Comment 6

[Rangan Sen]

CERT_VerifyCertNow() still returns SECFailure when next-update is in the past.

Actually, SEC_CheckCRL returns SECWouldBlock when next-update is in the past, 
but inside CERT_VerifyCertChain() we do something like:

rv = SEC_CheckCRL(handle, subjectCert, issuerCert, t, wincx);
if (rv == SECFailure) {
    LOG_ERROR_OR_EXIT(log,subjectCert,count,0);
} else if (rv == SECWouldBlock) {
    /* We found something fishy, so we intend to issue an
     * error to the user, but the user may wish to continue
     * processing, in which case we better make sure nothing
     * worse has happened... so keep cranking the loop */
    rvFinal = SECFailure;
    LOG_ERROR(log,subjectCert,count,0);
}


if ( issuerCert->trust ) {
    /*
     * check the trust parms of the issuer
     */
    if ( certUsage == certUsageVerifyCA ) {
	if ( subjectCert->nsCertType & NS_CERT_TYPE_EMAIL_CA ) {
	    trustType = trustEmail;
	} else if ( subjectCert->nsCertType & NS_CERT_TYPE_SSL_CA ) {
	    trustType = trustSSL;
	} else {
	    trustType = trustObjectSigning;
	}
    }
	    
    flags = SEC_GET_TRUST_FLAGS(issuerCert->trust, trustType);
    
    if ( (flags & CERTDB_VALID_CA) ||
	 (certUsage == certUsageStatusResponder)) {
	if ( ( flags & requiredFlags ) == requiredFlags ||
	     certUsage == certUsageStatusResponder ) {
	    /* we found a trusted one, so return */
	    rv = rvFinal; 
	    goto done; 


And finally, CERT_VerifyCertNow returns SECFailure ...

Comment 7

[Robert Relyea]

This is really disturbing. The fix was clearly not checked in, in any of the
files that should have affected this. I wonder if I did the commit from the
wrong tree at some point?

Comment 8

[Robert Relyea]

Attachment: Remove LastUpdate checks when verifying certs.

Checking this into the tip now...

Comment 9

[Wan-Teh Chang]

Comment on attachment 71704 [details] [diff] [review]
Remove LastUpdate checks when verifying certs.

r=wtc.

The local variable 'validity' should also be removed
because it is now an unused variable.

Bob, could you answer Rangan's question in comment #5?
He suggested that we provide the user with an option to
check the next-update field.

Comment 10

[Wan-Teh Chang]

Attachment: Remove unused variable 'validity'

Comment 11

[Robert Relyea]

The flag would require changes to the NSS 3 API in a number of functions
(VerifyCert, VerifyCertChain, VerifyCrl). It has always been 'out of plan'.

bob

Comment 12

[Stephane Saux]

No, I think that we had reached a compromise that didn't require us to modify
the API. The flag was "global".
On -> old behavior all cert verification fails is nextupdate is in the past.
off-> new behavior, nextupdate is not used.

I'm not sure it's super important to have it, especially since we don't have a
way to manipulate that global pref.

Comment 13

[Robert Relyea]

We *proposed* a global pref. I remember specifically not agreeing to the pref,
because it required to many changes to NSS. We settled on NSS not checking and
applications deciding if they want to deal with CRL update fields on their own.

bob

Comment 14

[Wan-Teh Chang]

The fix has been checked into the tip and the NSS_CLIENT_TAG
of NSS.  Marked the bug fixed.