From: Dr. David von Oheimb <dev@ddvo.net>
Date: Fri, 31 Jan 2025 13:38:53 +0000 (+0100)
Subject: x509_vfy.c: refactor parameter of get0_best_issuer_sk() and adapt its use by build_ch... 
X-Git-Tag: openssl-3.5.0-alpha1~624
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=5ebd6d26a805631230d8f8ea80a83034f7b65b2b;p=thirdparty%2Fopenssl.git

x509_vfy.c: refactor parameter of get0_best_issuer_sk() and adapt its use by build_chain()

Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/26600)
---

diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index 3776edbf6a7..0ad0e995113 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -381,7 +381,15 @@ static int sk_X509_contains(STACK_OF(X509) *sk, X509 *cert)
  *     exception that |x| is self-issued and |ctx->chain| has just one element.
  * Prefer the first match with suitable validity period or latest expiration.
  */
-static X509 *get0_best_issuer_sk(X509_STORE_CTX *ctx, int trusted,
+/*
+ * Note: so far, we do not check during chain building
+ * whether any key usage extension stands against a candidate issuer cert.
+ * Likely it would be good if build_chain() sets |check_signing_allowed|.
+ * Yet if |sk| is a list of trusted certs, as with X509_STORE_CTX_set0_trusted_stack(),
+ * better not set |check_signing_allowed|.
+ * Maybe not touch X509_STORE_CTX_get1_issuer(), for API backward compatiblity.
+ */
+static X509 *get0_best_issuer_sk(X509_STORE_CTX *ctx, int check_signing_allowed,
                                  int no_dup, STACK_OF(X509) *sk, X509 *x)
 {
     int i;
@@ -391,13 +399,13 @@ static X509 *get0_best_issuer_sk(X509_STORE_CTX *ctx, int trusted,
         candidate = sk_X509_value(sk, i);
         if (no_dup
             && !((x->ex_flags & EXFLAG_SI) != 0 && sk_X509_num(ctx->chain) == 1)
-                && sk_X509_contains(ctx->chain, candidate))
+            && sk_X509_contains(ctx->chain, candidate))
             continue;
         if (ctx->check_issued(ctx, x, candidate)) {
-            if (!trusted) { /* do not check key usage for trust anchors */
-                if (ossl_x509_signing_allowed(candidate, x) != X509_V_OK)
-                    continue;
-            }
+            if (check_signing_allowed
+                /* yet better not check key usage for trust anchors */
+                && ossl_x509_signing_allowed(candidate, x) != X509_V_OK)
+                continue;
             if (ossl_x509_check_cert_time(ctx, candidate, -1))
                 return candidate;
             /*
@@ -428,7 +436,7 @@ int X509_STORE_CTX_get1_issuer(X509 **issuer, X509_STORE_CTX *ctx, X509 *x)
 
     if (certs == NULL)
         return -1;
-    *issuer = get0_best_issuer_sk(ctx, 1 /* trusted */, 0, certs, x);
+    *issuer = get0_best_issuer_sk(ctx, 0, 0 /* allow duplicates */, certs, x);
     if (*issuer != NULL)
         ret = X509_up_ref(*issuer) ? 1 : -1;
     OSSL_STACK_OF_X509_free(certs);
@@ -455,8 +463,7 @@ static int check_issued(ossl_unused X509_STORE_CTX *ctx, X509 *x, X509 *issuer)
  */
 static int get1_best_issuer_other_sk(X509 **issuer, X509_STORE_CTX *ctx, X509 *x)
 {
-    *issuer = get0_best_issuer_sk(ctx, 1 /* trusted */, 1 /* no_dup */,
-                                  ctx->other_ctx, x);
+    *issuer = get0_best_issuer_sk(ctx, 0, 1 /* no_dup */, ctx->other_ctx, x);
     if (*issuer == NULL)
         return 0;
     return X509_up_ref(*issuer) ? 1 : -1;
@@ -3491,7 +3498,7 @@ static int build_chain(X509_STORE_CTX *ctx)
                 goto int_err;
             curr = sk_X509_value(ctx->chain, num - 1);
             issuer = (X509_self_signed(curr, 0) > 0 || num > max_depth) ?
-                NULL : get0_best_issuer_sk(ctx, 0, 1, sk_untrusted, curr);
+                NULL : get0_best_issuer_sk(ctx, 0, 1 /* no_dup */, sk_untrusted, curr);
             if (issuer == NULL) {
                 /*
                  * Once we have reached a self-signed cert or num > max_depth