Fix the legacy verifier callback behaviour for untrusted certs.

The verifier callback is used by mutt to do a form of certificate
pinning where the callback gets fired and depending on a
cert saved to a file will decide to accept an untrusted cert.

This corrects two problems that affected this. The callback was not
getting the correct depth and chain for the error where mutt would
save the certificate in the first place, and then the callback was not
getting fired to allow it to override the failing certificate
validation.

thanks to Avon Robertson <avon.r@xtra.co.nz> for the report and
sthen@ for analysis.

"The callback is not an API, it's a gordian knot - tb@"
ok jsing@

3 files changed, 347 insertions, 19 deletions

diff --git a/src/lib/libcrypto/x509/x509_verify.c b/src/lib/libcrypto/x509/x509_verify.c
index 83030672ef..aa14bc1933 100644
--- a/src/lib/libcrypto/x509/x509_verify.c
+++ b/src/lib/libcrypto/x509/x509_verify.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: x509_verify.c,v 1.57 2022/06/27 14:10:22 tb Exp $ */
+/* $OpenBSD: x509_verify.c,v 1.58 2022/06/28 07:56:34 beck Exp $ */
 /*
  * Copyright (c) 2020-2021 Bob Beck <beck@openbsd.org>
  *
@@ -258,6 +258,25 @@ x509_verify_cert_self_signed(X509 *cert)
         return (cert->ex_flags & EXFLAG_SS) ? 1 : 0;
 }
 
+/* XXX beck - clean up this mess of is_root */
+static int
+x509_verify_check_chain_end(X509 *cert, int full_chain)
+{
+        if (full_chain)
+                return x509_verify_cert_self_signed(cert);
+        return 1;
+}
+
+static int
+x509_verify_check_legacy_chain_end(struct x509_verify_ctx *ctx, X509 *cert,
+    int full_chain)
+{
+        if (X509_check_trust(cert, ctx->xsc->param->trust, 0) !=
+            X509_TRUST_TRUSTED)
+                return 0;
+        return x509_verify_check_chain_end(cert, full_chain);
+}
+
 static int
 x509_verify_ctx_cert_is_root(struct x509_verify_ctx *ctx, X509 *cert,
     int full_chain)
@@ -273,15 +292,16 @@ x509_verify_ctx_cert_is_root(struct x509_verify_ctx *ctx, X509 *cert,
                 if ((match = x509_vfy_lookup_cert_match(ctx->xsc,
                     cert)) != NULL) {
                         X509_free(match);
-                        return !full_chain ||
-                            x509_verify_cert_self_signed(cert);
+                        return x509_verify_check_legacy_chain_end(ctx, cert,
+                            full_chain);
+
                 }
         } else {
                 /* Check the provided roots */
                 for (i = 0; i < sk_X509_num(ctx->roots); i++) {
                         if (X509_cmp(sk_X509_value(ctx->roots, i), cert) == 0)
-                                return !full_chain ||
-                                    x509_verify_cert_self_signed(cert);
+                                return x509_verify_check_chain_end(cert,
+                                    full_chain);
                 }
         }
 
@@ -393,14 +413,23 @@ x509_verify_ctx_validate_legacy_chain(struct x509_verify_ctx *ctx,
         ctx->xsc->error = X509_V_OK;
         ctx->xsc->error_depth = 0;
 
-        trust = x509_vfy_check_trust(ctx->xsc);
-        if (trust == X509_TRUST_REJECTED)
-                goto err;
-
         if (!x509_verify_ctx_set_xsc_chain(ctx, chain, 0, 1))
                 goto err;
 
         /*
+         * Call the legacy code to walk the chain and check trust
+         * in the legacy way to handle partial chains and get the
+         * callback fired correctly.
+         */
+        trust = x509_vfy_check_trust(ctx->xsc);
+        if (trust == X509_TRUST_REJECTED)
+                goto err; /* callback was called in x509_vfy_check_trust */
+        if (trust != X509_TRUST_TRUSTED) {
+                /* NOTREACHED */
+                goto err;  /* should not happen if we get in here - abort? */
+        }
+
+        /*
          * XXX currently this duplicates some work done in chain
          * build, but we keep it here until we have feature parity
          */
@@ -432,10 +461,6 @@ x509_verify_ctx_validate_legacy_chain(struct x509_verify_ctx *ctx,
         if (!x509_vfy_check_policy(ctx->xsc))
                 goto err;
 
-        if ((!(ctx->xsc->param->flags & X509_V_FLAG_PARTIAL_CHAIN)) &&
-            trust != X509_TRUST_TRUSTED)
-                goto err;
-
         ret = 1;
 
  err:
@@ -688,8 +713,8 @@ x509_verify_build_chains(struct x509_verify_ctx *ctx, X509 *cert,
                 }
                 if (ret > 0) {
                         if (x509_verify_potential_parent(ctx, candidate, cert)) {
-                                is_root = !full_chain ||
-                                    x509_verify_cert_self_signed(candidate);
+                                is_root = x509_verify_check_legacy_chain_end(
+                                    ctx, candidate, full_chain);
                                 x509_verify_consider_candidate(ctx, cert,
                                     is_root, candidate, current_chain,
                                     full_chain, name);
@@ -701,8 +726,8 @@ x509_verify_build_chains(struct x509_verify_ctx *ctx, X509 *cert,
                 for (i = 0; i < sk_X509_num(ctx->roots); i++) {
                         candidate = sk_X509_value(ctx->roots, i);
                         if (x509_verify_potential_parent(ctx, candidate, cert)) {
-                                is_root = !full_chain ||
-                                    x509_verify_cert_self_signed(candidate);
+                                is_root = x509_verify_check_chain_end(candidate,
+                                    full_chain);
                                 x509_verify_consider_candidate(ctx, cert,
                                     is_root, candidate, current_chain,
                                     full_chain, name);
@@ -1168,6 +1193,8 @@ x509_verify(struct x509_verify_ctx *ctx, X509 *leaf, char *name)
                                  * on failure and will be needed for
                                  * that.
                                  */
+                                ctx->xsc->error = ctx->error;
+                                ctx->xsc->error_depth = ctx->error_depth;
                                 if (!x509_verify_ctx_save_xsc_error(ctx)) {
                                         x509_verify_chain_free(current_chain);
                                         goto err;
diff --git a/src/regress/lib/libcrypto/x509/Makefile b/src/regress/lib/libcrypto/x509/Makefile
index ca66df19cd..4635d63ed0 100644
--- a/src/regress/lib/libcrypto/x509/Makefile
+++ b/src/regress/lib/libcrypto/x509/Makefile
@@ -1,7 +1,7 @@
-#       $OpenBSD: Makefile,v 1.13 2022/06/25 20:01:43 beck Exp $
+#       $OpenBSD: Makefile,v 1.14 2022/06/28 07:56:34 beck Exp $
 
 PROGS = constraints verify x509attribute x509name x509req_ext callback
-PROGS += expirecallback
+PROGS += expirecallback callbackfailures
 LDADD = -lcrypto
 DPADD = ${LIBCRYPTO}
 
@@ -20,6 +20,7 @@ REGRESS_TARGETS += regress-x509name
 REGRESS_TARGETS += regress-x509req_ext
 REGRESS_TARGETS += regress-callback
 REGRESS_TARGETS += regress-expirecallback
+REGRESS_TARGETS += regress-callbackfailures
 
 CLEANFILES +=   x509name.result callbackout
 
@@ -54,4 +55,7 @@ regress-callback: callback
 regress-expirecallback: expirecallback
         ./expirecallback ${.CURDIR}/../certs
 
+regress-callbackfailures: callbackfailures
+        ./callbackfailures ${.CURDIR}/../certs
+
 .include <bsd.regress.mk>
diff --git a/src/regress/lib/libcrypto/x509/callbackfailures.c b/src/regress/lib/libcrypto/x509/callbackfailures.c
new file mode 100644
index 0000000000..f714adffff
--- /dev/null
+++ b/src/regress/lib/libcrypto/x509/callbackfailures.c
@@ -0,0 +1,297 @@
+/* $OpenBSD: callbackfailures.c,v 1.1 2022/06/28 07:56:34 beck Exp $ */
+/*
+ * Copyright (c) 2020 Joel Sing <jsing@openbsd.org>
+ * Copyright (c) 2020-2021 Bob Beck <beck@openbsd.org>
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include <err.h>
+#include <string.h>
+
+#include <openssl/bio.h>
+#include <openssl/err.h>
+#include <openssl/pem.h>
+#include <openssl/x509.h>
+#include <openssl/x509v3.h>
+#include <openssl/x509_verify.h>
+
+#define MODE_MODERN_VFY         0
+#define MODE_MODERN_VFY_DIR     1
+#define MODE_LEGACY_VFY         2
+#define MODE_VERIFY             3
+
+static int verbose = 1;
+
+static int expected_depth;
+static int expected_error;
+static int seen_depth;
+static int seen_error;
+
+static int
+passwd_cb(char *buf, int size, int rwflag, void *u)
+{
+        memset(buf, 0, size);
+        return (0);
+}
+
+static int
+certs_from_file(const char *filename, STACK_OF(X509) **certs, int clear)
+{
+        STACK_OF(X509_INFO) *xis = NULL;
+        STACK_OF(X509) *xs = NULL;
+        BIO *bio = NULL;
+        X509 *x;
+        int i;
+
+        if (clear) {
+                if ((xs = sk_X509_new_null()) == NULL)
+                        errx(1, "failed to create X509 stack");
+        } else
+                xs = *certs;
+        if ((bio = BIO_new_file(filename, "r")) == NULL) {
+                ERR_print_errors_fp(stderr);
+                errx(1, "failed to create bio");
+        }
+        if ((xis = PEM_X509_INFO_read_bio(bio, NULL, passwd_cb, NULL)) == NULL)
+                errx(1, "failed to read PEM");
+
+        for (i = 0; i < sk_X509_INFO_num(xis); i++) {
+                if ((x = sk_X509_INFO_value(xis, i)->x509) == NULL)
+                        continue;
+                if (!sk_X509_push(xs, x))
+                        errx(1, "failed to push X509");
+                X509_up_ref(x);
+        }
+
+        *certs = xs;
+        xs = NULL;
+
+        sk_X509_INFO_pop_free(xis, X509_INFO_free);
+        sk_X509_pop_free(xs, X509_free);
+        BIO_free(bio);
+
+        return 1;
+}
+
+static int
+verify_cert_cb(int ok, X509_STORE_CTX *xsc)
+{
+        X509 *current_cert;
+        int verify_err;
+
+        current_cert = X509_STORE_CTX_get_current_cert(xsc);
+        if (current_cert != NULL) {
+                X509_NAME_print_ex_fp(stderr,
+                    X509_get_subject_name(current_cert), 0,
+                    XN_FLAG_ONELINE);
+                fprintf(stderr, "\n");
+        }
+
+        verify_err = X509_STORE_CTX_get_error(xsc);
+        if (verify_err != X509_V_OK) {
+                seen_depth = X509_STORE_CTX_get_error_depth(xsc);
+                seen_error = verify_err;
+                fprintf(stderr, "verify error at depth %d: %s\n",
+                    X509_STORE_CTX_get_error_depth(xsc),
+                    X509_verify_cert_error_string(verify_err));
+        }
+
+        fprintf(stderr, "chain of length %d\n", sk_X509_num (X509_STORE_CTX_get0_chain (xsc)));
+
+        return ok;
+}
+
+static void
+verify_cert(const char *roots_dir, const char *roots_file,
+    const char *bundle_file, const char*bundle_file2, int *chains, int mode)
+{
+        STACK_OF(X509) *roots = NULL, *bundle = NULL;
+        X509_STORE_CTX *xsc = NULL;
+        X509_STORE *store = NULL;
+        int verify_err, use_dir;
+        X509 *leaf = NULL;
+
+        *chains = 0;
+        use_dir = (mode == MODE_MODERN_VFY_DIR);
+
+        if (!use_dir && !certs_from_file(roots_file, &roots, 1))
+                errx(1, "failed to load roots from '%s'", roots_file);
+        if (!certs_from_file(bundle_file, &bundle, 1))
+                errx(1, "failed to load bundle from '%s'", bundle_file);
+        if (!certs_from_file(bundle_file, &bundle, 0))
+                errx(1, "failed to load bundle from '%s'", bundle_file2);
+        if (sk_X509_num(bundle) < 1)
+                errx(1, "not enough certs in bundle");
+        leaf = sk_X509_shift(bundle);
+
+        if ((xsc = X509_STORE_CTX_new()) == NULL)
+                errx(1, "X509_STORE_CTX");
+        if (use_dir && (store = X509_STORE_new()) == NULL)
+                errx(1, "X509_STORE");
+        if (!X509_STORE_CTX_init(xsc, store, leaf, bundle)) {
+                ERR_print_errors_fp(stderr);
+                errx(1, "failed to init store context");
+        }
+
+        if (use_dir) {
+                if (!X509_STORE_load_locations(store, NULL, roots_dir))
+                        errx(1, "failed to set by_dir directory of %s", roots_dir);
+        }
+        if (mode == MODE_LEGACY_VFY)
+                X509_STORE_CTX_set_flags(xsc, X509_V_FLAG_LEGACY_VERIFY);
+        else
+                X509_VERIFY_PARAM_clear_flags(X509_STORE_CTX_get0_param(xsc),
+                    X509_V_FLAG_LEGACY_VERIFY);
+
+        if (verbose)
+                X509_STORE_CTX_set_verify_cb(xsc, verify_cert_cb);
+        if (!use_dir)
+                X509_STORE_CTX_set0_trusted_stack(xsc, roots);
+
+        if (X509_verify_cert(xsc) == 1) {
+                *chains = 1; /* XXX */
+                goto done;
+        }
+
+        verify_err = X509_STORE_CTX_get_error(xsc);
+        if (verify_err == 0)
+                errx(1, "Error unset on failure!\n");
+
+        fprintf(stderr, "failed to verify at %d: %s\n",
+            X509_STORE_CTX_get_error_depth(xsc),
+            X509_verify_cert_error_string(verify_err));
+
+ done:
+        sk_X509_pop_free(roots, X509_free);
+        sk_X509_pop_free(bundle, X509_free);
+        X509_STORE_free(store);
+        X509_STORE_CTX_free(xsc);
+        X509_free(leaf);
+}
+
+struct verify_cert_test {
+        const char *id;
+        int want_chains;
+        int failing;
+        int depth;
+        int error;
+};
+
+struct verify_cert_test verify_cert_tests[] = {
+        {
+                .id = "1a",
+                .want_chains = 0,
+                .depth = 0,
+                .error = X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY,
+        },
+        {
+                .id = "2a",
+                .want_chains = 0,
+                .depth = 1,
+                .error = X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY,
+        },
+        {
+                .id = "2c",
+                .want_chains = 0,
+                .depth = 2,
+                .error = X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN,
+        },
+};
+
+#define N_VERIFY_CERT_TESTS \
+    (sizeof(verify_cert_tests) / sizeof(*verify_cert_tests))
+
+static int
+verify_cert_test(const char *certs_path, int mode)
+{
+        char *roots_file, *bundle_file, *bundle_file2, *roots_dir;
+        struct verify_cert_test *vct;
+        int failed = 0;
+        int chains;
+        size_t i;
+
+        for (i = 0; i < N_VERIFY_CERT_TESTS; i++) {
+                vct = &verify_cert_tests[i];
+
+                if (asprintf(&roots_file, "/etc/ssl/cert.pem") == -1)
+                        errx(1, "asprintf");
+                if (asprintf(&bundle_file, "%s/%s/bundle.pem", certs_path,
+                    vct->id) == -1)
+                        errx(1, "asprintf");
+                if (asprintf(&bundle_file2, "%s/%s/roots.pem", certs_path,
+                    vct->id) == -1)
+                        errx(1, "asprintf");
+                if (asprintf(&roots_dir, "./%s/roots", vct->id) == -1)
+                        errx(1, "asprintf");
+
+                fprintf(stderr, "== Test %zu (%s)\n", i, vct->id);
+                fprintf(stderr, "== depth %d\n", vct->depth);
+                fprintf(stderr, "== error %d\n", vct->error);
+                expected_depth = vct->depth;
+                expected_error = vct->error;
+                verify_cert(roots_dir, roots_file, bundle_file, bundle_file2, &chains, mode);
+                if (chains == 0 && vct->want_chains == 0) {
+                        if (seen_error != expected_error) {
+                                fprintf(stderr, "FAIL: expected error %d, got %d\n",
+                                    seen_error, expected_error);
+                                failed |= 1;
+                        }
+                        if (seen_depth != expected_depth) {
+                                fprintf(stderr, "FAIL: expected depth %d, got %d\n",
+                                    seen_depth, expected_depth);
+                                failed |= 1;
+                        }
+                }
+
+                if ((mode == MODE_VERIFY && chains == vct->want_chains) ||
+                    (chains == 0 && vct->want_chains == 0) ||
+                    (chains == 1 && vct->want_chains > 0)) {
+                        fprintf(stderr, "INFO: Succeeded with %d chains%s\n",
+                            chains, vct->failing ? " (legacy failure)" : "");
+                        if (mode == MODE_LEGACY_VFY && vct->failing)
+                                failed |= 1;
+                } else {
+                        fprintf(stderr, "FAIL: Failed with %d chains%s\n",
+                            chains, vct->failing ? " (legacy failure)" : "");
+                        if (!vct->failing)
+                                failed |= 1;
+                }
+                fprintf(stderr, "\n");
+
+                free(roots_file);
+                free(bundle_file);
+                free(bundle_file2);
+                free(roots_dir);
+        }
+
+        return failed;
+}
+
+int
+main(int argc, char **argv)
+{
+        int failed = 0;
+
+        if (argc != 2) {
+                fprintf(stderr, "usage: %s <certs_path>\n", argv[0]);
+                exit(1);
+        }
+
+        fprintf(stderr, "\n\nTesting legacy x509_vfy\n");
+        failed |= verify_cert_test(argv[1], MODE_LEGACY_VFY);
+        fprintf(stderr, "\n\nTesting modern x509_vfy\n");
+        failed |= verify_cert_test(argv[1], MODE_MODERN_VFY);
+
+        return (failed);
+}