157218 - NSS 3.4+ misidentifies certs as self-signed

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

NSS 3.4 has reimplemented CERT_FindCertIssuer().  The new version 
incorrectly identifies some certs as self-signed that are not.
The problem is actually in function nssCertificate_BuildChain.

A self signed cert should have issuer name identical to subject name
AND either
a) no authority key identifier extension, or
b) an authority key identifier and a subject key identifier, both with 
the same identifier value.  

I believe that a cert with matching issuer and subject names, but whose 
authotity key identifier does not match its subject key identifier should
be treated as not self signed.  

In function nssCertificate_BuildChain, the loop that finds the issuer
begins with this while condition:
    while (!nssItem_Equal(&c->subject, &c->issuer, &status)) {
I believe that test is wrong, because it ignores the possibility that the
cert "c" has an authority key identifier that does not match its subject
key identifer.  

I have found a case of a certificate chain that fails to verify because
this code identifies a cert in the chain as self-signed, and that cert's
signature is incorrect when verified using the public key in that same cert.

Comment 1

[Wan-Teh Chang]

Assigned the bug to Bob.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

After studying RFC 3280 some more, especially pages 25-27, I have 
concluded that the current behavior of NSS is not in error, that is,
it does not violate the directions of the RFC.  

RFC 3280 says that Authority Key Identifier extensions MUST NOT be marked
critical.  Conforming CAs are required to put them into all certs EXCEPT
self-signed certs (which must have matching subject and issuer names).  
Applications SHOULD understand these extensions, but are not required to do so.  

The prohibition on marking them critical, together with the absent requirement 
for understanding them, clearly implies that certs should be constructed so 
that applications that do not recognize these extensions will still work 
properly.  Of course, any application that does not recognize these extensions 
will treat a cert with matching subject and issuer names as self signed, 
regardless of the extension values, just as NSS is doing now.  Therefore,
a cert that is not self signed should not have matching subject and issuer
names.

I conclude therefore that any cert with subject name matching its own
issuer name must also be self signed and NSS is not incorrect in assuming 
this.  If a self signed cert has an Authority Key ID extension, it must match 
the cert's own subject key ID extension.  The certs that have been the cause 
of this bug violate that rule.

So, the only reason to "fix" this bug and restore NSS's previous behavior
of looking at Authority Key IDs when determining whether or not a cert is
self signed is to be backwards compatible with certs that do not meet
the above criteria, but that used to work with NSS 3.3.  

I don't think this bug deserves to be "P1 major" any more.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

After further study of RFC 3280, I conclude that this bug is invalid.

On page 65, in the section on cert path validation, it says plainly 
that "A certificate is self-issued if the DNs that appear in the 
subject and issuer fields are identical and are not empty." 
There is no additional qualifier about key identifiers.  

Comment 4

[Ian McGreer]

reopening bug

Comment 5

[Ian McGreer]

taking bug

Comment 6

[Ian McGreer]

Attachment: suggested patch for nssCertificate_BuildChain

This patch should cause nssCertificate_BuildChain to rely on identifiers for
finding issuer certs.  The loop is truncated when the issuer of a cert is the
same cert (relying on the uniqueness of cert pointers), or when the issuer is
NULL.

Comment 7

[Ian McGreer]

Attachment: reorganize loop in cert_VerifyCertChain

Using the same logic of patch 1, detect a self-issued cert by comparing cert
pointers instead of subjectName/issuerName.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

I've found yet another place in the code that tests for a self-issued 
root cert by simply comparing subject DN to issuer DN, ignoring the 
presence of any authority key ID extension.  
It's in CERT_CertChainFromCert.  It stops us from sending an intermediate 
CA cert to the server if that intermediate CA cert's subject name matches
its own issuer DN.

Comment 9

[Ian McGreer]

Nelson-

I'm not sure the problem you mention in comment #8 exists.  I believe the code
you are looking at is not compiled, it is within the NSS_CLASSIC macro.

However, that function has another bug, as you noted in bug 169383.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Yeah, I think you're right.  I must've been reading the old code when I wrote 
comment #8.  Sorry.

Comment 11

[Ian McGreer]

Attachment: revised and combined patch

This patch combines the two previous patches.  It uses the semantic suggested
by Nelson via email to compare cert pointers and then actual values.  It also
fixes a bug in the last patch.	In the case where two self-issued certs have
the same DN, and neither has an authKeyID extension, a chain built with the
first patch would always select the "best" cert between the two.  If one of the
self-issued certs was expired, the other would appear above it in the chain,
since the other would be chosen as the "best" match for that DN.  This patch
includes a fix that detects a self-issued cert with no authKeyID extension, and
ends the chain there.

Comment 12

[Ian McGreer]

Even I don't understand my last comment.  Here's a sample diagram:

CA1 = [ issuer=subject=XXX, SKI=1, expired ]
CA2 = [ issuer=subject=XXX, SKI=2, valid ]

cert1 = [ issuer=XXX, subject=YYY, AKI=1 ]

With the first two patches applied, asking for the chain of cert1 would produce:

cert1 <-- CA1 <-- CA2

This is because asking for the issuer of CA1 would return CA2.  When we are at
the point of asking for the issuer of CA1, a search is done for subject=XXX,
which returns both CA1 and CA2.  There are no AKI extensions, so we fall back on
FindBestCert().  CA2 is valid and CA1 isn't, so it is "best".

With the third patch, when we reach CA1, we detect that it is self-issued, and
that there is no AKI, so we terminate the chain.

Comment 13

[Ian McGreer]

Comment on attachment 100272 [details] [diff] [review]
revised and combined patch


This patch is incorrect. See bug 171224.

Comment 14

[Ian McGreer]

marking fixed.