On 2022-12-15 20:08 +01, Theo Buehler <[email protected]> wrote:
> 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.
> */
The SAN entries do not come from the command line, they come from the
config file.
>
> - 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)
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...
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.)
I played around with SANs, adding and removing them, and I couldn't
break it.
With the comment fixed: OK florian FWIW
> 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);
>
--
I'm not entirely sure you are real.
