On Thu, Mar 03, 2022 at 11:06:45AM +1000, Alex Wilson wrote:
> On 2/3/22 18:21, Theo Buehler wrote:
> >
> > 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 stared at this code a bit longer and realised we could just call
> x509_constraints_validate() if it was declared in the internal header, so
> I've done that instead. That way our behaviour here when generating new
> certs matches our behaviour exactly when we're processing constraints during
> verification. Let me know what you think.
Cool.
> > 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.
> >
>
> This mostly affects generating new certificates, not parsing existing ones
> (since it's in v2i_GENERAL_NAME_ex).
>
> I've modified the CA test to cover generating a bunch of different name
> constraints on the intermediate, at any rate.
Ah yes, I forgot that makecerts.sh isn't run everytime, so indeed,
modifying the CA test makes much more sense.
> In the process of doing this I also discovered that
> x509_constraints_dirname() was requiring an exact match of the DN rather
> than allowing a subtree (as the RFC says to do). It's a very straightforward
> fix, so I've put that in.
Agreed.
> The "@domain" email constraint syntax actually turns out to not be in the
> RFC, it seems to be something someone did later. OpenSSL and MS both support
> it, though, so I've modified x509_constraints_validate() to allow it (it was
> more straightforward than I thought it would be).
Nice. This also makes sense.
Below is your diff with a few minor stylistic tweaks to avoid the churn
in x509_alt.c.
One thing I did not change is the name->name == NULL error check. It
should probably be done inside each part of the if else if cascade.
I am going to land this in a few smaller steps.
There is one issue visible in this diff that pervades the constraints
code: the code assumes in various places that bytes is a C string, while
it actually is some sort of deserialized ASN.1 thing, so it is only
NUL-terminated because someone once thought it would be a good idea to
do that. We should therefore avoid calling strlen and strdup on it or
printing it with %s. I will send a diff that fixes (some of) this in a
follow-up.
Index: lib/libcrypto/x509/x509_alt.c
===================================================================
RCS file: /cvs/src/lib/libcrypto/x509/x509_alt.c,v
retrieving revision 1.8
diff -u -p -r1.8 x509_alt.c
--- lib/libcrypto/x509/x509_alt.c 11 Feb 2022 17:41:55 -0000 1.8
+++ lib/libcrypto/x509/x509_alt.c 3 Mar 2022 02:45:33 -0000
@@ -652,9 +652,27 @@ v2i_GENERAL_NAME_ex(GENERAL_NAME *out, c
if (ret == NULL)
return NULL;
- /* Validate what we have for sanity */
+ /*
+ * Validate what we have for sanity
+ */
+
+ if (is_nc) {
+ struct x509_constraints_name cname;
+ int errc = 0;
+
+ memset(&cname, 0, sizeof(cname));
+ type = x509_constraints_validate(ret, &cname, &errc);
+ if (type == 0 || errc != 0) {
+ X509V3error(X509V3_R_BAD_OBJECT);
+ ERR_asprintf_error_data("name=%s", name);
+ goto err;
+ }
+ x509_constraints_name_clear(&cname);
+ return ret;
+ }
+
type = x509_constraints_general_to_bytes(ret, &bytes, &len);
- switch(type) {
+ switch (type) {
case GEN_DNS:
if (!x509_constraints_valid_sandns(bytes, len)) {
X509V3error(X509V3_R_BAD_OBJECT);
@@ -677,8 +695,7 @@ v2i_GENERAL_NAME_ex(GENERAL_NAME *out, c
}
break;
case GEN_IPADD:
- if ((!is_nc && len != 4 && len != 16) ||
- (is_nc && len != 8 && len != 32)) {
+ if (len != 4 && len != 16) {
X509V3error(X509V3_R_BAD_IP_ADDRESS);
ERR_asprintf_error_data("name=%s len=%zu", name, len);
goto err;
Index: lib/libcrypto/x509/x509_constraints.c
===================================================================
RCS file: /cvs/src/lib/libcrypto/x509/x509_constraints.c,v
retrieving revision 1.20
diff -u -p -r1.20 x509_constraints.c
--- lib/libcrypto/x509/x509_constraints.c 2 Mar 2022 17:53:03 -0000
1.20
+++ lib/libcrypto/x509/x509_constraints.c 3 Mar 2022 02:45:29 -0000
@@ -636,7 +636,11 @@ int
x509_constraints_dirname(uint8_t *dirname, size_t dlen,
uint8_t *constraint, size_t len)
{
- if (len != dlen)
+ /*
+ * Allow constraint shorter than name to check (it has to be a prefix
+ * of it in DER format).
+ */
+ if (len > dlen)
return 0;
return (memcmp(constraint, dirname, len) == 0);
}
@@ -921,19 +925,40 @@ x509_constraints_validate(GENERAL_NAME *
name->type = GEN_DNS;
break;
case GEN_EMAIL:
- if (memchr(bytes, '@', len) != NULL) {
+ if (bytes[0] == '@') {
+ /*
+ * Allow email constraints of the form "@domain.com":
+ * treat them as if they just said "domain.com".
+ *
+ * This is not in the RFC, but is allowed by both
+ * OpenSSL upstream and Microsoft's X.509 bits.
+ */
+ if (!x509_constraints_valid_domain_constraint(bytes + 1,
+ len - 1))
+ goto err;
+ name->local = NULL;
+ name->name = strdup(bytes + 1);
+ name->type = GEN_EMAIL;
+
+ } else if (memchr(bytes, '@', len) != NULL) {
+ /* Specific email address (no suffix match) */
if (!x509_constraints_parse_mailbox(bytes, len, name))
goto err;
+ name->type = GEN_EMAIL;
+
} else {
+ /* Just a domain name. */
if (!x509_constraints_valid_domain_constraint(bytes,
len))
goto err;
- if ((name->name = strdup(bytes)) == NULL) {
- *error = X509_V_ERR_OUT_OF_MEM;
- return 0;
- }
+ name->local = NULL;
+ name->name = strdup(bytes);
+ name->type = GEN_EMAIL;
+ }
+ if (name->name == NULL) {
+ *error = X509_V_ERR_OUT_OF_MEM;
+ return 0;
}
- name->type = GEN_EMAIL;
break;
case GEN_IPADD:
/* Constraints are ip then mask */
@@ -949,7 +974,10 @@ x509_constraints_validate(GENERAL_NAME *
case GEN_URI:
if (!x509_constraints_valid_domain_constraint(bytes, len))
goto err;
- name->name = strdup(bytes);
+ if ((name->name = strdup(bytes)) == NULL) {
+ *error = X509_V_ERR_OUT_OF_MEM;
+ return 0;
+ }
name->type = GEN_URI;
break;
default:
Index: lib/libcrypto/x509/x509_internal.h
===================================================================
RCS file: /cvs/src/lib/libcrypto/x509/x509_internal.h,v
retrieving revision 1.16
diff -u -p -r1.16 x509_internal.h
--- lib/libcrypto/x509/x509_internal.h 24 Nov 2021 05:38:12 -0000 1.16
+++ lib/libcrypto/x509/x509_internal.h 3 Mar 2022 01:57:32 -0000
@@ -126,6 +126,8 @@ int x509_constraints_extract_names(struc
int x509_constraints_extract_constraints(X509 *cert,
struct x509_constraints_names *permitted,
struct x509_constraints_names *excluded, int *error);
+int x509_constraints_validate(GENERAL_NAME *constraint,
+ struct x509_constraints_name *name, int *error);
int x509_constraints_check(struct x509_constraints_names *names,
struct x509_constraints_names *permitted,
struct x509_constraints_names *excluded, int *error);
Index: regress/lib/libcrypto/CA/Makefile
===================================================================
RCS file: /cvs/src/regress/lib/libcrypto/CA/Makefile,v
retrieving revision 1.3
diff -u -p -r1.3 Makefile
--- regress/lib/libcrypto/CA/Makefile 26 Dec 2020 14:42:09 -0000 1.3
+++ regress/lib/libcrypto/CA/Makefile 3 Mar 2022 01:33:28 -0000
@@ -59,7 +59,7 @@ server.key.pem: stamp-clean
server.csr.pem: intermediate.cnf server.key.pem
# server req
openssl req -batch -config ${.CURDIR}/intermediate.cnf -new -sha256 \
- -subj '/CN=server/O=OpenBSD/OU=So and Sos/C=CA' \
+ -subj '/CN=server.openbsd.org/OU=So and Sos/O=OpenBSD/C=CA' \
-key server.key.pem -out server.csr.pem
# Sign server key
@@ -77,7 +77,7 @@ client.key.pem: stamp-clean
client.csr.pem: intermediate.cnf intermediate.cert.pem client.key.pem
# client req
openssl req -batch -config ${.CURDIR}/intermediate.cnf -new -sha256 \
- -subj '/CN=client/O=OpenBSD/OU=So and Sos/C=CA' \
+ -subj '/CN=client/OU=So and Sos/O=OpenBSD/C=CA' \
-key client.key.pem -out client.csr.pem
# Sign client key
Index: regress/lib/libcrypto/CA/intermediate.cnf
===================================================================
RCS file: /cvs/src/regress/lib/libcrypto/CA/intermediate.cnf,v
retrieving revision 1.3
diff -u -p -r1.3 intermediate.cnf
--- regress/lib/libcrypto/CA/intermediate.cnf 26 Dec 2020 00:48:56 -0000
1.3
+++ regress/lib/libcrypto/CA/intermediate.cnf 3 Mar 2022 01:33:28 -0000
@@ -105,6 +105,10 @@ subjectKeyIdentifier = hash
authorityKeyIdentifier = keyid,issuer
keyUsage = critical, nonRepudiation, digitalSignature, keyEncipherment
extendedKeyUsage = clientAuth, emailProtection
+subjectAltName = critical, @usr_san
+
+[ usr_san ]
+email.0 = [email protected]
[ server_cert ]
# Extensions for server certificates (`man x509v3_config`).
Index: regress/lib/libcrypto/CA/root.cnf
===================================================================
RCS file: /cvs/src/regress/lib/libcrypto/CA/root.cnf,v
retrieving revision 1.3
diff -u -p -r1.3 root.cnf
--- regress/lib/libcrypto/CA/root.cnf 26 Dec 2020 00:48:56 -0000 1.3
+++ regress/lib/libcrypto/CA/root.cnf 3 Mar 2022 01:33:28 -0000
@@ -95,6 +95,22 @@ subjectKeyIdentifier = hash
authorityKeyIdentifier = keyid:always,issuer
basicConstraints = critical, CA:true, pathlen:0
keyUsage = critical, digitalSignature, cRLSign, keyCertSign
+nameConstraints = critical, @ca_name_constraints
+
+[ ca_name_constraints ]
+permitted;DNS.0 = .openbsd.org
+permitted;DNS.1 = client
+permitted;email.0 = openbsd.org
+permitted;email.1 = @test.openbsd.org
+permitted;URI.0 = .openbsd.org
+permitted;dirName.0 = openbsd_dn
+permitted;otherName.0 = 1.3.6.1.4.1.311.20.2.3;UTF8:@openbsd.org
+excluded;IP.0 = 0.0.0.0/0.0.0.0
+excluded;IP.1 = 0:0:0:0:0:0:0:0/0:0:0:0:0:0:0:0
+
+[ openbsd_dn ]
+C = CA
+O = OpenBSD
[ usr_cert ]
# Extensions for client certificates (`man x509v3_config`).
