acme-client: fix SAN-handling insanity

The revoke process, which does a lot more than revoking a cert, wants to
know the SANs in the cert to be revoked or renewed and check them against
the ones configured in the config file.

To find out which ones are, it prints the SAN extension to a BIO using
X509V3_EXT_print(), slurps that into a buffer, tokenizes the undocumented
output string and plucks out the "DNS:" names. This is reminiscent of
node's hilarious CVE-2021-44532 and on about the same level of crazy, but
fortunately not security relevant.

Get the SAN extension as a GENERAL_NAMES from libcrypto, then we have an
actual data structure to work with, which allows us to access the DNS names
without problems. This simplifies things quite a bit, but the actual logic
in this file remains unmodified. Be careful about ASN1_IA5STRINGs and do
not assume they are C strings.

Tested by florian, millert, Renaud Allard, thanks!

ok florian jsing

1 files changed, 44 insertions, 66 deletions

diff --git a/usr.sbin/acme-client/revokeproc.c b/usr.sbin/acme-client/revokeproc.c
index 87b3779f468..a9e2df6211f 100644
--- a/usr.sbin/acme-client/revokeproc.c
+++ b/usr.sbin/acme-client/revokeproc.c
@@ -1,4 +1,4 @@
-/*	$Id: revokeproc.c,v 1.23 2022/12/15 17:36:56 tb Exp $ */
+/*	$Id: revokeproc.c,v 1.24 2022/12/17 13:53:38 tb Exp $ */
 /*
  * Copyright (c) 2016 Kristaps Dzonsons <kristaps@bsd.lv>
  *
@@ -61,19 +61,15 @@ int
 revokeproc(int fd, const char *certfile, int force,
     int revocate, const char *const *alts, size_t altsz)
 {
+	GENERAL_NAMES			*sans = NULL;
 	char				*der = NULL, *dercp, *der64 = NULL;
-	char				*san = NULL, *str, *tok;
-	int				 rc = 0, cc, i, ssz, len;
+	int				 rc = 0, cc, i, len;
 	size_t				*found = NULL;
-	BIO				*bio = NULL;
 	FILE				*f = NULL;
 	X509				*x = NULL;
 	long				 lval;
 	enum revokeop			 op, rop;
 	time_t				 t;
-	const STACK_OF(X509_EXTENSION)	*exts;
-	X509_EXTENSION			*ex;
-	ASN1_OBJECT			*obj;
 	size_t				 j;
 
 	/*
@@ -119,6 +115,13 @@ revokeproc(int fd, const char *certfile, int force,
 		goto out;
 	}
 
+	/* Cache and sanity check X509v3 extensions. */
+
+	if (X509_check_purpose(x, -1, -1) <= 0) {
+		warnx("%s: invalid X509v3 extensions", certfile);
+		goto out;
+	}
+
 	/* Read out the expiration date. */
 
 	if ((t = X509expires(x)) == -1) {
@@ -126,50 +129,10 @@ revokeproc(int fd, const char *certfile, int force,
 		goto out;
 	}
 
-	/*
-	 * Next, the long process to make sure that the SAN entries
-	 * listed with the certificate fully cover those passed on the
-	 * command line.
-	 */
-
-	exts = X509_get0_extensions(x);
-
-	/* Scan til we find the SAN NID. */
+	/* Extract list of SAN entries from the certificate. */
 
-	for (i = 0; i < sk_X509_EXTENSION_num(exts); i++) {
-		ex = sk_X509_EXTENSION_value(exts, i);
-		assert(ex != NULL);
-		obj = X509_EXTENSION_get_object(ex);
-		assert(obj != NULL);
-		if (NID_subject_alt_name != OBJ_obj2nid(obj))
-			continue;
-
-		if (san != NULL) {
-			warnx("%s: two SAN entries", certfile);
-			goto out;
-		}
-
-		bio = BIO_new(BIO_s_mem());
-		if (bio == NULL) {
-			warnx("BIO_new");
-			goto out;
-		}
-		if (!X509V3_EXT_print(bio, ex, 0, 0)) {
-			warnx("X509V3_EXT_print");
-			goto out;
-		}
-		if ((san = calloc(1, BIO_number_written(bio) + 1)) == NULL) {
-			warn("calloc");
-			goto out;
-		}
-		ssz = BIO_read(bio, san, BIO_number_written(bio));
-		if (ssz < 0 || (unsigned)ssz != BIO_number_written(bio)) {
-			warnx("BIO_read");
-			goto out;
-		}
-	}
-
-	if (san == NULL) {
+	sans = X509_get_ext_d2i(x, NID_subject_alt_name, NULL, NULL);
+	if (sans == NULL) {
 		warnx("%s: does not have a SAN entry", certfile);
 		if (revocate)
 			goto out;
@@ -184,25 +147,39 @@ revokeproc(int fd, const char *certfile, int force,
 	}
 
 	/*
-	 * Parse the SAN line.
-	 * Make sure that all of the domains are represented only once.
+	 * Ensure the certificate's SAN entries fully cover those from the
+	 * configuration file and that all domains are represented only once.
 	 */
 
-	str = san;
-	while ((tok = strsep(&str, ",")) != NULL) {
-		if (*tok == '\0')
-			continue;
-		while (isspace((unsigned char)*tok))
-			tok++;
-		if (strncmp(tok, "DNS:", 4))
+	for (i = 0; i < sk_GENERAL_NAME_num(sans); i++) {
+		GENERAL_NAME		*gen_name;
+		const ASN1_IA5STRING	*name;
+		const unsigned char	*name_buf;
+		int			 name_len;
+		int			 name_type;
+
+		gen_name = sk_GENERAL_NAME_value(sans, i);
+		assert(gen_name != NULL);
+
+		name = GENERAL_NAME_get0_value(gen_name, &name_type);
+		if (name_type != GEN_DNS)
 			continue;
-		tok += 4;
-		for (j = 0; j < altsz; j++)
-			if (strcmp(tok, alts[j]) == 0)
+
+		/* name_buf isn't a C string and could contain embedded NULs. */
+		name_buf = ASN1_STRING_get0_data(name);
+		name_len = ASN1_STRING_length(name);
+
+		for (j = 0; j < altsz; j++) {
+			if ((size_t)name_len != strlen(alts[j]))
+				continue;
+			if (memcmp(name_buf, alts[j], name_len) == 0)
 				break;
+		}
 		if (j == altsz) {
 			if (revocate) {
-				warnx("%s: unknown SAN entry: %s", certfile, tok);
+				/* XXX strnvis? */
+				warnx("%s: unexpected SAN entry: %.*s",
+				    certfile, name_len, name_buf);
 				goto out;
 			}
 			force = 2;
@@ -210,7 +187,9 @@ revokeproc(int fd, const char *certfile, int force,
 		}
 		if (found[j]++) {
 			if (revocate) {
-				warnx("%s: duplicate SAN entry: %s", certfile, tok);
+				/* XXX strnvis? */
+				warnx("%s: duplicate SAN entry: %.*s",
+				    certfile, name_len, name_buf);
 				goto out;
 			}
 			force = 2;
@@ -310,8 +289,7 @@ out:
 	if (f != NULL)
 		fclose(f);
 	X509_free(x);
-	BIO_free(bio);
-	free(san);
+	GENERAL_NAMES_free(sans);
 	free(der);
 	free(found);
 	free(der64);