Move leaf certificate checks to the last thing after chain validation.

While seemingly illogical and not what is done in Go's validator, this
mimics OpenSSL's behavior so that callback overrides for the expiry of
a certificate will not "sticky" override a failure to build a chain.

ok jsing@

3 files changed, 317 insertions, 20 deletions

diff --git a/src/lib/libcrypto/x509/x509_verify.c b/src/lib/libcrypto/x509/x509_verify.c
index 630c9f9b5a..f6959d1f3a 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.55 2022/04/12 10:42:35 tb Exp $ */
+/* $OpenBSD: x509_verify.c,v 1.56 2022/06/25 20:01:43 beck Exp $ */
 /*
  * Copyright (c) 2020-2021 Bob Beck <beck@openbsd.org>
  *
@@ -32,8 +32,10 @@
 
 static int x509_verify_cert_valid(struct x509_verify_ctx *ctx, X509 *cert,
     struct x509_verify_chain *current_chain);
+static int x509_verify_cert_hostname(struct x509_verify_ctx *ctx, X509 *cert,
+    char *name);
 static void x509_verify_build_chains(struct x509_verify_ctx *ctx, X509 *cert,
-    struct x509_verify_chain *current_chain, int full_chain);
+    struct x509_verify_chain *current_chain, int full_chain, char *name);
 static int x509_verify_cert_error(struct x509_verify_ctx *ctx, X509 *cert,
     size_t depth, int error, int ok);
 static void x509_verify_chain_free(struct x509_verify_chain *chain);
@@ -233,7 +235,7 @@ x509_verify_ctx_clear(struct x509_verify_ctx *ctx)
         x509_verify_ctx_reset(ctx);
         sk_X509_pop_free(ctx->intermediates, X509_free);
         free(ctx->chains);
-        memset(ctx, 0, sizeof(*ctx));
+
 }
 
 static int
@@ -453,10 +455,11 @@ x509_verify_ctx_validate_legacy_chain(struct x509_verify_ctx *ctx,
 /* Add a validated chain to our list of valid chains */
 static int
 x509_verify_ctx_add_chain(struct x509_verify_ctx *ctx,
-    struct x509_verify_chain *chain)
+    struct x509_verify_chain *chain, char *name)
 {
         size_t depth;
         X509 *last = x509_verify_chain_last(chain);
+        X509 *leaf = x509_verify_chain_leaf(chain);
 
         depth = sk_X509_num(chain->certs);
         if (depth > 0)
@@ -488,6 +491,13 @@ x509_verify_ctx_add_chain(struct x509_verify_ctx *ctx,
                 return x509_verify_cert_error(ctx, last, depth,
                     X509_V_ERR_OUT_OF_MEM, 0);
         }
+
+        if (!x509_verify_cert_valid(ctx, leaf, NULL))
+                return 0;
+
+        if (!x509_verify_cert_hostname(ctx, leaf, name))
+                return 0;
+
         ctx->chains_count++;
         ctx->error = X509_V_OK;
         ctx->error_depth = depth;
@@ -539,7 +549,7 @@ x509_verify_parent_signature(X509 *parent, X509 *child, int *error)
 static int
 x509_verify_consider_candidate(struct x509_verify_ctx *ctx, X509 *cert,
     int is_root_cert, X509 *candidate, struct x509_verify_chain *current_chain,
-    int full_chain)
+    int full_chain, char *name)
 {
         int depth = sk_X509_num(current_chain->certs);
         struct x509_verify_chain *new_chain;
@@ -590,14 +600,14 @@ x509_verify_consider_candidate(struct x509_verify_ctx *ctx, X509 *cert,
                         x509_verify_chain_free(new_chain);
                         return 0;
                 }
-                if (!x509_verify_ctx_add_chain(ctx, new_chain)) {
+                if (!x509_verify_ctx_add_chain(ctx, new_chain, name)) {
                         x509_verify_chain_free(new_chain);
                         return 0;
                 }
                 goto done;
         }
 
-        x509_verify_build_chains(ctx, candidate, new_chain, full_chain);
+        x509_verify_build_chains(ctx, candidate, new_chain, full_chain, name);
 
  done:
         x509_verify_chain_free(new_chain);
@@ -621,7 +631,7 @@ x509_verify_cert_error(struct x509_verify_ctx *ctx, X509 *cert, size_t depth,
 
 static void
 x509_verify_build_chains(struct x509_verify_ctx *ctx, X509 *cert,
-    struct x509_verify_chain *current_chain, int full_chain)
+    struct x509_verify_chain *current_chain, int full_chain, char *name)
 {
         X509 *candidate;
         int i, depth, count, ret, is_root;
@@ -679,7 +689,7 @@ x509_verify_build_chains(struct x509_verify_ctx *ctx, X509 *cert,
                                     x509_verify_cert_self_signed(candidate);
                                 x509_verify_consider_candidate(ctx, cert,
                                     is_root, candidate, current_chain,
-                                    full_chain);
+                                    full_chain, name);
                         }
                         X509_free(candidate);
                 }
@@ -692,7 +702,7 @@ x509_verify_build_chains(struct x509_verify_ctx *ctx, X509 *cert,
                                     x509_verify_cert_self_signed(candidate);
                                 x509_verify_consider_candidate(ctx, cert,
                                     is_root, candidate, current_chain,
-                                    full_chain);
+                                    full_chain, name);
                         }
                 }
         }
@@ -704,7 +714,7 @@ x509_verify_build_chains(struct x509_verify_ctx *ctx, X509 *cert,
                         if (x509_verify_potential_parent(ctx, candidate, cert)) {
                                 x509_verify_consider_candidate(ctx, cert,
                                     0, candidate, current_chain,
-                                    full_chain);
+                                    full_chain, name);
                         }
                 }
         }
@@ -1116,16 +1126,18 @@ x509_verify(struct x509_verify_ctx *ctx, X509 *leaf, char *name)
                 ctx->xsc->current_cert = leaf;
         }
 
-        if (!x509_verify_cert_valid(ctx, leaf, NULL))
-                goto err;
-
-        if (!x509_verify_cert_hostname(ctx, leaf, name))
-                goto err;
-
         if ((current_chain = x509_verify_chain_new()) == NULL) {
                 ctx->error = X509_V_ERR_OUT_OF_MEM;
                 goto err;
         }
+
+        /*
+         * Add the leaf to the chain and try to build chains from it.
+         * Note that unlike Go's verifier, we have not yet checked
+         * anything about the leaf, This is intentional, so that we
+         * report failures in chain building before we report problems
+         * with the leaf.
+         */
         if (!x509_verify_chain_append(current_chain, leaf, &ctx->error)) {
                 x509_verify_chain_free(current_chain);
                 goto err;
@@ -1133,13 +1145,14 @@ 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)) {
-                        if (!x509_verify_ctx_add_chain(ctx, current_chain)) {
+                        if (!x509_verify_ctx_add_chain(ctx, current_chain,
+                            name)) {
                                 x509_verify_chain_free(current_chain);
                                 goto err;
                         }
                 } else {
                         x509_verify_build_chains(ctx, leaf, current_chain,
-                            full_chain);
+                            full_chain, name);
                         if (full_chain && ctx->chains_count == 0) {
                                 /*
                                  * Save the error state from the xsc
diff --git a/src/regress/lib/libcrypto/x509/Makefile b/src/regress/lib/libcrypto/x509/Makefile
index 919bef31d1..ca66df19cd 100644
--- a/src/regress/lib/libcrypto/x509/Makefile
+++ b/src/regress/lib/libcrypto/x509/Makefile
@@ -1,6 +1,7 @@
-#       $OpenBSD: Makefile,v 1.12 2022/06/02 12:08:41 tb Exp $
+#       $OpenBSD: Makefile,v 1.13 2022/06/25 20:01:43 beck Exp $
 
 PROGS = constraints verify x509attribute x509name x509req_ext callback
+PROGS += expirecallback
 LDADD = -lcrypto
 DPADD = ${LIBCRYPTO}
 
@@ -18,6 +19,7 @@ REGRESS_TARGETS += regress-x509attribute
 REGRESS_TARGETS += regress-x509name
 REGRESS_TARGETS += regress-x509req_ext
 REGRESS_TARGETS += regress-callback
+REGRESS_TARGETS += regress-expirecallback
 
 CLEANFILES +=   x509name.result callbackout
 
@@ -49,4 +51,7 @@ regress-callback: callback
         ./callback ${.CURDIR}/../certs
         perl ${.CURDIR}/callback.pl callback.out
 
+regress-expirecallback: expirecallback
+        ./expirecallback ${.CURDIR}/../certs
+
 .include <bsd.regress.mk>
diff --git a/src/regress/lib/libcrypto/x509/expirecallback.c b/src/regress/lib/libcrypto/x509/expirecallback.c
new file mode 100644
index 0000000000..3da49708ff
--- /dev/null
+++ b/src/regress/lib/libcrypto/x509/expirecallback.c
@@ -0,0 +1,279 @@
+/* $OpenBSD: expirecallback.c,v 1.1 2022/06/25 20:01:43 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
+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)
+{
+        STACK_OF(X509_INFO) *xis = NULL;
+        STACK_OF(X509) *xs = NULL;
+        BIO *bio = NULL;
+        X509 *x;
+        int i;
+
+        if ((xs = sk_X509_new_null()) == NULL)
+                errx(1, "failed to create X509 stack");
+        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) {
+                if (verify_err == X509_V_ERR_CERT_HAS_EXPIRED)
+                        fprintf(stderr, "IGNORING ");
+                fprintf(stderr, "verify error at depth %d: %s\n",
+                    X509_STORE_CTX_get_error_depth(xsc),
+                    X509_verify_cert_error_string(verify_err));
+        }
+
+        /*
+         * Ignore expired certs, in the way people are told to do it
+         * by OpenSSL
+         */
+
+        if (verify_err == X509_V_ERR_CERT_HAS_EXPIRED)
+                return 1;
+
+        return ok;
+}
+
+static void
+verify_cert(const char *roots_dir, const char *roots_file,
+    const char *bundle_file, int *chains, int mode)
+{
+        STACK_OF(X509) *roots = NULL, *bundle = NULL;
+        time_t future = 2000000000; /* May 17 2033 */
+        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))
+                errx(1, "failed to load roots from '%s'", roots_file);
+        if (!certs_from_file(bundle_file, &bundle))
+                errx(1, "failed to load bundle from '%s'", bundle_file);
+        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");
+        }
+
+        /*
+         * Set the time int the future to exercise the expired cert
+         * callback
+         */
+        X509_STORE_CTX_set_time(xsc, 0, future);
+
+        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;
+};
+
+struct verify_cert_test verify_cert_tests[] = {
+        {
+                .id = "1a",
+                .want_chains = 1,
+        },
+        {
+                .id = "2a",
+                .want_chains = 1,
+                .failing = 1,
+        },
+        {
+                .id = "2b",
+                .want_chains = 0,
+        },
+        {
+                .id = "2c",
+                .want_chains = 1,
+                .failing = 1,
+        },
+};
+
+#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, *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, "%s/%s/roots.pem", certs_path,
+                    vct->id) == -1)
+                        errx(1, "asprintf");
+                if (asprintf(&bundle_file, "%s/%s/bundle.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);
+                verify_cert(roots_dir, roots_file, bundle_file, &chains, mode);
+                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(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);
+        fprintf(stderr, "\n\nTesting modern x509_vfy by_dir\n");
+        failed |= verify_cert_test(argv[1], MODE_MODERN_VFY_DIR);
+
+        return (failed);
+}