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 */
>
