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