On Mon, Sep 25, 2023 at 02:47:37PM +0200, Claudio Jeker wrote:
> On Sat, Sep 23, 2023 at 01:23:34PM +0200, Theo Buehler wrote:
> > This is a second chunk split out of the diff mentioned in my previous
> > mail. It factors the parsing of ASIdentifiers and IPAddrBlocks out of
> > sbgp_assysnum() and sbgp_ipaddrblk() and makes the latter only extract
> > the info from the X509_EXTENSION. This should not change anything, but
> > the logic is a bit tricky.
> >
> > We could initialize *as and *asz, as well as *ips and *ipsz to NULL/0,
> > at the top of the two new sbgp_parse_*.
>
> It looks inded like nthing is changed. The thing I dislike a bit is how
> **as and *asz are updated inside the sbgp_parse_* functions. There is
> return 0 before and after the calloc / recallocarray calls and so it
> depends a lot on the caller to be careful here. The code right now is ok.
Thanks for that clue. I didn't particularly like my diff either. The
below is better, has less churn and should be easier to review. This way
the caller doesn't have to be careful.
I left the currently existing variables asz and ipsz untouched since it
becomes too confusing. I want to rename asz -> sz and new_asz -> asz in
a follow-up, similarly for ipsz.
Index: cert.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.115
diff -u -p -r1.115 cert.c
--- cert.c 12 Sep 2023 09:33:30 -0000 1.115
+++ cert.c 25 Sep 2023 14:29:56 -0000
@@ -153,40 +153,26 @@ sbgp_as_inherit(const char *fn, struct c
return append_as(fn, ases, asz, &as);
}
-/*
- * Parse RFC 6487 4.8.11 X509v3 extension, with syntax documented in RFC
- * 3779 starting in section 3.2.
- * Returns zero on failure, non-zero on success.
- */
-static int
-sbgp_assysnum(struct parse *p, X509_EXTENSION *ext)
+int
+sbgp_parse_assysnum(const char *fn, const ASIdentifiers *asidentifiers,
+ struct cert_as **out_as, size_t *out_asz)
{
- ASIdentifiers *asidentifiers = NULL;
const ASIdOrRanges *aors = NULL;
- size_t asz;
- int i, rc = 0;
+ struct cert_as *as = NULL;
+ size_t asz, new_asz = 0;
+ int i;
- if (!X509_EXTENSION_get_critical(ext)) {
- warnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
- "extension not critical", p->fn);
- goto out;
- }
-
- if ((asidentifiers = X509V3_EXT_d2i(ext)) == NULL) {
- warnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
- "failed extension parse", p->fn);
- goto out;
- }
+ assert(*out_as == NULL && *out_asz == 0);
if (asidentifiers->rdi != NULL) {
warnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
- "should not have RDI values", p->fn);
+ "should not have RDI values", fn);
goto out;
}
if (asidentifiers->asnum == NULL) {
warnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
- "no AS number resource set", p->fn);
+ "no AS number resource set", fn);
goto out;
}
@@ -200,26 +186,25 @@ sbgp_assysnum(struct parse *p, X509_EXTE
break;
default:
warnx("%s: RFC 3779 section 3.2.3.2: ASIdentifierChoice: "
- "unknown type %d", p->fn, asidentifiers->asnum->type);
+ "unknown type %d", fn, asidentifiers->asnum->type);
goto out;
}
if (asz == 0) {
- warnx("%s: RFC 6487 section 4.8.11: empty asIdsOrRanges",
- p->fn);
+ warnx("%s: RFC 6487 section 4.8.11: empty asIdsOrRanges", fn);
goto out;
}
if (asz >= MAX_AS_SIZE) {
warnx("%s: too many AS number entries: limit %d",
- p->fn, MAX_AS_SIZE);
+ fn, MAX_AS_SIZE);
goto out;
}
- p->res->as = calloc(asz, sizeof(struct cert_as));
- if (p->res->as == NULL)
+ as = calloc(asz, sizeof(struct cert_as));
+ if (as == NULL)
err(1, NULL);
if (aors == NULL) {
- if (!sbgp_as_inherit(p->fn, p->res->as, &p->res->asz))
+ if (!sbgp_as_inherit(fn, as, &new_asz))
goto out;
}
@@ -229,22 +214,58 @@ sbgp_assysnum(struct parse *p, X509_EXTE
aor = sk_ASIdOrRange_value(aors, i);
switch (aor->type) {
case ASIdOrRange_id:
- if (!sbgp_as_id(p->fn, p->res->as, &p->res->asz,
- aor->u.id))
+ if (!sbgp_as_id(fn, as, &new_asz, aor->u.id))
goto out;
break;
case ASIdOrRange_range:
- if (!sbgp_as_range(p->fn, p->res->as, &p->res->asz,
- aor->u.range))
+ if (!sbgp_as_range(fn, as, &new_asz, aor->u.range))
goto out;
break;
default:
warnx("%s: RFC 3779 section 3.2.3.5: ASIdOrRange: "
- "unknown type %d", p->fn, aor->type);
+ "unknown type %d", fn, aor->type);
goto out;
}
}
+ *out_as = as;
+ *out_asz = new_asz;
+
+ return 1;
+
+ out:
+ free(as);
+
+ return 0;
+}
+
+/*
+ * Parse RFC 6487 4.8.11 X509v3 extension, with syntax documented in RFC
+ * 3779 starting in section 3.2.
+ * Returns zero on failure, non-zero on success.
+ */
+static int
+sbgp_assysnum(struct parse *p, X509_EXTENSION *ext)
+{
+ ASIdentifiers *asidentifiers = NULL;
+ int rc = 0;
+
+ if (!X509_EXTENSION_get_critical(ext)) {
+ warnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
+ "extension not critical", p->fn);
+ goto out;
+ }
+
+ if ((asidentifiers = X509V3_EXT_d2i(ext)) == NULL) {
+ warnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
+ "failed extension parse", p->fn);
+ goto out;
+ }
+
+ if (!sbgp_parse_assysnum(p->fn, asidentifiers,
+ &p->res->as, &p->res->asz))
+ goto out;
+
rc = 1;
out:
ASIdentifiers_free(asidentifiers);
@@ -331,33 +352,19 @@ sbgp_addr_inherit(const char *fn, struct
return append_ip(fn, ips, ipsz, &ip);
}
-/*
- * Parse an sbgp-ipAddrBlock X509 extension, RFC 6487 4.8.10, with
- * syntax documented in RFC 3779 starting in section 2.2.
- * Returns zero on failure, non-zero on success.
- */
-static int
-sbgp_ipaddrblk(struct parse *p, X509_EXTENSION *ext)
+int
+sbgp_parse_ipaddrblk(const char *fn, const IPAddrBlocks *addrblk,
+ struct cert_ip **out_ips, size_t *out_ipsz)
{
- STACK_OF(IPAddressFamily) *addrblk = NULL;
- const IPAddressFamily *af;
- const IPAddressOrRanges *aors;
- const IPAddressOrRange *aor;
- enum afi afi;
- size_t ipsz;
- int i, j, rc = 0;
+ const IPAddressFamily *af;
+ const IPAddressOrRanges *aors;
+ const IPAddressOrRange *aor;
+ enum afi afi;
+ struct cert_ip *ips = NULL;
+ size_t ipsz, new_ipsz = 0;
+ int i, j;
- if (!X509_EXTENSION_get_critical(ext)) {
- warnx("%s: RFC 6487 section 4.8.10: sbgp-ipAddrBlock: "
- "extension not critical", p->fn);
- goto out;
- }
-
- if ((addrblk = X509V3_EXT_d2i(ext)) == NULL) {
- warnx("%s: RFC 6487 section 4.8.10: sbgp-ipAddrBlock: "
- "failed extension parse", p->fn);
- goto out;
- }
+ assert(*out_ips == NULL && *out_ipsz == 0);
for (i = 0; i < sk_IPAddressFamily_num(addrblk); i++) {
af = sk_IPAddressFamily_value(addrblk, i);
@@ -365,38 +372,37 @@ sbgp_ipaddrblk(struct parse *p, X509_EXT
switch (af->ipAddressChoice->type) {
case IPAddressChoice_inherit:
aors = NULL;
- ipsz = p->res->ipsz + 1;
+ ipsz = new_ipsz + 1;
break;
case IPAddressChoice_addressesOrRanges:
aors = af->ipAddressChoice->u.addressesOrRanges;
- ipsz = p->res->ipsz + sk_IPAddressOrRange_num(aors);
+ ipsz = new_ipsz + sk_IPAddressOrRange_num(aors);
break;
default:
warnx("%s: RFC 3779: IPAddressChoice: unknown type %d",
- p->fn, af->ipAddressChoice->type);
+ fn, af->ipAddressChoice->type);
goto out;
}
- if (ipsz == p->res->ipsz) {
+ if (ipsz == new_ipsz) {
warnx("%s: RFC 6487 section 4.8.10: "
- "empty ipAddressesOrRanges", p->fn);
+ "empty ipAddressesOrRanges", fn);
goto out;
}
if (ipsz >= MAX_IP_SIZE)
goto out;
- p->res->ips = recallocarray(p->res->ips, p->res->ipsz, ipsz,
+ ips = recallocarray(ips, new_ipsz, ipsz,
sizeof(struct cert_ip));
- if (p->res->ips == NULL)
+ if (ips == NULL)
err(1, NULL);
- if (!ip_addr_afi_parse(p->fn, af->addressFamily, &afi)) {
- warnx("%s: RFC 3779: invalid AFI", p->fn);
+ if (!ip_addr_afi_parse(fn, af->addressFamily, &afi)) {
+ warnx("%s: RFC 3779: invalid AFI", fn);
goto out;
}
if (aors == NULL) {
- if (!sbgp_addr_inherit(p->fn, p->res->ips,
- &p->res->ipsz, afi))
+ if (!sbgp_addr_inherit(fn, ips, &new_ipsz, afi))
goto out;
continue;
}
@@ -405,22 +411,59 @@ sbgp_ipaddrblk(struct parse *p, X509_EXT
aor = sk_IPAddressOrRange_value(aors, j);
switch (aor->type) {
case IPAddressOrRange_addressPrefix:
- if (!sbgp_addr(p->fn, p->res->ips,
- &p->res->ipsz, afi, aor->u.addressPrefix))
+ if (!sbgp_addr(fn, ips, &new_ipsz, afi,
+ aor->u.addressPrefix))
goto out;
break;
case IPAddressOrRange_addressRange:
- if (!sbgp_addr_range(p->fn, p->res->ips,
- &p->res->ipsz, afi, aor->u.addressRange))
+ if (!sbgp_addr_range(fn, ips, &new_ipsz, afi,
+ aor->u.addressRange))
goto out;
break;
default:
warnx("%s: RFC 3779: IPAddressOrRange: "
- "unknown type %d", p->fn, aor->type);
+ "unknown type %d", fn, aor->type);
goto out;
}
}
}
+
+ *out_ips = ips;
+ *out_ipsz = new_ipsz;
+
+ return 1;
+
+ out:
+ free(ips);
+
+ return 0;
+}
+
+/*
+ * Parse an sbgp-ipAddrBlock X509 extension, RFC 6487 4.8.10, with
+ * syntax documented in RFC 3779 starting in section 2.2.
+ * Returns zero on failure, non-zero on success.
+ */
+static int
+sbgp_ipaddrblk(struct parse *p, X509_EXTENSION *ext)
+{
+ STACK_OF(IPAddressFamily) *addrblk = NULL;
+ int rc = 0;
+
+ if (!X509_EXTENSION_get_critical(ext)) {
+ warnx("%s: RFC 6487 section 4.8.10: sbgp-ipAddrBlock: "
+ "extension not critical", p->fn);
+ goto out;
+ }
+
+ if ((addrblk = X509V3_EXT_d2i(ext)) == NULL) {
+ warnx("%s: RFC 6487 section 4.8.10: sbgp-ipAddrBlock: "
+ "failed extension parse", p->fn);
+ goto out;
+ }
+
+ if (!sbgp_parse_ipaddrblk(p->fn, addrblk, &p->res->ips, &p->res->ipsz))
+ goto out;
if (p->res->ipsz == 0) {
warnx("%s: RFC 6487 section 4.8.10: empty ipAddrBlock", p->fn);
