Call the callback on success in new verifier in a compatible way

when we succeed with a chain, and ensure we do not call the callback
twice when the caller doesn't expect it.  A refactor of the end of
the legacy verify code in x509_vfy is probably overdue, but this
should be done based on a piece that works. the important bit here
is this allows the perl regression tests in tree to pass.

Changes the previously committed regress tests to test the success
case callbacks to be known to pass.

ok bluhm@ tb@

diff --git a/lib/libcrypto/x509/x509_internal.h b/lib/libcrypto/x509/x509_internal.h
index 7d3250d..8891aec 100644 (file)
--- a/lib/libcrypto/x509/x509_internal.h
+++ b/lib/libcrypto/x509/x509_internal.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: x509_internal.h,v 1.11 2021/08/28 15:22:42 beck Exp $ */
+/* $OpenBSD: x509_internal.h,v 1.12 2021/09/03 08:58:53 beck Exp $ */
 /*
  * Copyright (c) 2020 Bob Beck <beck@openbsd.org>
  *
@@ -90,6 +90,7 @@ int x509_vfy_check_revocation(X509_STORE_CTX *ctx);
 int x509_vfy_check_policy(X509_STORE_CTX *ctx);
 int x509_vfy_check_trust(X509_STORE_CTX *ctx);
 int x509_vfy_check_chain_extensions(X509_STORE_CTX *ctx);
+int x509_vfy_callback_indicate_success(X509_STORE_CTX *ctx);
 void x509v3_cache_extensions(X509 *x);
 X509 *x509_vfy_lookup_cert_match(X509_STORE_CTX *ctx, X509 *x);
 

diff --git a/lib/libcrypto/x509/x509_verify.c b/lib/libcrypto/x509/x509_verify.c
index 39371ef..2ec53f6 100644 (file)
--- a/lib/libcrypto/x509/x509_verify.c
+++ b/lib/libcrypto/x509/x509_verify.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: x509_verify.c,v 1.47 2021/08/30 08:59:33 beck Exp $ */
+/* $OpenBSD: x509_verify.c,v 1.48 2021/09/03 08:58:53 beck Exp $ */
 /*
  * Copyright (c) 2020-2021 Bob Beck <beck@openbsd.org>
  *
@@ -383,6 +383,7 @@ x509_verify_ctx_validate_legacy_chain(struct x509_verify_ctx *ctx,
                        return 0;
                chain->cert_errors[ctx->xsc->error_depth] =
                    ctx->xsc->error;
+               ctx->error_depth = ctx->xsc->error_depth;
        }
 
        return ret;
@@ -537,10 +538,11 @@ x509_verify_consider_candidate(struct x509_verify_ctx *ctx, X509 *cert,
                        x509_verify_chain_free(new_chain);
                        return 0;
                }
-               if (x509_verify_cert_error(ctx, candidate, depth, X509_V_OK, 1)) {
-                       (void) x509_verify_ctx_add_chain(ctx, new_chain);
-                       goto done;
+               if (!x509_verify_ctx_add_chain(ctx, new_chain)) {
+                       x509_verify_chain_free(new_chain);
+                       return 0;
                }
+               goto done;
        }
 
        x509_verify_build_chains(ctx, candidate, new_chain, full_chain);
@@ -596,8 +598,15 @@ x509_verify_build_chains(struct x509_verify_ctx *ctx, X509 *cert,
                return;
 
        count = ctx->chains_count;
+
        ctx->error = X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY;
        ctx->error_depth = depth;
+
+       if (ctx->saved_error != 0)
+               ctx->error = ctx->saved_error;
+       if (ctx->saved_error_depth != 0)
+               ctx->error_depth = ctx->saved_error_depth;
+
        if (ctx->xsc != NULL) {
                /*
                 * Long ago experiments at Muppet labs resulted in a
@@ -663,8 +672,6 @@ x509_verify_build_chains(struct x509_verify_ctx *ctx, X509 *cert,
        } else if (ctx->error_depth == depth) {
                if (!x509_verify_ctx_set_xsc_chain(ctx, current_chain, 0, 0))
                        return;
-               (void) x509_verify_cert_error(ctx, cert, depth,
-                   ctx->error, 0);
        }
 }
 
@@ -1131,9 +1138,12 @@ x509_verify(struct x509_verify_ctx *ctx, X509 *leaf, char *name)
        }
        do {
                retry_chain_build = 0;
-               if (x509_verify_ctx_cert_is_root(ctx, leaf, full_chain))
-                       x509_verify_ctx_add_chain(ctx, current_chain);
-               else {
+               if (x509_verify_ctx_cert_is_root(ctx, leaf, full_chain)) {
+                       if (!x509_verify_ctx_add_chain(ctx, current_chain)) {
+                               x509_verify_chain_free(current_chain);
+                               goto err;
+                       }
+               } else {
                        x509_verify_build_chains(ctx, leaf, current_chain,
                            full_chain);
                        if (full_chain && ctx->chains_count == 0) {
@@ -1189,8 +1199,24 @@ x509_verify(struct x509_verify_ctx *ctx, X509 *leaf, char *name)
                        if (!x509_verify_ctx_set_xsc_chain(ctx, ctx->chains[0],
                            1, 1))
                                goto err;
+                       ctx->xsc->error = X509_V_OK;
+                       /*
+                        * Call the callback indicating success up our already
+                        * verified chain. The callback could still tell us to
+                        * fail.
+                        */
+                       if(!x509_vfy_callback_indicate_success(ctx->xsc))
+                               goto err;
+               } else {
+                       /*
+                        * We had a failure, indicate the failure, but
+                        * allow the callback to override at depth 0
+                        */
+                       if (ctx->xsc->verify_cb(0, ctx->xsc)) {
+                               ctx->xsc->error = X509_V_OK;
+                               return 1;
+                       }
                }
-               return ctx->xsc->verify_cb(ctx->chains_count > 0, ctx->xsc);
        }
        return (ctx->chains_count);
 

diff --git a/lib/libcrypto/x509/x509_vfy.c b/lib/libcrypto/x509/x509_vfy.c
index a161b33..2f69017 100644 (file)
--- a/lib/libcrypto/x509/x509_vfy.c
+++ b/lib/libcrypto/x509/x509_vfy.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: x509_vfy.c,v 1.88 2021/08/28 15:22:42 beck Exp $ */
+/* $OpenBSD: x509_vfy.c,v 1.89 2021/09/03 08:58:53 beck Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -1879,7 +1879,7 @@ x509_check_cert_time(X509_STORE_CTX *ctx, X509 *x, int depth)
 }
 
 static int
-internal_verify(X509_STORE_CTX *ctx)
+x509_vfy_internal_verify(X509_STORE_CTX *ctx, int chain_verified)
 {
        int n = sk_X509_num(ctx->chain) - 1;
        X509 *xi = sk_X509_value(ctx->chain, n);
@@ -1915,8 +1915,8 @@ internal_verify(X509_STORE_CTX *ctx)
                 * certificate and its depth (rather than the depth of
                 * the subject).
                 */
-               if (xs != xi ||
-                   (ctx->param->flags & X509_V_FLAG_CHECK_SS_SIGNATURE)) {
+               if (!chain_verified && ( xs != xi ||
+                   (ctx->param->flags & X509_V_FLAG_CHECK_SS_SIGNATURE))) {
                        EVP_PKEY *pkey;
                        if ((pkey = X509_get_pubkey(xi)) == NULL) {
                                if (!verify_cb_cert(ctx, xi, xi != xs ? n+1 : n,
@@ -1933,7 +1933,7 @@ internal_verify(X509_STORE_CTX *ctx)
                }
 check_cert:
                /* Calls verify callback as needed */
-               if (!x509_check_cert_time(ctx, xs, n))
+               if (!chain_verified && !x509_check_cert_time(ctx, xs, n))
                        return 0;
 
                /*
@@ -1954,6 +1954,18 @@ check_cert:
        return 1;
 }
 
+static int
+internal_verify(X509_STORE_CTX *ctx)
+{
+       return x509_vfy_internal_verify(ctx, 0);
+}
+
+int
+x509_vfy_callback_indicate_success(X509_STORE_CTX *ctx)
+{
+       return x509_vfy_internal_verify(ctx, 1);
+}
+
 int
 X509_cmp_current_time(const ASN1_TIME *ctm)
 {

diff --git a/regress/lib/libcrypto/x509/Makefile b/regress/lib/libcrypto/x509/Makefile
index b05bf0b..b5cf333 100644 (file)
--- a/regress/lib/libcrypto/x509/Makefile
+++ b/regress/lib/libcrypto/x509/Makefile
@@ -1,4 +1,4 @@
-#      $OpenBSD: Makefile,v 1.7 2021/09/01 08:12:15 beck Exp $
+#      $OpenBSD: Makefile,v 1.8 2021/09/03 08:58:53 beck Exp $
 
 PROGS =        constraints verify x509attribute x509name callback
 LDADD= -Wl,-Bstatic -lcrypto -Wl,-Bdynamic
@@ -14,8 +14,6 @@ REGRESS_TARGETS += regress-x509attribute
 REGRESS_TARGETS += regress-x509name
 REGRESS_TARGETS += regress-callback
 
-REGRESS_EXPECTED_FAILURES += regress-callback
-
 CLEANFILES+=   x509name.result callbackout
 
 .if make(clean) || make(cleandir)