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)

diff --git a/CHANGES b/CHANGES
index b927cf13614d3023c66b1e898f83914e1b17c389..044108d03642c52bc0373fd9a0ca2062824bac7a 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -19,6 +19,10 @@
      pass an EVP_PKEY instead.
      [Matt Caswell]
 
+  *) In 1.1.1h, an expired trusted (root) certificate was not anymore rejected
+     when validating a certificate path. This check is restored in 1.1.1i.
+     [David von Oheimb]
+
  Changes between 1.1.1g and 1.1.1h [22 Sep 2020]
 
   *) Certificates with explicit curve parameters are now disallowed in
@@ -44,6 +48,10 @@
      on renegotiation.
      [Tomas Mraz]
 
+  *) Accidentally, an expired trusted (root) certificate is not anymore rejected
+     when validating a certificate path.
+     [David von Oheimb]
+
   *) The Oracle Developer Studio compiler will start reporting deprecated APIs
 
   *) Add support for Apple Silicon M1 Macs with the darwin64-arm64-cc target.

diff --git a/crypto/x509/x509_cmp.c b/crypto/x509/x509_cmp.c
index d1600e1e8ddab4096b15042e757b3f97a0a00a5a..ad620af0aff4fc585cd7446c2ae8178921991443 100644 (file)
--- 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 ffa8d637ff675820528aeef56173926c40adb3e5..730a0160ff0a18954237aef121370e919ab12086 100644 (file)
--- 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;

diff --git a/doc/man1/verify.pod b/doc/man1/verify.pod
index 71288be40d4c995f0f6d02d5d2c12f09cb7805ba..da2b7024821d1980bec3dd8954423820457b61e1 100644 (file)
--- a/doc/man1/verify.pod
+++ b/doc/man1/verify.pod
@@ -382,10 +382,14 @@ should be trusted for the supplied purpose.
 For compatibility with previous versions of OpenSSL, a certificate with no
 trust settings is considered to be valid for all purposes.
 
-The final operation is to check the validity of the certificate chain. The validity
-period is checked against the current system time and the notBefore and notAfter
-dates in the certificate. The certificate signatures are also checked at this
-point.
+The final operation is to check the validity of the certificate chain.
+For each element in the chain, including the root CA certificate,
+the validity period as specified by the C<notBefore> and C<notAfter> fields
+is checked against the current system time.
+The B<-attime> flag may be used to use a reference time other than "now."
+The certificate signature is checked as well
+(except for the signature of the typically self-signed root CA certificate,
+which is verified only if the B<-check_ss_sig> option is given).
 
 If all operations complete successfully then certificate is considered valid. If
 any operation fails then the certificate is not valid.

diff --git a/doc/man3/X509_STORE_set_verify_cb_func.pod b/doc/man3/X509_STORE_set_verify_cb_func.pod
index 526790938acd5d21017d325ad350d61130f765ff..6d7098250d184842ea34b2ad4131a8460cafe411 100644 (file)
--- a/doc/man3/X509_STORE_set_verify_cb_func.pod
+++ b/doc/man3/X509_STORE_set_verify_cb_func.pod
@@ -137,9 +137,7 @@ I<If no function to get the issuer is provided, the internal default
 function will be used instead.>
 
 X509_STORE_set_check_issued() sets the function to check that a given
-certificate B<x> is issued by the issuer certificate B<issuer> and
-the issuer is not yet in the chain contained in <ctx>, where the exceptional
-case that B<x> is self-issued and ctx->chain has just one element is allowed.
+certificate B<x> is issued by the issuer certificate B<issuer>.
 This function must return 0 on failure (among others if B<x> hasn't
 been issued with B<issuer>) and 1 on success.
 I<If no function to get the issuer is provided, the internal default

diff --git a/test/certs/root-expired.pem b/test/certs/root-expired.pem
new file mode 100644 (file)
index 0000000..eb5b697
--- /dev/null
+++ b/test/certs/root-expired.pem
@@ -0,0 +1,18 @@
+-----BEGIN CERTIFICATE-----
+MIIC8jCCAdqgAwIBAgIBATANBgkqhkiG9w0BAQsFADASMRAwDgYDVQQDDAdSb290
+IENBMB4XDTIwMTIwMjE0MTYwOVoXDTIwMTIwMTE0MTYwOVowEjEQMA4GA1UEAwwH
+Um9vdCBDQTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAOHmAPUGvKBG
+OHkPPx5xGRNtAt8rm3Zr/KywIe3WkQhCO6VjNexSW6CiSsXWAJQDl1o9uWco0n3j
+IVyk7cY8jY6E0Z1Uwz3ZdKKWdmdx+cYaUHez/XjuW+DjjIkjwpoi7D7UN54HzcAr
+VREXOjRCHGkNOhiw7RWUXsb9nofGHOeUGpLAXwXBc0PlA94JkckkztiOi34u4DFI
+0YYqalUmeugLNk6XseCkydpcaUsDgAhWg6Mfsiq4wUz+xbFN1MABqu2+ziW97mmt
+9gfNbiuhiVT1aOuYCe3JYGbLM2JKA7Bo1g6rX8E1VX79Ru6669y2oqPthX9337Vo
+IkN+ZiQjr8UCAwEAAaNTMFEwHQYDVR0OBBYEFI71Ja8em2uEPXyAmslTnE1y96NS
+MB8GA1UdIwQYMBaAFI71Ja8em2uEPXyAmslTnE1y96NSMA8GA1UdEwEB/wQFMAMB
+Af8wDQYJKoZIhvcNAQELBQADggEBAH1uqov7eXVT6GbhJ7foASTQpIaVi4GXIfbS
+bYKCb0erWkLfW7EKalOTBp5TjWONSM4mX2OlZag7yq1P1YwMaBA51OkH0Ojic9fX
+majK2S/ZyFI6NLoPqN0Uw/K1HHU0DXpK/mf3YdFOEZMf9LVlXR0O6og19HxBmNnN
+LhTOQ29IGqNzayHGBi4U8LG+UAe5sxlC+gnnQEPGMrOS1XElybtHIxnqk2LJDvXj
+2Dj12TCISD9bQ53oRkudTvTPyvxK6OsnFC/wTBmHk03yxnZdQEKyj9guahiRb+hj
+sz4mDWWMmelcr6veEfzzlUZK7aoIrpJmgukhv/Qafwczo38J5U0=
+-----END CERTIFICATE-----

diff --git a/test/certs/setup.sh b/test/certs/setup.sh
index 2bb01fa13e5d41fe577da7e6dc066bb8e41f5af3..04591bcc05fe426d9117ff8b23379f0be695ec32 100755 (executable)
--- a/test/certs/setup.sh
+++ b/test/certs/setup.sh
@@ -1,10 +1,11 @@
-#! /bin/sh
+#! /bin/bash
 
 # Primary root: root-cert
 # root cert variants: CA:false, key2, DN2
 # trust variants: +serverAuth -serverAuth +clientAuth -clientAuth +anyEKU -anyEKU
 #
 ./mkcert.sh genroot "Root CA" root-key root-cert
+DAYS=-1 ./mkcert.sh genroot "Root CA" root-key root-expired
 ./mkcert.sh genss "Root CA" root-key root-nonca
 ./mkcert.sh genroot "Root CA" root-key2 root-cert2
 ./mkcert.sh genroot "Root Cert 2" root-key root-name2
@@ -168,7 +169,7 @@ openssl x509 -in sca-cert.pem -trustout \
 ./mkcert.sh genee server.example ee-key ee-name2 ca-key ca-name2
 ./mkcert.sh genee -p clientAuth server.example ee-key ee-client ca-key ca-cert
 ./mkcert.sh genee server.example ee-key ee-pathlen ca-key ca-cert \
-    -extfile <(echo "basicConstraints=CA:FALSE,pathlen:0")
+    -extfile <(echo "basicConstraints=CA:FALSE,pathlen:0") # bash needed here
 #
 openssl x509 -in ee-cert.pem -trustout \
     -addtrust serverAuth -out ee+serverAuth.pem

diff --git a/test/recipes/25-test_verify.t b/test/recipes/25-test_verify.t
index 9b8648e670643d0473f38dc56605c1df24a63d54..1336b8a7261570122440d2f93f7cb69674839441 100644 (file)
--- a/test/recipes/25-test_verify.t
+++ b/test/recipes/25-test_verify.t
@@ -27,7 +27,7 @@ sub verify {
     run(app([@args]));
 }
 
-plan tests => 143;
+plan tests => 145;
 
 # Canonical success
 ok(verify("ee-cert", "sslserver", ["root-cert"], ["ca-cert"]),
@@ -132,6 +132,10 @@ ok(!verify("ee-cert", "sslserver", [], [qw(ca-cert)], "-partial_chain"),
    "fail untrusted partial chain");
 ok(verify("ee-cert", "sslserver", [qw(ca-cert)], [], "-partial_chain"),
    "accept trusted partial chain");
+ok(!verify("ee-cert", "sslserver", [qw(ca-expired)], [], "-partial_chain"),
+   "reject expired trusted partial chain"); # this check is beyond RFC 5280
+ok(!verify("ee-cert", "sslserver", [qw(root-expired)], [qw(ca-cert)]),
+   "reject expired trusted root"); # this check is beyond RFC 5280
 ok(verify("ee-cert", "sslserver", [qw(sca-cert)], [], "-partial_chain"),
    "accept partial chain with server purpose");
 ok(!verify("ee-cert", "sslserver", [qw(cca-cert)], [], "-partial_chain"),