WiP: x590_vfy.c: (Re-)check that the root of the constructed cert chain is directly trusted #13770

DDvO · 2021-01-04T16:37:09Z

I think it would be good as a security precaution to
make ~~internal_verify()~~ verify_chain() (re-)check the trust placed in the last element of the chain being verified (i.e., its trust anchor),
such that the correctness of chain verification becomes independent of the checks done during chain building.

crypto/x509/x509_vfy.c

vdukhovni · 2021-01-13T16:36:37Z

crypto/x509/x509_vfy.c

-    /* Make sure that the root of the chain is trusted */
-    CB_FAIL_IF(!X509_check_trust(xi, ctx->param->trust, ctx->param->flags),
+    /* Make sure that the root of the chain is directly trusted */
+    CB_FAIL_IF(!directly_trusted(ctx, xi),


Sorry, on balance I think this PR should be abandoned. The internal_verify() function is not the right place to second-guess the trust-path construction. Its job is solely to check chain integrity (signatures and dates).

As meanwhile discussed at #13780 (comment) the internal_verify() function is not the right/best place to add the intended check, so I've disabled it there for now.
Yet where else to place it then?

As meanwhile discussed at #13780 (comment) the internal_verify() function is not the right/best place to add the intended check, so I've disabled it there for now.
Yet where else to place it then?

Nowhere that comes to mind. Hence the suggestion to drop this PR.

I meanwhile think that the check is best done just before calling internal_verify(), in verify_chain().

I don't see how that solves my fundamental objection that the context for making decisions is no longer available, and we just need to make sure that the build_chain code is correct.

DDvO · 2021-01-19T07:54:15Z

I've adapted and rebased this PR, moving out the rather unrelated renaming of CHECK_CB() to a separate PR: #13895.

vdukhovni · 2021-01-24T03:15:55Z

I'm placing on OTC hold on this PR. I don't think it should be merged, but if there's a consensus around a sound way forward, I'm willing to reconsider my objections. If you decide to just close this instead, that'd be fine too.

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:36Z

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

vdukhovni · 2021-04-27T18:18:43Z

crypto/x509/x509_vfy.c

@@ -131,7 +131,7 @@ static int lookup_cert_match(X509 **result, X509_STORE_CTX *ctx, X509 *x)
        xtmp = NULL;
    }
    ret = xtmp != NULL;
-    if (ret) {
+    if (ret && result != NULL) {


This change is not needed, or is not what you had in mind. The result output parameter (pointer) is never NULL, we'd have crashed at *result = NULL on line 119 if it were.

Oh, right! Fixed that.
Also added a comment on the use of result:

* If result is not NULL, on success *result will hold the up-ref'd found cert.

The idea is that the function can now also be used as follows:

static int directly_trusted(X509_STORE_CTX *ctx, X509 *cert) { return X509_check_trust(cert, ctx->param->trust, ctx->param->flags) != X509_TRUST_REJECTED && lookup_cert_match(NULL, ctx, cert) != 0; }

vdukhovni · 2021-04-27T18:22:40Z

crypto/x509/x509_vfy.c

@@ -224,6 +231,15 @@ static int verify_chain(X509_STORE_CTX *ctx)
                                  ctx->param->flags);
    CB_FAIL_IF(err != X509_V_OK, ctx, NULL, ctx->error_depth, err);

+    /* In the non-DANE case (re-)check direct trust in the root of the chain */


The DANE case can be more strict than non-DANE when the matched TLSA record is PKIX-TA(0) or PKIX-EE(1) so such a check would need to also happen in that case (and not with DANE-TA(2)/DANE-EE(3)). BUT: I think this PR is not warranted. Test coverage should ensure that build chains correctly, and I don't see much point in doing duplicate work at runtime.

Well, I suggest postponing the further handling of this PR including the fundamental discussion to post-3.0.
I just rebased this PR (and others) recently in order to fix merge conflicts before more such conflicts pile up.

crypto/x509/x509_vfy.c

…n-DANE Forms a safety-net making verification independendent of the chain building algorithm.

… for non-DANE

vdukhovni · 2021-04-28T19:02:08Z

At this point I don't see any remaining substantial bugs, but still question the need for this PR. The minor issues are:

Applications that deliberately ignore certificate errors now get an extra callback for the same issue (untrusted certificate chain)
Duplication of work (the X509 store lookups are not free, some stores may add significant latency)
The DANE exception is somewhat wrong if the PKIX-TA/PKIX-EE DANE usages are involved.
The check should perhaps only run if the verification status is X509_V_OK.
These are not major difficulties, and the code would not be wrong with these left unaddressed, but I still think that it would be better to invest in test coverage, rather than a runtime redundant check.

mattcaswell · 2021-09-21T09:44:39Z

OTC: We see no need for this PR.

kroeckx · 2021-09-21T16:36:45Z

Did you intend to close this?

t8m · 2021-09-21T16:42:13Z

Did you intend to close this?

Yes, it was intentionally closed by OTC after a discussion on the meeting.

kroeckx · 2021-09-21T16:55:33Z

Clearly I need to read better.

DDvO added the triaged: feature The issue/pr requests/adds a feature label Jan 4, 2021

This was referenced Jan 5, 2021

[PKI meta] Cert path building and verification needs many further improvements #13780

Open

x509_vfy.c: Make chain building more intuitive, flexible, and complete #13748

Open

vdukhovni reviewed Jan 12, 2021

View reviewed changes

crypto/x509/x509_vfy.c Outdated Show resolved Hide resolved

DDvO changed the title ~~internal_verify(): (Re-)check that the chain root is trusted~~ internal_verify(): (Re-)check that the chain root is direclty trusted Jan 13, 2021

DDvO changed the title ~~internal_verify(): (Re-)check that the chain root is direclty trusted~~ WiP: internal_verify(): (Re-)check that the chain root is direclty trusted Jan 13, 2021

DDvO force-pushed the internal_verify_check_trust branch from 1da5865 to 92c4576 Compare January 13, 2021 12:30

vdukhovni reviewed Jan 13, 2021

View reviewed changes

DDvO changed the title ~~WiP: internal_verify(): (Re-)check that the chain root is direclty trusted~~ WiP: x590_vfy.c: (Re-)check that the root of the constructed cert chain is direclty trusted Jan 14, 2021

DDvO force-pushed the internal_verify_check_trust branch from 95ff73d to 68cdf2a Compare January 19, 2021 07:49

DDvO changed the title ~~WiP: x590_vfy.c: (Re-)check that the root of the constructed cert chain is direclty trusted~~ WiP: x590_vfy.c: (Re-)check that the root of the constructed cert chain is directly trusted Jan 19, 2021

DDvO force-pushed the internal_verify_check_trust branch from 68cdf2a to 57a337d Compare January 19, 2021 07:58

vdukhovni added the hold: need otc decision The OTC needs to make a decision label Jan 24, 2021

DDvO force-pushed the internal_verify_check_trust branch 3 times, most recently from a1547d3 to ae1dcdc Compare April 27, 2021 10:23

vdukhovni requested changes Apr 27, 2021

View reviewed changes

DDvO added this to the Post 3.0.0 milestone Apr 28, 2021

DDvO added 2 commits April 28, 2021 20:43

X509 verify_chain(): (Re-)check that the chain root is trusted for no…

abee4dc

…n-DANE Forms a safety-net making verification independendent of the chain building algorithm.

fixup! X509 verify_chain(): (Re-)check that the chain root is trusted…

87d7b4b

… for non-DANE

DDvO force-pushed the internal_verify_check_trust branch from ae1dcdc to 87d7b4b Compare April 28, 2021 18:44

mattcaswell closed this Sep 21, 2021

mattcaswell removed the hold: need otc decision The OTC needs to make a decision label Sep 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WiP: x590_vfy.c: (Re-)check that the root of the constructed cert chain is directly trusted #13770

WiP: x590_vfy.c: (Re-)check that the root of the constructed cert chain is directly trusted #13770

DDvO commented Jan 4, 2021 •

edited

Loading

vdukhovni Jan 13, 2021

DDvO Jan 14, 2021 •

edited

Loading

vdukhovni Jan 14, 2021

DDvO Jan 19, 2021

vdukhovni Jan 24, 2021

DDvO commented Jan 19, 2021

vdukhovni commented Jan 24, 2021

openssl-machine commented Feb 24, 2021

openssl-machine commented Mar 27, 2021

openssl-machine commented Apr 27, 2021

vdukhovni Apr 27, 2021

DDvO Apr 28, 2021

vdukhovni Apr 27, 2021 •

edited

Loading

DDvO Apr 28, 2021 •

edited

Loading

vdukhovni commented Apr 28, 2021

mattcaswell commented Sep 21, 2021

kroeckx commented Sep 21, 2021 via email

t8m commented Sep 21, 2021

kroeckx commented Sep 21, 2021 via email

WiP: x590_vfy.c: (Re-)check that the root of the constructed cert chain is directly trusted #13770

WiP: x590_vfy.c: (Re-)check that the root of the constructed cert chain is directly trusted #13770

Conversation

DDvO commented Jan 4, 2021 • edited Loading

vdukhovni Jan 13, 2021

Choose a reason for hiding this comment

DDvO Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

vdukhovni Jan 14, 2021

Choose a reason for hiding this comment

DDvO Jan 19, 2021

Choose a reason for hiding this comment

vdukhovni Jan 24, 2021

Choose a reason for hiding this comment

DDvO commented Jan 19, 2021

vdukhovni commented Jan 24, 2021

openssl-machine commented Feb 24, 2021

openssl-machine commented Mar 27, 2021

openssl-machine commented Apr 27, 2021

vdukhovni Apr 27, 2021

Choose a reason for hiding this comment

DDvO Apr 28, 2021

Choose a reason for hiding this comment

vdukhovni Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

DDvO Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

vdukhovni commented Apr 28, 2021

mattcaswell commented Sep 21, 2021

kroeckx commented Sep 21, 2021 via email

t8m commented Sep 21, 2021

kroeckx commented Sep 21, 2021 via email

DDvO commented Jan 4, 2021 •

edited

Loading

DDvO Jan 14, 2021 •

edited

Loading

vdukhovni Apr 27, 2021 •

edited

Loading

DDvO Apr 28, 2021 •

edited

Loading