X509_get_ext_d2i() is one of those very special OpenSSL interfaces...
It can return NULL for various reasons. If it returns NULL and crit is
not -1, something bad happened. If crit is -2, multiple extensions with
the same OID as the one corresponding to the nid were found (this is not
allowed per RFC 5280, 4.2). It returns NULL if it failed to deserialize
the extension, be it due to bad DER or an allocation failure. In these
cases crit will be 1 or 0, depending on whether the extension was marked
critical.
So instead of accepting an object in the situation that crit != -1, we
should warn and throw an error. We can't errx() since we can't really
tell allocation failure from failure due to a malformed extension.
I also added a check for NULL and criticality of basic constraints,
which were missing (per RFC 5280 criticality is optional, so libcrypto
doesn't check that, but RFC 6487 is clear here).
The warnings in x509*inherits() are minimal. The callers of
x509_inherits() warn. For x509_any_inherits() this is not so. The
annoying bit is that it is used in auth_insert(). I know how I want
to deal with that, but that is largely independent of this diff.
Index: x509.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
retrieving revision 1.71
diff -u -p -r1.71 x509.c
--- x509.c 22 May 2023 15:07:02 -0000 1.71
+++ x509.c 20 Jun 2023 11:55:18 -0000
@@ -146,8 +146,14 @@ x509_get_aki(X509 *x, const char *fn, ch
*aki = NULL;
akid = X509_get_ext_d2i(x, NID_authority_key_identifier, &crit, NULL);
- if (akid == NULL)
+ if (akid == NULL) {
+ if (crit != -1) {
+ warnx("%s: RFC 6487 section 4.8.3: error parsing AKI",
+ fn);
+ return 0;
+ }
return 1;
+ }
if (crit != 0) {
warnx("%s: RFC 6487 section 4.8.3: "
"AKI: extension not non-critical", fn);
@@ -200,8 +206,14 @@ x509_get_ski(X509 *x, const char *fn, ch
*ski = NULL;
os = X509_get_ext_d2i(x, NID_subject_key_identifier, &crit, NULL);
- if (os == NULL)
+ if (os == NULL) {
+ if (crit != -1) {
+ warnx("%s: RFC 6487 section 4.8.2: error parsing SKI",
+ fn);
+ return 0;
+ }
return 1;
+ }
if (crit != 0) {
warnx("%s: RFC 6487 section 4.8.2: "
"SKI: extension not non-critical", fn);
@@ -258,6 +270,20 @@ x509_get_purpose(X509 *x, const char *fn
if (X509_check_ca(x) == 1) {
bc = X509_get_ext_d2i(x, NID_basic_constraints, &crit, NULL);
+ if (bc == NULL) {
+ if (crit != -1)
+ warnx("%s: RFC 6487 section 4.8.1: "
+ "error parsing basic constraints", fn);
+ else
+ warnx("%s: RFC 6487 section 4.8.1: "
+ "missing basic constraints", fn);
+ goto out;
+ }
+ if (crit != 1) {
+ warnx("%s: RFC 6487 section 4.8.1: Basic Constraints "
+ "must be marked critical", fn);
+ goto out;
+ }
if (bc->pathlen != NULL) {
warnx("%s: RFC 6487 section 4.8.1: Path Length "
"Constraint must be absent", fn);
@@ -274,7 +300,10 @@ x509_get_purpose(X509 *x, const char *fn
eku = X509_get_ext_d2i(x, NID_ext_key_usage, &crit, NULL);
if (eku == NULL) {
- warnx("%s: EKU: extension missing", fn);
+ if (crit != -1)
+ warnx("%s: error parsing EKU", fn);
+ else
+ warnx("%s: EKU: extension missing", fn);
goto out;
}
if (crit != 0) {
@@ -372,13 +401,13 @@ x509_get_aia(X509 *x, const char *fn, ch
*aia = NULL;
info = X509_get_ext_d2i(x, NID_info_access, &crit, NULL);
- if (info == NULL)
+ if (info == NULL) {
+ if (crit != -1) {
+ warnx("%s: RFC 6487 section 4.8.7: error parsing AIA",
+ fn);
+ return 0;
+ }
return 1;
-
- if ((X509_get_extension_flags(x) & EXFLAG_SS) != 0) {
- warnx("%s: RFC 6487 section 4.8.7: AIA must be absent from "
- "a self-signed certificate", fn);
- goto out;
}
if (crit != 0) {
@@ -387,6 +416,12 @@ x509_get_aia(X509 *x, const char *fn, ch
goto out;
}
+ if ((X509_get_extension_flags(x) & EXFLAG_SS) != 0) {
+ warnx("%s: RFC 6487 section 4.8.7: AIA must be absent from "
+ "a self-signed certificate", 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,
@@ -428,8 +463,13 @@ x509_get_sia(X509 *x, const char *fn, ch
*sia = NULL;
info = X509_get_ext_d2i(x, NID_sinfo_access, &crit, NULL);
- if (info == NULL)
+ if (info == NULL) {
+ if (crit != -1) {
+ warnx("%s: error parsing SIA", fn);
+ return 0;
+ }
return 1;
+ }
if (crit != 0) {
warnx("%s: RFC 6487 section 4.8.8: "
@@ -543,11 +583,14 @@ x509_inherits(X509 *x)
STACK_OF(IPAddressFamily) *addrblk = NULL;
ASIdentifiers *asidentifiers = NULL;
const IPAddressFamily *af;
- int i, rc = 0;
+ int crit, i, rc = 0;
- addrblk = X509_get_ext_d2i(x, NID_sbgp_ipAddrBlock, NULL, NULL);
- if (addrblk == NULL)
+ addrblk = X509_get_ext_d2i(x, NID_sbgp_ipAddrBlock, &crit, NULL);
+ if (addrblk == NULL) {
+ if (crit != -1)
+ warnx("error parsing ipAddrBlock");
goto out;
+ }
/*
* Check by hand, since X509v3_addr_inherits() success only means that
@@ -561,8 +604,11 @@ x509_inherits(X509 *x)
asidentifiers = X509_get_ext_d2i(x, NID_sbgp_autonomousSysNum, NULL,
NULL);
- if (asidentifiers == NULL)
+ if (asidentifiers == NULL) {
+ if (crit != -1)
+ warnx("error parsing asIdentifiers");
goto out;
+ }
/* We need to have AS numbers and don't want RDIs. */
if (asidentifiers->asnum == NULL || asidentifiers->rdi != NULL)
@@ -587,14 +633,18 @@ x509_any_inherits(X509 *x)
{
STACK_OF(IPAddressFamily) *addrblk = NULL;
ASIdentifiers *asidentifiers = NULL;
- int rc = 0;
+ int crit, rc = 0;
- addrblk = X509_get_ext_d2i(x, NID_sbgp_ipAddrBlock, NULL, NULL);
+ addrblk = X509_get_ext_d2i(x, NID_sbgp_ipAddrBlock, &crit, NULL);
+ if (addrblk == NULL && crit != -1)
+ warnx("error parsing ipAddrBlock");
if (X509v3_addr_inherits(addrblk))
rc = 1;
- asidentifiers = X509_get_ext_d2i(x, NID_sbgp_autonomousSysNum, NULL,
+ asidentifiers = X509_get_ext_d2i(x, NID_sbgp_autonomousSysNum, &crit,
NULL);
+ if (asidentifiers == NULL && crit != -1)
+ warnx("error parsing asIdentifiers");
if (X509v3_asid_inherits(asidentifiers))
rc = 1;
@@ -621,8 +671,14 @@ x509_get_crl(X509 *x, const char *fn, ch
*crl = NULL;
crldp = X509_get_ext_d2i(x, NID_crl_distribution_points, &crit, NULL);
- if (crldp == NULL)
+ if (crldp == NULL) {
+ if (crit != -1) {
+ warnx("%s: RFC 6487 section 4.8.6: failed to parse "
+ "CRL distribution points", fn);
+ return 0;
+ }
return 1;
+ }
if (crit != 0) {
warnx("%s: RFC 6487 section 4.8.6: "
