On Fri, Apr 01, 2022 at 05:01:00PM +0200, Claudio Jeker wrote:
> cert_parse_inner() now only uses the ta flag to change behaviour of
> loading the various x509 extensions (AKI, SKI, AIA und CRL DP).
>
> This diff changes these functions to work always. Make AKI, AIA and CRL DP
> optional and have the code calling those functions check if they must have
> the extension. I modelled the functions after x509_get_expire() so they
> return 0 on failure and 1 on success.
This looks pretty good.
There is a certain asymmetry with the SKI handling that confused me a
bit. At first, the strcmp(p->aki, p->ski) in ta_parse() and cert_parse()
looked like potential NULL derefs since it looks as if p->ski isn't
checked.
The check for p.res->ski != NULL at the end of cert_parse_inner() is no
longer necessary since x509_get_ski() would have failed. Strictly
speaking, the ...->ski == NULL tests in {gbr,mft,roa}_parse() are also
no longer needed.
I would probably prefer to make x509_get_ski() behave the same way as
the AKI, AIA, CRL getters and add a p->ski != NULL check to ta_parse()
and cert_parse().
Couple more comments inline.
>
> Adjust the code to work with these new functions. Most checks for the
> optional attributes are already present.
> --
> :wq Claudio
>
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.58
> diff -u -p -r1.58 cert.c
> --- cert.c 1 Apr 2022 13:27:38 -0000 1.58
> +++ cert.c 1 Apr 2022 14:46:25 -0000
> @@ -1058,7 +1058,7 @@ certificate_policies(struct parse *p, X5
The doc comment still mentions "ta" and the long dead "xp", so it
could lose a bit of fat.
> * is also dereferenced.
> */
> static struct cert *
> -cert_parse_inner(const char *fn, const unsigned char *der, size_t len, int
> ta)
> +cert_parse_inner(const char *fn, const unsigned char *der, size_t len)
> {
> int rc = 0, extsz, c;
> int sia_present = 0;
> @@ -1132,12 +1132,14 @@ cert_parse_inner(const char *fn, const u
> goto out;
> }
>
> - p.res->aki = x509_get_aki(x, ta, p.fn);
> - p.res->ski = x509_get_ski(x, p.fn);
> - if (!ta) {
> - p.res->aia = x509_get_aia(x, p.fn);
> - p.res->crl = x509_get_crl(x, p.fn);
> - }
> + if (!x509_get_aki(x, p.fn, &p.res->aki))
> + goto out;
> + if (!x509_get_ski(x, p.fn, &p.res->ski))
> + goto out;
> + if (!x509_get_aia(x, p.fn, &p.res->aia))
> + goto out;
> + if (!x509_get_crl(x, p.fn, &p.res->crl))
> + goto out;
> if (!x509_get_expire(x, p.fn, &p.res->expires))
> goto out;
> p.res->purpose = x509_get_purpose(x, p.fn);
> @@ -1198,7 +1200,7 @@ cert_parse(const char *fn, const unsigne
> {
> struct cert *p;
>
> - if ((p = cert_parse_inner(fn, der, len, 0)) == NULL)
> + if ((p = cert_parse_inner(fn, der, len)) == NULL)
> return NULL;
>
> if (p->aki == NULL) {
> @@ -1212,8 +1214,12 @@ cert_parse(const char *fn, const unsigne
> goto badcert;
> }
> if (p->aia == NULL) {
> - warnx("%s: RFC 6487 section 8.4.7: "
> - "non-trust anchor missing AIA", fn);
> + warnx("%s: RFC 6487 section 8.4.7: AIA: extension missing", fn);
The warnings in this function aren't super consistent. We can clean this
up in a later pass.
> + goto badcert;
> + }
> + if (p->crl == NULL) {
> + warnx("%s: RFC 6487 section 4.8.6: CRL: "
> + "no CRL distribution point extension", fn);
> goto badcert;
> }
> return p;
> @@ -1231,7 +1237,7 @@ ta_parse(const char *fn, const unsigned
> EVP_PKEY *pk = NULL, *opk = NULL;
> struct cert *p;
>
> - if ((p = cert_parse_inner(fn, der, len, 1)) == NULL)
> + if ((p = cert_parse_inner(fn, der, len)) == NULL)
> return NULL;
>
> /* first check pubkey against the one from the TAL */
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.122
> diff -u -p -r1.122 extern.h
> --- extern.h 31 Mar 2022 12:00:00 -0000 1.122
> +++ extern.h 1 Apr 2022 13:46:32 -0000
> @@ -578,11 +578,11 @@ struct ibuf *io_buf_recvfd(int, struct i
> /* X509 helpers. */
>
> void x509_init_oid(void);
> -char *x509_get_aia(X509 *, const char *);
> -char *x509_get_aki(X509 *, int, const char *);
> -char *x509_get_ski(X509 *, const char *);
> +int x509_get_aia(X509 *, const char *, char **);
> +int x509_get_aki(X509 *, const char *, char **);
> +int x509_get_ski(X509 *, const char *, char **);
> int x509_get_expire(X509 *, const char *, time_t *);
> -char *x509_get_crl(X509 *, const char *);
> +int x509_get_crl(X509 *, const char *, char **);
> char *x509_crl_get_aki(X509_CRL *, const char *);
> char *x509_get_pubkey(X509 *, const char *);
> enum cert_purpose x509_get_purpose(X509 *, const char *);
> Index: gbr.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/gbr.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 gbr.c
> --- gbr.c 18 Jan 2022 16:24:55 -0000 1.14
> +++ gbr.c 1 Apr 2022 14:53:47 -0000
> @@ -63,19 +63,24 @@ gbr_parse(X509 **x509, const char *fn, c
> err(1, NULL);
> free(cms);
>
> - p.res->aia = x509_get_aia(*x509, fn);
> - p.res->aki = x509_get_aki(*x509, 0, fn);
> - p.res->ski = x509_get_ski(*x509, fn);
> + if (!x509_get_aia(*x509, fn, &p.res->aia))
> + goto out;
> + if (!x509_get_aki(*x509, fn, &p.res->aki))
> + goto out;
> + if (!x509_get_ski(*x509, fn, &p.res->ski))
> + goto out;
> if (p.res->aia == NULL || p.res->aki == NULL || p.res->ski == NULL) {
> warnx("%s: RFC 6487 section 4.8: "
> "missing AIA, AKI or SKI X509 extension", fn);
> - gbr_free(p.res);
> - X509_free(*x509);
> - *x509 = NULL;
> - return NULL;
> + goto out;
> }
> -
> return p.res;
> +
> +out:
> + gbr_free(p.res);
> + X509_free(*x509);
> + *x509 = NULL;
> + return NULL;
> }
>
> /*
> Index: mft.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 mft.c
> --- mft.c 31 Mar 2022 12:00:00 -0000 1.54
> +++ mft.c 1 Apr 2022 13:52:47 -0000
> @@ -444,9 +444,12 @@ mft_parse(X509 **x509, const char *fn, c
> if ((p.res = calloc(1, sizeof(struct mft))) == NULL)
> err(1, NULL);
>
> - p.res->aia = x509_get_aia(*x509, fn);
> - p.res->aki = x509_get_aki(*x509, 0, fn);
> - p.res->ski = x509_get_ski(*x509, fn);
> + if (!x509_get_aia(*x509, fn, &p.res->aia))
> + goto out;
> + if (!x509_get_aki(*x509, fn, &p.res->aki))
> + goto out;
> + if (!x509_get_ski(*x509, fn, &p.res->ski))
> + goto out;
> if (p.res->aia == NULL || p.res->aki == NULL || p.res->ski == NULL) {
> warnx("%s: RFC 6487 section 4.8: "
> "missing AIA, AKI or SKI X509 extension", fn);
> Index: parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 parser.c
> --- parser.c 10 Feb 2022 15:33:47 -0000 1.64
> +++ parser.c 1 Apr 2022 14:17:05 -0000
> @@ -360,7 +360,11 @@ proc_parser_mft_pre(char *file, const un
>
> a = valid_ski_aki(file, &auths, mft->ski, mft->aki);
> /* load CRL by hand, since it is referenced by the MFT itself */
> - c = x509_get_crl(x509, file);
> + if (!x509_get_crl(x509, file, &c) || c == NULL) {
The c == NULL case now fails silently. Previously this would have
warned and crashed so it doesn't seem to occur in practice.
> + mft_free(mft);
> + X509_free(x509);
> + return NULL;
> + }
> crlfile = strrchr(c, '/');
> if (crlfile != NULL)
> crlfile++;
> @@ -1078,7 +1082,7 @@ proc_parser_file(char *file, unsigned ch
> struct crl *c;
> char *crl_uri;
>
> - crl_uri = x509_get_crl(x509, file);
> + x509_get_crl(x509, file, &crl_uri);
> parse_load_crl(crl_uri);
> free(crl_uri);
> if (auth_find(&auths, aki) == NULL)
> Index: roa.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/roa.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 roa.c
> --- roa.c 10 Feb 2022 15:33:47 -0000 1.38
> +++ roa.c 1 Apr 2022 14:00:48 -0000
> @@ -351,9 +351,12 @@ roa_parse(X509 **x509, const char *fn, c
> if ((p.res = calloc(1, sizeof(struct roa))) == NULL)
> err(1, NULL);
>
> - p.res->aia = x509_get_aia(*x509, fn);
> - p.res->aki = x509_get_aki(*x509, 0, fn);
> - p.res->ski = x509_get_ski(*x509, fn);
> + if (!x509_get_aia(*x509, fn, &p.res->aia))
> + goto out;
> + if (!x509_get_aki(*x509, fn, &p.res->aki))
> + goto out;
> + if (!x509_get_ski(*x509, fn, &p.res->ski))
> + goto out;
> if (p.res->aia == NULL || p.res->aki == NULL || p.res->ski == NULL) {
> warnx("%s: RFC 6487 section 4.8: "
> "missing AIA, AKI or SKI X509 extension", fn);
> Index: x509.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
> retrieving revision 1.37
> diff -u -p -r1.37 x509.c
> --- x509.c 25 Mar 2022 08:19:04 -0000 1.37
> +++ x509.c 1 Apr 2022 14:06:23 -0000
> @@ -83,22 +83,18 @@ x509_init_oid(void)
> * Returns the AKI or NULL if it could not be parsed.
> * The AKI is formatted as a hex string.
> */
> -char *
> -x509_get_aki(X509 *x, int ta, const char *fn)
> +int
> +x509_get_aki(X509 *x, const char *fn, char **aki)
> {
> const unsigned char *d;
> AUTHORITY_KEYID *akid;
> ASN1_OCTET_STRING *os;
> - int dsz, crit;
> - char *res = NULL;
> + int dsz, crit, rc = 0;
>
> + *aki = NULL;
> akid = X509_get_ext_d2i(x, NID_authority_key_identifier, &crit, NULL);
> - if (akid == NULL) {
> - if (!ta)
> - warnx("%s: RFC 6487 section 4.8.3: AKI: "
> - "extension missing", fn);
> - return NULL;
> - }
> + if (akid == NULL)
> + return 1;
> if (crit != 0) {
> warnx("%s: RFC 6487 section 4.8.3: "
> "AKI: extension not non-critical", fn);
> @@ -128,11 +124,11 @@ x509_get_aki(X509 *x, int ta, const char
> goto out;
> }
>
> - res = hex_encode(d, dsz);
> -
> + *aki = hex_encode(d, dsz);
> + rc = 1;
> out:
> AUTHORITY_KEYID_free(akid);
> - return res;
> + return rc;
> }
>
> /*
> @@ -140,18 +136,18 @@ out:
> * Returns the SKI or NULL if it could not be parsed.
> * The SKI is formatted as a hex string.
> */
> -char *
> -x509_get_ski(X509 *x, const char *fn)
> +int
> +x509_get_ski(X509 *x, const char *fn, char **ski)
> {
> const unsigned char *d;
> ASN1_OCTET_STRING *os;
> - int dsz, crit;
> - char *res = NULL;
> + int dsz, crit, rc = 0;
>
> + *ski = NULL;
> os = X509_get_ext_d2i(x, NID_subject_key_identifier, &crit, NULL);
> if (os == NULL) {
> warnx("%s: RFC 6487 section 4.8.2: SKI: extension missing", fn);
> - return NULL;
> + goto out;
As mentioned above, consider making this behave the same way as the
other x509 extension getters.
> }
> if (crit != 0) {
> warnx("%s: RFC 6487 section 4.8.2: "
> @@ -169,10 +165,11 @@ x509_get_ski(X509 *x, const char *fn)
> goto out;
> }
>
> - res = hex_encode(d, dsz);
> + *ski = hex_encode(d, dsz);
> + rc = 1;
> out:
> ASN1_OCTET_STRING_free(os);
> - return res;
> + return rc;
> }
>
> /*
> @@ -281,19 +278,18 @@ x509_get_pubkey(X509 *x, const char *fn)
> * Returns NULL on failure, on success returns the AIA URI
> * (which has to be freed after use).
> */
> -char *
> -x509_get_aia(X509 *x, const char *fn)
> +int
> +x509_get_aia(X509 *x, const char *fn, char **aia)
> {
> ACCESS_DESCRIPTION *ad;
> AUTHORITY_INFO_ACCESS *info;
> - char *aia = NULL;
> - int crit;
> + int crit, rc = 0;
>
> + *aia = NULL;
> info = X509_get_ext_d2i(x, NID_info_access, &crit, NULL);
> - if (info == NULL) {
> - warnx("%s: RFC 6487 section 4.8.7: AIA: extension missing", fn);
> - return NULL;
> - }
> + if (info == NULL)
> + return 1;
> +
> if (crit != 0) {
> warnx("%s: RFC 6487 section 4.8.7: "
> "AIA: extension not non-critical", fn);
> @@ -325,15 +321,16 @@ x509_get_aia(X509 *x, const char *fn)
> goto out;
> }
>
> - aia = strndup(
> + *aia = strndup(
> ASN1_STRING_get0_data(ad->location->d.uniformResourceIdentifier),
> ASN1_STRING_length(ad->location->d.uniformResourceIdentifier));
Unrelated to your diff: I think we may want to ensure that the URI doesn't
contain embedded NULs before calling strndup on it.
> - if (aia == NULL)
> + if (*aia == NULL)
> err(1, NULL);
> + rc = 1;
>
> out:
> AUTHORITY_INFO_ACCESS_free(info);
> - return aia;
> + return rc;
> }
>
> /*
> @@ -364,21 +361,19 @@ x509_get_expire(X509 *x, const char *fn,
> * Returns NULL on failure, the crl URI on success which has to be freed
> * after use.
> */
> -char *
> -x509_get_crl(X509 *x, const char *fn)
> +int
> +x509_get_crl(X509 *x, const char *fn, char **crl)
> {
> CRL_DIST_POINTS *crldp;
> DIST_POINT *dp;
> GENERAL_NAME *name;
> - char *crl = NULL;
> - int crit;
> + int crit, rc = 0;
>
> + *crl = NULL;
> crldp = X509_get_ext_d2i(x, NID_crl_distribution_points, &crit, NULL);
> - if (crldp == NULL) {
> - warnx("%s: RFC 6487 section 4.8.6: CRL: "
> - "no CRL distribution point extension", fn);
> - return NULL;
> - }
> + if (crldp == NULL)
> + return 1;
> +
> if (crit != 0) {
> warnx("%s: RFC 6487 section 4.8.6: "
> "CRL distribution point: extension not non-critical", fn);
> @@ -425,14 +420,15 @@ x509_get_crl(X509 *x, const char *fn)
> goto out;
> }
>
> - crl = strndup(ASN1_STRING_get0_data(name->d.uniformResourceIdentifier),
> + *crl = strndup(ASN1_STRING_get0_data(name->d.uniformResourceIdentifier),
> ASN1_STRING_length(name->d.uniformResourceIdentifier));
same here.
> - if (crl == NULL)
> + if (*crl == NULL)
> err(1, NULL);
> + rc = 1;
>
> out:
> CRL_DIST_POINTS_free(crldp);
> - return crl;
> + return rc;
> }
>
> /*
>
