556792 - CERT_GetCertChainFromCert() goes into a loop due to RDNs of the authorityCertIssuer

Status: NEW

(NSS :: Libraries, defect, P5)

Description

[Kathleen Wilson]

Creating a separate bug to track this patch from Kaspar Brand in
https://bugzilla.mozilla.org/show_bug.cgi?id=555156#c5

Patch for preventing a CERT_GetCertChainFromCert loop
https://bugzilla.mozilla.org/attachment.cgi?id=436650

(In reply to comment #4)
> The Certificate Hierarchy shows a long list of “ANF Server CA” root
> certificates.

It's another case where CERT_GetCertChainFromCert() goes into a loop (which is
terminated after 20 iterations thanks to the fix from bug 477186).

Looking at the cert's properties, this is due to a partly "broken" way of how
the authorityKeyIdentifier is encoded, which has the following value:

        Name: Certificate Authority Key Identifier
        Key ID:
            be:3b:f6:b4:31:b7:73:24:48:39:c5:57:13:94:75:aa:
            9f:81:3f:2c
        Issuer:
            Directory Name: "CN=ANF Server CA,serialNumber=G-63287510,OU=ANF
                Clase 1 CA,O=ANF Autoridad de Certificacion,L=Barcelona (see
                current address at https://www.anf.es/address/ ),ST=Barcelona
                ,C=ES"
        Serial Number: 78923 (0x1344b)

Some RDNs of the authorityCertIssuer ("Directory Name") field have a different
ASN.1 encoding than the one in the issuer DN field, unfortunately: CN, OU, O
and ST are encoded as PrintableString, while they appear as UTF8String in the
issuer DN.

One might argue that this an encoding error in the certificate and do nothing
about it, but I think a small change to modification certdb.c:cert_IsRootCert()
would make sense: if we have a match for the keyIdentifier, then consider this
a sufficient check for determining whether it's a root certificate or not (and
do not insist on additional checks for
authorityCertIssuer/authorityCertSerialNumber - note that the primary purpose
of the AKID extension is to provide "a means of identifying the public key
corresponding to the private key used to sign a certificate", not to indicate
if it's a root cert or not).

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Please attach to this bug the relevant certificate(s) necessary to reproduce 
the issue, or a URL from which they can be obtained, and with which the 
problem can be reproduced.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Kaspar's patch

Comment 3

[Kaspar]

Attachment: ANF Server CA cert (serial 0x1344b), in DER format

(In reply to comment #1)
> Please attach to this bug the relevant certificate(s) necessary to reproduce 
> the issue, or a URL from which they can be obtained, and with which the 
> problem can be reproduced.

Here it is (originally submitted in bug 555156 comment #0).

(In reply to comment bug 555156 comment #8)
I think that this patch would be an overall improvement to cert_IsRootCert(), irrespective of whether we consider this specific certificate as "broken" (in the context of RFC 5280 and the matching rules defined in section 7.1, the two DNs will actually match).

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

This is not a review comment on the patch, per se', but a comment on the 
nature of the proposed change. I disagree in principle with the proposed 
change.  

First, Root certificates are not supposed to have an AKID extension at all,
IIRC, according to RFCs 3280 and 5280.  They are the ultimate authority,
and hence need no reference to any higher authority.

But second, if a supposed root cert DOES cite some cert as a higher authority, 
it must cite itself and not some other cert. If it cites some other cert then 
IT IS NOT A ROOT, by definition.  

For intermediate CA certs, I'm willing to ignore the "issuer's issuer name
and serial number" component of an AKID extension, and in fact I changed NSS 
to do just that for bug 384459 back in 2008.
See https://bugzilla.mozilla.org/attachment.cgi?id=327914&action=diff
An intermediate CA's cert validation path can change with time. 
But a root is different.  If a CA that issues a root can't do it right, 
they don't belong in Mozilla's root CA list.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Wait, the "IsRootCert" test must be looking at the AKID in the root cert,
not in the server cert, unless the server cert is a root.  

Was your server cert issued directly by a root?  If so, please attach that
root cert.

Comment 6

[Kaspar]

(In reply to comment #5)
> Wait, the "IsRootCert" test must be looking at the AKID in the root cert,
> not in the server cert, unless the server cert is a root.  
> 
> Was your server cert issued directly by a root?  If so, please attach that
> root cert.

Attachment 436845 [details] *is* a root cert.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Correctly encoded ANF Root cert

CERT_GetCertChainFromCert() has no problem with this one.  Try it.

Comment 8

[Kaspar]

It's true that my previous statement "the two DNs will actually match" (comment 3) was wrong, as I found out myself in the meantime. In addition to different encodings, ANF also managed to vary the spelling of some RDNs, that's true :-/ (you fixed them as well in attachment 436856 [details]).

However, I think you're missing my main point: I suggest that cert_IsRootCert is relaxed to not insist on an exact binary match when comparing two DNs. The authorityKeyIdentifier extension is meant as a hint when constructing the path, not as an extension to indicate whether a certificate is self-signed or not (maybe the whole AKID checks should be dropped from IsRootCert).

The drawback with the modified version, btw, is that the signature no longer verifies successfully ;-)

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

We never check the signatures on trusted roots, so the fact that the 
signature on my modified root is invalid is immaterial.

It *IS* important to check AKIDs, when they're present, because if you 
do not, you may stop constructing the chain, thinking that you have 
arrived at a root, when you have actually arrived at a "key roll over" 
cert that is subordinate to the root.  If you stopped at such a cert, 
your chain would fail the trusted root check.  

I know this because libPKIX has this very problem right now.  :-/

Comment 10

[Kaspar]

(In reply to comment #9)
> We never check the signatures on trusted roots, so the fact that the 
> signature on my modified root is invalid is immaterial.

For NSS, yes - but I would certainly not propose to add this modified version to the nssckbi... (Windows CAPI e.g. clearly complains about the signature, shipping an "NSS-tuned" version of that root would seem like a bad idea to me).

> It *IS* important to check AKIDs, when they're present, because if you 
> do not, you may stop constructing the chain, thinking that you have 
> arrived at a root

Ok, if IsRootCert is also used for this purpose, then this can be an argument for keeping the check. Given all the woes with DN name matching (and c14n), I still think that short-circuiting after the key id check would be a sensible thing to do. (The key id would certainly *not* match for rollover certs, so path construction wouldn't stop.)

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

The cert is question was incorrectly encoded.  Our present code alerted us 
to this fact, I think that's good.  "If it ain't broken, don't fix it."

Comment 12

[Kaspar]

Attachment: Sample cert which triggers a loop in CERT_GetCertChainFromCert

(In reply to comment #11)
> "If it ain't broken, don't fix it."

I beg to differ. In the end, I think we both agree that CERT_GetCertChainFromCert should not go into a loop, irrespective of whether a cert has a defect or not.

I'm attaching attaching another cert for demonstration purposes, which I would not consider "incorrectly encoded" - RFC 5280 allows the use of different string encodings for referring to the same DN (in section 7.1, it requires support for the LDAP StringPrep profile [RFC4518] for PrintableString and UTF8String when performing name chaining).

Comment 13

[Kaspar]

Attachment: Patch 2: use CERT_CompareName instead of SECITEM_ItemsAreEqual

To improve the situation as far as name chaining is concerned, patch 2 could help: use CERT_CompareName instead of SECITEM_ItemsAreEqual when comparing the issuer DN and the authorityCertIssuer from the AKID extension.

Comment 14

[Kaspar]

Attachment: Patch 3: make find_cert_issuer not return itself (proof of concept)

Patch 3 - another idea: find_cert_issuer (which is used by nssCertificate_BuildChain) should not return a certificate as its own issuer. This is mostly a proof of concept implementation, maybe a new (STAN?) function for comparing two certs should be added (instead of explicitly memcmp'ing the items).

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

> In the end, I think we both agree that CERT_GetCertChainFromCert should 
> not go into a loop, irrespective of whether a cert has a defect or not.

Loops are inevitable.  Find_issuer *should* find itself when it has arrived 
at a root, at a self-referential certificate.  That's the definition of a 
root.  Perhaps the failure lies in some other code, which fails to see that
find issuer has returned the same cert twice in a row.

Regarding a switch to CERT_CompareName, this is not something we'd want to 
do for just one area in NSS.  We want all of NSS to be consistant.  Either
all of NSS should use the equivalent of bcmp (ItemsAreEqual) or all of NSS
should use the equivalent of CompareName.  Today, NSS is quite consistent 
about using the equivalent of ItemsAreEqual.  We're not going to change 
just this one function by itself without changing all the rest.

I think the NSS team would be (and has been, for a long time) amenable to switch to a scheme like CompareName, but there's a problem.  It's the one
big obstacle.  It's PKCS#11.  PKCS#11's definition of searches for objects 
by any and all attribute values is on the basis of bcmp.  IMO, it simply makes no sense to define the rest of NSS to do full canonicalized name comparison while PKCS#11 continues to do bmcp name comparison.

I have lobbied in the past for a canonicalization scheme, and defining 
searches for objects by DER Names to be searches by canonicalized forms, and the other participants in the PKCS#11 vendor community were not interested.  They are all quite happy with the presente definition.  I even lobbied for
the addition of new attributes, canonicalized subject names and canonicalized issuer names.  Begin canonicalized, searches for them could be done with bcmp.
Again, there has been no interest.

Comment 16

[Kaspar]

(In reply to comment #15)
> Find_issuer *should* find itself when it has arrived 
> at a root, at a self-referential certificate.  That's the definition of a 
> root.  Perhaps the failure lies in some other code, which fails to see that
> find issuer has returned the same cert twice in a row.

Nelson, patch 3 isn't about CERT_FindCertIssuer, it's about a private function which is only used by nssCertificate_BuildChain, in the following loop:

    for (rvCount = 1; (!rvLimit || rvCount < rvLimit); ++rvCount) {
        CERTCertificate *cCert = STAN_GetCERTCertificate(c);
        if (cCert->isRoot) {
            /* not including the issuer of the self-signed cert, which is,
             * of course, itself
             */
            break;
        }
        c = find_cert_issuer(c, timeOpt, &issuerUsage, policiesOpt, td, cc);
        if (!c) {
            ret = PR_FAILURE;
            break;
        }
        st = nssPKIObjectCollection_AddObject(collection, (nssPKIObject *)c);
        nssCertificate_Destroy(c); /* collection has it */
        if (st != PR_SUCCESS)
            goto loser;
    }

For the cert in attachment 438328 [details], cCert->isRoot is false (for known reasons), which is why nssCertificate_BuildChain (mistakenly) goes on with calling find_cert_issuer. ret should not be set to PR_FAILURE, though, when find_cert_issuer returns NULL, that's true - something which the current version of my patch doesn't take care of yet.

> I think the NSS team would be (and has been, for a long time) amenable to
> switch to a scheme like CompareName, but there's a problem.  It's the one
> big obstacle.  It's PKCS#11.  PKCS#11's definition of searches for objects 
> by any and all attribute values is on the basis of bcmp.  IMO, it simply makes
> no sense to define the rest of NSS to do full canonicalized name comparison
> while PKCS#11 continues to do bmcp name comparison.

I agree, but on the other hand, if the PKCS#11 community is not interested in enhancing the specs in this direction, should that really prevent NSS from moving towards a more RFC-5280 compliant name matching? Lack of a standard for name c14n in PKCS#11 shouldn't imply that NSS should stall (forever?) when it comes to improving its own code.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Kaspar, 
I hope it doesn't come as a surprise to you to learn that I'm extremely 
familiar with this code.  

The NSS team is no longer the large full time paid staff that it once was.
AFAIK, there is one full time NSS developer and a few occasional/part time 
paid developers. I am now purely a volunteer contributor. 

IMO, the only way that NSS will transition from a strict binary comparison
basis for names to a fancier (e.g. canonicalized) basis, is if it can be 
done systematically, not piecemeal.  Perhaps we can slowly amass a large 
set of patches that, when all applied, will convert all of NSS.  I invite 
you to participate in discussions with the rest of the NSS team in the 
newsgroup to see if that is an end that the team as a whole wants to persue.

Comment 18

[Kaspar]

(In reply to comment #17)
> I hope it doesn't come as a surprise to you to learn that I'm extremely 
> familiar with this code.

Yes, sure. But then I don't fully understand your previous statement "Find_issuer *should* find itself when it has arrived at a root, at a self-referential certificate." When looking at the code quoted in comment 16, then I see no particular reason why find_cert_issuer should indeed find itself (it's called at a time when nssCertificate_BuildChain thinks that it's dealing with a non-self-issued cert).

To me, modifying find_cert_issuer seemed like the most straightforward solution to the problem you described in comment 15: "the failure [...] in some other code, which fails to see that find issuer has returned the same cert twice in a row" (this other code is nssCertificate_BuildChain). Or let me ask differently: are there strong arguments against touching find_cert_issuer and filter_certs_for_valid_issuers (even though they are only used in a single place)?

Comment 19

[Kaspar]

Attachment: Patch 3, updated version: make find_cert_issuer not return itself

(In reply to comment #16)
> ret should not be set to PR_FAILURE, though, when
> find_cert_issuer returns NULL, that's true - something which the current
> version of my patch doesn't take care of yet.

Here is an updated version, which corrects this mistake.

Comment 20

[Kaspar]

Comment on attachment 438329 [details] [diff] [review]
Patch 2: use CERT_CompareName instead of SECITEM_ItemsAreEqual

(In reply to comment #17)
> IMO, the only way that NSS will transition from a strict binary comparison
> basis for names to a fancier (e.g. canonicalized) basis, is if it can be 
> done systematically, not piecemeal.

Agreed. Canceling review request and marking obsolete.

Comment 21

[Kaspar]

Comment on attachment 436844 [details] [diff] [review]
Kaspar's patch

Nelson, I would appreciate if you would reconsiderate this issue, in order to come to a conclusion/decision (I'm not that keen on getting the patch[es] committed, but would at least prefer a clear statement, even if it's an explicit r-).

To recap my opinion:

1) the first patch (attachment 436844 [details] [diff] [review]) helps to prevent loops if we have self-issued certs where the DNs in the subject and the AKID extension use different encodings for their RDNs, but the AKID extension also includes the keyid portion at the same time. Relying on the comparison of SKID and the keyid portion of the AKID seems sufficient to me for determining if we're dealing with a root.

2) the second patch is more generic, but IMO still makes sense, because it helps to avoid other cases where CERT_GetCertChainFromCert() could possibly loop - namely if cert_IsRootCert (for some other reason) failed to identify a cert as a root cert.

Thanks for your review (whatever the outcome will be).

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch 3 v3: variant of patch 3 v2

Kaspar, does this variant of your patch v3 satisfy you?

Comment 23

[Kaspar]

Comment on attachment 510113 [details] [diff] [review]
Patch 3 v3: variant of patch 3 v2

I'm fine with adding an additional size check, yes.

What I'm not yet fully satisfied about is the error handling - when find_cert_issuer() returns nothing, we shouldn't unconditionally set ret to PR_FAILURE in nssCertificate_BuildChain, I think.

Currently, the comment for nssCertificate_BuildChain reads:

/* This function returns the built chain, as far as it gets,
** even if/when it fails to find an issuer, and returns PR_FAILURE
*/

Assuming that we have found a chain with at least one intermediate, and then reach the "(!c)" condition, should we perhaps take the current rvCount value into consideration before setting ret to PR_FAILURE?

I realize that my patch 3 (attachment 446204 [details] [diff] [review]) also suffers from this problem - simply breaking out of the loop isn't the proper solution either.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

The remaining issue for this bug concerns the value output via the statusopt
output argument.  As I understand comment 23, Kaspar thinks the function 
ought not to output PR_FAILURE when it returns a non-NULL/non-empty chain.

As seen at 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pki/certificate.c&rev=1.67&mark=549,562#547
private function nssCertificate_BuildChain is the implementation of 
public function NSSCertificate_BuildChain.  The behavior of 
NSSCertificate_BuildChain is specified at 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pki/nsspki.h&rev=1.13&mark=230-233#222
That specification makes it clear that an incomplete cert chain should 
return PR_FAILURE with a specific error code, even when/if the chain is
not empty.  

Does that settle the mater?

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

er, matter. :)

Comment 26

[Kaspar]

Attachment: Patch 4: immediately detect a loop in CERT_GetCertChainFromCert

(In reply to comment #24)
> That specification makes it clear that an incomplete cert chain should 
> return PR_FAILURE with a specific error code, even when/if the chain is
> not empty.  
> 
> Does that settle the matter?

Not completely, unfortunately. It does provide an answer for the case where the chain ends with a non-root cert, but it doesn't properly deal with the case the "patch 3" is trying to address: "avoid other cases where CERT_GetCertChainFromCert() could possibly loop - namely if cert_IsRootCert (for some other reason) failed to identify a cert as a root cert" (comment 21).

After another look at the story, my new suggestion would be:

1) the first patch (attachment 436844 [details] [diff] [review]) should go in anyway - it makes cert_IsRootCert more robust when trying to answer the question "Is this a root cert?"

2) instead of trying to fix/modify find_cert_issuer (with possible side effects on the behavior of NSSCertificate_BuildChain, and also CERT_CertChainFromCert, in the end), it seems more straightforward to add a check to CERT_GetCertChainFromCert, as proposed in the attached patch.

Note that the numbering of the patches for this bug might be somewhat confusing (mea culpa). Attachment 446204 [details] [diff] is actually v2 of "Patch 3", because it's independent of the first patch ("Kaspar's patch").

I'm therefore calling this new one "Patch 4" and will change the name of attachment 510113 [details] [diff] [review] next.

Comment 27

[Kaspar]

Comment on attachment 510113 [details] [diff] [review]
Patch 3 v3: variant of patch 3 v2

Renaming (obsolete) patch.

Nelson, could you have a look at the "new" patch 4 instead? Thanks.

Comment 28

[Isabel Fabregas]

Hi Kaspar
Let me introduce myself, I'm isabel fabregas and I recently I have joined the company ANFAC, one of my first tasks is to respond to your request in order to solve the problem of of “ANF Server CA” root certificates in Mozzilla.


Just now, I've changed the account from [e-mail] to mine, so from now surely receive comments from my account

REgards

Isabel

Comment 29

[Florian Bender]

Is this still a valid bug after the mozpkix rewrite?

Comment 30

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Florian, this is a bug in the NSS product, not the Firefox or Gecko products. NSS doens't use mozilla::pkix, so it doesn't have any bearing on the validity of this bug. Also, mozilla::pkix doesn't use any of NSS's certificate stuff any more, so mozilla::pkix should be affected by this.

Maybe you are wondering how this bug affects Firefox and other Gecko-based software. Gecko uses a mix of NSS stuff and mozilla::pkix. Thus, I imagine that Gecko is sometimes affected by this bug still. David Keeler is the best person to ask regarding how Gecko/Firefox work in this respect.

Comment 31

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

> so mozilla::pkix should be affected by this.

so mozilla::pkix should **NOT** be affected by this.

Comment 32

[Ryan Sleevi]

Firefox is affected by this, because this code is reachable by the client cert submission logic. Any user sending client certs may have this issue exploited, presumably by having the attacker prime the in-memory NSS cert cache first by sending a server cert (that mozilla::pkix) can handle, but then poisons the CERT_GetCertChainFromCert logic