x509_vfy.c: Fix a regression in find_isser()

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

Fixes #13739

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

diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index 730a0160ff0a18954237aef121370e919ab12086..883c6d7118ac7b0386aa8c0ce8736d634c502db4 100644 (file)
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -323,9 +323,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)
 {
@@ -334,11 +335,7 @@ 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))) {
             rv = issuer;

diff --git a/test/recipes/70-test_verify_extra.t b/test/recipes/70-test_verify_extra.t
index 79a33cd01679ded9f8b997efbf47ff41b1bb7bc0..e3bdcbaaf9f08735a4573a1352bd6057d1d80c87 100644 (file)
--- a/test/recipes/70-test_verify_extra.t
+++ b/test/recipes/70-test_verify_extra.t
@@ -16,4 +16,5 @@ plan tests => 1;
 ok(run(test(["verify_extra_test",
              srctop_file("test", "certs", "roots.pem"),
              srctop_file("test", "certs", "untrusted.pem"),
-             srctop_file("test", "certs", "bad.pem")])));
+             srctop_file("test", "certs", "bad.pem"),
+             srctop_file("test", "certs", "rootCA.pem")])));

diff --git a/test/verify_extra_test.c b/test/verify_extra_test.c
index d9d1498954b110123fe519a27e50611c9bef583c..94faa4c78b31623b10cbee1d025da6848cdb1781 100644 (file)
--- a/test/verify_extra_test.c
+++ b/test/verify_extra_test.c
@@ -18,6 +18,21 @@
 static const char *roots_f;
 static const char *untrusted_f;
 static const char *bad_f;
+static const char *good_f;
+
+static X509 *load_cert_pem(const char *file)
+{
+    X509 *cert = NULL;
+    BIO *bio = NULL;
+
+    if (!TEST_ptr(bio = BIO_new(BIO_s_file())))
+        return NULL;
+    if (TEST_int_gt(BIO_read_filename(bio, file), 0))
+        (void)TEST_ptr(cert = PEM_read_bio_X509(bio, NULL, NULL, NULL));
+
+    BIO_free(bio);
+    return cert;
+}
 
 static STACK_OF(X509) *load_certs_from_file(const char *filename)
 {
@@ -58,7 +73,7 @@ static STACK_OF(X509) *load_certs_from_file(const char *filename)
     return certs;
 }
 
-/*
+/*-
  * Test for CVE-2015-1793 (Alternate Chains Certificate Forgery)
  *
  * Chain is as follows:
@@ -175,16 +190,48 @@ static int test_store_ctx(void)
     return testresult;
 }
 
+static int test_self_signed(const char *filename, int expected)
+{
+    X509 *cert = load_cert_pem(filename);
+    STACK_OF(X509) *trusted = sk_X509_new_null();
+    X509_STORE_CTX *ctx = X509_STORE_CTX_new();
+    int ret;
+
+    ret = TEST_ptr(cert)
+        && TEST_true(sk_X509_push(trusted, cert))
+        && TEST_true(X509_STORE_CTX_init(ctx, NULL, cert, NULL));
+    X509_STORE_CTX_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(good_f, 1);
+}
+
+static int test_self_signed_bad(void)
+{
+    return test_self_signed(bad_f, 0);
+}
+
 int setup_tests(void)
 {
     if (!TEST_ptr(roots_f = test_get_argument(0))
             || !TEST_ptr(untrusted_f = test_get_argument(1))
-            || !TEST_ptr(bad_f = test_get_argument(2))) {
-        TEST_error("usage: verify_extra_test roots.pem untrusted.pem bad.pem\n");
+            || !TEST_ptr(bad_f = test_get_argument(2))
+            || !TEST_ptr(good_f = test_get_argument(3))) {
+        TEST_error("usage: verify_extra_test roots.pem untrusted.pem bad.pem good.pem\n");
         return 0;
     }
 
     ADD_TEST(test_alt_chains_cert_forgery);
     ADD_TEST(test_store_ctx);
+    ADD_TEST(test_self_signed_good);
+    ADD_TEST(test_self_signed_bad);
     return 1;
 }