217387 - CRL's AuthorityKeyID ignored during CRL verification

Status: ASSIGNED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

RFC 3280 states, under CRL extensions :

5.2.1  Authority Key Identifier

   The authority key identifier extension provides a means of
   identifying the public key corresponding to the private key used to
   sign a CRL.  The identification can be based on either the key
   identifier (the subject key identifier in the CRL signer's
   certificate) or on the issuer name and serial number.  This extension
   is especially useful where an issuer has more than one signing key,
   either due to multiple concurrent key pairs or due to changeover.

   Conforming CRL issuers MUST use the key identifier method, and MUST
   include this extension in all CRLs issued.

   The syntax for this CRL extension is defined in section 4.2.1.1.

Section 4.2.1.1 describes AuthorityKeyID for certificates. The extension is the
same in a CRL.

The consequence of not supporting AuthorityKeyID is that CRLs will not verify in
the following situations :
- the CA is using a different key to sign the CRL than for certificates
- the CA's key was compromised and replaced with a new one

When CRLs are present but do not verify, NSS automatically assumes that all
certificates for the CA are invalid, as a security precaution.

Therefore, downloading a CRL containing an alternate AuthorityKeyID would break
NSS applications at the present time.

To implement the AuthorityKeyID support, the CRL cache needs to be modified in
the following way :
1. allow the CRL cache to cache more than one issuer certificate per subject.
There would be a list/table of issuer certs, which would have the same subject,
but different SubjectKeyIDs
2. look for the presence of the AuthorityKeyID extension in the CRL after
decoding it
3. make sure the AuthorityKeyID matches the SubjectKeyID in one of the cached
issuer certificates
4. if it does not match, look for the correct issuer certificate by subjectkeyid .
5. add that issuer certificate to the CRL cache so we don't have to look it up again

In the past, the CRL cache has never had to actually look up the issuer
certificate. The issuer of the cert being verified was passed in from the
CERT_VerifyCert call. However, it is possible that different keys are used to
sign the certs than the CRL, so we can not rely on that cert to be the correct
cert in this case.

Comment 1

[Julien Pierre]

Adding dependency. Implementing this requires CERT_FindCertBySubjectKeyID to
work for CA certs.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

I am removing the dependency on bug 211051, which complains that 
CERT_FindCertBySubjectKeyID doesn't find certs with known SubjectKeyIDs.

That function searches for keys using ONLY the keyID, and not using the 
subjectName.  The present (broken) implementation searches only through
the user's "user certs" that were present when NSS was initialized.

In the case of CRLs, we want to find a CA cert that has the subject name
and SubjectKeyID that are found as the issuer name and AuthorityKeyID in
a CRL we possess.  There is a function to do exactly that, named
CERT_FindCertByKeyID.   Bug 233019 describes a crasher bug in that function.

So, I am making this bug depend on bug 233019 instead of bug 211051.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch part 1 v1 - use keyID at CRL import time

This patch does all the following things:

a) Declares and implements new function CERT_FindCRLAuthKeyIDExten,
   which finds and parses a CRL's AuthorityKeyID extension.  
   It shares nearly all code with the function that does the same for certs.

b) Declares and implements new funcion CERT_FindCRLIssuer, which finds 
   the CA cert for a given CRL in a given trustdomain at a given time.
   Perhaps this new code is in the wrong source file.  Please suggest
   another file if you have a preference.

c) Changes PK11_ImportCert to use CERT_FindCRLIssuer instead of 
   CERT_FindCertByName

d) changes PK11_ImportCert to return a more accurate set of error codes,
   rather that convering all errors to SEC_ERROR_CRL_BAD_SIGNATURE.

e) Fixes CERT_KeyUsageAndTypeForCertUsage, cert_VerifyCertChain, 
   CERT_VerifyCACertForUsage, and CERT_VerifyCert so that they all 
   accept all the enumerated values of type SECCertUsage as input arguments.
   Previously, some accepted certUsageAnyCA but not certificateUsageVerifyCA
   while others were just the opposite, making it difficult to find certs
   for CRL issuers wihtout assertion failures.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 140581 [details] [diff] [review]
patch part 1 v1 - use keyID at CRL import time

Please read the comments above in this bug prior to review.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

The above patch addresses the problems only for CRL importing.
There are similar issues when doing cert chain validation, and that 
patch does not address those issues. That's why I called it patch "part 1".

Comment 6

[Julien Pierre]

Nelson,

The bug was originally opened about the CRL verification failing during
certificate verification, because the CRL was verified against the wrong CA
certificate.

The failure to import a CRL due to the wrong CA certificate being looked up is a
separate problem.

In both cases, the failure is due to the CRL being verified against the
incorrect certificate.

However, there is presently no lookup for the CRL issuer during certificate
verification - the issuer of the cert being verified is assumed to also be the
CRL issuer.

In the case of the import of the CRL, there is a lookup, so your patch works.

So I am going to update the title for this bug once more.

There are however more problems with your patch :

- Your patch fixes the verification after looking up a KRL during certificate
verification, as well as when verifying a CRL or KRL before importing it.
It does not, however patch the CRL verification check that occurs during
certificate verification, in the CRL cache. To do that requires the fix
described at the end of the initial comment in this bug.

- There is also an issue in CRL_FindCRLIssuer. We should check the cert's key
usage extension, using CERT_CheckCertUsage to make sure that it is KU_CRL_SIGN
if present. This might break the KRL issuer lookup that you added though. I
don't see any KRL signing usage in RFC3280. In fact I don't see the name KRL.
You may want to have a separate function for finding KRL issuer that does not do
this check.

- With your patch, we now look up the KRL issuer (and when completed, also the
CRL issuer) separately from the certificate issuer. The two certs may be
different. If they match, then the issuer will be verified at the next level up
in the certificate chain verification. But if they don't match. Well, then it
won't be verified at all ! The CRL issuer cert could be a totally bogus one
that's been imported into the database with the correct subject name and key id,
along with a "recent" fake CRL that matches that cert. And then the real ones
would be obscured. So once we fix do a separate look up for the CRL/KRL issuer,
we need to make an extra call to CERT_VerifyCertificate to verify the CRL issuer
cert.

Comment 7

[Julien Pierre]

Adding 233118 as a dependency, since it must be fixed before this support is added.

Comment 8

[Robert Relyea]

Comment on attachment 140581 [details] [diff] [review]
patch part 1 v1 - use keyID at CRL import time

r=relyea.

Solicited suggestions for CERT_FindCRLIssuer(). It's clearly in the correct
directory (certhigh).
It might be slightly better in certhigh.c or crlv2.c where it's at isn't bad
either.

Comment 9

[Julien Pierre]

This is required for fixing many parts of the PKITS test suite. Do we want to
fix this in a patch to 3.9 or in 3.10 ? I'd like to set a target here.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

putting on my 3.10 radar

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

I'm giving this bug back to Julien for 3.12

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Unsetting target milestone in unresolved bugs whose targets have passed.

Comment 13

[Kai Engert [:KaiE:]]

Bugs that are currently assigned to Julien => assigning to nobody.
Search for 20100628-kaie-jp