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