From: Theo Buehler Subject: Re: rpki-client: introduce cert_parse_internal() To: Job Snijders Cc: tech@openbsd.org Date: Wed, 2 Jul 2025 15:29:14 +0200 On Wed, Jul 02, 2025 at 01:15:32PM +0000, Job Snijders wrote: > On Wed, Jul 02, 2025 at 02:07:54PM +0200, Theo Buehler wrote: > > Apart from the extensions, EE certs are still insufficiently parsed. > > The Certificate and the TBSCertificate are independent of the cert > > purpose, so we need one function to deal with those. > > > > cert_parse_internal() does part of this: walk the TBSCertificate's > > SEQUENCE per RFC 5280 and validate one entry after the other and check > > the restrictions imposed by RFC 6487. Well, almost. Since we > > deal with subject and issuer in one go, the order isn't 100% the same. > > > > cert_parse_internal() brings part of a simplification promised earlier. > > It allocates struct cert itself and populates it, and takes ownership > > of the cert passed in. > > > > The new cert_check_sigalg() and cert_check_validity_period() are > > straightforward enough. Two missing bits are indicated in an XXX: > > the SPKI and checking for completeness of extensions. > > I suppose some thought needs to be given to filemode, with this diff we're > bowing quite soon, but I'm not entirely sure "EE being expired" does rises > to the level of "this was entirely unparsable" It's always the late additions that cause problems :) During normal runs I would argue it is a parse failure. But let's just punt on this for now and only check that notbefore < notafter. The only change to the previous diff is that I dropped the now check. diff --git a/usr.sbin/rpki-client/cert.c b/usr.sbin/rpki-client/cert.c index e03e6192497..75b2f833a8a 100644 --- a/usr.sbin/rpki-client/cert.c +++ b/usr.sbin/rpki-client/cert.c @@ -1106,18 +1106,41 @@ certificate_policies(const char *fn, struct cert *cert, X509_EXTENSION *ext) } static int -cert_check_subject_and_issuer(const char *fn, const X509 *x) +cert_check_sigalg(const char *fn, const struct cert *cert) +{ + int nid; + + if ((nid = X509_get_signature_nid(cert->x509)) == NID_undef) { + warnx("%s: unknown signature type", fn); + return 0; + } + + if (nid == NID_sha256WithRSAEncryption) + return 1; + if (experimental && nid == NID_ecdsa_with_SHA256) { + if (verbose) + warnx("%s: P-256 support is experimental", fn); + return 1; + } + + warnx("%s: RFC 7935: wrong signature algorithm %s, want %s", + fn, nid2str(nid), LN_sha256WithRSAEncryption); + return 0; +} + +static int +cert_check_subject_and_issuer(const char *fn, const struct cert *cert) { const X509_NAME *name; - if ((name = X509_get_subject_name(x)) == NULL) { + if ((name = X509_get_subject_name(cert->x509)) == 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) { + if ((name = X509_get_issuer_name(cert->x509)) == NULL) { warnx("%s: X509_get_issuer_name", fn); return 0; } @@ -1127,6 +1150,22 @@ cert_check_subject_and_issuer(const char *fn, const X509 *x) return 1; } +static int +cert_check_validity_period(const char *fn, struct cert *cert) +{ + if (!x509_get_notbefore(cert->x509, fn, &cert->notbefore)) + return 0; + if (!x509_get_notafter(cert->x509, fn, &cert->notafter)) + return 0; + + if (cert->notbefore > cert->notafter) { + warnx("%s: RFC 6487, 4.6: notAfter precedes notBefore", fn); + return 0; + } + + return 1; +} + /* * Check the cert's purpose: the cA bit in basic constraints distinguishes * between TA/CA and EE/BGPsec router and the key usage bits must match. @@ -1400,6 +1439,8 @@ cert_parse_extensions(const char *fn, struct cert *cert) } } + /* XXX - validate required fields. */ + return 1; dup: @@ -1409,38 +1450,82 @@ cert_parse_extensions(const char *fn, struct cert *cert) return 0; } -/* - * Lightweight version of cert_parse_pre() for EE certs. - * Parses the two RFC 3779 extensions, and performs some sanity checks. - * Returns cert on success and NULL on failure. - */ -struct cert * -cert_parse_ee_cert(const char *fn, int talid, X509 *x) +static struct cert * +cert_parse_internal(const char *fn, X509 *x) { struct cert *cert; + const ASN1_INTEGER *serial; + const ASN1_BIT_STRING *issuer_uid = NULL, *subject_uid = NULL; - if ((cert = calloc(1, sizeof(struct cert))) == NULL) + if ((cert = calloc(1, sizeof(*cert))) == NULL) err(1, NULL); + cert->x509 = x; - if (!X509_up_ref(x)) { - warnx("%s: X509_up_ref failed", fn); + if (!x509_cache_extensions(x, fn)) goto out; - } - - cert->x509 = x; - cert->talid = talid; if (X509_get_version(x) != 2) { warnx("%s: RFC 6487 4.1: X.509 version must be v3", fn); goto out; } - if (!cert_check_subject_and_issuer(fn, x)) + if ((serial = X509_get0_serialNumber(x)) == NULL) { + warnx("%s: RFC 6487 4.2: missing serialNumber", fn); + goto out; + } + if (!x509_valid_seqnum(fn, "RFC 6487 4.2: serialNumber", serial)) + goto out; + + if (!cert_check_sigalg(fn, cert)) + goto out; + + if (!cert_check_subject_and_issuer(fn, cert)) + goto out; + + if (!cert_check_validity_period(fn, cert)) + goto out; + + /* XXX - add SPKI here. */ + + /* + * Disallowed for CA certs in RFC 5280, 4.1.2.8. Uniqueness of subjects + * per RFC 6487, 4.5 makes them meaningless. + */ + X509_get0_uids(x, &issuer_uid, &subject_uid); + if (issuer_uid != NULL || subject_uid != NULL) { + warnx("%s: issuer or subject unique identifiers not allowed", + fn); goto out; + } if (!cert_parse_extensions(fn, cert)) goto out; + return cert; + + out: + cert_free(cert); + return NULL; +} + +/* + * Parse an EE cert extracted from a CMS signed object. Store all cert and + * extension data we need later in the returned struct cert. + * Check the cretificate's purpose and validate the TA constraints. + * Returns cert on success and NULL on failure. + */ +struct cert * +cert_parse_ee_cert(const char *fn, int talid, X509 *x) +{ + struct cert *cert = NULL; + + if (!X509_up_ref(x)) + goto out; + + if ((cert = cert_parse_internal(fn, x)) == NULL) + goto out; + cert->talid = talid; + if (cert->purpose != CERT_PURPOSE_EE) { warnx("%s: expected EE cert, got %s", fn, purpose2str(cert->purpose)); @@ -1465,21 +1550,16 @@ cert_parse_ee_cert(const char *fn, int talid, X509 *x) struct cert * cert_parse_pre(const char *fn, const unsigned char *der, size_t len) { - struct cert *cert; + struct cert *cert = NULL; const unsigned char *oder; size_t j; X509 *x = NULL; - const ASN1_BIT_STRING *issuer_uid = NULL, *subject_uid = NULL; EVP_PKEY *pkey; - int nid; /* just fail for empty buffers, the warning was printed elsewhere */ if (der == NULL) return NULL; - if ((cert = calloc(1, sizeof(struct cert))) == NULL) - err(1, NULL); - oder = der; if ((x = d2i_X509(NULL, &der, len)) == NULL) { warnx("%s: d2i_X509", fn); @@ -1494,49 +1574,7 @@ cert_parse_pre(const char *fn, const unsigned char *der, size_t len) warnx("%s: X509_up_ref failed", fn); goto out; } - cert->x509 = x; - - if (!x509_cache_extensions(x, fn)) - goto out; - - if (X509_get_version(x) != 2) { - warnx("%s: RFC 6487 4.1: X.509 version must be v3", fn); - goto out; - } - - if ((nid = X509_get_signature_nid(x)) == NID_undef) { - warnx("%s: unknown signature type", fn); - goto out; - } - if (experimental && nid == NID_ecdsa_with_SHA256) { - if (verbose) - warnx("%s: P-256 support is experimental", fn); - } else if (nid != NID_sha256WithRSAEncryption) { - warnx("%s: RFC 7935: wrong signature algorithm %s, want %s", - fn, nid2str(nid), LN_sha256WithRSAEncryption); - goto out; - } - - /* - * Disallowed for CA certs in RFC 5280, 4.1.2.8. Uniqueness of subjects - * per RFC 6487, 4.5 makes them meaningless. - */ - X509_get0_uids(x, &issuer_uid, &subject_uid); - if (issuer_uid != NULL || subject_uid != NULL) { - warnx("%s: issuer or subject unique identifiers not allowed", - fn); - goto out; - } - - if (!cert_check_subject_and_issuer(fn, x)) - goto out; - - if (!cert_parse_extensions(fn, cert)) - goto out; - - if (!x509_get_notbefore(x, fn, &cert->notbefore)) - goto out; - if (!x509_get_notafter(x, fn, &cert->notafter)) + if ((cert = cert_parse_internal(fn, x)) == NULL) goto out; /* Validation on required fields. */