Only CA certificates can be self-issued (all branches, master first).

[vdukhovni]

At the bottom of https://tools.ietf.org/html/rfc5280#page-12 and
top of https://tools.ietf.org/html/rfc5280#page-13 (last paragraph
of above https://tools.ietf.org/html/rfc5280#section-3.3), we see:

This specification covers two classes of certificates: CA
certificates and end entity certificates. CA certificates may be
further divided into three classes: cross-certificates, self-issued
certificates, and self-signed certificates. Cross-certificates are
CA certificates in which the issuer and subject are different
entities. Cross-certificates describe a trust relationship between
the two CAs. Self-issued certificates are CA certificates in which
the issuer and subject are the same entity. Self-issued certificates
are generated to support changes in policy or operations. Self-
signed certificates are self-issued certificates where the digital
signature may be verified by the public key bound into the
certificate. Self-signed certificates are used to convey a public
key for use to begin certification paths. End entity certificates
are issued to subjects that are not authorized to issue certificates.

that the term "self-issued" is only applicable to CAs, not end-entity
certificates. In https://tools.ietf.org/html/rfc5280#section-4.2.1.9
the description of path length constraints says:

The pathLenConstraint field is meaningful only if the cA boolean is
asserted and the key usage extension, if present, asserts the
keyCertSign bit (Section 4.2.1.3). In this case, it gives the
maximum number of non-self-issued intermediate certificates that may
follow this certificate in a valid certification path. (Note: The
last certificate in the certification path is not an intermediate
certificate, and is not included in this limit. Usually, the last
certificate is an end entity certificate, but it can be a CA
certificate.)

This makes it clear that exclusion of self-issued certificates from
the path length count applies only to some intermediate CA
certificates. A leaf certificate whether it has identical issuer
and subject or whether it is a CA or not is never part of the
intermediate certificate count. The handling of all leaf certificates
must be the same, in the case of our code to post-increment the
path count by 1, so that we ultimately reach a non-self-issued
intermediate it will be the first one (not zeroth) in the chain
of intermediates.

[blaufish]

Wouldn't it be better to fix the off-by-one calculation of path length?
e.g.

if (is_ca && (x->ex_flags & EXFLAG_SI) == 0)
plen++;

with

is_ca = X509_check_ca(x);

and

- (plen > (x->ex_pathlen + proxy_path_length + 1))) {
+ (plen > (x->ex_pathlen + proxy_path_length))) {

Code easier to read with correct values used in calculations. And code able to abort if the leaf is violating the constraint, which would indicate something is wrong on the issuing CA.

[vdukhovni]

The code is not intended to fail to validate leaf-position CAs that have "CA:true" whose issuer has path length:0. While the former cannot be used as intermediate CAs to issue EE certs, they are valid leaf certificates, when used as such. There is no "off by one", the patched code produces correct results with "plen" (at the time of the test) being the count of CAs in the path including the present CA, so a violation entails of plen > ex_pathlen + 1 (ignoring the proxy stuff).

[vdukhovni]

If someone can point to a good reason to fail to validate chains leading to a leaf CA that would violate path length if it did occur as an intermediate, then perhaps we could reconsider the logic. But since that case would be caught by any correct validator implementation, it is not clear why we need to preclude such certificates being used as EE certificates. The path:0 in the issuer is sufficient to effectively make the CA:true in the leaf irrelevant, and allows some EE certs that forgot to clear the CA bit to continue to work as EE certs and continue to fail if ever misused as CA certs.

[blaufish]

What I meant by off-by-one is that the implementation has plen=1 for path length 0, plen=2 for path length 1 and so on, as per RFC5280 definition of path length (only authorities are applicable to path length, non-authorities are not part of path length).

(There is one more issue in the path length code a few lines higher up, the path length constrains are not validated for any certificate that is deemed self-issued. So for example path length constraints issued root authority are ignored completely. Maybe that should be a separate issue?)

[blaufish]

@vdukhovni Regarding Leaf certificate violating constraint not triggering an error; after a careful reading of RFC5820 I think the OpenSSL implementation is correct in that aspect.
Errors on path length constraint should be caught in "Prepare for Next Cert" part of the RFC algorithm, which SHOULD NOT executed for the leaf certificate. So the correct behaviour as defined by RFC is to not check leaf for path length constraint violations.

[vdukhovni]

Root CA certificates are self-signed not self-issued, and path length constraints cannot apply to them because they are never in the middle of the chain. There's no separate issue to open.

As for the "off by one", it makes no difference. Sure, instead of always incrementing at depth 0, we could instead never increment at depth 0, and adjust the test accordingly, but that does not really change anything.

[blaufish]

I think this resolves all issues I've found. The commit message on this change might be a bit misleading as it fixes other issues than just self-signed root. For example the commit fixes invalid path such as:

• Root
• IntermediateA
• IntermediateB
• IntermediateB(PathLengthConstraint:0) (self issued)
• EvilCA (violates constraint above)
• Leaf

[vdukhovni]

@mattcaswell @davidben anyone else care to review? This is ready I believe.

[mattcaswell]

Approved. As an aside I notice that the comment above the initialisation of must_be_ca in this function seems out-of-date. It says that a value of 0 is never used - but it clearly is later on in the function in the handling of proxy certs.

[vdukhovni]

Pushed to all branches. Thanks.