> > - * 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.
> > */
>
> The SAN entries do not come from the command line, they come from the
> config file.
Ah yes, it raised an eyebrow but then I forgot. Changed to
* Ensure the certificate's SAN entries fully cover those from the
* configuration file and that all domains are represented only once.
> > + 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)
>
> This one tripped me up. Would it make sense to turn name_buf into a
> proper C-string? Maybe this is normal for people who deal with this all
> the time. I'm sure I will get this wrong...
I added a comment
/* name_buf is not a string. There could be embedded NULs. */
name_buf = ASN1_STRING_get0_data(name);
> Anyway, I checked the documentation, and it gave me this:
>
> RETURN VALUES
> ASN1_STRING_data() and ASN1_STRING_get0_data() return an internal pointer
> to the data of x.
>
> So that's cool. (Yes, yes, it documents in a different place that it's
> not a C-string.)
Yes, I tried to hint at that by calling it _buf. Arguably it's wrong to
call strncmp(). Using memcmp() makes it more explicit that name_buf
is not a string. I'll switch to that.
I really, really want to avoid any additional complications from
allocations as this function is already more than hairy enough.
> I played around with SANs, adding and removing them, and I couldn't
> break it.
Thanks. I had several positive test reports (thanks all), so I think I
didn't mess anything obvious up.
This is the diff I have in my tree now. I'll give jsing and beck a
few days for speaking.
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 16 Dec 2022 13:28:42 -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. */
-
- 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;
- }
- }
+ /* Extract list of SAN entries from the certificate. */
- 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,
}
/*
- * 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;
+ 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 is not a string. There could be 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: unknown SAN entry: %.*s",
+ certfile, name_len, name_buf);
goto out;
}
force = 2;
@@ -210,7 +187,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 +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);
