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@
Few questions below.
> 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 09:09:38 -0000
> @@ -29,6 +29,7 @@
>
> #include <openssl/asn1.h>
> #include <openssl/x509.h>
> +#include <openssl/x509v3.h>
>
> #include "extern.h"
>
> @@ -969,6 +970,77 @@ 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);
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.
> + if ((nid = OBJ_obj2nid(policy->policyid)) != NID_ipAddr_asNumber) {
> + 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);
Same as above.
> + 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 +1096,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 00:15:02 -0000
> @@ -43,9 +43,10 @@ static void build_chain(const struct a
> static struct crl *get_crl(const struct auth *);
> static void build_crls(const struct crl *, STACK_OF(X509_CRL) **);
>
> -static X509_STORE_CTX *ctx;
> -static struct auth_tree auths = RB_INITIALIZER(&auths);
> -static struct crl_tree crlt = RB_INITIALIZER(&crlt);
> +static X509_STORE_CTX *ctx;
> +static STACK_OF(ASN1_OBJECT) *cert_policies;
> +static struct auth_tree auths = RB_INITIALIZER(&auths);
> +static struct crl_tree crlt = RB_INITIALIZER(&crlt);
>
> struct parse_repo {
> RB_ENTRY(parse_repo) entry;
> @@ -206,6 +207,7 @@ static int
> valid_x509(char *file, X509 *x509, struct auth *a, struct crl *crl,
> unsigned long flags, int nowarn)
> {
> + X509_VERIFY_PARAM *params;
> STACK_OF(X509) *chain;
> STACK_OF(X509_CRL) *crls = NULL;
> int c;
> @@ -217,11 +219,17 @@ 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 (!X509_VERIFY_PARAM_set1_policies(params, cert_policies))
> + cryptoerrx("X509_VERIFY_PARAM_set1_policies");
> +
> 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;
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.
> + 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);
> @@ -1116,6 +1124,7 @@ parse_file(struct entityq *q, struct msg
> void
> proc_parser(int fd)
> {
> + ASN1_OBJECT *cp_oid;
> struct entityq q;
> struct msgbuf msgq;
> struct pollfd pfd;
> @@ -1130,6 +1139,13 @@ proc_parser(int fd)
> if ((ctx = X509_STORE_CTX_new()) == NULL)
> cryptoerrx("X509_STORE_CTX_new");
>
> + if ((cp_oid = OBJ_nid2obj(NID_ipAddr_asNumber)) == NULL)
> + cryptoerrx("OBJ_nid2obj");
> + if ((cert_policies = sk_ASN1_OBJECT_new_null()) == NULL)
> + cryptoerrx("sk_ASN1_OBJECT_new_null");
> + if (!sk_ASN1_OBJECT_push(cert_policies, cp_oid))
> + cryptoerrx("sk_ASN1_OBJECT_push");
> +
> TAILQ_INIT(&q);
>
> msgbuf_init(&msgq);
> @@ -1190,6 +1206,7 @@ proc_parser(int fd)
> /* XXX free auths and crl tree */
>
> X509_STORE_CTX_free(ctx);
> + sk_ASN1_OBJECT_pop_free(cert_policies, ASN1_OBJECT_free);
> msgbuf_clear(&msgq);
>
> exit(0);
>
--
:wq Claudio
