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.
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) {
