From 8435fb8d89e81f8ad93dfa85246963ade917bc60 Mon Sep 17 00:00:00 2001 From: tb Date: Fri, 23 Jun 2023 15:32:15 +0000 Subject: [PATCH] Use consistent idiom for X509_get_ext_d2i() X509_get_ext_d2i() is special. A NULL return value can be either a success or a failure scenario: an extension may legitimately be absent. However, to find out whether it was absent or an error ocurred, you need to pass in &crit, a pointer to an int. Its purpose is to indicate whether the extension was marked critical or not. If the return value was NULL, crit becomes an error indicator: crit == -1 means the extension was not found. This can be an error or fine depending on the extension. Handle this accordingly. In particular for basic constraints, if they are missing or non-critical, this is an error. If crit == -2 then multiple extensions with the same OID as the nid requested are present. this means the cert is non-conformant to RFC 5280. If crit >= 0, then something weird happened. Either memory allocation failed or the extension could not be parsed. It is not easily possible to tell which. In short, if crit != -1, drop the cert on the floor like a hot potato. Add warnings where possible. For x509_any_inherits() this needs some more work, but that will be done in a different diff another day. ok job --- usr.sbin/rpki-client/x509.c | 94 +++++++++++++++++++++++++++++-------- 1 file changed, 75 insertions(+), 19 deletions(-) diff --git a/usr.sbin/rpki-client/x509.c b/usr.sbin/rpki-client/x509.c index db1d21cde91..5d63a8fb48d 100644 --- a/usr.sbin/rpki-client/x509.c +++ b/usr.sbin/rpki-client/x509.c @@ -1,4 +1,4 @@ -/* $OpenBSD: x509.c,v 1.72 2023/06/20 11:06:47 job Exp $ */ +/* $OpenBSD: x509.c,v 1.73 2023/06/23 15:32:15 tb Exp $ */ /* * Copyright (c) 2022 Theo Buehler * Copyright (c) 2021 Claudio Jeker @@ -146,8 +146,14 @@ x509_get_aki(X509 *x, const char *fn, char **aki) *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, char **ski) *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, char **aia) *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, char **aia) 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, char **sia) *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: " @@ -546,11 +586,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 @@ -564,8 +607,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) @@ -590,14 +636,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; @@ -624,8 +674,14 @@ x509_get_crl(X509 *x, const char *fn, char **crl) *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: " -- 2.20.1