162983 - Revoked certificate verification succeeds if there is an error during CRL lookup

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

The function SEC_CheckCrl, which is used as part of certificate verification,
uses  SEC_FindCrlByName, whicih returns a pointer to a decoded CRL object, a
CERTSignedCRL*.

If a CRL is found in a token, but its DER fails to decode, then a NULL pointer
is returned to SEC_CheckCRL, which never knows that there was a bad DER CRL, and
therefore returns success.

The correct behavior is to do it all in SEC_CheckCrl . It should call
PK11_FindCRLByName, which will return the DER CRL.

It can then do the decoding by calling CERT_DecodeDERCrl, and fail the
verification if appropriate.

Doing the decoding in SEC_CheckCrl also provides the opportunity for a
performance optimization : it can do only a partial decoding of the CRL before
the signature verification, instead of a full decode. The entries don't need to
be decoded until after the signature verification has succeeded, and this can be
done with CERT_CompleteCRLDecodeEntries.

An actual example of how this can happen is if there is some disk corruption in
the cert database, which affects one of the ASN.1 tags in the CRL, that causes
the decoding to fail, and therefore the CRL validation to be bypassed.

If CRLs are stored on other tokens, the type of attack to corrupt it may be
different - and our current code would happily discard the corrupt CRL and
validate the cert.

Comment 1

[Julien Pierre]

FYI, much of this would become irrelevant with the CRL cache, as SEC_CheckCrl
will end up going to the cache. This is an existing issue in our previous
implementations. I need to make sure that the cache handles this correctly and
it has an "invalid" state in which all the certs are rejected.

Comment 2

[Julien Pierre]

There are other conditions that can cause this erroneous successful verification
to occur. The CRL lookup can also  fail because of an insufficient memory condition.

Comment 3

[Julien Pierre]

Resolved by CRL cache changes.

Comment 4

[Julien Pierre]

Actually, I wasn't checking for invalid cache status. I need to automatically
consider all certs revoked in case I have a bad CRL in the cache . Patch to follow.

Comment 5

[Julien Pierre]

Attachment: patch to consider certs revoked when the cache has found a bad CRL for the cert issuer

as checked in to the tip

Comment 6

[Julien Pierre]

FYI, a test case for this can be created. A bogus CRL can be inserted into the
database via crlutil -B (bypass checks, which skips the signature check).
It should be a valid DER CRL, but some data should be modified to make the
signature fail.

The CRL cache will then load the CRL from the token during the cert
verification. It will verify its signature, and that will fail. The cert
verification will therefore be failed for all certs from that issuer.

This prevents an attack against the CRL revocation mechanism. The only way to
defeat the revocation at that point is to completely delete the CRL from the token.

FYI, I ran into this in another case :

The softoken provided an invalid DER CRL to the cache. It was a SECItem with a 0
length and non-NULL pointer. This was caused by an out-of-memory condition. See
bug 164501. The attached patch also catches that case.