From: Claudio Jeker Subject: Re: rpki-client: check issuer for certs and CRLs To: Theo Buehler Cc: tech@openbsd.org Date: Thu, 30 May 2024 22:01:23 +0200 On Thu, May 30, 2024 at 04:43:42PM +0200, Theo Buehler wrote: > This slightly generalizes x509_valid_subject() into a Name validating > function, applies it to both subject and issuer of certs and uses it for > CRLs as well. > > Now the verifier does check that the issuer's subject matches the > subject's issuer when building chains, but what exactly it checks > on the CRL side of things is not quite so obvious. > > I think we're better off checking both, as the check is simple and > cheap enough. I haven't looked into adding some smarts for avoiding > the afrinic special #if 0, but I'm not sure it's worth it. Looks good to me. Is the afrinic special still needed or did they fail to re-issue CA certs in the last year? > Index: cert.c > =================================================================== > RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v > diff -u -p -r1.131 cert.c > --- cert.c 20 May 2024 15:51:43 -0000 1.131 > +++ cert.c 30 May 2024 13:27:24 -0000 > @@ -651,6 +651,28 @@ certificate_policies(const char *fn, str > return rc; > } > > +static int > +cert_check_subject_and_issuer(const char *fn, const X509 *x) > +{ > + const X509_NAME *name; > + > + if ((name = X509_get_subject_name(x)) == NULL) { > + warnx("%s: X509_get_subject_name", fn); > + return 0; > + } > + if (!x509_valid_name(fn, "subject", name)) > + return 0; > + > + if ((name = X509_get_issuer_name(x)) == NULL) { > + warnx("%s: X509_get_issuer_name", fn); > + return 0; > + } > + if (!x509_valid_name(fn, "issuer", name)) > + return 0; > + > + return 1; > +} > + > /* > * Lightweight version of cert_parse_pre() for EE certs. > * Parses the two RFC 3779 extensions, and performs some sanity checks. > @@ -671,7 +693,7 @@ cert_parse_ee_cert(const char *fn, int t > goto out; > } > > - if (!x509_valid_subject(fn, x)) > + if (!cert_check_subject_and_issuer(fn, x)) > goto out; > > if (X509_get_key_usage(x) != KU_DIGITAL_SIGNATURE) { > @@ -791,7 +813,7 @@ cert_parse_pre(const char *fn, const uns > goto out; > } > > - if (!x509_valid_subject(fn, x)) > + if (!cert_check_subject_and_issuer(fn, x)) > goto out; > > /* Look for X509v3 extensions. */ > Index: crl.c > =================================================================== > RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v > diff -u -p -r1.35 crl.c > --- crl.c 29 May 2024 13:26:24 -0000 1.35 > +++ crl.c 30 May 2024 13:27:37 -0000 > @@ -166,6 +166,7 @@ crl_parse(const char *fn, const unsigned > const unsigned char *oder; > struct crl *crl; > const X509_ALGOR *palg; > + const X509_NAME *name; > const ASN1_OBJECT *cobj; > const ASN1_TIME *at; > int count, nid, rc = 0; > @@ -191,6 +192,13 @@ crl_parse(const char *fn, const unsigned > warnx("%s: RFC 6487 section 5: version 2 expected", fn); > goto out; > } > + > + if ((name = X509_CRL_get_issuer(crl->x509_crl)) == NULL) { > + warnx("%s: X509_CRL_get_issuer", fn); > + goto out; > + } > + if (!x509_valid_name(fn, "issuer", name)) > + goto out; > > X509_CRL_get0_signature(crl->x509_crl, NULL, &palg); > if (palg == NULL) { > Index: extern.h > =================================================================== > RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v > diff -u -p -r1.219 extern.h > --- extern.h 29 May 2024 13:26:24 -0000 1.219 > +++ extern.h 30 May 2024 12:07:07 -0000 > @@ -917,7 +917,7 @@ int x509_location(const char *, const > GENERAL_NAME *, char **); > int x509_inherits(X509 *); > int x509_any_inherits(X509 *); > -int x509_valid_subject(const char *, const X509 *); > +int x509_valid_name(const char *, const char *, const X509_NAME *); > time_t x509_find_expires(time_t, struct auth *, struct crl_tree *); > > /* printers */ > Index: x509.c > =================================================================== > RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v > diff -u -p -r1.88 x509.c > --- x509.c 29 May 2024 13:26:24 -0000 1.88 > +++ x509.c 30 May 2024 13:28:58 -0000 > @@ -842,24 +842,18 @@ x509_location(const char *fn, const char > } > > /* > - * Check that the subject only contains commonName and serialNumber. > + * Check that subject or issuer only contain commonName and serialNumber. > * Return 0 on failure. > */ > int > -x509_valid_subject(const char *fn, const X509 *x) > +x509_valid_name(const char *fn, const char *descr, const X509_NAME *xn) > { > - const X509_NAME *xn; > const X509_NAME_ENTRY *ne; > const ASN1_OBJECT *ao; > const ASN1_STRING *as; > int cn = 0, sn = 0; > int i, nid; > > - if ((xn = X509_get_subject_name(x)) == NULL) { > - warnx("%s: X509_get_subject_name", fn); > - return 0; > - } > - > for (i = 0; i < X509_NAME_entry_count(xn); i++) { > if ((ne = X509_NAME_get_entry(xn, i)) == NULL) { > warnx("%s: X509_NAME_get_entry", fn); > @@ -874,8 +868,8 @@ x509_valid_subject(const char *fn, const > switch (nid) { > case NID_commonName: > if (cn++ > 0) { > - warnx("%s: duplicate commonName in subject", > - fn); > + warnx("%s: duplicate commonName in %s", > + fn, descr); > return 0; > } > if ((as = X509_NAME_ENTRY_get_data(ne)) == NULL) { > @@ -897,8 +891,8 @@ x509_valid_subject(const char *fn, const > break; > case NID_serialNumber: > if (sn++ > 0) { > - warnx("%s: duplicate serialNumber in subject", > - fn); > + warnx("%s: duplicate serialNumber in %s", > + fn, descr); > return 0; > } > break; > @@ -907,14 +901,14 @@ x509_valid_subject(const char *fn, const > return 0; > default: > warnx("%s: RFC 6487 section 4.5: unexpected attribute" > - " %s", fn, nid2str(nid)); > + " %s in %s", fn, nid2str(nid), descr); > return 0; > } > } > > if (cn == 0) { > - warnx("%s: RFC 6487 section 4.5: subject missing commonName", > - fn); > + warnx("%s: RFC 6487 section 4.5: %s missing commonName", > + fn, descr); > return 0; > } > > -- :wq Claudio