Download raw body.
rpki-client: introduce cert_parse_internal()
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. */
rpki-client: introduce cert_parse_internal()