From: beck <beck@openbsd.org>
Date: Fri, 12 Jul 2024 18:15:10 +0000 (+0000)
Subject: Fix the horrible and undocumented behaviour of X509_check_trust
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=75e21034e2ed7cc3514816a96606fcbfe50e069f;p=openbsd

Fix the horrible and undocumented behaviour of X509_check_trust

Of allowing you to pass in a NID directly, instead of a trust_id,
and have it work, as long as the trust_id's and the NID's did not
overlap.

This screwball behaviour was depended upon by the OCSP code that
called X509_check_trust with the NID, instead of the trust id, so
let's fix that.

We also rename the confusingly named X509_TRUST_DEFAULT to
X509_TRUST_ACCEPT_ALL which makes a lot more sense, and rototill
this to remove the confusingly named static functions.

This will shortly be follwed up by making this function private,
so we have not bothered to fix the amazingly obtuse man page
as it will be taken behind the barn at that time.

ok tb@
---

diff --git a/lib/libcrypto/ocsp/ocsp_vfy.c b/lib/libcrypto/ocsp/ocsp_vfy.c
index d197fe4ea7b..27d2283ea70 100644
--- a/lib/libcrypto/ocsp/ocsp_vfy.c
+++ b/lib/libcrypto/ocsp/ocsp_vfy.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ocsp_vfy.c,v 1.23 2023/07/08 10:44:00 beck Exp $ */
+/* $OpenBSD: ocsp_vfy.c,v 1.24 2024/07/12 18:15:10 beck Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 2000.
  */
@@ -168,8 +168,8 @@ OCSP_basic_verify(OCSP_BASICRESP *bs, STACK_OF(X509) *certs, X509_STORE *st,
 			goto end;
 
 		x = sk_X509_value(chain, sk_X509_num(chain) - 1);
-		if (X509_check_trust(x, NID_OCSP_sign, 0) !=
-			X509_TRUST_TRUSTED) {
+		if (X509_check_trust(x, X509_TRUST_OCSP_SIGN, 0) !=
+		    X509_TRUST_TRUSTED) {
 			OCSPerror(OCSP_R_ROOT_CA_NOT_TRUSTED);
 			goto end;
 		}
diff --git a/lib/libcrypto/x509/x509_local.h b/lib/libcrypto/x509/x509_local.h
index 5b74b0d1bda..6b72678e7a2 100644
--- a/lib/libcrypto/x509/x509_local.h
+++ b/lib/libcrypto/x509/x509_local.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: x509_local.h,v 1.24 2024/04/08 23:46:21 beck Exp $ */
+/*	$OpenBSD: x509_local.h,v 1.25 2024/07/12 18:15:10 beck Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 2013.
  */
@@ -71,6 +71,14 @@ __BEGIN_HIDDEN_DECLS
 #define X509_CRL_HASH_EVP	EVP_sha512()
 #define X509_CRL_HASH_LEN	SHA512_DIGEST_LENGTH
 
+/*
+ * Used internally instead of the confusing X509_TRUST_DEFAULT,
+ * which is not the default for X509_check_trust.
+ * XXX Make X509_check_trust internal, and move the other
+ * X509_TRUST values here to clean up this mess.
+ */
+#define X509_TRUST_ACCEPT_ALL	-1
+
 struct X509_pubkey_st {
 	X509_ALGOR *algor;
 	ASN1_BIT_STRING *public_key;
diff --git a/lib/libcrypto/x509/x509_purp.c b/lib/libcrypto/x509/x509_purp.c
index d2e9277013b..619a4b890ab 100644
--- a/lib/libcrypto/x509/x509_purp.c
+++ b/lib/libcrypto/x509/x509_purp.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: x509_purp.c,v 1.42 2024/05/15 18:10:03 tb Exp $ */
+/* $OpenBSD: x509_purp.c,v 1.43 2024/07/12 18:15:10 beck Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 2001.
  */
@@ -150,7 +150,7 @@ static const X509_PURPOSE xstandard[] = {
 	},
 	{
 		.purpose = X509_PURPOSE_ANY,
-		.trust = X509_TRUST_DEFAULT,
+		.trust = X509_TRUST_ACCEPT_ALL,
 		.check_purpose = no_check,
 		.name = "Any Purpose",
 		.sname = "any",
diff --git a/lib/libcrypto/x509/x509_trs.c b/lib/libcrypto/x509/x509_trs.c
index 78eb29555ed..9ba8194ee0d 100644
--- a/lib/libcrypto/x509/x509_trs.c
+++ b/lib/libcrypto/x509/x509_trs.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: x509_trs.c,v 1.56 2024/07/12 15:53:51 beck Exp $ */
+/* $OpenBSD: x509_trs.c,v 1.57 2024/07/12 18:15:10 beck Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 1999.
  */
@@ -66,6 +66,23 @@
 #include "x509_internal.h"
 #include "x509_local.h"
 
+static int
+trust_if_self_signed(const X509 *x)
+{
+	/* Extensions already cached in X509_check_trust(). */
+	if ((x->ex_flags & EXFLAG_SS) != 0)
+		return X509_TRUST_TRUSTED;
+
+	return X509_TRUST_UNTRUSTED;
+}
+
+static int
+trust_was_set(const X509 *x)
+{
+	return x->aux != NULL && (x->aux->trust != NULL ||
+	    x->aux->reject != NULL);
+}
+
 static int
 obj_trust(int id, const X509 *x)
 {
@@ -94,33 +111,31 @@ obj_trust(int id, const X509 *x)
 }
 
 static int
-trust_if_self_signed(const X509 *x)
-{
-	/* Extensions already cached in X509_check_trust(). */
-	if ((x->ex_flags & EXFLAG_SS) != 0)
-		return X509_TRUST_TRUSTED;
-
-	return X509_TRUST_UNTRUSTED;
-}
-
-static int
-trust_1oidany(int nid, const X509 *x)
+nid_from_trust_id(int trust_id)
 {
-	/* Inspect the certificate's trust settings if there are any. */
-	if (x->aux != NULL && (x->aux->trust != NULL || x->aux->reject != NULL))
-		return obj_trust(nid, x);
-
-	/* For compatibility we return trusted if the cert is self signed. */
-	return trust_if_self_signed(x);
-}
+	OPENSSL_assert(trust_id == 0 ||
+	    (trust_id >= X509_TRUST_MIN && trust_id <= X509_TRUST_MAX));
 
-static int
-trust_1oid(int nid, const X509 *x)
-{
-	if (x->aux != NULL)
-		return obj_trust(nid, x);
-
-	return X509_TRUST_UNTRUSTED;
+	switch (trust_id) {
+	case X509_TRUST_COMPAT:
+		return NID_undef;
+	case X509_TRUST_SSL_CLIENT:
+		return NID_client_auth;
+	case X509_TRUST_SSL_SERVER:
+		return NID_server_auth;
+	case X509_TRUST_EMAIL:
+		return NID_email_protect;
+	case X509_TRUST_OBJECT_SIGN:
+		return NID_code_sign;
+	case X509_TRUST_OCSP_SIGN:
+		return NID_OCSP_sign;
+	case X509_TRUST_OCSP_REQUEST:
+		return NID_ad_OCSP;
+	case X509_TRUST_TSA:
+		return NID_time_stamp;
+	default:
+		return NID_undef;
+	}
 }
 
 int
@@ -128,40 +143,36 @@ X509_check_trust(X509 *x, int trust_id, int flags)
 {
 	int rv;
 
-	if (trust_id == -1)
-		return 1;
-
 	/* Call early so the trust handlers don't need to modify the certs. */
 	if (!x509v3_cache_extensions(x))
 		return X509_TRUST_UNTRUSTED;
 
+	/*
+	 * XXX make X509_TRUST_ACCEPT_ALL a real boy once it does not
+	 * need to have the same -1 value as X509_TRUST_DEFAULT
+	 */
+	if (trust_id == X509_TRUST_ACCEPT_ALL)
+		return 1;
+
 	switch (trust_id) {
-	case 0: /*
-		 * The default behaviour: If the certificate has EKU any, or it
-		 * is self-signed, it is trusted. Otherwise it is untrusted.
-		 */
-		rv = obj_trust(NID_anyExtendedKeyUsage, x);
-		if (rv != X509_TRUST_UNTRUSTED)
-			return rv;
-		return trust_if_self_signed(x);
 	case X509_TRUST_COMPAT:
 		return trust_if_self_signed(x);
-	case X509_TRUST_SSL_CLIENT:
-		return trust_1oidany(NID_client_auth, x);
-	case X509_TRUST_SSL_SERVER:
-		return trust_1oidany(NID_server_auth, x);
 	case X509_TRUST_EMAIL:
-		return trust_1oidany(NID_email_protect, x);
 	case X509_TRUST_OBJECT_SIGN:
-		return trust_1oidany(NID_code_sign, x);
+	case X509_TRUST_SSL_SERVER:
+	case X509_TRUST_SSL_CLIENT:
+	case X509_TRUST_TSA:
+		if (trust_was_set(x))
+			return obj_trust(nid_from_trust_id(trust_id), x);
+		return trust_if_self_signed(x);
 	case X509_TRUST_OCSP_SIGN:
-		return trust_1oid(NID_OCSP_sign, x);
 	case X509_TRUST_OCSP_REQUEST:
-		return trust_1oid(NID_ad_OCSP, x);
-	case X509_TRUST_TSA:
-		return trust_1oidany(NID_time_stamp, x);
+		return obj_trust(nid_from_trust_id(trust_id), x);
 	default:
-		return obj_trust(trust_id, x);
+		rv = obj_trust(NID_anyExtendedKeyUsage, x);
+		if (rv != X509_TRUST_UNTRUSTED)
+			return rv;
+		return trust_if_self_signed(x);
 	}
 }
 LCRYPTO_ALIAS(X509_check_trust);