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.
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.
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.
Resolved by CRL cache changes.
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.
Created attachment 98095 [details] [diff] [review] patch to consider certs revoked when the cache has found a bad CRL for the cert issuer as checked in to the tip
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.