On Fri, Oct 23, 2020 at 09:13:23AM +0200, Theo Buehler wrote:
> On Thu, Oct 22, 2020 at 08:44:29PM -0700, Jeremy Evans wrote:
> > I was trying to diagnose a certificate validation failure in Ruby's
> > openssl extension tests with LibreSSL 3.2.2, and it was made more
> > difficult because the verification error type was dropped, resulting
> > in a SSL_R_CERTIFICATE_VERIFY_FAILED error where
> > SSL_get_verify_result returned X509_V_OK.
>
> Could you perhaps isolate a test case or explain the reproducer in a bit
> more detail? I tried running ruby 2.7's regress from ports but that
> always resulted in a SIGABRT (usually while running test_ftp.rb with or
> without your diff).
>
> With my diff below I once got past this abort and saw this:
>
> OpenSSL::TestSSL#test_verify_result
> [/usr/ports/pobj/ruby-2.7.2/ruby-2.7.2/test/openssl/utils.rb:279]:
> exceptions on 1 threads:
> <19> expected but was
> <20>.
>
> Did you see <0> here instead?
>
> > I think this patch will fix it. Compile tested only. OKs, or is there
> > a better way to fix it?
>
> This will probably also address the issue with the haproxy test reported
> on the libressl list:
>
> https://marc.info/?l=libressl&m=160339671313132&w=2
> https://github.com/haproxy/haproxy/issues/916
>
> Regarding your diff, I think setting the error on the store context
> should not be the responsibility of x509_verify()'s caller. After all,
> x509_verify() will likely end up being public API.
>
> The below diff should have the same effect as yours.
This looks ok to me tb@.. and appears to be the correct fix.
>
> > Thanks,
> > Jeremy
> >
> > Index: x509_vfy.c
> > ===================================================================
> > RCS file: /cvs/src/lib/libcrypto/x509/x509_vfy.c,v
> > retrieving revision 1.81
> > diff -u -p -r1.81 x509_vfy.c
> > --- x509_vfy.c 26 Sep 2020 02:06:28 -0000 1.81
> > +++ x509_vfy.c 23 Oct 2020 03:34:10 -0000
> > @@ -680,6 +680,9 @@ X509_verify_cert(X509_STORE_CTX *ctx)
> > if ((vctx = x509_verify_ctx_new_from_xsc(ctx, roots)) != NULL) {
> > ctx->error = X509_V_OK; /* Initialize to OK */
> > chain_count = x509_verify(vctx, NULL, NULL);
> > + if (vctx->error) {
> > + ctx->error = vctx->error;
> > + }
> > }
> >
> > sk_X509_pop_free(roots, X509_free);
> >
>
>
> Index: x509/x509_verify.c
> ===================================================================
> RCS file: /var/cvs/src/lib/libcrypto/x509/x509_verify.c,v
> retrieving revision 1.13
> diff -u -p -r1.13 x509_verify.c
> --- x509/x509_verify.c 26 Sep 2020 15:44:06 -0000 1.13
> +++ x509/x509_verify.c 23 Oct 2020 05:04:05 -0000
> @@ -853,13 +853,13 @@ x509_verify(struct x509_verify_ctx *ctx,
>
> if (ctx->roots == NULL || ctx->max_depth == 0) {
> ctx->error = X509_V_ERR_INVALID_CALL;
> - return 0;
> + goto err;
> }
>
> if (ctx->xsc != NULL) {
> if (leaf != NULL || name != NULL) {
> ctx->error = X509_V_ERR_INVALID_CALL;
> - return 0;
> + goto err;
> }
> leaf = ctx->xsc->cert;
>
> @@ -872,34 +872,34 @@ x509_verify(struct x509_verify_ctx *ctx,
> */
> if ((ctx->xsc->chain = sk_X509_new_null()) == NULL) {
> ctx->error = X509_V_ERR_OUT_OF_MEM;
> - return 0;
> + goto err;
> }
> if (!X509_up_ref(leaf)) {
> ctx->error = X509_V_ERR_OUT_OF_MEM;
> - return 0;
> + goto err;
> }
> if (!sk_X509_push(ctx->xsc->chain, leaf)) {
> X509_free(leaf);
> ctx->error = X509_V_ERR_OUT_OF_MEM;
> - return 0;
> + goto err;
> }
> ctx->xsc->error_depth = 0;
> ctx->xsc->current_cert = leaf;
> }
>
> if (!x509_verify_cert_valid(ctx, leaf, NULL))
> - return 0;
> + goto err;
>
> if (!x509_verify_cert_hostname(ctx, leaf, name))
> - return 0;
> + goto err;
>
> if ((current_chain = x509_verify_chain_new()) == NULL) {
> ctx->error = X509_V_ERR_OUT_OF_MEM;
> - return 0;
> + goto err;
> }
> if (!x509_verify_chain_append(current_chain, leaf, &ctx->error)) {
> x509_verify_chain_free(current_chain);
> - return 0;
> + goto err;
> }
> if (x509_verify_ctx_cert_is_root(ctx, leaf))
> x509_verify_ctx_add_chain(ctx, current_chain);
> @@ -925,4 +925,9 @@ x509_verify(struct x509_verify_ctx *ctx,
> return ctx->xsc->verify_cb(ctx->chains_count, ctx->xsc);
> }
> return (ctx->chains_count);
> +
> + err:
> + if (ctx->xsc != NULL)
> + ctx->xsc->error = ctx->error;
> + return 0;
> }
>
