From 1b14589d7355675ee5f0a7f48d40bc0065748e1b Mon Sep 17 00:00:00 2001 From: tb Date: Thu, 21 Apr 2022 04:48:12 +0000 Subject: [PATCH] Avoid expensive RFC 3779 checks during cert verification X509v3_{addr,asid}_is_canonical() check that the ipAddrBlocks and autonomousSysIds extension conform to RFC 3779. These checks are not cheap. Certs containing non-conformant extensions should not be considered valid, so mark them with EXFLAG_INVALID while caching the extension information in x509v3_cache_extensions(). This way the expensive check while walking the chains during X509_verify_cert() is replaced with a cheap check of the extension flags. This avoids a lot of superfluous work when validating numerous certs with similar chains against the same roots as is done in rpki-client. Issue noticed and fix suggested by claudio ok claudio inoguchi jsing --- lib/libcrypto/x509/x509_addr.c | 20 ++++++++++---------- lib/libcrypto/x509/x509_asid.c | 12 +++++++----- lib/libcrypto/x509/x509_purp.c | 6 +++++- 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/lib/libcrypto/x509/x509_addr.c b/lib/libcrypto/x509/x509_addr.c index 035353826b8..d33d4f2f8ed 100644 --- a/lib/libcrypto/x509/x509_addr.c +++ b/lib/libcrypto/x509/x509_addr.c @@ -1,4 +1,4 @@ -/* $OpenBSD: x509_addr.c,v 1.78 2022/03/16 11:44:36 tb Exp $ */ +/* $OpenBSD: x509_addr.c,v 1.79 2022/04/21 04:48:12 tb Exp $ */ /* * Contributed to the OpenSSL Project by the American Registry for * Internet Numbers ("ARIN"). @@ -1780,11 +1780,11 @@ addr_validate_path_internal(X509_STORE_CTX *ctx, STACK_OF(X509) *chain, if (ext == NULL) { depth = 0; cert = sk_X509_value(chain, depth); + if ((X509_get_extension_flags(cert) & EXFLAG_INVALID) != 0) + goto done; if ((ext = cert->rfc3779_addr) == NULL) goto done; - } - - if (!X509v3_addr_is_canonical(ext)) { + } else if (!X509v3_addr_is_canonical(ext)) { if ((ret = verify_error(ctx, cert, X509_V_ERR_INVALID_EXTENSION, depth)) == 0) goto done; @@ -1806,6 +1806,12 @@ addr_validate_path_internal(X509_STORE_CTX *ctx, STACK_OF(X509) *chain, for (depth++; depth < sk_X509_num(chain); depth++) { cert = sk_X509_value(chain, depth); + if ((X509_get_extension_flags(cert) & EXFLAG_INVALID) != 0) { + if ((ret = verify_error(ctx, cert, + X509_V_ERR_INVALID_EXTENSION, depth)) == 0) + goto done; + } + if ((parent = cert->rfc3779_addr) == NULL) { for (i = 0; i < sk_IPAddressFamily_num(child); i++) { child_af = sk_IPAddressFamily_value(child, i); @@ -1822,12 +1828,6 @@ addr_validate_path_internal(X509_STORE_CTX *ctx, STACK_OF(X509) *chain, continue; } - if (!X509v3_addr_is_canonical(parent)) { - if ((ret = verify_error(ctx, cert, - X509_V_ERR_INVALID_EXTENSION, depth)) == 0) - goto done; - } - /* * Check that the child's resources are covered by the parent. * Each covered resource is replaced with the parent's resource diff --git a/lib/libcrypto/x509/x509_asid.c b/lib/libcrypto/x509/x509_asid.c index c82f2f32cc4..5f43b3030dc 100644 --- a/lib/libcrypto/x509/x509_asid.c +++ b/lib/libcrypto/x509/x509_asid.c @@ -1,4 +1,4 @@ -/* $OpenBSD: x509_asid.c,v 1.30 2021/12/25 15:46:05 tb Exp $ */ +/* $OpenBSD: x509_asid.c,v 1.31 2022/04/21 04:48:12 tb Exp $ */ /* * Contributed to the OpenSSL Project by the American Registry for * Internet Numbers ("ARIN"). @@ -1006,14 +1006,16 @@ asid_validate_path_internal(X509_STORE_CTX *ctx, STACK_OF(X509) *chain, if (ext != NULL) { i = -1; x = NULL; + if (!X509v3_asid_is_canonical(ext)) + validation_err(X509_V_ERR_INVALID_EXTENSION); } else { i = 0; x = sk_X509_value(chain, i); + if ((X509_get_extension_flags(x) & EXFLAG_INVALID) != 0) + goto done; if ((ext = x->rfc3779_asid) == NULL) goto done; } - if (!X509v3_asid_is_canonical(ext)) - validation_err(X509_V_ERR_INVALID_EXTENSION); if (ext->asnum != NULL) { switch (ext->asnum->type) { case ASIdentifierChoice_inherit: @@ -1042,13 +1044,13 @@ asid_validate_path_internal(X509_STORE_CTX *ctx, STACK_OF(X509) *chain, for (i++; i < sk_X509_num(chain); i++) { x = sk_X509_value(chain, i); + if ((X509_get_extension_flags(x) & EXFLAG_INVALID) != 0) + validation_err(X509_V_ERR_INVALID_EXTENSION); if (x->rfc3779_asid == NULL) { if (child_as != NULL || child_rdi != NULL) validation_err(X509_V_ERR_UNNESTED_RESOURCE); continue; } - if (!X509v3_asid_is_canonical(x->rfc3779_asid)) - validation_err(X509_V_ERR_INVALID_EXTENSION); if (x->rfc3779_asid->asnum == NULL && child_as != NULL) { validation_err(X509_V_ERR_UNNESTED_RESOURCE); child_as = NULL; diff --git a/lib/libcrypto/x509/x509_purp.c b/lib/libcrypto/x509/x509_purp.c index 4d833f73ba6..ffd986b4a1a 100644 --- a/lib/libcrypto/x509/x509_purp.c +++ b/lib/libcrypto/x509/x509_purp.c @@ -1,4 +1,4 @@ -/* $OpenBSD: x509_purp.c,v 1.14 2022/04/21 04:24:51 tb Exp $ */ +/* $OpenBSD: x509_purp.c,v 1.15 2022/04/21 04:48:12 tb Exp $ */ /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL * project 2001. */ @@ -600,9 +600,13 @@ x509v3_cache_extensions(X509 *x) x->rfc3779_addr = X509_get_ext_d2i(x, NID_sbgp_ipAddrBlock, &i, NULL); if (x->rfc3779_addr == NULL && i != -1) x->ex_flags |= EXFLAG_INVALID; + if (!X509v3_addr_is_canonical(x->rfc3779_addr)) + x->ex_flags |= EXFLAG_INVALID; x->rfc3779_asid = X509_get_ext_d2i(x, NID_sbgp_autonomousSysNum, &i, NULL); if (x->rfc3779_asid == NULL && i != -1) x->ex_flags |= EXFLAG_INVALID; + if (!X509v3_asid_is_canonical(x->rfc3779_asid)) + x->ex_flags |= EXFLAG_INVALID; #endif for (i = 0; i < X509_get_ext_count(x); i++) { -- 2.20.1