Gracefully handle root certificates being both trusted and untrusted.

When a certificate (namely a root) is specified as both a trusted and
untrusted certificate, the new verifier will find multiple chains - the
first being back to the trusted root certificate and a second via the root
that is untrusted, followed by the trusted root certificate. This situation
can be triggered by a server that (unnecessarily) includes the root
certificate in its certificate list.

While this validates correctly (using the first chain), it means that we
encounter a failure while building the second chain due to the root
certificate already being in the chain. When this occurs we call the verify
callback indicating a bad certificate. Some sensitive software (including
bacula and icinga), treat this single bad chain callback as terminal, even
though we successfully verify the certificate.

Avoid this problem by simply dumping the chain if we encounter a situation
where the certificate is already in the chain and also a trusted root -
we'll have already picked up the trusted root as a shorter path.

Issue with icinga2 initially reported by Theodore Wynnychenko.

Fix tested by sthen@ for both bacula and icinga2.

ok tb@

2 files changed, 16 insertions, 4 deletions

diff --git a/lib/libcrypto/x509/x509_internal.h b/lib/libcrypto/x509/x509_internal.h
index 2f2fe47a8f9..1ede7b6bad3 100644
--- a/lib/libcrypto/x509/x509_internal.h
+++ b/lib/libcrypto/x509/x509_internal.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: x509_internal.h,v 1.5 2020/11/18 17:00:59 tb Exp $ */
+/* $OpenBSD: x509_internal.h,v 1.6 2021/01/05 16:45:59 jsing Exp $ */
 /*
  * Copyright (c) 2020 Bob Beck <beck@openbsd.org>
  *
@@ -65,6 +65,7 @@ struct x509_verify_ctx {
 	X509_STORE_CTX *xsc;
 	struct x509_verify_chain **chains;	/* Validated chains */
 	size_t chains_count;
+	int dump_chain;			/* Dump current chain without erroring */
 	STACK_OF(X509) *roots;		/* Trusted roots for this validation */
 	STACK_OF(X509) *intermediates;	/* Intermediates provided by peer */
 	time_t *check_time;		/* Time for validity checks */
diff --git a/lib/libcrypto/x509/x509_verify.c b/lib/libcrypto/x509/x509_verify.c
index 88a7ef034d3..a5b41afb859 100644
--- a/lib/libcrypto/x509/x509_verify.c
+++ b/lib/libcrypto/x509/x509_verify.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: x509_verify.c,v 1.25 2020/12/16 18:46:29 tb Exp $ */
+/* $OpenBSD: x509_verify.c,v 1.26 2021/01/05 16:45:59 jsing Exp $ */
 /*
  * Copyright (c) 2020 Bob Beck <beck@openbsd.org>
  *
@@ -381,8 +381,18 @@ x509_verify_consider_candidate(struct x509_verify_ctx *ctx, X509 *cert,
 	/* Fail if the certificate is already in the chain */
 	for (i = 0; i < sk_X509_num(current_chain->certs); i++) {
 		if (X509_cmp(sk_X509_value(current_chain->certs, i),
-		    candidate) == 0)
+		    candidate) == 0) {
+			if (is_root_cert) {
+				/*
+				 * Someone made a boo-boo and put their root
+				 * in with their intermediates - handle this
+				 * gracefully as we'll have already picked
+				 * this up as a shorter chain.
+				 */
+				ctx->dump_chain = 1;
+			}
 			return 0;
+		}
 	}
 
 	if (ctx->sig_checks++ > X509_VERIFY_MAX_SIGCHECKS) {
@@ -475,6 +485,7 @@ x509_verify_build_chains(struct x509_verify_ctx *ctx, X509 *cert,
 		return;
 
 	count = ctx->chains_count;
+	ctx->dump_chain = 0;
 	ctx->error = X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY;
 	ctx->error_depth = depth;
 	if (ctx->xsc != NULL) {
@@ -528,7 +539,7 @@ x509_verify_build_chains(struct x509_verify_ctx *ctx, X509 *cert,
 			ctx->xsc->current_cert = cert;
 			(void) ctx->xsc->verify_cb(1, ctx->xsc);
 		}
-	} else if (ctx->error_depth == depth) {
+	} else if (ctx->error_depth == depth && !ctx->dump_chain) {
 		(void) x509_verify_cert_error(ctx, cert, depth,
 		    ctx->error, 0);
 	}