156802 - CERT_DecodeDERCrl returns NULL on decoding a v2 CRL with no revoked certificates in it.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

This bug is reported by Robert List in the mozilla.crypto
newsgroup.

CERT_DecodeDERCrl returns a NULL pointer on decoding a v2
CRL with no revoked certificates in it.  Since it should
be legal for a CRL to have no revoked certificates in it,
CERT_DecodeDERCrl should not return NULL.

Comment 1

[Wan-Teh Chang]

Attachment: Zip file containing the CA certificates and the CRLs

Comment 2

[Terry Hayes]

One data point:  I built a test program to call CERT_DecodeDERCrl on the empty 
CRL data.  Using libraries built from the NSS tip around April 23, 2002 this 
test worked.  I haven't tried any other NSS versions.

Comment 3

[Julien Pierre]

Terry,

I was able to successfully decode the TrustSign-Enc-01.crl and
TrustTest-Enc-01.crl using crlutil.

However, nQua-01.crl poses a problem. The decode fails in cert_check_crl_version
in mozilla/security/nss/lib/certdb/crl.c :

    if (crl->entries == NULL) {
	if (hasCriticalExten == PR_FALSE && version == SEC_CRL_VERSION_2) {
	    PORT_SetError (SEC_ERROR_BAD_DER);
	    return (SECFailure);
	}
        return (SECSuccess);
    }

version is set to 1, which apparently is equal to SEC_CRL_VERSION 2, and there
are no critical extensions, and no reasons. For some reason, we consider this an
error. Do you know why we have that check in place ?

Comment 4

[Terry Hayes]

This check should be removed.  The version number must be v2 (1) when critical 
extensions are present so that older software can detect that information that 
they must process is present but cannot be decoded.

This check should be replaced with one that requires that the version is v2 or 
less.  In other words we should not accept a v3 (or later) CRL.

Comment 5

[Wan-Teh Chang]

Julien wrote:
> The decode fails in cert_check_crl_version
> in mozilla/security/nss/lib/certdb/crl.c :
>
>     if (crl->entries == NULL) {
>         if (hasCriticalExten == PR_FALSE && version == SEC_CRL_VERSION_2) {
>             PORT_SetError (SEC_ERROR_BAD_DER);
>             return (SECFailure);
>         }
>         return (SECSuccess);
>     }

Looking at the cert_check_crl_version() code, I think there
are two possible explanations for the (inner) check.

1. The check is a mistake and the original intent is

      if (hasCriticalExten && version != SEC_CRL_VERSION_2) {

which is used in two other places.  If this is the case, then
the intended check has already been done earlier in the
function and this check is redundant.

2. The check requires that if a CRL has no critical extensions
its version must not be v2.  If this is the case, there are two
problems with this check.
- The check is done only when crl->entries is NULL.  It should
  always be done.
- There is no such requirement in the Internet cert and CRL
  profile (RFC 2459 or RFC 3280).

So I agree with Terry that this check should be removed.

Terry wrote:
> This check should be replaced with one that requires that
> the version is v2 or less.

That check already exists (at the beginning of the function).

Comment 6

[Julien Pierre]

Attachment: remove invalid check

Comment 7

[Wan-Teh Chang]

Comment on attachment 91867 [details] [diff] [review]
remove invalid check

r=wtc.	Thanks for fixing this, Julien.

Comment 8

[Julien Pierre]

I verified that this fixed the problem with the CRL that still failed yesterday.

(strange)/export/home/jpierre/nss/36/mozilla/dist/SunOS5.8_DBG.OBJ/bin{179}
crlutil -d . -B -I -i ./nQual-01.crl 
(strange)/export/home/jpierre/nss/36/mozilla/dist/SunOS5.8_DBG.OBJ/bin{180}
crlutil -d . -L


CRL names                                CRL Type


CN=A-Trust-Test-nQual-01, OU=A-Trust-Test-nQual-01, O=A-Trust, C=AT CRL  
(strange)/export/home/jpierre/nss/36/mozilla/dist/SunOS5.8_DBG.OBJ/bin{181} 

Checking in to the tip :

Checking in crl.c;
/cvsroot/mozilla/security/nss/lib/certdb/crl.c,v  <--  crl.c
new revision: 1.11; previous revision: 1.10
done

Comment 9

[Robert Relyea]

 re comment #5 . Wan Teh I think the reason for the check was the latter. We
used to have a check on all CRL's to make sure the meet this criteria (somewhere
the spec suggests that any CRL that doesn't have critical extensions should not
be v2, however a number of people want to use the v2 extensions with are not
marked critical). We removed that check about 2 years ago. It looks like the
NULL crl check must have been paranoia.

Anyway I believe it should be safe to remove the check. If we need to check
version number > 2, then we should do that where we check for unrecognized
critical extensions, not here.

bob

Comment 10

[Wan-Teh Chang]

Terry:

I verified that we are not doing an equivalent cert
version check.

Comment 11

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 1.1

Comment 12

[Wan-Teh Chang]

I checked in the fix into the NSS_3_5_BRANCH and NSS_CLIENT_TAG.