From 356f9aec6fc409e8f24cb6609ee9cdf7d4d265e9 Mon Sep 17 00:00:00 2001 From: claudio Date: Thu, 18 Feb 2021 16:23:17 +0000 Subject: [PATCH] Use X509_get_ext_d2i() also for x509_get_aki() and x509_get_ski(). Now x509_get_extensions() is no longer required to loop over all extensions and the code becomes a lot simpler. While there cleanup x509_get_crl(), as explained by tb@ X509_get_ext_d2i() allocates memory so one needs to free the pointer at the end. For x509_crl_get_aki() use X509_CRL_get_ext_d2i() and more or less copy the rest over from x509_get_aki(). Warn if extensions are missing or present when not expected and also check the the extensions are marked non-critical as required. OK job@ tb@ --- usr.sbin/rpki-client/cert.c | 27 ++-- usr.sbin/rpki-client/extern.h | 8 +- usr.sbin/rpki-client/parser.c | 5 +- usr.sbin/rpki-client/x509.c | 249 ++++++++++++++++++---------------- 4 files changed, 153 insertions(+), 136 deletions(-) diff --git a/usr.sbin/rpki-client/cert.c b/usr.sbin/rpki-client/cert.c index 6e3d7bf7a22..42b460f96d0 100644 --- a/usr.sbin/rpki-client/cert.c +++ b/usr.sbin/rpki-client/cert.c @@ -1,4 +1,4 @@ -/* $OpenBSD: cert.c,v 1.26 2021/02/16 07:58:30 job Exp $ */ +/* $OpenBSD: cert.c,v 1.27 2021/02/18 16:23:17 claudio Exp $ */ /* * Copyright (c) 2019 Kristaps Dzonsons * @@ -1080,19 +1080,10 @@ cert_parse_inner(X509 **xp, const char *fn, int ta) /* ignored here, handled later */ break; case NID_info_access: - free(p.res->aia); - p.res->aia = x509_get_aia(x, p.fn); - c = (p.res->aia != NULL); break; case NID_authority_key_identifier: - free(p.res->aki); - p.res->aki = x509_get_aki_ext(ext, p.fn); - c = (p.res->aki != NULL); break; case NID_subject_key_identifier: - free(p.res->ski); - p.res->ski = x509_get_ski_ext(ext, p.fn); - c = (p.res->ski != NULL); break; default: /* { @@ -1107,8 +1098,12 @@ cert_parse_inner(X509 **xp, const char *fn, int ta) goto out; } - if (!ta) + 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); + } /* Validation on required fields. */ @@ -1134,6 +1129,16 @@ cert_parse_inner(X509 **xp, const char *fn, int ta) goto out; } + if (!ta && p.res->aia == NULL) { + warnx("%s: RFC 6487 section 8.4.7: " + "non-trust anchor missing AIA", p.fn); + goto out; + } else if (ta && p.res->aia != NULL) { + warnx("%s: RFC 6487 section 8.4.7: " + "trust anchor must not have AIA", p.fn); + goto out; + } + if (ta && p.res->crl != NULL) { warnx("%s: RFC 6487 section 8.4.2: " "trust anchor may not specify CRL resource", p.fn); diff --git a/usr.sbin/rpki-client/extern.h b/usr.sbin/rpki-client/extern.h index 6c699cfd75c..f613a5677c4 100644 --- a/usr.sbin/rpki-client/extern.h +++ b/usr.sbin/rpki-client/extern.h @@ -1,4 +1,4 @@ -/* $OpenBSD: extern.h,v 1.44 2021/02/16 08:52:00 claudio Exp $ */ +/* $OpenBSD: extern.h,v 1.45 2021/02/18 16:23:17 claudio Exp $ */ /* * Copyright (c) 2019 Kristaps Dzonsons * @@ -421,12 +421,12 @@ void io_str_read(int, char **); /* X509 helpers. */ char *x509_get_aia(X509 *, const char *); -char *x509_get_aki_ext(X509_EXTENSION *, const char *); -char *x509_get_ski_ext(X509_EXTENSION *, const char *); +char *x509_get_aki(X509 *, int, const char *); +char *x509_get_ski(X509 *, const char *); int x509_get_extensions(X509 *, const char *, char **, char **, char **); char *x509_get_crl(X509 *, const char *); -char *x509_crl_get_aki(X509_CRL *); +char *x509_crl_get_aki(X509_CRL *, const char *); /* Output! */ diff --git a/usr.sbin/rpki-client/parser.c b/usr.sbin/rpki-client/parser.c index b635c3abf72..acb9be499a9 100644 --- a/usr.sbin/rpki-client/parser.c +++ b/usr.sbin/rpki-client/parser.c @@ -1,4 +1,4 @@ -/* $OpenBSD: parser.c,v 1.4 2021/02/04 14:32:01 claudio Exp $ */ +/* $OpenBSD: parser.c,v 1.5 2021/02/18 16:23:17 claudio Exp $ */ /* * Copyright (c) 2019 Claudio Jeker * Copyright (c) 2019 Kristaps Dzonsons @@ -361,7 +361,8 @@ proc_parser_crl(struct entity *entp, X509_STORE *store, if ((x509_crl = crl_parse(entp->file)) != NULL) { if ((crl = malloc(sizeof(*crl))) == NULL) err(1, NULL); - if ((crl->aki = x509_crl_get_aki(x509_crl)) == NULL) + if ((crl->aki = x509_crl_get_aki(x509_crl, entp->file)) == + NULL) errx(1, "x509_crl_get_aki failed"); crl->x509_crl = x509_crl; diff --git a/usr.sbin/rpki-client/x509.c b/usr.sbin/rpki-client/x509.c index 89a99e3ff14..85fdd24a8d6 100644 --- a/usr.sbin/rpki-client/x509.c +++ b/usr.sbin/rpki-client/x509.c @@ -1,4 +1,4 @@ -/* $OpenBSD: x509.c,v 1.15 2021/02/16 07:58:30 job Exp $ */ +/* $OpenBSD: x509.c,v 1.16 2021/02/18 16:23:17 claudio Exp $ */ /* * Copyright (c) 2019 Kristaps Dzonsons * @@ -29,89 +29,69 @@ #include "extern.h" -/* - * Wrapper around ASN1_get_object() that preserves the current start - * state and returns a more meaningful value. - * Return zero on failure, non-zero on success. - */ -static int -ASN1_frame(const char *fn, size_t sz, - const unsigned char **cnt, long *cntsz, int *tag) -{ - int ret, pcls; - - assert(cnt != NULL && *cnt != NULL); - assert(sz > 0); - ret = ASN1_get_object(cnt, cntsz, tag, &pcls, sz); - if ((ret & 0x80)) { - cryptowarnx("%s: ASN1_get_object", fn); - return 0; - } - return ASN1_object_size((ret & 0x01) ? 2 : 0, *cntsz, *tag); -} - /* * Parse X509v3 authority key identifier (AKI), RFC 6487 sec. 4.8.3. * Returns the AKI or NULL if it could not be parsed. * The AKI is formatted as aa:bb:cc:dd, with each being a hex value. */ char * -x509_get_aki_ext(X509_EXTENSION *ext, const char *fn) +x509_get_aki(X509 *x, int ta, const char *fn) { const unsigned char *d; - const ASN1_TYPE *t; - const ASN1_OCTET_STRING *os = NULL; - ASN1_SEQUENCE_ANY *seq = NULL; - int dsz, ptag; - long i, plen; + AUTHORITY_KEYID *akid; + ASN1_OCTET_STRING *os; + int i, dsz, crit; char buf[4]; char *res = NULL; - assert(NID_authority_key_identifier == - OBJ_obj2nid(X509_EXTENSION_get_object(ext))); - os = X509_EXTENSION_get_data(ext); - assert(os != NULL); - - d = os->data; - dsz = os->length; - - if ((seq = d2i_ASN1_SEQUENCE_ANY(NULL, &d, dsz)) == NULL) { - cryptowarnx("%s: RFC 6487 section 4.8.3: AKI: " - "failed ASN.1 sub-sequence parse", fn); + 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 (crit != 0) { + warnx("%s: RFC 6487 section 4.8.3: " + "AKI: extension not non-critical", fn); goto out; } - if (sk_ASN1_TYPE_num(seq) != 1) { + if (akid->issuer != NULL || akid->serial != NULL) { warnx("%s: RFC 6487 section 4.8.3: AKI: " - "want 1 element, have %d", fn, sk_ASN1_TYPE_num(seq)); + "authorityCertIssuer or authorityCertSerialNumber present", + fn); goto out; } - t = sk_ASN1_TYPE_value(seq, 0); - if (t->type != V_ASN1_OTHER) { + os = akid->keyid; + if (os == NULL) { warnx("%s: RFC 6487 section 4.8.3: AKI: " - "want ASN.1 external, have %s (NID %d)", - fn, ASN1_tag2str(t->type), t->type); + "Key Identifier missing", fn); goto out; } - d = t->value.asn1_string->data; - dsz = t->value.asn1_string->length; + d = os->data; + dsz = os->length; - if (!ASN1_frame(fn, dsz, &d, &plen, &ptag)) + if (dsz != SHA_DIGEST_LENGTH) { + warnx("%s: RFC 6487 section 4.8.2: AKI: " + "want %d bytes SHA1 hash, have %d bytes", + fn, SHA_DIGEST_LENGTH, dsz); goto out; + } /* Make room for [hex1, hex2, ":"]*, NUL. */ - if ((res = calloc(plen * 3 + 1, 1)) == NULL) + if ((res = calloc(dsz * 3 + 1, 1)) == NULL) err(1, NULL); - for (i = 0; i < plen; i++) { + for (i = 0; i < dsz; i++) { snprintf(buf, sizeof(buf), "%02X:", d[i]); - strlcat(res, buf, plen * 3 + 1); + strlcat(res, buf, dsz * 3 + 1); } - res[plen * 3 - 1] = '\0'; + res[dsz * 3 - 1] = '\0'; out: - sk_ASN1_TYPE_pop_free(seq, ASN1_TYPE_free); + AUTHORITY_KEYID_free(akid); return res; } @@ -121,35 +101,32 @@ out: * The SKI is formatted as aa:bb:cc:dd, with each being a hex value. */ char * -x509_get_ski_ext(X509_EXTENSION *ext, const char *fn) +x509_get_ski(X509 *x, const char *fn) { const unsigned char *d; - const ASN1_OCTET_STRING *os; - ASN1_OCTET_STRING *oss = NULL; - int i, dsz; + ASN1_OCTET_STRING *os; + int i, dsz, crit; char buf[4]; char *res = NULL; - assert(NID_subject_key_identifier == - OBJ_obj2nid(X509_EXTENSION_get_object(ext))); - - os = X509_EXTENSION_get_data(ext); - assert(os != NULL); - d = os->data; - dsz = os->length; - - if ((oss = d2i_ASN1_OCTET_STRING(NULL, &d, dsz)) == NULL) { - cryptowarnx("%s: RFC 6487 section 4.8.2: SKI: " - "failed ASN.1 octet string parse", fn); + 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; + } + if (crit != 0) { + warnx("%s: RFC 6487 section 4.8.2: " + "SKI: extension not non-critical", fn); goto out; } - d = oss->data; - dsz = oss->length; + d = os->data; + dsz = os->length; - if (dsz != 20) { + if (dsz != SHA_DIGEST_LENGTH) { warnx("%s: RFC 6487 section 4.8.2: SKI: " - "want 20 B SHA1 hash, have %d B", fn, dsz); + "want %d bytes SHA1 hash, have %d bytes", + fn, SHA_DIGEST_LENGTH, dsz); goto out; } @@ -164,7 +141,7 @@ x509_get_ski_ext(X509_EXTENSION *ext, const char *fn) } res[dsz * 3 - 1] = '\0'; out: - ASN1_OCTET_STRING_free(oss); + ASN1_OCTET_STRING_free(os); return res; } @@ -180,12 +157,18 @@ x509_get_aia(X509 *x, const char *fn) ACCESS_DESCRIPTION *ad; AUTHORITY_INFO_ACCESS *info; char *aia = NULL; + int crit; - info = X509_get_ext_d2i(x, NID_info_access, NULL, 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 (crit != 0) { + warnx("%s: RFC 6487 section 4.8.7: " + "AIA: extension not non-critical", fn); + goto out; + } if (sk_ACCESS_DESCRIPTION_num(info) != 1) { warnx("%s: RFC 6487 section 4.8.7: AIA: " "want 1 element, have %d", fn, @@ -217,45 +200,21 @@ out: } /* - * Wraps around x509_get_ski_ext, x509_get_aki_ext, and x509_get_aia. + * Wraps around x509_get_aia, x509_get_aki, and x509_get_ski. * Returns zero on failure (out pointers are NULL) or non-zero on * success (out pointers must be freed). */ int x509_get_extensions(X509 *x, const char *fn, char **aia, char **aki, char **ski) { - X509_EXTENSION *ext = NULL; - const ASN1_OBJECT *obj; - int extsz, i; - *aia = *aki = *ski = NULL; - if ((extsz = X509_get_ext_count(x)) < 0) - cryptoerrx("X509_get_ext_count"); - - for (i = 0; i < extsz; i++) { - ext = X509_get_ext(x, i); - assert(ext != NULL); - obj = X509_EXTENSION_get_object(ext); - assert(obj != NULL); - switch (OBJ_obj2nid(obj)) { - case NID_subject_key_identifier: - free(*ski); - *ski = x509_get_ski_ext(ext, fn); - break; - case NID_authority_key_identifier: - free(*aki); - *aki = x509_get_aki_ext(ext, fn); - break; - case NID_info_access: - free(*aia); - *aia = x509_get_aia(x, fn); - break; - } - } + *aia = x509_get_aia(x, fn); + *aki = x509_get_aki(x, 0, fn); + *ski = x509_get_ski(x, fn); if (*aia == NULL || *aki == NULL || *ski == NULL) { - cryptowarnx("%s: RFC 6487 section 4.8: " + warnx("%s: RFC 6487 section 4.8: " "missing AIA, AKI or SKI X509 extension", fn); free(*aia); free(*aki); @@ -277,49 +236,55 @@ x509_get_extensions(X509 *x, const char *fn, char **aia, char **aki, char **ski) char * x509_get_crl(X509 *x, const char *fn) { - STACK_OF(DIST_POINT) *crldp; + CRL_DIST_POINTS *crldp; DIST_POINT *dp; GENERAL_NAME *name; - char *crl; + char *crl = NULL; + int crit; - crldp = X509_get_ext_d2i(x, NID_crl_distribution_points, NULL, 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 (crit != 0) { + warnx("%s: RFC 6487 section 4.8.6: " + "CRL distribution point: extension not non-critical", fn); + goto out; + } if (sk_DIST_POINT_num(crldp) != 1) { warnx("%s: RFC 6487 section 4.8.6: CRL: " "want 1 element, have %d", fn, sk_DIST_POINT_num(crldp)); - return NULL; + goto out; } dp = sk_DIST_POINT_value(crldp, 0); if (dp->distpoint == NULL) { warnx("%s: RFC 6487 section 4.8.6: CRL: " "no distribution point name", fn); - return NULL; + goto out; } if (dp->distpoint->type != 0) { warnx("%s: RFC 6487 section 4.8.6: CRL: " "expected GEN_OTHERNAME, have %d", fn, dp->distpoint->type); - return NULL; + goto out; } if (sk_GENERAL_NAME_num(dp->distpoint->name.fullname) != 1) { warnx("%s: RFC 6487 section 4.8.6: CRL: " "want 1 full name, have %d", fn, sk_GENERAL_NAME_num(dp->distpoint->name.fullname)); - return NULL; + goto out; } name = sk_GENERAL_NAME_value(dp->distpoint->name.fullname, 0); if (name->type != GEN_URI) { warnx("%s: RFC 6487 section 4.8.6: CRL: " "want URI type, have %d", fn, name->type); - return NULL; + goto out; } crl = strndup(ASN1_STRING_get0_data(name->d.uniformResourceIdentifier), @@ -327,21 +292,67 @@ x509_get_crl(X509 *x, const char *fn) if (crl == NULL) err(1, NULL); +out: + CRL_DIST_POINTS_free(crldp); return crl; } char * -x509_crl_get_aki(X509_CRL *crl) +x509_crl_get_aki(X509_CRL *crl, const char *fn) { - X509_EXTENSION *ext; - int loc; + const unsigned char *d; + AUTHORITY_KEYID *akid; + ASN1_OCTET_STRING *os; + int i, dsz, crit; + char buf[4]; + char *res = NULL; - loc = X509_CRL_get_ext_by_NID(crl, NID_authority_key_identifier, -1); - if (loc == -1) { - warnx("%s: CRL without AKI extension", __func__); + akid = X509_CRL_get_ext_d2i(crl, NID_authority_key_identifier, &crit, + NULL); + if (akid == NULL) { + warnx("%s: RFC 6487 section 4.8.3: AKI: extension missing", fn); return NULL; } - ext = X509_CRL_get_ext(crl, loc); + if (crit != 0) { + warnx("%s: RFC 6487 section 4.8.3: " + "AKI: extension not non-critical", fn); + goto out; + } + if (akid->issuer != NULL || akid->serial != NULL) { + warnx("%s: RFC 6487 section 4.8.3: AKI: " + "authorityCertIssuer or authorityCertSerialNumber present", + fn); + goto out; + } + + os = akid->keyid; + if (os == NULL) { + warnx("%s: RFC 6487 section 4.8.3: AKI: " + "Key Identifier missing", fn); + goto out; + } + + d = os->data; + dsz = os->length; + + if (dsz != SHA_DIGEST_LENGTH) { + warnx("%s: RFC 6487 section 4.8.2: AKI: " + "want %d bytes SHA1 hash, have %d bytes", + fn, SHA_DIGEST_LENGTH, dsz); + goto out; + } - return x509_get_aki_ext(ext, "x509_crl_get_aki"); + /* Make room for [hex1, hex2, ":"]*, NUL. */ + + if ((res = calloc(dsz * 3 + 1, 1)) == NULL) + err(1, NULL); + + for (i = 0; i < dsz; i++) { + snprintf(buf, sizeof(buf), "%02X:", d[i]); + strlcat(res, buf, dsz * 3 + 1); + } + res[dsz * 3 - 1] = '\0'; +out: + AUTHORITY_KEYID_free(akid); + return res; } -- 2.20.1