On Wed, Mar 02, 2022 at 04:38:46PM +1000, Alex Wilson wrote:
> I've been trying to create new CA certificates with nameConstraints on them
> using the libcrypto in -current, and it doesn't work.
>
> Example snippet from config:
>
> [name_constraints]
> permitted;DNS.0 = .foo.com
>
> This blows up because in v2i_GENERAL_NAME_ex() we've added a call to
> x509_constraints_valid_sandns() which returns 0 on this name -- because it
> wouldn't be valid in a subjectAltName, but it *is* valid in a
> nameConstraint.
>
> There's other variants on this theme, too: "@foo.com" is a valid email
> constraint, also rejected (since it's not a valid actual mailbox name).
> Similarly for URIs.
>
> v2i_GENERAL_NAME_ex() already knows that we're parsing a nameConstraint, not
> a subjectAltName (via the is_nc argument), so it should use a different
> validator for constraints as opposed to actual names.
Thanks. The same issue was discovered and fixed with GEN_IPADD already,
but we missed these other cases...
> We already have x509_constraints_valid_domain_constraint() which can cover
> GEN_DNS and GEN_URI, though GEN_EMAIL and others are much more involved.
I think it's fine to punt on GEN_EMAIL for nameConstraints.
> I've attached a super basic draft patch to fix this up for GEN_DNS and
> GEN_URI at least. It also skips the check for GEN_EMAIL nameConstraints for
> now until someone wants to battle the mailbox parser for that (I don't trust
> myself to modify it right now, I blame kids).
At this point it would probably make more sense to use two switches, one
for is_nc and one for !is_nc, and perhaps factor them into two
helper functions.
> I'm assuming a regress test for this is probably wanted as well? Any
> opinions on adding this to regress/lib/libcrypto/CA/intermediate.cnf maybe?
> I'm happy to fiddle with it and get it going, just would love an opinion on
> whether to add it there or make something new.
A regress would be great. I would probably look into extending
regress/lib/libcrypto/certs rather than the CA test, but whichever works
best for you. If you want to do something new, it should probably go
into regress/lib/libcrypto/x509.
> --- lib/libcrypto/x509/x509_alt.c
> +++ lib/libcrypto/x509/x509_alt.c
> @@ -652,25 +652,44 @@ v2i_GENERAL_NAME_ex(GENERAL_NAME *out, const
> X509V3_EXT_METHOD *method,
> if (ret == NULL)
> return NULL;
>
> - /* Validate what we have for sanity */
> + /*
> + * Validate what we have for sanity. Note that nameconstraints requires
> + * different syntax for most of these (since it performs a pure suffix
> + * match, has no explicit wildcard, and does not require the constraint
> + * to be a fully valid name on its own)
> + */
> type = x509_constraints_general_to_bytes(ret, &bytes, &len);
> switch(type) {
> case GEN_DNS:
> - if (!x509_constraints_valid_sandns(bytes, len)) {
> + if (!is_nc && !x509_constraints_valid_sandns(bytes, len)) {
> + X509V3error(X509V3_R_BAD_OBJECT);
> + ERR_asprintf_error_data("name=%s value='%s'", name,
> bytes);
> + goto err;
> + }
> + if (is_nc && !x509_constraints_valid_domain_constraint(bytes,
> len)) {
> X509V3error(X509V3_R_BAD_OBJECT);
> ERR_asprintf_error_data("name=%s value='%s'", name,
> bytes);
> goto err;
> }
> break;
> case GEN_URI:
> - if (!x509_constraints_uri_host(bytes, len, NULL)) {
> + if (!is_nc && !x509_constraints_uri_host(bytes, len, NULL)) {
> + X509V3error(X509V3_R_BAD_OBJECT);
> + ERR_asprintf_error_data("name=%s value='%s'", name,
> bytes);
> + goto err;
> + }
> + if (is_nc && !x509_constraints_valid_domain_constraint(bytes,
> len)) {
> X509V3error(X509V3_R_BAD_OBJECT);
> ERR_asprintf_error_data("name=%s value='%s'", name,
> bytes);
> goto err;
> }
> break;
> case GEN_EMAIL:
> - if (!x509_constraints_parse_mailbox(bytes, len, NULL)) {
> + /*
> + * TODO: make a separate parser for mailbox constraints?
> + * needs to allow e.g. "@foo.com" or "@.foo.com"
> + */
> + if (!is_nc && !x509_constraints_parse_mailbox(bytes, len,
> NULL)) {
> X509V3error(X509V3_R_BAD_OBJECT);
> ERR_asprintf_error_data("name=%s value='%s'", name,
> bytes);
> goto err;
