LibreSSL legacy verifier regression

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

LibreSSL legacy verifier regression

Jan Klemkow
Hi,

another co-worker of mine has found an other regress in the LibreSSL
legacy verifier.  I took his diff and made a test for our regression
framework.

The legacy verifier seems not to check the certificate if no root CA was
given.  The following test creates an expired certificate and tries to
verify it.  In one case it found the expected error in another, it does
not.

OK?

bye,
Jan

Index: lib/libcrypto/Makefile
===================================================================
RCS file: /cvs/src/regress/lib/libcrypto/Makefile,v
retrieving revision 1.41
diff -u -p -r1.41 Makefile
--- lib/libcrypto/Makefile 26 Dec 2020 00:48:56 -0000 1.41
+++ lib/libcrypto/Makefile 24 Feb 2021 05:29:51 -0000
@@ -23,6 +23,7 @@ SUBDIR += ecdsa
 SUBDIR += engine
 SUBDIR += evp
 SUBDIR += exp
+SUBDIR += expcert
 SUBDIR += free
 SUBDIR += gcm128
 SUBDIR += gost
Index: lib/libcrypto/expcert/Makefile
===================================================================
RCS file: lib/libcrypto/expcert/Makefile
diff -N lib/libcrypto/expcert/Makefile
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ lib/libcrypto/expcert/Makefile 24 Feb 2021 05:39:38 -0000
@@ -0,0 +1,29 @@
+# $OpenBSD$
+
+LDFLAGS += -lcrypto
+
+PROG = expcrt
+
+PKG_REQUIRE != pkg_info -e 'p5-IO-Socket-SSL-*'
+.if empty (PKG_REQUIRE)
+regress:
+ @echo "missing package p5-IO-Socket-SSL"
+ @echo SKIPPED
+.endif
+
+REGRESS_TARGETS = test-chain-with-root-CA
+REGRESS_TARGETS += test-chain-without-root-CA
+REGRESS_SETUP_ONCE = create-certs
+
+REGRESS_EXPECTED_FAILURES = test-chain-without-root-CA
+
+create-certs: create-certs.pl ${PROG}
+ perl ${.CURDIR}/create-certs.pl
+
+test-chain-with-root-CA:
+ ./expcrt -e 10 -r
+
+test-chain-without-root-CA:
+ ./expcrt -e 10
+
+.include <bsd.regress.mk>
Index: lib/libcrypto/expcert/create-certs.pl
===================================================================
RCS file: lib/libcrypto/expcert/create-certs.pl
diff -N lib/libcrypto/expcert/create-certs.pl
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ lib/libcrypto/expcert/create-certs.pl 24 Feb 2021 05:27:46 -0000
@@ -0,0 +1,46 @@
+#!/usr/bin/perl
+
+# Copyright (c) 2021 Anton Borowka <[hidden email]>
+#
+# Permission to use, copy, modify, and distribute this software for any
+# purpose with or without fee is hereby granted, provided that the above
+# copyright notice and this permission notice appear in all copies.
+#
+# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
+use strict;
+use warnings;
+
+use IO::Socket::SSL::Utils;
+
+my %certs;
+
+@{$certs{root}}{qw/cert key/} = CERT_create(
+    CA => 1,
+    not_after => time() + 315360000,
+    subject => { commonName => 'Root CA' },
+);
+
+@{$certs{intermediate}}{qw/cert key/} = CERT_create(
+    CA => 1,
+    issuer => [@{$certs{root}}{qw/cert key/}],
+    not_after => time() + 315360000,
+    subject => { commonName => 'Intermediate CA' },
+);
+
+@{$certs{expired}}{qw/cert key/} = CERT_create(
+    issuer => [@{$certs{intermediate}}{qw/cert key/}],
+    not_before => time() - 7200,
+    not_after => time() - 3600,
+    subject => { commonName => 'Expired' },
+);
+
+for (sort keys %certs) {
+    PEM_cert2file($certs{$_}{cert}, "$_.crt");
+}
Index: lib/libcrypto/expcert/expcrt.c
===================================================================
RCS file: lib/libcrypto/expcert/expcrt.c
diff -N lib/libcrypto/expcert/expcrt.c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ lib/libcrypto/expcert/expcrt.c 24 Feb 2021 05:27:46 -0000
@@ -0,0 +1,218 @@
+/* $OpenBSD$ */
+/*
+ * Copyright (c) 2021 Jan Klemkow <[hidden email]>
+ * Copyright (c) 2021 Anton Borowka <[hidden email]>
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include <err.h>
+#include <limits.h>
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <openssl/err.h>
+#include <openssl/pem.h>
+#include <openssl/x509_vfy.h>
+
+int error_found = 0;
+int expected_error = 0;
+
+static int
+x509verify_error_handler(int ok, X509_STORE_CTX *ctx)
+{
+ char name[BUFSIZ] = { 0 };
+
+ if (ctx->current_cert) {
+ X509_NAME_oneline(X509_get_subject_name(ctx->current_cert),
+    name, sizeof name);
+ }
+
+ if (ok == 0)
+ warnx("cert: %s error: %d %s", name, ctx->error,
+    X509_verify_cert_error_string(ctx->error));
+
+ if (ctx->error == expected_error)
+ error_found = 1;
+
+ return 1;
+}
+
+X509_STORE *
+store_init(const char *path)
+{
+ X509_STORE *store;
+ X509_LOOKUP *lookup;
+
+ /* init store object */
+ if ((store = X509_STORE_new()) == NULL)
+ return NULL;
+
+ lookup = X509_STORE_add_lookup(store, X509_LOOKUP_file());
+ if (X509_LOOKUP_load_file(lookup, path, X509_FILETYPE_PEM) == 0) {
+ X509_STORE_free(store);
+ return NULL;
+ }
+
+ X509_STORE_set_flags(store, X509_V_FLAG_PARTIAL_CHAIN);
+ X509_STORE_set_verify_cb(store, x509verify_error_handler);
+
+ ERR_clear_error();
+ return store;
+}
+
+int
+store_verify(X509_STORE *store, X509 *cert, STACK_OF(X509) *chain)
+{
+ X509_STORE_CTX *csc;
+ int status = 0;
+
+ /* prepare ctx:  */
+ if ((csc = X509_STORE_CTX_new()) == NULL) {
+ warnx("X509_STORE_CTX_new failed");
+ return 1;
+ }
+
+ if (X509_STORE_CTX_init(csc, store, cert, chain) == 0) {
+ X509_STORE_CTX_free(csc);
+ warnx("X509_STORE_CTX_init failed");
+ return 1;
+ }
+
+ X509_STORE_CTX_set_flags(csc, X509_V_FLAG_TRUSTED_FIRST);
+#ifdef X509_V_FLAG_LEGACY_VERIFY
+ X509_STORE_CTX_set_flags(csc, X509_V_FLAG_LEGACY_VERIFY);
+#endif
+ /* verify */
+ status = X509_verify_cert(csc);
+
+ if (error_found != 1) {
+ warnx("expected error %d not found", expected_error);
+ return 1;
+ }
+
+ return 0;
+}
+
+X509 *
+cert_init(const char *path)
+{
+ FILE *fh;
+ X509 *x509;
+
+ if ((fh = fopen(path, "r")) == NULL)
+ err(1, "fopen: %s", path);
+
+ x509 = PEM_read_X509(fh, NULL, NULL, NULL);
+
+ if (fclose(fh) == EOF)
+ err(1, "fclose: %s", path);
+
+ return x509;
+}
+
+STACK_OF(X509) *
+chain_init(const char *path)
+{
+ FILE *fh;
+ STACK_OF(X509_INFO) *sk;
+ STACK_OF(X509) *st;
+ X509_INFO *xi;
+
+ if (path == NULL)
+ return NULL;
+
+ if ((fh = fopen(path, "r")) == NULL)
+ err(1, "fopen: %s", path);
+
+ if ((sk = PEM_X509_INFO_read(fh, NULL, NULL, NULL)) == NULL)
+ err(1, "error reading chain info");
+
+ if (fclose(fh) == EOF)
+ err(1, "fclose: %s", path);
+
+ if ((st = sk_X509_new_null()) == NULL) {
+ sk_X509_INFO_pop_free(sk, X509_INFO_free);
+ err(1, "reading chain memory error");
+ }
+
+ while (sk_X509_INFO_num(sk)) {
+ xi = sk_X509_INFO_shift(sk);
+ if (xi->x509 != NULL) {
+ sk_X509_push(st, xi->x509);
+ xi->x509=NULL;
+ }
+ X509_INFO_free(xi);
+ }
+
+ sk_X509_INFO_free(sk);
+
+ if (sk_X509_num(st) == 0)
+ printf("no chain certificates loaded\n");
+
+ return st;
+}
+
+void
+usage(void)
+{
+ err(1, "usage: expcrt [-r] [-e error]");
+}
+
+int
+main(int argc, char *argv[])
+{
+ X509_STORE *store;
+ X509 *cert;
+ STACK_OF(X509) *chain;
+ char *path_store = NULL;
+ char *path_chain = NULL;
+ const char *errstr;
+ int root_flag = 0;
+ int ch;
+
+ while ((ch = getopt(argc, argv, "e:r")) != -1) {
+ switch (ch) {
+ case 'e':
+ expected_error = strtonum(optarg, 0, INT_MAX, &errstr);
+ if (errstr != NULL)
+ errx(1, "%s: %s", errstr, optarg);
+ break;
+ case 'r':
+ root_flag = 1;
+ break;
+ default:
+ usage();
+ }
+ }
+
+ if (root_flag == 1) {
+ /*
+ * with root CA
+ */
+ path_store = strdup("root.crt");
+ path_chain = strdup("intermediate.crt");
+ } else {
+ /*
+ * without root CA
+ */
+ path_store = strdup("intermediate.crt");
+ }
+
+ store = store_init(path_store);
+ chain = chain_init(path_chain);
+ cert = cert_init("expired.crt");
+
+ return store_verify(store, cert, chain);
+}

Reply | Threaded
Open this post in threaded view
|

Re: LibreSSL legacy verifier regression

Theo Buehler-3
On Wed, Feb 24, 2021 at 06:47:00AM +0100, Jan Klemkow wrote:

> Hi,
>
> another co-worker of mine has found an other regress in the LibreSSL
> legacy verifier.  I took his diff and made a test for our regression
> framework.
>
> The legacy verifier seems not to check the certificate if no root CA was
> given.  The following test creates an expired certificate and tries to
> verify it.  In one case it found the expected error in another, it does
> not.

Thanks for the report and the test case, that's very helpful. The diff
at the end addresses this.

The verifier does not find the expected error because it now bails out
earlier.  This is a consequence of a refactoring of X509_verify_cert()
(x509_vfy.c r1.75) that was done to integrate the new verifier.

https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libcrypto/x509/x509_vfy.c.diff?r1=1.74&r2=1.75

What happens is that x509_legacy_verify_build_chain() returns ok == 0 in
your test case. The safety net at the end of x509_verify_cert_legacy()
sets ctx->error to X509_V_ERR_UNSPECIFIED (so the unchecked call to
X509_verify_cert() in your regress test actually indicates verification
failure).


The diff below restores the previous behavior and fixes a bug.

Prior to the the refactoring, each 'goto end' in the code that is now in
x509_legacy_verify_build_chain() would stop validation, while in other
cases validation would have carried on. So indicate this via the return
value and return ok via a pointer.

The bug is that the return value check of x509_legacy_verify_build_chain()
should have been if (ok <= 0), not if (!ok).


Regarding your regress diff: I don't think we want to land it as it is.

The verifier lives in libcrypto/x509, so the regress test belongs in
there.

We really need to come up with an extensible design that can check a
number of such cases (and ideally includes the bulk of your openssl/x509
regress tests). I don't want to add a directory for every bug in the
verifier or legacy verifier. As jsing already mentioned, I expect that
we want to commit the test certs so we don't need perl modules from
ports to run the regress. Then we want to add generating scripts and a
README that gives instructions on how to regenerate the certs if needed.

I would like to have one C program that runs all tests in a loop (or
perhaps one C program per family of regressions). It would be nice if
this C program could also be compiled against OpenSSL 1.1.1 so we can
easily check for differences of behavior (see x509/bettertls/Makefile
for an example that does this).  For your test program this just means:
don't access csc->blah, but use X509_STORE_CTX_get_blah(csc) instead.

Why does the test set TRUSTED_FIRST?

The code also needs a bit of cleaning. There are a number of unchecked
return values, for example strdup and sk_*_push, and csc is leaked
after X509_verify_cert().

It would also be nice to run this test against the new verifier.


Index: x509/x509_vfy.c
===================================================================
RCS file: /cvs/src/lib/libcrypto/x509/x509_vfy.c,v
retrieving revision 1.85
diff -u -p -r1.85 x509_vfy.c
--- x509/x509_vfy.c 11 Feb 2021 04:56:43 -0000 1.85
+++ x509/x509_vfy.c 24 Feb 2021 19:09:03 -0000
@@ -240,7 +240,7 @@ x509_vfy_check_id(X509_STORE_CTX *ctx) {
  * Oooooooh..
  */
 static int
-X509_verify_cert_legacy_build_chain(X509_STORE_CTX *ctx, int *bad)
+X509_verify_cert_legacy_build_chain(X509_STORE_CTX *ctx, int *bad, int *out_ok)
 {
  X509 *x, *xtmp, *xtmp2, *chain_ss = NULL;
  int bad_chain = 0;
@@ -517,11 +517,21 @@ X509_verify_cert_legacy_build_chain(X509
  if (!ok)
  goto end;
  }
+
+ sk_X509_free(sktmp);
+ X509_free(chain_ss);
+ *bad = bad_chain;
+ *out_ok = ok;
+
+ return 1;
+
  end:
  sk_X509_free(sktmp);
  X509_free(chain_ss);
  *bad = bad_chain;
- return ok;
+ *out_ok = ok;
+
+ return 0;
 }
 
 static int
@@ -531,8 +541,7 @@ X509_verify_cert_legacy(X509_STORE_CTX *
 
  ctx->error = X509_V_OK; /* Initialize to OK */
 
- ok = X509_verify_cert_legacy_build_chain(ctx, &bad_chain);
- if (!ok)
+ if (!X509_verify_cert_legacy_build_chain(ctx, &bad_chain, &ok))
  goto end;
 
  /* We have the chain complete: now we need to check its purpose */

Reply | Threaded
Open this post in threaded view
|

Re: LibreSSL legacy verifier regression

Theo Buehler-3
On Wed, Feb 24, 2021 at 09:00:05PM +0100, Theo Buehler wrote:

> On Wed, Feb 24, 2021 at 06:47:00AM +0100, Jan Klemkow wrote:
> > Hi,
> >
> > another co-worker of mine has found an other regress in the LibreSSL
> > legacy verifier.  I took his diff and made a test for our regression
> > framework.
> >
> > The legacy verifier seems not to check the certificate if no root CA was
> > given.  The following test creates an expired certificate and tries to
> > verify it.  In one case it found the expected error in another, it does
> > not.
>
> Thanks for the report and the test case, that's very helpful. The diff
> at the end addresses this.
>
> The verifier does not find the expected error because it now bails out
> earlier.  This is a consequence of a refactoring of X509_verify_cert()
> (x509_vfy.c r1.75) that was done to integrate the new verifier.
>
> https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libcrypto/x509/x509_vfy.c.diff?r1=1.74&r2=1.75
>
> What happens is that x509_legacy_verify_build_chain() returns ok == 0 in
> your test case. The safety net at the end of x509_verify_cert_legacy()
> sets ctx->error to X509_V_ERR_UNSPECIFIED (so the unchecked call to
> X509_verify_cert() in your regress test actually indicates verification
> failure).
>
>
> The diff below restores the previous behavior and fixes a bug.
>
> Prior to the the refactoring, each 'goto end' in the code that is now in
> x509_legacy_verify_build_chain() would stop validation, while in other
> cases validation would have carried on. So indicate this via the return
> value and return ok via a pointer.
>
> The bug is that the return value check of x509_legacy_verify_build_chain()
> should have been if (ok <= 0), not if (!ok).
>
>
> Regarding your regress diff: I don't think we want to land it as it is.
>
> The verifier lives in libcrypto/x509, so the regress test belongs in
> there.
>
> We really need to come up with an extensible design that can check a
> number of such cases (and ideally includes the bulk of your openssl/x509
> regress tests). I don't want to add a directory for every bug in the
> verifier or legacy verifier. As jsing already mentioned, I expect that
> we want to commit the test certs so we don't need perl modules from
> ports to run the regress. Then we want to add generating scripts and a
> README that gives instructions on how to regenerate the certs if needed.
>
> I would like to have one C program that runs all tests in a loop (or
> perhaps one C program per family of regressions). It would be nice if
> this C program could also be compiled against OpenSSL 1.1.1 so we can
> easily check for differences of behavior (see x509/bettertls/Makefile
> for an example that does this).  For your test program this just means:
> don't access csc->blah, but use X509_STORE_CTX_get_blah(csc) instead.
>
> Why does the test set TRUSTED_FIRST?
>
> The code also needs a bit of cleaning. There are a number of unchecked
> return values, for example strdup and sk_*_push, and csc is leaked
> after X509_verify_cert().
>
> It would also be nice to run this test against the new verifier.

Missed an obvious simplification.

Index: x509/x509_vfy.c
===================================================================
RCS file: /cvs/src/lib/libcrypto/x509/x509_vfy.c,v
retrieving revision 1.85
diff -u -p -r1.85 x509_vfy.c
--- x509/x509_vfy.c 11 Feb 2021 04:56:43 -0000 1.85
+++ x509/x509_vfy.c 24 Feb 2021 20:19:34 -0000
@@ -240,12 +240,13 @@ x509_vfy_check_id(X509_STORE_CTX *ctx) {
  * Oooooooh..
  */
 static int
-X509_verify_cert_legacy_build_chain(X509_STORE_CTX *ctx, int *bad)
+X509_verify_cert_legacy_build_chain(X509_STORE_CTX *ctx, int *bad, int *out_ok)
 {
  X509 *x, *xtmp, *xtmp2, *chain_ss = NULL;
  int bad_chain = 0;
  X509_VERIFY_PARAM *param = ctx->param;
- int depth, i, ok = 0;
+ int ok = 0, ret = 0;
+ int depth, i;
  int num, j, retry, trust;
  int (*cb) (int xok, X509_STORE_CTX *xctx);
  STACK_OF(X509) *sktmp = NULL;
@@ -517,11 +518,15 @@ X509_verify_cert_legacy_build_chain(X509
  if (!ok)
  goto end;
  }
+
+ ret = 1;
  end:
  sk_X509_free(sktmp);
  X509_free(chain_ss);
  *bad = bad_chain;
- return ok;
+ *out_ok = ok;
+
+ return ret;
 }
 
 static int
@@ -531,8 +536,7 @@ X509_verify_cert_legacy(X509_STORE_CTX *
 
  ctx->error = X509_V_OK; /* Initialize to OK */
 
- ok = X509_verify_cert_legacy_build_chain(ctx, &bad_chain);
- if (!ok)
+ if (!X509_verify_cert_legacy_build_chain(ctx, &bad_chain, &ok))
  goto end;
 
  /* We have the chain complete: now we need to check its purpose */

Reply | Threaded
Open this post in threaded view
|

Re: LibreSSL legacy verifier regression

Jan Klemkow
On Wed, Feb 24, 2021 at 09:21:56PM +0100, Theo Buehler wrote:

> On Wed, Feb 24, 2021 at 09:00:05PM +0100, Theo Buehler wrote:
> > On Wed, Feb 24, 2021 at 06:47:00AM +0100, Jan Klemkow wrote:
> > > another co-worker of mine has found an other regress in the LibreSSL
> > > legacy verifier.  I took his diff and made a test for our regression
> > > framework.
> > >
> > > The legacy verifier seems not to check the certificate if no root CA was
> > > given.  The following test creates an expired certificate and tries to
> > > verify it.  In one case it found the expected error in another, it does
> > > not.
> >
> > Thanks for the report and the test case, that's very helpful. The diff
> > at the end addresses this.
> >
> > The verifier does not find the expected error because it now bails out
> > earlier.  This is a consequence of a refactoring of X509_verify_cert()
> > (x509_vfy.c r1.75) that was done to integrate the new verifier.
> >
> > https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libcrypto/x509/x509_vfy.c.diff?r1=1.74&r2=1.75
> >
> > What happens is that x509_legacy_verify_build_chain() returns ok == 0 in
> > your test case. The safety net at the end of x509_verify_cert_legacy()
> > sets ctx->error to X509_V_ERR_UNSPECIFIED (so the unchecked call to
> > X509_verify_cert() in your regress test actually indicates verification
> > failure).
> >
> >
> > The diff below restores the previous behavior and fixes a bug.
> >
> > Prior to the the refactoring, each 'goto end' in the code that is now in
> > x509_legacy_verify_build_chain() would stop validation, while in other
> > cases validation would have carried on. So indicate this via the return
> > value and return ok via a pointer.
> >
> > The bug is that the return value check of x509_legacy_verify_build_chain()
> > should have been if (ok <= 0), not if (!ok).
> >
> > Regarding your regress diff: I don't think we want to land it as it is.

Ok.

> > The verifier lives in libcrypto/x509, so the regress test belongs in
> > there.

You are right, its the better place.  At least I want to send you a bug
report with concrete code to test.

> > We really need to come up with an extensible design that can check a
> > number of such cases (and ideally includes the bulk of your openssl/x509
> > regress tests). I don't want to add a directory for every bug in the
> > verifier or legacy verifier. As jsing already mentioned, I expect that
> > we want to commit the test certs so we don't need perl modules from
> > ports to run the regress. Then we want to add generating scripts and a
> > README that gives instructions on how to regenerate the certs if needed.
> >
> > I would like to have one C program that runs all tests in a loop (or
> > perhaps one C program per family of regressions). It would be nice if
> > this C program could also be compiled against OpenSSL 1.1.1 so we can
> > easily check for differences of behavior (see x509/bettertls/Makefile
> > for an example that does this).  For your test program this just means:
> > don't access csc->blah, but use X509_STORE_CTX_get_blah(csc) instead.
> >
> > Why does the test set TRUSTED_FIRST?

I just forget to remove the this line, from the original version.

> > The code also needs a bit of cleaning. There are a number of unchecked
> > return values, for example strdup and sk_*_push, and csc is leaked
> > after X509_verify_cert().
> >
> > It would also be nice to run this test against the new verifier.

The test passes with the new verifier in current, but not in 6.8.

> Missed an obvious simplification.

The diff looks fine to me and it fixes our regressions.
I would give you an OK jan, fwiw.

Thanks,
Jan

> Index: x509/x509_vfy.c
> ===================================================================
> RCS file: /cvs/src/lib/libcrypto/x509/x509_vfy.c,v
> retrieving revision 1.85
> diff -u -p -r1.85 x509_vfy.c
> --- x509/x509_vfy.c 11 Feb 2021 04:56:43 -0000 1.85
> +++ x509/x509_vfy.c 24 Feb 2021 20:19:34 -0000
> @@ -240,12 +240,13 @@ x509_vfy_check_id(X509_STORE_CTX *ctx) {
>   * Oooooooh..
>   */
>  static int
> -X509_verify_cert_legacy_build_chain(X509_STORE_CTX *ctx, int *bad)
> +X509_verify_cert_legacy_build_chain(X509_STORE_CTX *ctx, int *bad, int *out_ok)
>  {
>   X509 *x, *xtmp, *xtmp2, *chain_ss = NULL;
>   int bad_chain = 0;
>   X509_VERIFY_PARAM *param = ctx->param;
> - int depth, i, ok = 0;
> + int ok = 0, ret = 0;
> + int depth, i;
>   int num, j, retry, trust;
>   int (*cb) (int xok, X509_STORE_CTX *xctx);
>   STACK_OF(X509) *sktmp = NULL;
> @@ -517,11 +518,15 @@ X509_verify_cert_legacy_build_chain(X509
>   if (!ok)
>   goto end;
>   }
> +
> + ret = 1;
>   end:
>   sk_X509_free(sktmp);
>   X509_free(chain_ss);
>   *bad = bad_chain;
> - return ok;
> + *out_ok = ok;
> +
> + return ret;
>  }
>  
>  static int
> @@ -531,8 +536,7 @@ X509_verify_cert_legacy(X509_STORE_CTX *
>  
>   ctx->error = X509_V_OK; /* Initialize to OK */
>  
> - ok = X509_verify_cert_legacy_build_chain(ctx, &bad_chain);
> - if (!ok)
> + if (!X509_verify_cert_legacy_build_chain(ctx, &bad_chain, &ok))
>   goto end;
>  
>   /* We have the chain complete: now we need to check its purpose */
>