From: Dr. David von Oheimb <David.von.Oheimb@siemens.com>
Date: Mon, 28 Dec 2020 10:25:59 +0000 (+0100)
Subject: x509_vfy.c: Fix a regression in find_issuer()
X-Git-Tag: openssl-3.0.0-alpha11~131
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=4dd009180a06ad973620c5beec28f2a6839c16ca;p=thirdparty%2Fopenssl.git

x509_vfy.c: Fix a regression in find_issuer()

...in case the candidate issuer cert is identical to the target cert.

This is the v3.0.0 variant of #13749 fixing #13739 for v1.1.1.

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/13762)
---

diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index 9b09cf8b6d7..f5849a56037 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -316,9 +316,10 @@ static int sk_X509_contains(STACK_OF(X509) *sk, X509 *cert)
 }
 
 /*
- * Find in given STACK_OF(X509) sk a non-expired issuer cert (if any) of given cert x.
- * The issuer must not be the same as x and must not yet be in ctx->chain, where the
- * exceptional case x is self-issued and ctx->chain has just one element is allowed.
+ * Find in given STACK_OF(X509) sk an issuer cert of given cert x.
+ * The issuer must not yet be in ctx->chain, where the exceptional case
+ * that x is self-issued and ctx->chain has just one element is allowed.
+ * Prefer the first one that is not expired, else take the last expired one.
  */
 static X509 *find_issuer(X509_STORE_CTX *ctx, STACK_OF(X509) *sk, X509 *x)
 {
@@ -327,16 +328,12 @@ static X509 *find_issuer(X509_STORE_CTX *ctx, STACK_OF(X509) *sk, X509 *x)
 
     for (i = 0; i < sk_X509_num(sk); i++) {
         issuer = sk_X509_value(sk, i);
-        /*
-         * Below check 'issuer != x' is an optimization and safety precaution:
-         * Candidate issuer cert cannot be the same as the subject cert 'x'.
-         */
-        if (issuer != x && ctx->check_issued(ctx, x, issuer)
+        if (ctx->check_issued(ctx, x, issuer)
             && (((x->ex_flags & EXFLAG_SI) != 0 && sk_X509_num(ctx->chain) == 1)
                 || !sk_X509_contains(ctx->chain, issuer))) {
+            if (x509_check_cert_time(ctx, issuer, -1))
+                return issuer;
             rv = issuer;
-            if (x509_check_cert_time(ctx, rv, -1))
-                break;
         }
     }
     return rv;
diff --git a/test/verify_extra_test.c b/test/verify_extra_test.c
index 300cca3fe47..1b308ca84b2 100644
--- a/test/verify_extra_test.c
+++ b/test/verify_extra_test.c
@@ -24,7 +24,7 @@ static const char *req_f;
 
 #define load_cert_from_file(file) load_cert_pem(file, NULL)
 
-/*
+/*-
  * Test for CVE-2015-1793 (Alternate Chains Certificate Forgery)
  *
  * Chain is as follows:
@@ -99,37 +99,6 @@ static int test_alt_chains_cert_forgery(void)
     return ret;
 }
 
-static int test_store_ctx(void)
-{
-    X509_STORE_CTX *sctx = NULL;
-    X509 *x = NULL;
-    int testresult = 0, ret;
-
-    x = load_cert_from_file(bad_f);
-    if (x == NULL)
-        goto err;
-
-    sctx = X509_STORE_CTX_new();
-    if (sctx == NULL)
-        goto err;
-
-    if (!X509_STORE_CTX_init(sctx, NULL, x, NULL))
-        goto err;
-
-    /* Verifying a cert where we have no trusted certs should fail */
-    ret = X509_verify_cert(sctx);
-
-    if (ret == 0) {
-        /* This is the result we were expecting: Test passed */
-        testresult = 1;
-    }
-
- err:
-    X509_STORE_CTX_free(sctx);
-    X509_free(x);
-    return testresult;
-}
-
 OPT_TEST_DECLARE_USAGE("roots.pem untrusted.pem bad.pem\n")
 
 static int test_distinguishing_id(void)
@@ -206,30 +175,49 @@ static int test_req_distinguishing_id(void)
     return ret;
 }
 
-static int test_self_signed(const char *filename, int expected)
+static int test_self_signed(const char *filename, int use_trusted, int expected)
 {
     X509 *cert;
+    STACK_OF(X509) *trusted = sk_X509_new_null();
+    X509_STORE_CTX *ctx = X509_STORE_CTX_new();
     int ret;
 
     cert = load_cert_from_file(filename); /* may result in NULL */
     ret = TEST_int_eq(X509_self_signed(cert, 1), expected);
+
+    if (cert != NULL) {
+        if (use_trusted)
+            ret = ret && TEST_true(sk_X509_push(trusted, cert));
+        ret = ret && TEST_true(X509_STORE_CTX_init(ctx, NULL, cert, NULL));
+        X509_STORE_CTX_set0_trusted_stack(ctx, trusted);
+        ret = ret && TEST_int_eq(X509_verify_cert(ctx), expected);
+    }
+
+    X509_STORE_CTX_free(ctx);
+    sk_X509_free(trusted);
     X509_free(cert);
     return ret;
 }
 
 static int test_self_signed_good(void)
 {
-    return test_self_signed(root_f, 1);
+    return test_self_signed(root_f, 1, 1);
 }
 
 static int test_self_signed_bad(void)
 {
-    return test_self_signed(bad_f, 0);
+    return test_self_signed(bad_f, 1, 0);
 }
 
 static int test_self_signed_error(void)
 {
-    return test_self_signed("nonexistent file name", -1);
+    return test_self_signed("nonexistent file name", 1, -1);
+}
+
+static int test_store_ctx(void)
+{
+    /* Verifying a cert where we have no trusted certs should fail */
+    return test_self_signed(bad_f, 0, 0);
 }
 
 int setup_tests(void)