On Mon, Sep 11, 2023 at 01:42:03AM +0000, Job Snijders wrote:
> This adds another compliance check for the X.509 subject name.
>
> Only commonName, and optionally serialNumber, are permitted in the
> certificate subject name. See RFC 6487 section 4.4 and 4.5.
>
> It seems the one CA who was not compliant with this requirement got
> their act together, so now is an opportune time to get this in.
I wish there was a better place for this, but due to the unfortunate
parse/validate split, there doesn't seem to be.
> OK?
Some comments below.
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.114
> diff -u -p -r1.114 cert.c
> --- cert.c 29 Jun 2023 10:28:25 -0000 1.114
> +++ cert.c 9 Jul 2023 13:38:02 -0000
> @@ -595,14 +595,13 @@ certificate_policies(struct parse *p, X5
>
> /*
> * Lightweight version of cert_parse_pre() for ASPA, ROA, and RSC EE certs.
"ASPA, ROA, and RSC" is a lie, isn't it.
> - * This only parses the RFC 3779 extensions since these are necessary for
> - * validation.
Isn't this still true? You don't really parse the subject name.
> * Returns cert on success and NULL on failure.
> */
> struct cert *
> cert_parse_ee_cert(const char *fn, X509 *x)
> {
> struct parse p;
> + X509_NAME *xn;
> X509_EXTENSION *ext;
> int index;
>
> @@ -616,6 +615,13 @@ cert_parse_ee_cert(const char *fn, X509
> goto out;
> }
>
> + if ((xn = X509_get_subject_name(x)) == NULL) {
> + warnx("%s: X509_get_subject_name", fn);
> + goto out;
> + }
> + if (!x509_valid_subject(fn, xn))
> + goto out;
> +
> if (X509_get_key_usage(x) != KU_DIGITAL_SIGNATURE) {
> warnx("%s: RFC 6487 section 4.8.4: KU must be digitalSignature",
> fn);
> @@ -669,6 +675,7 @@ cert_parse_pre(const char *fn, const uns
> const X509_ALGOR *palg;
> const ASN1_BIT_STRING *piuid = NULL, *psuid = NULL;
> const ASN1_OBJECT *cobj;
> + const X509_NAME *xn;
> ASN1_OBJECT *obj;
> EVP_PKEY *pkey;
> struct parse p;
> @@ -726,6 +733,13 @@ cert_parse_pre(const char *fn, const uns
> fn);
> goto out;
> }
> +
> + if ((xn = X509_get_subject_name(x)) == NULL) {
> + warnx("%s: X509_get_subject_name", p.fn);
> + goto out;
> + }
> + if (!x509_valid_subject(p.fn, xn))
> + goto out;
>
> /* Look for X509v3 extensions. */
>
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.188
> diff -u -p -r1.188 extern.h
> --- extern.h 29 Jun 2023 14:33:35 -0000 1.188
> +++ extern.h 9 Jul 2023 13:38:02 -0000
> @@ -839,6 +839,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_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
> retrieving revision 1.73
> diff -u -p -r1.73 x509.c
> --- x509.c 23 Jun 2023 15:32:15 -0000 1.73
> +++ x509.c 9 Jul 2023 13:38:02 -0000
> @@ -861,6 +861,80 @@ x509_location(const char *fn, const char
> }
>
> /*
> + * Check that the subject only contains commonName and serialNumber.
> + * Return 0 on failure.
> + */
> +int
> +x509_valid_subject(const char *fn, const X509_NAME *xn)
I would pass in the X509 itself rather than extracting the name in the
caller. This adds
> +{
> + X509_NAME_ENTRY *ne;
> + ASN1_OBJECT *ao;
> + ASN1_STRING *as;
> + int cn = 0, sn = 0;
> + int i, nid;
And get the subject name here. Then you only have to do this once and
the caller only grows by three lines and without additional variable.
> +
> + 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);
> + return 0;
> + }
> + if ((ao = X509_NAME_ENTRY_get_object(ne)) == NULL) {
> + warnx("%s: X509_NAME_ENTRY_get_object", fn);
> + return 0;
> + }
> +
> + nid = OBJ_obj2nid(ao);
> + switch (nid) {
> + case NID_commonName:
> + if (cn++ > 0) {
> + warnx("%s: duplicate commonName in subject",
> + fn);
> + return 0;
> + }
> + if ((as = X509_NAME_ENTRY_get_data(ne)) == NULL) {
> + warnx("%s: X509_NAME_ENTRY_get_data failed",
> + fn);
> + return 0;
> + }
> +/*
> + * The following check can be enabled after AFRINIC re-issues CA certs.
> + * https://lists.afrinic.net/pipermail/dbwg/2023-March/000436.html
> + */
> +#if 0
> + if (as->type != V_ASN1_PRINTABLESTRING) {
> + warnx("%s: RFC 6487 section 4.5: commonName is"
> + " not PrintableString", fn);
> + return 0;
> + }
> +#endif
> + break;
> + case NID_serialNumber:
> + if (sn++ > 0) {
> + warnx("%s: duplicate serialNumber in subject",
> + fn);
> + return 0;
> + }
> + break;
> + case NID_undef:
> + warnx("%s: OBJ_obj2nid failed", fn);
> + return 0;
> + default:
> + warnx("%s: RFC 6487 section 4.5: invalid attribute "
s/invalid/unexpected ?
> + "(NID: %d)", fn, nid);
Since it's not NID_undef, it should have a short name, so I think this
would be more user friendly:
"%s", fn, OBJ_nid2sn(nid));
> + return 0;
> + }
> + }
> +
> + if (!cn) {
if (cn == 0)
> + warnx("%s: RFC 6487 section 4.5: subject missing commonName",
> + fn);
> + return 0;
> + }
> +
> + return 1;
> +}
> +
> +/*
> * Convert an ASN1_INTEGER into a hexstring.
> * Returned string needs to be freed by the caller.
> */
>
