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

diff --git a/lib/libcrypto/x509/x509_addr.c b/lib/libcrypto/x509/x509_addr.c
index 0353538..d33d4f2 100644 (file)
--- 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 c82f2f3..5f43b30 100644 (file)
--- 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 4d833f7..ffd986b 100644 (file)
--- 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++) {