From: beck <beck@openbsd.org>
Date: Wed, 24 Nov 2021 05:38:12 +0000 (+0000)
Subject: In some situations, the verifier would discard the error on an unvalidated
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=3f851282810fa0ab4b90b3b1ecec2e8717ef16f8;p=openbsd

In some situations, the verifier would discard the error on an unvalidated
certificte chain. This would happen when the verification callback was
in use, instructing the verifier to continue unconditionally. This could
lead to incorrect decisions being made in software.
---

diff --git a/lib/libcrypto/x509/x509_internal.h b/lib/libcrypto/x509/x509_internal.h
index a9b584b13e1..9ac0c2bbde1 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.15 2021/11/04 23:52:34 beck Exp $ */
+/* $OpenBSD: x509_internal.h,v 1.16 2021/11/24 05:38:12 beck Exp $ */
 /*
  * Copyright (c) 2020 Bob Beck <beck@openbsd.org>
  *
@@ -92,7 +92,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);
+int x509_vfy_callback_indicate_completion(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 e7493fdbf05..6a73cb74e25 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.53 2021/11/14 08:21:47 jsing Exp $ */
+/* $OpenBSD: x509_verify.c,v 1.54 2021/11/24 05:38:12 beck Exp $ */
 /*
  * Copyright (c) 2020-2021 Bob Beck <beck@openbsd.org>
  *
@@ -1164,62 +1164,99 @@ x509_verify(struct x509_verify_ctx *ctx, X509 *leaf, char *name)
 	x509_verify_chain_free(current_chain);
 
 	/*
-	 * Bring back the failure case we wanted to the xsc if
-	 * we saved one.
+	 * Do the new verifier style return, where we don't have an xsc
+	 * that allows a crazy callback to turn invalid things into valid.
 	 */
-	if (!x509_verify_ctx_restore_xsc_error(ctx))
-		goto err;
+	if (ctx->xsc == NULL) {
+		/*
+		 * Safety net:
+		 * We could not find a validated chain, and for some reason do not
+		 * have an error set.
+		 */
+		if (ctx->chains_count == 0 && ctx->error == X509_V_OK)
+			ctx->error = X509_V_ERR_UNSPECIFIED;
+
+		/*
+		 * If we are not using an xsc, and have no possibility for the
+		 * crazy OpenSSL callback API changing the results of
+		 * validation steps (because the callback can make validation
+		 * proceed in the presence of invalid certs), any chains we
+		 * have here are correctly built and verified.
+		 */
+		if (ctx->chains_count > 0)
+			ctx->error = X509_V_OK;
+
+		return ctx->chains_count;
+	}
 
 	/*
-	 * Safety net:
-	 * We could not find a validated chain, and for some reason do not
-	 * have an error set.
+	 * Otherwise we are doing compatibility with an xsc, which means that we
+	 * will have one chain, which might actually be a bogus chain because
+	 * the callback told us to ignore errors and proceed to build an invalid
+	 * chain. Possible return values from this include returning 1 with an
+	 * invalid chain and a value of xsc->error != X509_V_OK (It's tradition
+	 * that makes it ok).
 	 */
-	if (ctx->chains_count == 0 && ctx->error == X509_V_OK) {
-		ctx->error = X509_V_ERR_UNSPECIFIED;
-		if (ctx->xsc != NULL && ctx->xsc->error != X509_V_OK)
-			ctx->error = ctx->xsc->error;
-	}
 
-	/* Clear whatever errors happened if we have any validated chain */
-	if (ctx->chains_count > 0)
-		ctx->error = X509_V_OK;
+	if (ctx->chains_count > 0) {
+		/*
+		 * The chain we have using an xsc might not be a verified chain
+		 * if the callback perverted things while we built it to ignore
+		 * failures and proceed with chain building. We put this chain
+		 * and the error associated with it on the xsc.
+		 */
+		if (!x509_verify_ctx_set_xsc_chain(ctx, ctx->chains[0], 1, 1))
+			goto err;
 
-	if (ctx->xsc != NULL) {
-		ctx->xsc->error = ctx->error;
-		if (ctx->chains_count > 0) {
-			/* Take the first chain we found. */
-			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)) {
-				/* The callback can change the error code */
-				ctx->error = ctx->xsc->error;
-				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;
-			}
+		/*
+		 * Call the callback for completion up our built
+		 * chain. The callback could still tell us to
+		 * fail. Since this chain might exist as the result of
+		 * callback doing perversions, we could still return
+		 * "success" with something other than X509_V_OK set
+		 * as the error.
+		 */
+		if (!x509_vfy_callback_indicate_completion(ctx->xsc))
+			goto err;
+	} else {
+		/*
+		 * We did not find a chain. Bring back the failure
+		 * case we wanted to the xsc if we saved one. If we
+		 * did not we should have just the leaf on the xsc.
+		 */
+		if (!x509_verify_ctx_restore_xsc_error(ctx))
+			goto err;
+
+		/*
+		 * Safety net, ensure we have an error set in the
+		 * failing case.
+		 */
+		if (ctx->xsc->error == X509_V_OK) {
+			if (ctx->error == X509_V_OK)
+				ctx->error = X509_V_ERR_UNSPECIFIED;
+			ctx->xsc->error = ctx->error;
 		}
+
+		/*
+		 * Let the callback override the return value
+		 * at depth 0 if it chooses to
+		 */
+		return ctx->xsc->verify_cb(0, ctx->xsc);
 	}
-	return (ctx->chains_count);
+
+	/* We only ever find one chain in compat mode with an xsc. */
+	return 1;
 
  err:
 	if (ctx->error == X509_V_OK)
 		ctx->error = X509_V_ERR_UNSPECIFIED;
-	if (ctx->xsc != NULL)
-		ctx->xsc->error = ctx->error;
+
+	if (ctx->xsc != NULL) {
+		if (ctx->xsc->error == X509_V_OK)
+			ctx->xsc->error = X509_V_ERR_UNSPECIFIED;
+		ctx->error = ctx->xsc->error;
+	}
+
 	return 0;
 }
+
diff --git a/lib/libcrypto/x509/x509_vfy.c b/lib/libcrypto/x509/x509_vfy.c
index b044f4931e9..db2125b48d7 100644
--- a/lib/libcrypto/x509/x509_vfy.c
+++ b/lib/libcrypto/x509/x509_vfy.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: x509_vfy.c,v 1.97 2021/11/13 18:24:45 schwarze Exp $ */
+/* $OpenBSD: x509_vfy.c,v 1.98 2021/11/24 05:38:12 beck Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -1989,8 +1989,12 @@ internal_verify(X509_STORE_CTX *ctx)
 	return x509_vfy_internal_verify(ctx, 0);
 }
 
+/*
+ * Internal verify, but with a chain where the verification
+ * math has already been performed.
+ */
 int
-x509_vfy_callback_indicate_success(X509_STORE_CTX *ctx)
+x509_vfy_callback_indicate_completion(X509_STORE_CTX *ctx)
 {
 	return x509_vfy_internal_verify(ctx, 1);
 }