I would appreciate some testing by people who actually use acme-client
with multiple SANs. The diff works for me and should not change any
important behavior.
When I learned about CVE-2021-44532 in node, I was horrified, but oh,
well, it was node. Little did I suspect that acme-client did something
rather similar (I had even touched some of this). It's also not really
security relevant here. Still gross.
Instead of having libcrypto turn the SANs into an undocumented string
(via X509V3_EXT_print and i2v_GENERAL_NAMES if you really need to know),
then tokenizing and parsing that, simply grab the relevant info out of
libcrypto. In detail:
1. Cache extensions up front. This ensures that the cert has at least
somewhat good extensions, in particular most extensions including the
SAN can't occur more than once. This trick is standard but not well
documented (I have sent out a diff for this to the relevant parties).
2. Let libcrypto walk the extensions and hand us the list of SANs rather
than finding the SANs by hand. We then have a data structure to work
with, so the first for loop can go.
3. Extract the DNS names and compare them to the list passed in.
What lands in name_buf is exactly what's printed after "DNS:",
so we can minimally modify the second for loop. I do not want to
rely on ASN1_IA5STRING being NUL-terminated (they currently are),
hence the additional length check and the switch from %s to %.*s.
Finally, I have added two comments that suggest using strnvis().
libcrypto doesn't do that, but at this point the cert has been
parsed from disk but we don't know whether the SANs ever went
through x509_constraints_valid_sandns() or similar successfully.
Anyway, that's better left to a follow up.
Index: revokeproc.c
===================================================================
RCS file: /cvs/src/usr.sbin/acme-client/revokeproc.c,v
retrieving revision 1.23
diff -u -p -r1.23 revokeproc.c
--- revokeproc.c 15 Dec 2022 17:36:56 -0000 1.23
+++ revokeproc.c 15 Dec 2022 17:38:18 -0000
@@ -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,
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,
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,38 @@ revokeproc(int fd, const char *certfile,
}
/*
- * Parse the SAN line.
- * Make sure that all of the domains are represented only once.
+ * Ensure the certificate's SAN entries fully cover those passed on
+ * the command line 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;
+ ASN1_IA5STRING *name;
+ const unsigned char *name_buf;
+ int name_len;
+ int ntype;
+
+ gen_name = sk_GENERAL_NAME_value(sans, i);
+ assert(gen_name != NULL);
+
+ name = GENERAL_NAME_get0_value(gen_name, &ntype);
+ if (ntype != GEN_DNS)
continue;
- tok += 4;
- for (j = 0; j < altsz; j++)
- if (strcmp(tok, alts[j]) == 0)
+
+ 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 (strncmp(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: unknown SAN entry: %.*s",
+ certfile, name_len, name_buf);
goto out;
}
force = 2;
@@ -210,7 +186,9 @@ revokeproc(int fd, const char *certfile,
}
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 +288,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);
