On Sun, Oct 25, 2020 at 01:43:10PM -0600, Bob Beck wrote:
>
>
>
> 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.
>
Actually I rescind the ok -> your diff introduces a new problem.
I added regress to catch this problem and it found an issue:
the call into the x509_vfy_check_id can set the xsc->error but
then we don't set the ctx->error and it's sitting still as V_OK
when your diff sets it. and that's bad.
So this correctly sets the ctx->error coming back from
x509_vfy_check_id so your diff doesn't introduce another regression
you have my ok on this modified version :) I'll commit
the change to the bettertls regress to catch it once you commit.
Index: x509/x509_verify.c
===================================================================
RCS file: /cvs/src/lib/libcrypto/x509/x509_verify.c,v
retrieving revision 1.13
diff -u -p -u -p -r1.13 x509_verify.c
--- x509/x509_verify.c 26 Sep 2020 15:44:06 -0000 1.13
+++ x509/x509_verify.c 25 Oct 2020 23:58:08 -0000
@@ -458,8 +458,12 @@ x509_verify_cert_hostname(struct x509_ve
size_t len;
if (name == NULL) {
- if (ctx->xsc != NULL)
- return x509_vfy_check_id(ctx->xsc);
+ if (ctx->xsc != NULL) {
+ int ret;
+ if ((ret = x509_vfy_check_id(ctx->xsc)) == 0)
+ ctx->error = ctx->xsc->error;
+ return ret;
+ }
return 1;
}
if ((candidate = strdup(name)) == NULL) {
@@ -853,13 +857,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 +876,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 +929,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;
}
> > 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;
> > }
> >
