On Thu, Sep 09, 2021 at 02:51:24PM +0200, Claudio Jeker wrote:
> Trying to remove work that is done over and over again.
> One of those checks are the various OID compares.
> Instead of converting the ASN1_OBJECT into a string and comparing the
> strings, convert the string into an ASN1_OBJECT once and then compare
> these objects with OBJ_cmp().
>
> Any comments about this?
Makes sense to me. Not only does it save some work, it also makes the
code more readable.
ok tb (some comments inline)
> --
> :wq Claudio
>
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 cert.c
> --- cert.c 13 Jul 2021 18:39:39 -0000 1.31
> +++ cert.c 9 Sep 2021 12:25:47 -0000
> @@ -46,6 +46,21 @@ struct parse {
> const char *fn; /* currently-parsed file */
> };
>
> +static ASN1_OBJECT *carepo_oid; /* 1.3.6.1.5.5.7.48.5 (caRepository) */
> +static ASN1_OBJECT *mft_oid; /* 1.3.6.1.5.5.7.48.10 (rpkiManifest) */
> +static ASN1_OBJECT *notify_oid; /* 1.3.6.1.5.5.7.48.13 (rpkiNotify) */
> +
> +static void
> +cert_init_oid(void)
> +{
> + if ((carepo_oid = OBJ_txt2obj("1.3.6.1.5.5.7.48.5", 1)) == NULL)
> + errx(1, "OBJ_txt2obj for %s failed", "1.3.6.1.5.5.7.48.5");
> + if ((mft_oid = OBJ_txt2obj("1.3.6.1.5.5.7.48.10", 1)) == NULL)
> + errx(1, "OBJ_txt2obj for %s failed", "1.3.6.1.5.5.7.48.10");
> + if ((notify_oid = OBJ_txt2obj("1.3.6.1.5.5.7.48.13", 1)) == NULL)
> + errx(1, "OBJ_txt2obj for %s failed", "1.3.6.1.5.5.7.48.13");
> +}
> +
> /*
> * Append an IP address structure to our list of results.
> * This will also constrain us to having at most one inheritence
> @@ -207,9 +222,9 @@ sbgp_sia_resource_entry(struct parse *p,
> const unsigned char *d, size_t dsz)
> {
> ASN1_SEQUENCE_ANY *seq;
> + ASN1_OBJECT *oid;
> const ASN1_TYPE *t;
> int rc = 0, ptag;
> - char buf[128];
> long plen;
>
> if ((seq = d2i_ASN1_SEQUENCE_ANY(NULL, &d, dsz)) == NULL) {
> @@ -233,7 +248,7 @@ sbgp_sia_resource_entry(struct parse *p,
> p->fn, ASN1_tag2str(t->type), t->type);
> goto out;
> }
> - OBJ_obj2txt(buf, sizeof(buf), t->value.object, 1);
> + oid = t->value.object;
>
> t = sk_ASN1_TYPE_value(seq, 1);
> if (t->type != V_ASN1_OTHER) {
> @@ -257,11 +272,14 @@ sbgp_sia_resource_entry(struct parse *p,
> * - 1.3.6.1.5.5.7.48.10 (rpkiManifest)
> * - 1.3.6.1.5.5.7.48.13 (rpkiNotify)
The above comment seems somewhat stale. I'd at least drop the OIDs, now
that they no longer explicitly appear in the code below.
> */
> - if (strcmp(buf, "1.3.6.1.5.5.7.48.5") == 0)
> + if (carepo_oid == NULL)
> + cert_init_oid();
> +
> + if (OBJ_cmp(oid, carepo_oid) == 0)
> rc = sbgp_sia_resource_carepo(p, d, plen);
> - else if (strcmp(buf, "1.3.6.1.5.5.7.48.10") == 0)
> + else if (OBJ_cmp(oid, mft_oid) == 0)
> rc = sbgp_sia_resource_mft(p, d, plen);
> - else if (strcmp(buf, "1.3.6.1.5.5.7.48.13") == 0)
> + else if (OBJ_cmp(oid, notify_oid) == 0)
> rc = sbgp_sia_resource_notify(p, d, plen);
> else
> rc = 1; /* silently ignore */
> Index: cms.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cms.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 cms.c
> --- cms.c 13 Jul 2021 18:39:39 -0000 1.9
> +++ cms.c 9 Sep 2021 12:25:47 -0000
> @@ -35,16 +35,15 @@
> * Return the eContent as a string and set "rsz" to be its length.
> */
> unsigned char *
> -cms_parse_validate(X509 **xp, const char *fn,
> - const char *oid, size_t *rsz)
> +cms_parse_validate(X509 **xp, const char *fn, const ASN1_OBJECT *oid,
> + size_t *rsz)
> {
> const ASN1_OBJECT *obj;
> ASN1_OCTET_STRING **os = NULL;
> BIO *bio = NULL;
> CMS_ContentInfo *cms;
> FILE *f;
> - char buf[128];
> - int rc = 0, sz;
> + int rc = 0;
> STACK_OF(X509) *certs = NULL;
> unsigned char *res = NULL;
>
> @@ -84,16 +83,12 @@ cms_parse_validate(X509 **xp, const char
> /* RFC 6488 section 2.1.3.1: check the object's eContentType. */
>
> obj = CMS_get0_eContentType(cms);
I think a NULL check for obj would be appropriate (OBJ_cmp() will crash
on NULL). Previously, OBJ_obj2txt() would always return a NUL
terminated string, even if obj was NULL, so you'd hit the warnx() you
modify below.
> - if ((sz = OBJ_obj2txt(buf, sizeof(buf), obj, 1)) < 0)
> - cryptoerrx("OBJ_obj2txt");
Looks like you're removing a bogus check; I don't think OBJ_obj2txt()
can return < 0.
> -
> - if ((size_t)sz >= sizeof(buf)) {
> - warnx("%s: RFC 6488 section 2.1.3.1: "
> - "eContentType: OID too long", fn);
> - goto out;
> - } else if (strcmp(buf, oid)) {
> + if (OBJ_cmp(obj, oid) != 0) {
> + char buf[128], obuf[128];
I'd add an empty line
> + OBJ_obj2txt(buf, sizeof(buf), obj, 1);
> + OBJ_obj2txt(obuf, sizeof(obuf), oid, 1);
> warnx("%s: RFC 6488 section 2.1.3.1: eContentType: "
> - "unknown OID: %s, want %s", fn, buf, oid);
> + "unknown OID: %s, want %s", fn, buf, obuf);
> goto out;
> }
>
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.66
> diff -u -p -r1.66 extern.h
> --- extern.h 1 Sep 2021 08:09:41 -0000 1.66
> +++ extern.h 9 Sep 2021 12:25:47 -0000
> @@ -410,9 +410,9 @@ int valid_uri(const char *, size_t, co
>
> /* Working with CMS. */
> unsigned char *cms_parse_validate(X509 **, const char *,
> - const char *, size_t *);
> + const ASN1_OBJECT *, size_t *);
> int cms_econtent_version(const char *, const unsigned char **,
> - size_t, long *);
> + size_t, long *);
> /* Helper for ASN1 parsing */
> int ASN1_frame(const char *, size_t,
> const unsigned char **, long *, int *);
> Index: gbr.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/gbr.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 gbr.c
> --- gbr.c 29 Mar 2021 06:50:44 -0000 1.9
> +++ gbr.c 9 Sep 2021 12:25:47 -0000
> @@ -36,6 +36,8 @@ struct parse {
> struct gbr *res; /* results */
> };
>
> +static ASN1_OBJECT *gbr_oid;
> +
> /*
> * Parse a full RFC 6493 file and signed by the certificate "cacert"
> * (the latter is optional and may be passed as NULL to disable).
> @@ -52,9 +54,14 @@ gbr_parse(X509 **x509, const char *fn)
> p.fn = fn;
>
> /* OID from section 9.1, RFC 6493. */
> + if (gbr_oid == NULL) {
> + gbr_oid = OBJ_txt2obj("1.2.840.113549.1.9.16.1.35", 1);
> + if (gbr_oid == NULL)
> + errx(1, "OBJ_txt2obj for %s failed",
> + "1.2.840.113549.1.9.16.1.35");
> + }
>
> - cms = cms_parse_validate(x509, fn,
> - "1.2.840.113549.1.9.16.1.35", &cmsz);
> + cms = cms_parse_validate(x509, fn, gbr_oid, &cmsz);
> if (cms == NULL)
> return NULL;
>
> Index: mft.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
> retrieving revision 1.37
> diff -u -p -r1.37 mft.c
> --- mft.c 8 Sep 2021 16:37:20 -0000 1.37
> +++ mft.c 9 Sep 2021 12:25:47 -0000
> @@ -40,6 +40,8 @@ struct parse {
> struct mft *res; /* result object */
> };
>
> +static ASN1_OBJECT *mft_oid;
> +
> static const char *
> gentime2str(const ASN1_GENERALIZEDTIME *time)
> {
> @@ -417,8 +419,14 @@ mft_parse(X509 **x509, const char *fn)
> memset(&p, 0, sizeof(struct parse));
> p.fn = fn;
>
> - cms = cms_parse_validate(x509, fn, "1.2.840.113549.1.9.16.1.26",
> - &cmsz);
> + if (mft_oid == NULL) {
> + mft_oid = OBJ_txt2obj("1.2.840.113549.1.9.16.1.26", 1);
> + if (mft_oid == NULL)
> + errx(1, "OBJ_txt2obj for %s failed",
> + "1.2.840.113549.1.9.16.1.26");
> + }
> +
> + cms = cms_parse_validate(x509, fn, mft_oid, &cmsz);
> if (cms == NULL)
> return NULL;
> assert(*x509 != NULL);
> Index: roa.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/roa.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 roa.c
> --- roa.c 8 Sep 2021 16:37:20 -0000 1.24
> +++ roa.c 9 Sep 2021 12:25:47 -0000
> @@ -36,6 +36,8 @@ struct parse {
> struct roa *res; /* results */
> };
>
> +static ASN1_OBJECT *roa_oid;
> +
> /*
> * Parse IP address (ROAIPAddress), RFC 6482, section 3.3.
> * Returns zero on failure, non-zero on success.
> @@ -339,9 +341,14 @@ roa_parse(X509 **x509, const char *fn)
> p.fn = fn;
>
> /* OID from section 2, RFC 6482. */
> + if (roa_oid == NULL) {
> + roa_oid = OBJ_txt2obj("1.2.840.113549.1.9.16.1.24", 1);
> + if (roa_oid == NULL)
> + errx(1, "OBJ_txt2obj for %s failed",
> + "1.2.840.113549.1.9.16.1.24");
> + }
>
> - cms = cms_parse_validate(x509, fn,
> - "1.2.840.113549.1.9.16.1.24", &cmsz);
> + cms = cms_parse_validate(x509, fn, roa_oid, &cmsz);
> if (cms == NULL)
> return NULL;
>
>
