x509_vfy.c: Restore rejection of expired trusted (root) certificate

The certificate path validation procedure specified in RFC 5280 does not
include checking the validity period of the trusted (root) certificate.
Still it is common good practice to perform this check.
Also OpenSSL did this until version 1.1.1h, yet
commit e2590c3a162eb118c36b09c2168164283aa099b4 accidentally killed it.

The current commit restores the previous behavior.
It also removes the cause of that bug, namely counter-intuitive design
of the internal function check_issued(), which was complicated by checks
that actually belong to some other internal function, namely find_issuer().

Moreover, this commit adds a regression check and proper documentation of
the root cert validity period check feature, which had been missing so far.

Fixes #13471

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

2 files changed, 28 insertions, 25 deletions

diff --git a/crypto/x509/x509_cmp.c b/crypto/x509/x509_cmp.c
index d1600e1e8d..ad620af0af 100644
--- a/crypto/x509/x509_cmp.c
+++ b/crypto/x509/x509_cmp.c
@@ -135,6 +135,8 @@ int X509_cmp(const X509 *a, const X509 *b)
 {
     int rv;
 
+    if (a == b) /* for efficiency */
+        return 0;
     /* ensure hash is valid */
     if (X509_check_purpose((X509 *)a, -1, 0) != 1)
         return -2;
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index ffa8d637ff..730a0160ff 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -312,8 +312,20 @@ int X509_verify_cert(X509_STORE_CTX *ctx)
     return ret;
 }
 
+static int sk_X509_contains(STACK_OF(X509) *sk, X509 *cert)
+{
+    int i, n = sk_X509_num(sk);
+
+    for (i = 0; i < n; i++)
+        if (X509_cmp(sk_X509_value(sk, i), cert) == 0)
+            return 1;
+    return 0;
+}
+
 /*
- * Given a STACK_OF(X509) find the issuer of cert (if any)
+ * 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.
  */
 static X509 *find_issuer(X509_STORE_CTX *ctx, STACK_OF(X509) *sk, X509 *x)
 {
@@ -322,7 +334,13 @@ 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);
-        if (ctx->check_issued(ctx, x, issuer)) {
+        /*
+         * 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)
+            && (((x->ex_flags & EXFLAG_SI) != 0 && sk_X509_num(ctx->chain) == 1)
+                || !sk_X509_contains(ctx->chain, issuer))) {
             rv = issuer;
             if (x509_check_cert_time(ctx, rv, -1))
                 break;
@@ -331,30 +349,13 @@ static X509 *find_issuer(X509_STORE_CTX *ctx, STACK_OF(X509) *sk, X509 *x)
     return rv;
 }
 
-/*
- * Check that the given certificate 'x' is issued by the certificate 'issuer'
- * and the issuer is not yet in ctx->chain, where the exceptional case
- * that 'x' is self-issued and ctx->chain has just one element is allowed.
- */
+/* Check that the given certificate 'x' is issued by the certificate 'issuer' */
 static int check_issued(X509_STORE_CTX *ctx, X509 *x, X509 *issuer)
 {
-    if (x509_likely_issued(issuer, x) != X509_V_OK)
-        return 0;
-    if ((x->ex_flags & EXFLAG_SI) == 0 || sk_X509_num(ctx->chain) != 1) {
-        int i;
-        X509 *ch;
-
-        for (i = 0; i < sk_X509_num(ctx->chain); i++) {
-            ch = sk_X509_value(ctx->chain, i);
-            if (ch == issuer || X509_cmp(ch, issuer) == 0)
-                return 0;
-        }
-    }
-    return 1;
+    return x509_likely_issued(issuer, x) == X509_V_OK;
 }
 
 /* Alternative lookup method: look from a STACK stored in other_ctx */
-
 static int get_issuer_sk(X509 **issuer, X509_STORE_CTX *ctx, X509 *x)
 {
     *issuer = find_issuer(ctx, ctx->other_ctx, x);
@@ -1740,7 +1741,7 @@ static int internal_verify(X509_STORE_CTX *ctx)
     if (ctx->bare_ta_signed) {
         xs = xi;
         xi = NULL;
-        goto check_cert;
+        goto check_cert_time;
     }
 
     if (ctx->check_issued(ctx, xi, xi))
@@ -1748,7 +1749,7 @@ static int internal_verify(X509_STORE_CTX *ctx)
     else {
         if (ctx->param->flags & X509_V_FLAG_PARTIAL_CHAIN) {
             xs = xi;
-            goto check_cert;
+            goto check_cert_time;
         }
         if (n <= 0) {
             if (!verify_cb_cert(ctx, xi, 0,
@@ -1756,7 +1757,7 @@ static int internal_verify(X509_STORE_CTX *ctx)
                 return 0;
 
             xs = xi;
-            goto check_cert;
+            goto check_cert_time;
         }
 
         n--;
@@ -1817,7 +1818,7 @@ static int internal_verify(X509_STORE_CTX *ctx)
             }
         }
 
- check_cert:
+    check_cert_time: /* in addition to RFC 5280, do also for trusted (root) cert */
         /* Calls verify callback as needed */
         if (!x509_check_cert_time(ctx, xs, n))
             return 0;