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@

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 <kristaps@bsd.lv>
  *
@@ -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 <kristaps@bsd.lv>
  *
@@ -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 <claudio@openbsd.org>
  * Copyright (c) 2019 Kristaps Dzonsons <kristaps@bsd.lv>
@@ -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 <kristaps@bsd.lv>
  *
@@ -30,88 +30,68 @@
 #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;
 }