171224 - changes to path construction and validation in NSS

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Ian McGreer]

I'm setting the version to 3.0 since we're fixing some very old bugs here. 
After meeting to discuss issues surrounding the problem of path construction, we
arrived at the following consensus (as written by Nelson):

1. We agreed on the definition of a condition for determining whether a 
given cert is the end of a cert chain, or whether the search for another 
issuer cert should continue.  The condition is called "isRoot".  Here is 
an English definition of isRoot.

A cert is "self issued" iff the cert's subject DN is identical to 
its issuer DN, and neither is empty.

isRoot: A cert is a root (end of a chain) iff it is self issued, and either
a) there is no Authority Key Identifier (AKI) extension, or 
b) each and every present one of the 3 optional components of the AKI 
   extension exactly matches the corresponding item in the same cert, i.e.  
   If the AKI keyIdentifer is present, the cert must also have a Subject
	Key Identifier extension, and it must match the AKI keyIdentifier
	exactly,  AND
   If the AKI authorityCertIssuer field is present, it must match the cert's
	issuer Name(*), AND
   If the AKI authorityCertSerialNumber field is present, it must match the
	cert's serial number.  

   This is called a "strict match" on the AKI.  

(*): it's not clear to me whether subject alternative names and/or issuer
alternative names need to also be included in this comparison, or not.

This isRoot condition can be evaluated when a cert is imported (e.g. into
a new temp cert) and stored as part of the cert's structure to avoid
repeated re-evaluation of this condition.

Each place in NSS where we now simply compare issuer and subject names to 
decide if we're at the end of a loop, the code should be changed to test 
the "isRoot" test.  

Testing to see if the cert returned by CERT_FindCertIssuer is the same as 
the subject cert passed to that function may also be useful, but is 
insufficient to identify the end of a chain in all cases.  Some self-signed 
root CA certs may appear to chain to others with the same names. So the
isRoot condition should be checked before attempting to find another issuer.

Validation of a cert chain should proceed until:
- a trusted cert is found, or 
- a cert is found with the isRoot condition, or 
- no issuer is found, or 
- some path length limit is reached.

Construction of a cert chain for sending should proceed until:
- a cert is found with the isRoot condition, or 
- no issuer is found, or 
- some path length limit is reached,
and then the last cert in the chain should be dropped only if it has the
isRoot condition.


2. When searching for the issuer of a given cert, we concluded that
the authority key identifier extension should be handled, when present, 
as follows:

If the issuer and serial number components of the AKI are present, they 
MUST match the issuer DN and serial number fields of the issuer cert.  
Any certs noth matching them cannot be considered a candidate to be the
issuer cert.  (**)

If the keyIdentifier component of the AKI is present, then candidate issuer
certs must be filtered to include only (a) certs with Subject Key Identifiers
that strictly exactly match the AKI's key identifier ("Yes certs"), and 
(b) certs with no Subject Key Identifier extension ("Maybe certs").  
Cert with Subject Key Identifiers present that do not match the AKI
key identifier must not be considered as possible issuers ("No certs").
If any Yes certs are found, then all Maybe certs must be eliminated 
(filtered out).  The result will be a list of all Yes certs, or all Maybe
certs.  This is known as a "relaxed match" on the AKI key identifier.  

The best cert from among the certs in the list must be selected by some
criteria.


Notes for further discussion:

(**) Presently, the optional AKI issuer and serial number fields are treated 
only as a pair.  If both are present, then we may look for a cert that
matches both.  But if only one is present in the AKI (e.g. serial number
is present, but not issuer name, or vice versa) then we ignore it.  
I think we should honor each component separately.

Comment 1

[Ian McGreer]

This bug unifies bugs 157218 and 169383.  Both will be addressed here.

Comment 2

[Ian McGreer]

Attachment: the GUP (grand unified patch) for cert chaining

Here it is.  I believe this should follow the steps outlined by Nelson to the
letter.

Other notes:

1)  As we discussed, I added an isRoot boolean to CERTCertificateStr.  I also
cached the authKeyID value, so that we only look for it once.  To do this, I
removed two apparently unneccessary fields from CERTCertificateStr.  Please
verify that this is OK.  I know of nobody that uses those fields, and they
never appeared in NSS code other than in the struct definition.

2) nssCertificate_BuildChain had some very strange code that was series of
piled-on hacks to replicate NSS 3.3 behavior.  NSS 3.3 was very forgiving in
that it would form a chain link even if the issuer cert did not have the
requested usage.  Inexplicably, nssCertificate_BuildChain worked around this by
repeating the search with a new usage parameter.  I removed this.

I replaced the behavior with the code seen in
nssCertificateArray_FindBestCertificate.  This function will now return a cert
even if the usage does not match, but will, naturally, still prefer one for
which the usage does match.  This change obviates the need for the repeated
search.

The patch is long, but I think as many people should review it as possible,
since it is very critical.  We especially need to ensure that it meets the
requirements laid out in this bug exactly.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Ian, Thanks for filing this bug based on my email.  I see that my email 
omitted one other detail that we discussed in the meeting, and I include
it here for completeness.  It is a detail necessary to avoid one of the 
problems we foresaw for test scenario number 4. The code may already
implement this test.  Perhaps I should write up the 5 scenarios (if I can
remember them) and add that to this bug?

When looking for an issuer, in addition to the requirements set forth 
above, a candidate issuer cert must satsify at least one of the following
criteria to show that it is a valid issuer:
- The cert has a basic constraints extension that allows it to be a CA cert, or
- The cert has the isRoot condition, or 
- The cert has the "valid CA" trust flag.

Comment 4

[Ian McGreer]

The function CERT_IsCACert would seem to do this, but not the way we would like.
 Its structure is:

if (trust is present) {
  check that cert is valid or trusted CA
} else {
  if (nsCertType is present) {
    check that nsCertType is CA
  } else if (basicConstraints is present) {
    check basicConstraints.isCA
  }
}

That checks most of what we want, and we can add an isRoot check, but the
problem is the if-then-else structure.  What Nelson describes in comment #3 is
that if any one of the conditions is met, the cert is a CA cert.  With this
function, if the trust is present, but not marked as valid or trusted CA, the
basicConstraints would never be checked.

Is there any reason not to change the function CERT_IsCACert to do what Nelson
describes?

Comment 5

[Ian McGreer]

After reading more closely, I need to revise that comment.

The first test is actually "if (trust is present and not zero)".

Any cert that has a trust value will pass this test, and not have
basicConstraints checked.  Do we want trust to explicitly disallow some certs
from being issuers (for example, one trusted as "p,p,p")?  That may help the
case 5 discussed in the meeting (distuinguishing a peer from a CA without the
help of extensions).

If we do assign that meaning to trust, CERT_IsCACert will do exactly what we
need, with the addition of a check for isRoot.

Comment 6

[Ian McGreer]

I'll answer my own questions with a proposal.

* root certs are always valid issuers, no matter what
trust/nsCertType/basicConstraints are

* non-root certs can be made invalid issuers by setting peer trust

* any non-root cert can be made an issuer by setting valid CA or trust CA trust

* non-root certs with no trust defer to nsCertType and then basic constraints to
determine if they can be issuers

This proposal makes it convenient to use CERT_IsCACert for determining if a cert
is a valid issuer (certainly seems logical), with the following addition:

if (cert->isRoot) {
  return true;
} else if (trust is present and not zero) {
  ... continue as described above
}

Comment 7

[Ian McGreer]

Attachment: patch rev 2

Based on Nelson's review, fixed a bug in FindBestCertificate.

Also implements proposal of comment #6.  That issue is still open.

Comment 8

[Ian McGreer]

Attachment: diff of patches

This is a diff of the two patches, should make comparing them easier.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Ian, your new patch does fix the problem in FindBestCertificate and 
appears to implement something equivalent to the proposal of comment 6.

Comment 10

[Ian McGreer]

Patch was checked in prior to beta3, no complaints, marking fixed.