Correct check_chain() on X509_V_FLAG_X509_STRICT (-x509_strict option) by DDvO · Pull Request #13832 · openssl/openssl

DDvO · 2021-01-11T07:15:50Z

This has been extracted from #13658 as requested by #13658 (comment).
Once that PR is merged the three new tests marked there as TODO can be re-enabled.

x509_vfy.c: strict check_chain() should check also single root CA certs
x509_vfy.c: rejecting unknown extensions in certs used solely as trust anchors is now done only in strict mode because such certs have been explicitly exempted from RFC requirements.
x509_vfy.c: ~~check_chain() must allow V1 self-signed cert also in strict mode~~
the expectations on whether a cert is (likely) a CA or not are no more checked for directly trusted EE certs because such certs have been explicitly exempted from RFC requirements.

Checklist

documentation is added or updated
tests are added or updated

crypto/x509/x509_vfy.c

t8m

LGTM, although maybe adding something to Changes.md would be a good idea.

DDvO · 2021-01-11T13:47:11Z

LGTM, although maybe adding something to Changes.md would be a good idea.

Done, and also extended openssl-verification-options.pod (and one reference to it).

DDvO · 2021-01-13T11:33:01Z

@t8m, your comments have been addressed;
can you approve this as well?

crypto/x509/x509_vfy.c

kaduk

Some "drive by" comments; I didn't get to really review in depth.

crypto/x509/x509_vfy.c

vdukhovni

Overall, the code reorg and some of the changes look like progress, but I think we still need to sort out a few details.

crypto/x509/x509_vfy.c

vdukhovni · 2021-01-14T05:57:44Z

crypto/x509/x509_vfy.c

This check was previously only applied to non-EE certificates, i.e. those actually used in chain-integrity signature verification. If the application (not SSL, where this is not allowed) wants to allow use of explicit EC parameters, this is not the place to override that policy.

The only quibble is perhaps whether the check should be applied if we're checking "SS" signatures (even of EE certs)

Actually, the check was previously done for all certs in the chain if the chain had more than one element.
Note that this PR stops checking directly trusted EE certs already before reaching this point and on the other hand removes the num > 1 guard that preceded these checks because I believe this guard has become pretty redundant.
If you want to have that it only applies to non-EE certificates, (which has not really the case before this PR), this check needs to be guarded differently.

DDvO · 2021-01-14T21:22:02Z

Only tangentially related, I wonder if we want to add named symbols for these various values instead of hardcoding magic numbers.

I fully agree - yet another form of unreadable code 😉
If you like, I can fix that as 2nd commit as part of this PR.

How about adding #defines to x509_local.h, or where else?
Which name prefix shall be used for them?

vdukhovni · 2021-01-15T00:26:32Z

Only tangentially related, I wonder if we want to add named symbols for these various values instead of hardcoding magic numbers.

I fully agree - yet another form of unreadable code 😉
If you like, I can fix that as 2nd commit as part of this PR.

How about adding #defines to x509_local.h, or where else?
Which name prefix shall be used for them?

I don't have strong view on which header, the prefix should indicate that that these are CA validity code points, so something like (your call):

X509_CA_STATUS_NON_CA
X509_CA_STATUS_VALID
...

But if you think of something better, go with that. Since these are internal code points, in some suitable non-public header.

DDvO · 2021-01-15T08:03:11Z

It turns out that those wonderful(ly) magic numbers returned by {X509_,}check_ca need to be defined publicly because they are returned by the public function X509_check_ca and have already been documented along with it.
So added the mnemonic names to x509v3.h.in where also X509_check_ca is declared.

DDvO · 2021-01-15T08:39:52Z

I've just tentatively added a further check to check_chain():

        /*
         * Check the validity of the cert representation beforehand
         * because otherwise the return code of X509_check_ca(x)
         * and various other checks actually have no (useful) value
         */
        CHECK_CB((x->ex_flags & EXFLAG_INVALID) != 0,
                 ctx, x, i, X509_V_ERR_INVALID_CERTIFICATE_FORMAT);

but maybe this is redundant because it is guaranteed not to happen when reaching it?

DDvO · 2021-01-23T15:08:19Z

Rebased to solve merge conflicts.
Is this okay now?

vdukhovni · 2021-01-24T00:58:08Z

CHANGES.md

I don't recall seeing a rationale for allowing v1 certs in strict mode. Did I miss somewhere amidst all the comments? Also, I don't actually see a matching change in the code. Is this still a thing???

Oh, I just found that I had neglected not only this PR in general
but also your (@vdukhovni 's) latest comments there for long - sorry for that!

Well, to me the rationale is simply that v1 certs are not forbidden by RFC 5280, which is aged but still in charge.
The only thing I found there in this direction is in https://datatracker.ietf.org/doc/html/rfc5280#section-6.1.4:

Conforming implementations may choose to reject all version 1 and version 2 intermediate certificates.)

As far as I understand, our strict mode enforces strict requirements on X.509 certs (mostly from RFC 5280),
part of which OpenSSL so far has not really checked (for whatever reasons).
In other words, strict mode should not be more strict than RFC 5280.

crypto/x509/x509_txt.c

vdukhovni · 2021-01-24T01:50:59Z

crypto/x509/x509_vfy.c

It seems to me that the original code here (added recently in 1.1.1, after I last took a deep dive into the X.509 verification code) may not have been doing what was intended. The test num > 1 [addition by DvO for clarity: which has been removed here] tests the chain to see whether it has any CAs, and if so restricts every certificate in the chain (including the leaf) to not use explicit curve parameters.

Is this really the correct criterion? If we're also testing the leaf cert when num > 1, why not test it when num == 1? Alternatively, if the idea is to make sure none of the CA certs have been "tweaked", shouldn't the test be i > 0 rather than num > 1 and test everything but the leaf cert?

Finally, OpenSSL does not IIRC share the Windows Curveball bug, so it is not entirely clear why this has been added to OpenSSL.

The reason I mention all this, is that the new code is subtly different. It no longer applies the num > 1 constraint, but it is only enforced after self-issued EE certificates are exempted from further checks. This means that with partial_chain and a non-self-issued, but trusted EE certificate will now also be restricted when it is the only one in the chain.

So there are many subtle edge conditions here, and I don't have a clear sense that anyone knows what the expected semantics are. So I'd like to see that resolved.

Good point that the intention which certs should actually be checked here under which circumstances is somewhat unclear.
AFAIR, I thought that the guard num > 1 makes not much sense and thus simply removed it.
Yet to be on the conservative side, I just re-added it and added a TODO comment about it such that this PR is relieved from solving this pre-existing issue.

crypto/x509/x509_vfy.c

doc/man3/X509_STORE_CTX_get_error.pod

vdukhovni · 2021-01-24T01:56:20Z

doc/man3/X509_STORE_CTX_get_error.pod

Suggested change

Non-CA certificate required but has CA markings.

A CA certificate was encountered in a context where only a non-CA certificates are expected.

Changed to: CA certificate was encountered where only a non-CA certificate is expected.

openssl-machine · 2021-02-24T00:07:10Z

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

openssl-machine · 2021-03-27T00:07:28Z

This PR is in a state where it requires action by @openssl/otc but the last update was 61 days ago

openssl-machine · 2021-04-27T00:07:35Z

This PR is in a state where it requires action by @openssl/otc but the last update was 92 days ago

Extend checking single root CA certs also to strict mode. Unknown critical extensions in certs used only as trust anchors are now rejected only in strict mode because they are explicitly exempted from RFC requirements. The expectations on whether a cert is (likely) a CA or not are no more checked for directly trusted EE certs because such certs have been explicitly exempted from RFC requirements.

openssl-machine · 2022-09-27T00:08:46Z

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

openssl-machine · 2022-10-28T00:08:43Z

This PR is in a state where it requires action by @openssl/otc but the last update was 61 days ago

openssl-machine · 2022-11-28T00:08:47Z

This PR is in a state where it requires action by @openssl/otc but the last update was 92 days ago

openssl-machine · 2022-12-29T00:09:00Z

This PR is in a state where it requires action by @openssl/otc but the last update was 123 days ago

openssl-machine · 2023-01-29T00:09:27Z

This PR is in a state where it requires action by @openssl/otc but the last update was 154 days ago

openssl-machine · 2023-03-01T00:09:48Z

This PR is in a state where it requires action by @openssl/otc but the last update was 185 days ago

openssl-machine · 2023-04-01T00:09:38Z

This PR is in a state where it requires action by @openssl/otc but the last update was 216 days ago

openssl-machine · 2023-05-02T00:09:37Z

This PR is in a state where it requires action by @openssl/otc but the last update was 247 days ago

openssl-machine · 2023-06-02T00:09:33Z

This PR is in a state where it requires action by @openssl/otc but the last update was 278 days ago

openssl-machine · 2023-07-03T00:09:26Z

This PR is in a state where it requires action by @openssl/otc but the last update was 309 days ago

openssl-machine · 2023-08-03T00:09:33Z

This PR is in a state where it requires action by @openssl/otc but the last update was 340 days ago

openssl-machine · 2023-09-03T00:09:23Z

This PR is in a state where it requires action by @openssl/otc but the last update was 371 days ago

openssl-machine · 2023-10-04T00:09:43Z

This PR is in a state where it requires action by @openssl/otc but the last update was 402 days ago

openssl-machine · 2023-11-04T00:09:50Z

This PR is in a state where it requires action by @openssl/otc but the last update was 433 days ago

openssl-machine · 2023-12-05T00:10:06Z

This PR is in a state where it requires action by @openssl/otc but the last update was 464 days ago

openssl-machine · 2024-01-05T00:09:30Z

This PR is in a state where it requires action by @openssl/otc but the last update was 495 days ago

t8m · 2024-01-05T11:26:21Z

We should revive this as it seems to make sense. Unfortunately it needs a rebase.

richsalz · 2024-01-05T13:07:38Z

We should revive this as it seems to make sense. Unfortunately it needs a rebase.

After languishing for two years without any action, that is not surprising.

t8m · 2024-07-03T13:49:28Z

Could you please rebase the PR to resolve conflicts?

DDvO · 2026-01-30T12:40:39Z

@vdukhovni, @t8m, unfortunately, I won't have the time to continue working on this.
Can someone take over, or shall I close this PR, or how else to handle this?

DDvO · 2026-02-18T07:02:39Z

Closing for the time being.

DDvO changed the title ~~WIP: Correct check_chain() on X509_V_FLAG_X509_STRICT (-x509_strict)~~ WIP: Correct check_chain() on X509_V_FLAG_X509_STRICT (-x509_strict option) Jan 11, 2021

DDvO mentioned this pull request Jan 11, 2021

apps/{req,x509,ca}.c Make sure certs have SKID and AKID by default #13658

Closed

2 tasks

t8m reviewed Jan 11, 2021

View reviewed changes