On Fri, Feb 04, 2022 at 03:56:18PM +0100, Theo Buehler wrote: > 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().
Sounds good to me. A comment below to that. > > > + 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. I think the assert() is OK. I just want to make sure we don't abuse assert() for proper error checking. There were some of those in the codebase and in the end less assert() calls are preferred. > > > + 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. Yes, I came to a similar conclusion after reading too much of rfc5280 for my taste. Maybe something to reconsider once rfc8360 is needed (I still hope it will just die and instead the original policy will be updated). > 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)); Since you use OBJ_cmp() here because NID are undefined shouldn't this code try to avoid NID and use OBJ_obj2txt() instead? At least for OBJ_nid2sn(). Apart the diff still reads fine. OK claudio@ -- :wq Claudio
