On Tue, Apr 05, 2022 at 06:33:35PM +0200, Theo Buehler wrote:
> Instead of manually unpacking the SIA extension with super low-level
> ASN.1 fiddling, we can let the templated ASN.1 in libcrypto do this work
> for us, which makes the code quite a bit simpler. This resolves one
> FIXME and removes one use of the magic ASN1_frame().
>
> I kept the current split of the functions to avoid noise and make the
> diff easier to review. I'm going to undo this split in a follow-up.
>
> The relevant structs and typedefs are in x509/x509v3.h and the ASN.1
> templates are in x509/x509_info.c if you want to take a look at that.
>
> The main point is the X509V3_EXT_d2i() in sbgp_sia(), which deserializes
> the extension based on its OID, which was checked in cert_parse_pre().
> We (ab)use the AUTHORITY_INFO_ACCESS type since there is unfortunately
> no SUBJECT_INFO_ACCESS type (the extensions have the same syntax, see
> RFC 5280, 4.2.2). This is documented in AUTHORITY_INFO_ACCESS_new(3).
> A typedef might be appropriate here.
>
> AUTHORITY_INFO_ACCESS is a STACK_OF(ACCESS_DESCRIPTION), the unpacking
> of which is done in sbgp_sia_resource().
I'm not too woried about this, an option could be to use
STACK_OF(ACCESS_DESCRIPTION) directly but I'm totally fine with this.
> ACCESS_DESCRIPTION has an oid (ASN1_OBJECT) called method and a
> GENERAL_NAME called location. All three resources we currently care
> about are of the URI type, the new GEN_URI check makes sure of that
> (the discarded ptag argument of ASN1_frame() should have been checked
> to be GEN_URI).
>
> Overall, I think the resulting code is a lot easier to follow and more
> correct.
This is a lot cleaner and indeed an improvement. I think some of the rc
handling can also be simplified. The code in sbgp_sia_resource_entry()
and sbgp_sia_resource() no longer require cleanup on error so we can just
return 0 instead of goto out. It is OK to do this cleanup in a 2nd step
(which you probably already planned).
OK claudio@
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.62
> diff -u -p -r1.62 cert.c
> --- cert.c 5 Apr 2022 03:56:20 -0000 1.62
> +++ cert.c 5 Apr 2022 04:54:18 -0000
> @@ -214,63 +214,45 @@ sbgp_sia_resource_carepo(struct parse *p
> * Returns zero on failure, non-zero on success.
> */
> static int
> -sbgp_sia_resource_entry(struct parse *p,
> - const unsigned char *d, size_t dsz)
> +sbgp_sia_resource_entry(struct parse *p, ACCESS_DESCRIPTION *ad)
> {
> - ASN1_SEQUENCE_ANY *seq;
> ASN1_OBJECT *oid;
> - const ASN1_TYPE *t;
> - int rc = 0, ptag;
> - long plen;
> + ASN1_IA5STRING *uri = NULL;
> + int rc = 0;
>
> - if ((seq = d2i_ASN1_SEQUENCE_ANY(NULL, &d, dsz)) == NULL) {
> - cryptowarnx("%s: RFC 6487 section 4.8.8: SIA: "
> - "failed ASN.1 sequence parse", p->fn);
> - goto out;
> - }
> - if (sk_ASN1_TYPE_num(seq) != 2) {
> - warnx("%s: RFC 6487 section 4.8.8: SIA: "
> - "want 2 elements, have %d",
> - p->fn, sk_ASN1_TYPE_num(seq));
> - goto out;
> - }
> + if (ad->location->type == GEN_URI)
> + uri = ad->location->d.uniformResourceIdentifier;
>
> - /* Composed of an OID and its continuation. */
> + oid = ad->method;
>
> - t = sk_ASN1_TYPE_value(seq, 0);
> - if (t->type != V_ASN1_OBJECT) {
> - warnx("%s: RFC 6487 section 4.8.8: SIA: "
> - "want ASN.1 object, have %s (NID %d)",
> - p->fn, ASN1_tag2str(t->type), t->type);
> - goto out;
> - }
> - oid = t->value.object;
> -
> - t = sk_ASN1_TYPE_value(seq, 1);
> - if (t->type != V_ASN1_OTHER) {
> - warnx("%s: RFC 6487 section 4.8.8: SIA: "
> - "want ASN.1 external, have %s (NID %d)",
> - p->fn, ASN1_tag2str(t->type), t->type);
> - goto out;
> + if (OBJ_cmp(oid, carepo_oid) == 0) {
> + if (uri == NULL) {
> + warnx("%s: RFC 6487: 4.8.8.1 caRepository without URI",
> + p->fn);
> + goto out;
> + }
> + if (!sbgp_sia_resource_carepo(p, uri->data, uri->length))
> + goto out;
> + } else if (OBJ_cmp(oid, manifest_oid) == 0) {
> + if (uri == NULL) {
> + warnx("%s: RFC 6487: 4.8.8 SIA manifest without URI",
> + p->fn);
> + goto out;
> + }
> + if (!sbgp_sia_resource_mft(p, uri->data, uri->length))
> + goto out;
> + } else if (OBJ_cmp(oid, notify_oid) == 0) {
> + if (uri == NULL) {
> + warnx("%s: RFC 6487: 4.8.8 SIA resourceNnotify "
> + "without URI", p->fn);
> + goto out;
> + }
> + if (!sbgp_sia_resource_notify(p, uri->data, uri->length))
> + goto out;
> }
>
> - /* FIXME: there must be a way to do this without ASN1_frame. */
> -
> - d = t->value.asn1_string->data;
> - dsz = t->value.asn1_string->length;
> - if (!ASN1_frame(p->fn, dsz, &d, &plen, &ptag))
> - goto out;
> -
> - if (OBJ_cmp(oid, carepo_oid) == 0)
> - rc = sbgp_sia_resource_carepo(p, d, plen);
> - else if (OBJ_cmp(oid, manifest_oid) == 0)
> - rc = sbgp_sia_resource_mft(p, d, plen);
> - else if (OBJ_cmp(oid, notify_oid) == 0)
> - rc = sbgp_sia_resource_notify(p, d, plen);
> - else
> - rc = 1; /* silently ignore */
> -out:
> - sk_ASN1_TYPE_pop_free(seq, ASN1_TYPE_free);
> + rc = 1;
> + out:
> return rc;
> }
>
> @@ -279,29 +261,14 @@ out:
> * Returns zero on failure, non-zero on success.
> */
> static int
> -sbgp_sia_resource(struct parse *p, const unsigned char *d, size_t dsz)
> +sbgp_sia_resource(struct parse *p, AUTHORITY_INFO_ACCESS *sia)
> {
> - ASN1_SEQUENCE_ANY *seq;
> - const ASN1_TYPE *t;
> - int rc = 0, i;
> + ACCESS_DESCRIPTION *ad;
> + int i, rc = 0;
>
> - if ((seq = d2i_ASN1_SEQUENCE_ANY(NULL, &d, dsz)) == NULL) {
> - cryptowarnx("%s: RFC 6487 section 4.8.8: SIA: "
> - "failed ASN.1 sequence parse", p->fn);
> - goto out;
> - }
> -
> - for (i = 0; i < sk_ASN1_TYPE_num(seq); i++) {
> - t = sk_ASN1_TYPE_value(seq, i);
> - if (t->type != V_ASN1_SEQUENCE) {
> - warnx("%s: RFC 6487 section 4.8.8: SIA: "
> - "want ASN.1 sequence, have %s (NID %d)",
> - p->fn, ASN1_tag2str(t->type), t->type);
> - goto out;
> - }
> - d = t->value.asn1_string->data;
> - dsz = t->value.asn1_string->length;
> - if (!sbgp_sia_resource_entry(p, d, dsz))
> + for (i = 0; i < sk_ACCESS_DESCRIPTION_num(sia); i++) {
> + ad = sk_ACCESS_DESCRIPTION_value(sia, i);
> + if (!sbgp_sia_resource_entry(p, ad))
> goto out;
> }
>
> @@ -317,9 +284,9 @@ sbgp_sia_resource(struct parse *p, const
> p->fn);
> goto out;
> }
> +
> rc = 1;
> -out:
> - sk_ASN1_TYPE_pop_free(seq, ASN1_TYPE_free);
> + out:
> return rc;
> }
>
> @@ -330,11 +297,8 @@ out:
> static int
> sbgp_sia(struct parse *p, X509_EXTENSION *ext)
> {
> - unsigned char *sv = NULL;
> - const unsigned char *d;
> - ASN1_SEQUENCE_ANY *seq = NULL;
> - const ASN1_TYPE *t;
> - int dsz, rc = 0;
> + AUTHORITY_INFO_ACCESS *sia = NULL;
> + int rc = 0;
>
> if (X509_EXTENSION_get_critical(ext)) {
> warnx("%s: RFC 6487 section 4.8.8: SIA: "
> @@ -342,57 +306,17 @@ sbgp_sia(struct parse *p, X509_EXTENSION
> goto out;
> }
>
> - if ((dsz = i2d_X509_EXTENSION(ext, &sv)) < 0) {
> + if ((sia = X509V3_EXT_d2i(ext)) == NULL) {
> cryptowarnx("%s: RFC 6487 section 4.8.8: SIA: "
> "failed extension parse", p->fn);
> goto out;
> }
> - d = sv;
> -
> - if ((seq = d2i_ASN1_SEQUENCE_ANY(NULL, &d, dsz)) == NULL) {
> - cryptowarnx("%s: RFC 6487 section 4.8.8: SIA: "
> - "failed ASN.1 sequence parse", p->fn);
> - goto out;
> - }
> - if (sk_ASN1_TYPE_num(seq) != 2) {
> - warnx("%s: RFC 6487 section 4.8.8: SIA: "
> - "want 2 elements, have %d", p->fn,
> - sk_ASN1_TYPE_num(seq));
> - goto out;
> - }
> -
> - t = sk_ASN1_TYPE_value(seq, 0);
> - if (t->type != V_ASN1_OBJECT) {
> - warnx("%s: RFC 6487 section 4.8.8: SIA: "
> - "want ASN.1 object, have %s (NID %d)",
> - p->fn, ASN1_tag2str(t->type), t->type);
> - goto out;
> - }
> - if (OBJ_obj2nid(t->value.object) != NID_sinfo_access) {
> - warnx("%s: RFC 6487 section 4.8.8: SIA: "
> - "incorrect OID, have %s (NID %d)", p->fn,
> - ASN1_tag2str(OBJ_obj2nid(t->value.object)),
> - OBJ_obj2nid(t->value.object));
> - goto out;
> - }
> -
> - t = sk_ASN1_TYPE_value(seq, 1);
> - if (t->type != V_ASN1_OCTET_STRING) {
> - warnx("%s: RFC 6487 section 4.8.8: SIA: "
> - "want ASN.1 octet string, have %s (NID %d)",
> - p->fn, ASN1_tag2str(t->type), t->type);
> - goto out;
> - }
> -
> - d = t->value.octet_string->data;
> - dsz = t->value.octet_string->length;
> - if (!sbgp_sia_resource(p, d, dsz))
> + if (!sbgp_sia_resource(p, sia))
> goto out;
>
> rc = 1;
> -out:
> - sk_ASN1_TYPE_pop_free(seq, ASN1_TYPE_free);
> - free(sv);
> + out:
> + AUTHORITY_INFO_ACCESS_free(sia);
> return rc;
> }
>
>
--
:wq Claudio
