On Sat, Aug 13, 2022 at 04:51:05PM +0200, Theo Buehler wrote:
> On Fri, Aug 12, 2022 at 09:59:11PM +0200, Theo Buehler wrote:
> > On Wed, Aug 10, 2022 at 06:16:30PM +0200, Theo Buehler wrote:
> > > On Wed, Aug 10, 2022 at 03:10:19PM +0000, Job Snijders wrote:
> > > > Hi all,
> > > >
> > > > An errata exists for RFC 6482, which informs us: """The EE certificate
> > > > MUST NOT use "inherit" elements as described in [RFC3779].""" Read the
> > > > full report here: https://www.rfc-editor.org/errata/eid3166
> > > >
> > > > Although it might seem a bit 'wasteful' to d2i the IP Resources
> > > > extension in multiple places, noodling through parameters when to check
> > > > for inheritance and when not to check didn't improve code readability.
> > > > I'm open to suggestions how to perform this check differently.
> > >
> > > As I understand it, what really is missing isn't a check for inheritance
> > > per se, but rather a check whether the prefixes in the ROA are covered
> > > by the EE cert's IP address delegation extension (the bullet point in
> > > RFC 6482, section 4). If we had such a check, that would be the natural
> > > place for adding an inheritance check for the EE cert.
> > >
> > > Below is my "overclaim" diff from a few weeks back that prepended the EE
> > > cert to the auth chain for ROAs and RSCs so that we check their
> > > resources against the EE cert instead of our currently incorrect checks
> > > that permitted overclaiming. The diff was ok job and claudio told me
> > > that it looked ok - I will need to think it through in detail once more,
> > > however.
> > >
> > > I believe that with something like this diff, your desired inheritance
> > > check should be added to valid_roa() above the for() loop.
> > >
> > > Does that make sense?
> >
> > Here's a less intrusive version of this diff that parses the RFC 3779
> > extensions of the EE cert "by hand". That's all we need for proper
> > verification of the resources in ROAs and RSCs. Maybe that's preferable.
> >
> > If we decide to go with this diff, the "no AS numbers for ROA EE certs"
> > and the "no inheritance" check should be moved or added to the XXX.
>
> job mentioned that it might be preferable to do the validation in
> parse_{roa,rsc,aspa}(). So here's a diff that does this. It reworks
> valid_{roa,rsc}() to compare only against the EE cert's resources since
> it doesn't really make sense to walk the auth chain for this anyway.
> That the EE cert's resources are covered by the auth chain is checked
> later as part of valid_x509().
>
> Inheritance in the EE cert will now result in a warning and the roa/rsc
> won't be considered valid.
For me this diff is OK. At least it is the best we can do without totally
refactoring a lot more code. Having to work with struct cert in this case
is a bit annoying but the functions only work this way.
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.84
> diff -u -p -r1.84 cert.c
> --- cert.c 31 May 2022 18:51:35 -0000 1.84
> +++ cert.c 13 Aug 2022 14:34:38 -0000
> @@ -571,6 +571,49 @@ certificate_policies(struct parse *p, X5
> }
>
> /*
> + * Lightweight version of cert_parse_pre() for ASPA, ROA, and RSC certs.
> + * This only parses the RFC 3779 extensions since these are necessary for
> + * validation.
> + * Returns cert on success and NULL on failure.
> + */
> +struct cert *
> +cert_parse_ee_cert(const char *fn, X509 *x)
> +{
> + struct parse p;
> + X509_EXTENSION *ext;
> + int index;
> +
> + memset(&p, 0, sizeof(struct parse));
> + p.fn = fn;
> + if ((p.res = calloc(1, sizeof(struct cert))) == NULL)
> + err(1, NULL);
> +
> + index = X509_get_ext_by_NID(x, NID_sbgp_ipAddrBlock, -1);
> + if ((ext = X509_get_ext(x, index)) != NULL) {
> + if (!sbgp_ipaddrblk(&p, ext))
> + goto out;
> + }
> +
> + index = X509_get_ext_by_NID(x, NID_sbgp_autonomousSysNum, -1);
> + if ((ext = X509_get_ext(x, index)) != NULL) {
> + if (!sbgp_assysnum(&p, ext))
> + goto out;
> + }
> +
> + if (!X509_up_ref(x)) {
> + cryptowarnx("%s: X509_up_ref failed", fn);
> + goto out;
> + }
> +
> + p.res->x509 = x;
> + return p.res;
> +
> + out:
> + cert_free(p.res);
> + return NULL;
> +}
> +
> +/*
> * Parse and partially validate an RPKI X509 certificate (either a trust
> * anchor or a certificate) as defined in RFC 6487.
> * Returns the parse results or NULL on failure.
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.147
> diff -u -p -r1.147 extern.h
> --- extern.h 10 Aug 2022 10:27:03 -0000 1.147
> +++ extern.h 13 Aug 2022 14:34:38 -0000
> @@ -465,6 +465,7 @@ struct tal *tal_read(struct ibuf *);
>
> void cert_buffer(struct ibuf *, const struct cert *);
> void cert_free(struct cert *);
> +struct cert *cert_parse_ee_cert(const char *, X509 *);
> struct cert *cert_parse_pre(const char *, const unsigned char *, size_t);
> struct cert *cert_parse(const char *, struct cert *);
> struct cert *ta_parse(const char *, struct cert *, const unsigned char *,
> @@ -509,7 +510,7 @@ struct auth *valid_ski_aki(const char *,
> int valid_ta(const char *, struct auth_tree *,
> const struct cert *);
> int valid_cert(const char *, struct auth *, const struct cert *);
> -int valid_roa(const char *, struct auth *, struct roa *);
> +int valid_roa(const char *, struct cert *, struct roa *);
> int valid_filehash(int, const char *, size_t);
> int valid_hash(unsigned char *, size_t, const char *, size_t);
> int valid_filename(const char *, size_t);
> @@ -517,7 +518,7 @@ int valid_uri(const char *, size_t, co
> int valid_origin(const char *, const char *);
> int valid_x509(char *, X509_STORE_CTX *, X509 *, struct auth *,
> struct crl *, int);
> -int valid_rsc(const char *, struct auth *, struct rsc *);
> +int valid_rsc(const char *, struct cert *, struct rsc *);
> int valid_econtent_version(const char *, const ASN1_INTEGER *);
>
> /* Working with CMS. */
> Index: filemode.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 filemode.c
> --- filemode.c 11 May 2022 14:42:01 -0000 1.7
> +++ filemode.c 13 Aug 2022 14:34:38 -0000
> @@ -392,9 +392,9 @@ proc_parser_file(char *file, unsigned ch
>
> if ((status = valid_x509(file, ctx, x509, a, c, 0))) {
> if (type == RTYPE_ROA)
> - status = valid_roa(file, a, roa);
> + status = roa->valid;
> else if (type == RTYPE_RSC)
> - status = valid_rsc(file, a, rsc);
> + status = rsc->valid;
> }
> if (status)
> printf("OK");
> Index: parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> retrieving revision 1.73
> diff -u -p -r1.73 parser.c
> --- parser.c 21 Apr 2022 12:59:03 -0000 1.73
> +++ parser.c 13 Aug 2022 14:34:38 -0000
> @@ -149,14 +149,6 @@ proc_parser_roa(char *file, const unsign
> roa->talid = a->cert->talid;
>
> /*
> - * If the ROA isn't valid, we accept it anyway and depend upon
> - * the code around roa_read() to check the "valid" field itself.
> - */
> -
> - if (valid_roa(file, a, roa))
> - roa->valid = 1;
> -
> - /*
> * Check CRL to figure out the soonest transitive expiry moment
> */
> if (crl != NULL && roa->expires > crl->expires)
> Index: roa.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/roa.c,v
> retrieving revision 1.49
> diff -u -p -r1.49 roa.c
> --- roa.c 10 Aug 2022 14:54:03 -0000 1.49
> +++ roa.c 13 Aug 2022 14:34:38 -0000
> @@ -202,6 +202,7 @@ struct roa *
> roa_parse(X509 **x509, const char *fn, const unsigned char *der, size_t len)
> {
> struct parse p;
> + struct cert *cert = NULL;
> size_t cmsz;
> unsigned char *cms;
> int rc = 0;
> @@ -229,11 +230,6 @@ roa_parse(X509 **x509, const char *fn, c
> goto out;
> }
>
> - if (X509_get_ext_by_NID(*x509, NID_sbgp_autonomousSysNum, -1) != -1) {
> - warnx("%s: superfluous AS Resources extension present", fn);
> - goto out;
> - }
> -
> at = X509_get0_notAfter(*x509);
> if (at == NULL) {
> warnx("%s: X509_get0_notAfter failed", fn);
> @@ -247,6 +243,20 @@ roa_parse(X509 **x509, const char *fn, c
> if (!roa_parse_econtent(cms, cmsz, &p))
> goto out;
>
> + if ((cert = cert_parse_ee_cert(fn, *x509)) == NULL)
> + goto out;
> +
> + if (cert->asz > 0) {
> + warnx("%s: superfluous AS Resources extension present", fn);
> + goto out;
> + }
> +
> + /*
> + * If the ROA isn't valid, we accept it anyway and depend upon
> + * the code around roa_read() to check the "valid" field itself.
> + */
> + p.res->valid = valid_roa(fn, cert, p.res);
> +
> rc = 1;
> out:
> if (rc == 0) {
> @@ -255,6 +265,7 @@ out:
> X509_free(*x509);
> *x509 = NULL;
> }
> + cert_free(cert);
> free(cms);
> return p.res;
> }
> Index: rsc.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/rsc.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 rsc.c
> --- rsc.c 10 Jun 2022 10:41:09 -0000 1.12
> +++ rsc.c 13 Aug 2022 14:34:38 -0000
> @@ -378,6 +378,7 @@ rsc_parse(X509 **x509, const char *fn, c
> unsigned char *cms;
> size_t cmsz;
> const ASN1_TIME *at;
> + struct cert *cert = NULL;
> int rc = 0;
>
> memset(&p, 0, sizeof(struct parse));
> @@ -412,9 +413,16 @@ rsc_parse(X509 **x509, const char *fn, c
> goto out;
> }
>
> + /* XXX - check that SIA is absent. */
> +
> if (!rsc_parse_econtent(cms, cmsz, &p))
> goto out;
>
> + if ((cert = cert_parse_ee_cert(fn, *x509)) == NULL)
> + goto out;
> +
> + p.res->valid = valid_rsc(fn, cert, p.res);
> +
> rc = 1;
> out:
> if (rc == 0) {
> @@ -423,6 +431,7 @@ rsc_parse(X509 **x509, const char *fn, c
> X509_free(*x509);
> *x509 = NULL;
> }
> + cert_free(cert);
> free(cms);
> return p.res;
> }
> Index: validate.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/validate.c,v
> retrieving revision 1.40
> diff -u -p -r1.40 validate.c
> --- validate.c 10 Jun 2022 10:36:43 -0000 1.40
> +++ validate.c 13 Aug 2022 14:34:38 -0000
> @@ -201,19 +201,19 @@ valid_cert(const char *fn, struct auth *
> * Returns 1 if valid, 0 otherwise.
> */
> int
> -valid_roa(const char *fn, struct auth *a, struct roa *roa)
> +valid_roa(const char *fn, struct cert *cert, struct roa *roa)
> {
> size_t i;
> char buf[64];
>
> for (i = 0; i < roa->ipsz; i++) {
> - if (valid_ip(a, roa->ips[i].afi, roa->ips[i].min,
> - roa->ips[i].max))
> + if (ip_addr_check_covered(roa->ips[i].afi, roa->ips[i].min,
> + roa->ips[i].max, cert->ips, cert->ipsz) > 0)
> continue;
> - ip_addr_print(&roa->ips[i].addr,
> - roa->ips[i].afi, buf, sizeof(buf));
> - warnx("%s: RFC 6482: uncovered IP: "
> - "%s", fn, buf);
> +
> + ip_addr_print(&roa->ips[i].addr, roa->ips[i].afi, buf,
> + sizeof(buf));
> + warnx("%s: RFC 6482: uncovered IP: %s", fn, buf);
> return 0;
> }
>
> @@ -442,7 +442,7 @@ valid_x509(char *file, X509_STORE_CTX *s
> * Returns 1 if valid, 0 otherwise.
> */
> int
> -valid_rsc(const char *fn, struct auth *a, struct rsc *rsc)
> +valid_rsc(const char *fn, struct cert *cert, struct rsc *rsc)
> {
> size_t i;
> uint32_t min, max;
> @@ -459,7 +459,7 @@ valid_rsc(const char *fn, struct auth *a
> max = rsc->as[i].type == CERT_AS_RANGE ? rsc->as[i].range.max
> : rsc->as[i].id;
>
> - if (valid_as(a, min, max))
> + if (as_check_covered(min, max, cert->as, cert->asz) > 0)
> continue;
>
> switch (rsc->as[i].type) {
> @@ -483,8 +483,8 @@ valid_rsc(const char *fn, struct auth *a
> return 0;
> }
>
> - if (valid_ip(a, rsc->ips[i].afi, rsc->ips[i].min,
> - rsc->ips[i].max))
> + if (ip_addr_check_covered(rsc->ips[i].afi, rsc->ips[i].min,
> + rsc->ips[i].max, cert->ips, cert->ipsz) > 0)
> continue;
>
> switch (rsc->ips[i].type) {
>
--
:wq Claudio
