On Fri, Feb 04, 2022 at 12:03:41PM +0100, Claudio Jeker wrote: > On Fri, Feb 04, 2022 at 10:41:03AM +0100, Theo Buehler wrote: > > It was pointed out to Claudio that rpki-client does not enforce > > certificate policies. > > > > The diff below does that. It has two parts. > > > > In cert.c we check that the certificate policy extension matches the > > specification in RFC 6487, section 4.8.9, as amended by RFC 7318 > > section 2. That's maybe a bit lengthy but completely straightforward. > > If you're curious what's in a CPS URI, it might be this: > > https://www.arin.net/resources/manage/rpki/cps.pdf > > > > The second bit is in parser.c and makes sure that the verifier builds a > > policy tree enforcing that the certification path uses a policy OID of > > id-cp-ipAddr-asNumber (see RFC 6484). This is a bit trickier since it > > involves X509_policy_check() under the hood. In short, we set up a > > policy containing that OID in the verify parameters and set verifier > > flags that ensure that the user set policy is enforced. > > > > If you will, the second part improves the validation. The verifier > > doesn't have a mechanism to enforce things like there's exactly one > > policy identifier, etc. That's what's done in cert.c > > > > This works for me. Please test. > > Looks good to me. OK claudio@
Unfortunately, NID_ipAddr_asNumber is only available in LibreSSL 3.3 and later and didn't make it into OpenSSL 1.1 so I had to do some things a bit differently. I added a certpol_oid to x509_init_oid() and use it instead of NID_ipAddr_asNumber(). Since we have this global already, it's easier to use X509_VERIFY_PARAM_add0_policy(). It also forces one check to use OBJ_cmp() instead OBJ_obj2nid(). > > + policy = sk_POLICYINFO_value(policies, 0); > > + assert(policy != NULL && policy->policyid != NULL); > > Should this be an assert() or a proper error check? > I guess sk_POLICYINFO_value() can not really fail because of the check > above. What about policy->policyid? I gess X509V3_EXT_d2i() sets this > correctly or it failed above. Yes. Both != NULL are guaranteed. You're right about sk_value(). Also your guess is correct: policy->policyid != NULL is guaranteed by the templated ASN.1: In POLICYINFO_seq_tt (lib/libcrypto/x509/x509_cpols.c:147), there is no ASN1_TFLG_OPTIONAL flag to indicate that policy->policyid can be NULL (whereas policy->qualifiers is optional). Therefore if policy != NULL and since X509V3_EXT_d2i didn't fail, it must have deserialized correctly, so policy->policyid != NULL. I left the assert as it is for now, but I can change it into an error check or drop it if you prefer. > > + qualifier = sk_POLICYQUALINFO_value(qualifiers, 0); > > + assert(qualifier != NULL && qualifier->pqualid != NULL); > > Same as above. The answer is the same again, this time arguing with the flags in POLICYQUALINFO_seq_tt. > > + flags |= X509_V_FLAG_EXPLICIT_POLICY; > > + flags |= X509_V_FLAG_INHIBIT_MAP; > > I do not really understand the meaning of X509_V_FLAG_INHIBIT_MAP. Neither > the manpage nor the referenced RFC really help to explain what is > inhibited and if we really want that or not. It seems to be the more > strict option so I think it is the right thing to do but wow this is > complicated. It is super complicated indeed. Policy mappings are explained here: https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.5 It's another extension that allows translation between policy OIDs. I'm pretty sure we don't want to allow this. Index: cert.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v retrieving revision 1.53 diff -u -p -r1.53 cert.c --- cert.c 20 Jan 2022 16:36:19 -0000 1.53 +++ cert.c 4 Feb 2022 14:32:50 -0000 @@ -29,6 +29,7 @@ #include <openssl/asn1.h> #include <openssl/x509.h> +#include <openssl/x509v3.h> #include "extern.h" @@ -47,6 +48,7 @@ struct parse { const char *fn; /* currently-parsed file */ }; +extern ASN1_OBJECT *certpol_oid; /* id-cp-ipAddr-asNumber cert policy */ extern ASN1_OBJECT *carepo_oid; /* 1.3.6.1.5.5.7.48.5 (caRepository) */ extern ASN1_OBJECT *manifest_oid; /* 1.3.6.1.5.5.7.48.10 (rpkiManifest) */ extern ASN1_OBJECT *notify_oid; /* 1.3.6.1.5.5.7.48.13 (rpkiNotify) */ @@ -969,6 +971,78 @@ out: return rc; } +static int +certificate_policies(struct parse *p, X509_EXTENSION *ext) +{ + STACK_OF(POLICYINFO) *policies = NULL; + POLICYINFO *policy; + STACK_OF(POLICYQUALINFO) *qualifiers; + POLICYQUALINFO *qualifier; + int nid; + int rc = 0; + + if (!X509_EXTENSION_get_critical(ext)) { + cryptowarnx("%s: RFC 6487 section 4.8.9: certificatePolicies: " + "extension not critical", p->fn); + goto out; + } + + if ((policies = X509V3_EXT_d2i(ext)) == NULL) { + cryptowarnx("%s: RFC 6487 section 4.8.9: certificatePolicies: " + "failed extension parse", p->fn); + goto out; + } + + if (sk_POLICYINFO_num(policies) != 1) { + warnx("%s: RFC 6487 section 4.8.9: certificatePolicies: " + "want 1 policy, got %d", p->fn, + sk_POLICYINFO_num(policies)); + goto out; + } + + policy = sk_POLICYINFO_value(policies, 0); + assert(policy != NULL && policy->policyid != NULL); + + if (OBJ_cmp(policy->policyid, certpol_oid) != 0) { + nid = OBJ_obj2nid(policy->policyid); + warnx("%s: RFC 6487 section 4.8.9: certificatePolicies: " + "unexpected policy identifier %d (%s)", p->fn, nid, + OBJ_nid2sn(nid)); + goto out; + } + + /* Policy qualifiers are optional. If they're absent, we're done. */ + if ((qualifiers = policy->qualifiers) == NULL) { + rc = 1; + goto out; + } + + if (sk_POLICYQUALINFO_num(qualifiers) != 1) { + warnx("%s: RFC 7318 section 2: certificatePolicies: " + "want 1 policy qualifier, got %d", p->fn, + sk_POLICYQUALINFO_num(qualifiers)); + goto out; + } + + qualifier = sk_POLICYQUALINFO_value(qualifiers, 0); + assert(qualifier != NULL && qualifier->pqualid != NULL); + + if ((nid = OBJ_obj2nid(qualifier->pqualid)) != NID_id_qt_cps) { + warnx("%s: RFC 7318 section 2: certificatePolicies: " + "want CPS, got %d (%s)", p->fn, nid, OBJ_nid2sn(nid)); + goto out; + } + + if (verbose > 1) + warnx("%s: CPS %.*s", p->fn, qualifier->d.cpsuri->length, + qualifier->d.cpsuri->data); + + rc = 1; + out: + sk_POLICYINFO_pop_free(policies, POLICYINFO_free); + return rc; +} + /* * Parse and partially validate an RPKI X509 certificate (either a trust * anchor or a certificate) as defined in RFC 6487. @@ -1024,6 +1098,9 @@ cert_parse_inner(const char *fn, const u case NID_sinfo_access: sia_present = 1; c = sbgp_sia(&p, ext); + break; + case NID_certificate_policies: + c = certificate_policies(&p, ext); break; case NID_crl_distribution_points: /* ignored here, handled later */ Index: parser.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v retrieving revision 1.58 diff -u -p -r1.58 parser.c --- parser.c 28 Jan 2022 15:30:23 -0000 1.58 +++ parser.c 4 Feb 2022 14:22:19 -0000 @@ -47,6 +47,8 @@ static X509_STORE_CTX *ctx; static struct auth_tree auths = RB_INITIALIZER(&auths); static struct crl_tree crlt = RB_INITIALIZER(&crlt); +extern ASN1_OBJECT *certpol_oid; + struct parse_repo { RB_ENTRY(parse_repo) entry; char *path; @@ -206,6 +208,8 @@ static int valid_x509(char *file, X509 *x509, struct auth *a, struct crl *crl, unsigned long flags, int nowarn) { + X509_VERIFY_PARAM *params; + ASN1_OBJECT *cp_oid; STACK_OF(X509) *chain; STACK_OF(X509_CRL) *crls = NULL; int c; @@ -217,11 +221,19 @@ valid_x509(char *file, X509 *x509, struc if (!X509_STORE_CTX_init(ctx, NULL, x509, NULL)) cryptoerrx("X509_STORE_CTX_init"); + if ((params = X509_STORE_CTX_get0_param(ctx)) == NULL) + cryptoerrx("X509_STORE_CTX_get0_param"); + if ((cp_oid = OBJ_dup(certpol_oid)) == NULL) + cryptoerrx("OBJ_dup"); + if (!X509_VERIFY_PARAM_add0_policy(params, cp_oid)) + cryptoerrx("X509_VERIFY_PARAM_add0_policy"); + X509_STORE_CTX_set_verify_cb(ctx, verify_cb); if (!X509_STORE_CTX_set_app_data(ctx, file)) cryptoerrx("X509_STORE_CTX_set_app_data"); - if (flags != 0) - X509_STORE_CTX_set_flags(ctx, flags); + flags |= X509_V_FLAG_EXPLICIT_POLICY; + flags |= X509_V_FLAG_INHIBIT_MAP; + X509_STORE_CTX_set_flags(ctx, flags); X509_STORE_CTX_set_depth(ctx, MAX_CERT_DEPTH); X509_STORE_CTX_set0_trusted_stack(ctx, chain); X509_STORE_CTX_set0_crls(ctx, crls); Index: x509.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v retrieving revision 1.33 diff -u -p -r1.33 x509.c --- x509.c 2 Feb 2022 12:10:40 -0000 1.33 +++ x509.c 4 Feb 2022 14:22:19 -0000 @@ -30,6 +30,7 @@ #include "extern.h" +ASN1_OBJECT *certpol_oid; /* id-cp-ipAddr-asNumber cert policy */ ASN1_OBJECT *carepo_oid; /* 1.3.6.1.5.5.7.48.5 (caRepository) */ ASN1_OBJECT *manifest_oid; /* 1.3.6.1.5.5.7.48.10 (rpkiManifest) */ ASN1_OBJECT *notify_oid; /* 1.3.6.1.5.5.7.48.13 (rpkiNotify) */ @@ -42,6 +43,8 @@ void x509_init_oid(void) { + if ((certpol_oid = OBJ_txt2obj("1.3.6.1.5.5.7.14.2", 1)) == NULL) + errx(1, "OBJ_txt2obj for %s failed", "1.3.6.1.5.5.7.14.2"); if ((carepo_oid = OBJ_txt2obj("1.3.6.1.5.5.7.48.5", 1)) == NULL) errx(1, "OBJ_txt2obj for %s failed", "1.3.6.1.5.5.7.48.5"); if ((manifest_oid = OBJ_txt2obj("1.3.6.1.5.5.7.48.10", 1)) == NULL)
