From 0cffdb45a9bb573ce4665f5540d1a0d50ff2e37f Mon Sep 17 00:00:00 2001 From: tb Date: Sat, 17 Dec 2022 13:53:38 +0000 Subject: [PATCH] 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 --- usr.sbin/acme-client/revokeproc.c | 110 ++++++++++++------------------ 1 file 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 * @@ -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); -- 2.20.1