CRL: Enforce proper handling of ASN1_TIME validation results

ASN1 correctly validates date fields and reports errors to the error
stack. Previously, even when validation failed, a CRL object was still
returned and could, in some cases, be successfully used for
verification.

This change fixes that behavior by ensuring validation errors are
properly handled and invalid CRLs are rejected.

Fixes #27445

Reviewed-by: Saša Nedvědický <sashan@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/29107)

diff --git a/CHANGES.md b/CHANGES.md
index 9fb19a6fb2d36b8461620a33f5379375eb69463c..a5cfe309845d2763d3df85464b4b0611e4f44269 100644 (file)
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -57,6 +57,14 @@ OpenSSL 4.0
 
    *Ryan Hooper*
 
+ * Fixed CRLs with invalid ASN1_TIME in invalidityDate extensions,
+   where verification incorrectly succeeded. Enforced proper
+   handling of ASN1_TIME validation results so that any CRL
+   containing invalid time fields is rejected immediately,
+   preventing the error from propagating to verification.
+
+   *Daniel Kubec*
+
  * Added `OSSL_[EN|DE]CODER_CTX_[set|get]_finalized()` functions.
    `OSSL_[EN|DE]CODER_CTX_set_*()` and `OSSL_[EN|DE]CODER_CTX_add_*()`
    functions return 0 if the context is already finalised.

diff --git a/crypto/x509/x_crl.c b/crypto/x509/x_crl.c
index eccd57e2433661d7f4f9125e2b073a113616ecd6..1a413d2d5411c5c5e5e39f0eac72158b7b71bbcb 100644 (file)
--- a/crypto/x509/x_crl.c
+++ b/crypto/x509/x_crl.c
@@ -86,6 +86,15 @@ static int crl_set_issuers(X509_CRL *crl)
     GENERAL_NAMES *gens, *gtmp;
     STACK_OF(X509_REVOKED) *revoked;
 
+    /*
+     * The CRL's nextUpdate field is optional, but we will still verify
+     * lastUpdate and treat missing or invalid values as a failure.
+     */
+    if (crl->crl.lastUpdate == NULL) {
+        crl->flags |= EXFLAG_INVALID;
+        return 0;
+    }
+
     revoked = X509_CRL_get_REVOKED(crl);
 
     gens = NULL;
@@ -94,6 +103,19 @@ static int crl_set_issuers(X509_CRL *crl)
         STACK_OF(X509_EXTENSION) *exts;
         ASN1_ENUMERATED *reason;
         X509_EXTENSION *ext;
+        const ASN1_TIME *rev_date;
+        ASN1_GENERALIZEDTIME *inv_date;
+        int nid;
+
+        /*
+         * The revocation date is a mandatory attribute. If we get NULL here, it
+         * means the validation has failed and an error has been pushed onto the
+         * error stack.
+         */
+        if ((rev_date = X509_REVOKED_get0_revocationDate(rev)) == NULL) {
+            crl->flags |= EXFLAG_INVALID;
+            return 0;
+        }
 
         gtmp = X509_REVOKED_get_ext_d2i(rev,
                                         NID_certificate_issuer, &j, NULL);
@@ -142,18 +164,30 @@ static int crl_set_issuers(X509_CRL *crl)
         } else
             rev->reason = CRL_REASON_NONE;
 
-        /* Check for critical CRL entry extensions */
+        /* Check for critical CRL entry extensions and validate time. */
 
         exts = rev->extensions;
 
         for (j = 0; j < sk_X509_EXTENSION_num(exts); j++) {
             ext = sk_X509_EXTENSION_value(exts, j);
+            nid = OBJ_obj2nid(X509_EXTENSION_get_object(ext));
+
+            /*
+             * If we obtain the extension but the time is NULL, it means that
+             * the validation has failed and the reason will be pushed onto the
+             * error stack.
+             */
+            if (nid == NID_invalidity_date) {
+                if ((inv_date = X509V3_EXT_d2i(ext)) == NULL) {
+                    crl->flags |= EXFLAG_INVALID;
+                    return 0;
+                }
+                ASN1_GENERALIZEDTIME_free(inv_date);
+            }
             if (X509_EXTENSION_get_critical(ext)) {
-                if (OBJ_obj2nid(X509_EXTENSION_get_object(ext))
-                    == NID_certificate_issuer)
+                if (nid == NID_certificate_issuer)
                     continue;
                 crl->flags |= EXFLAG_CRITICAL;
-                break;
             }
         }
 

diff --git a/test/crltest.c b/test/crltest.c
index 7920966b342eaf2b96c0f43601c28aa721fc22c3..e6c891978b8ab61fea7439091ee5d8e89803bf3e 100644 (file)
--- a/test/crltest.c
+++ b/test/crltest.c
@@ -295,6 +295,72 @@ static const char *kCertIssuerNoIDPCRL[] = {
     "0/FegR60yNqYaMERJz0jJv8SJ3Co38TlhH/Zr+N86RLYj3tPOsxcY5K1P8VZVPV/\n",
     "DxVqhesv7EaeiXDhiSTFcRXytqOQX3wju4RdxiyqMd4iT98N8nTxRdbBo4EVQKql\n",
     "PNhJBxQG0VQ=\n",
+    NULL
+};
+
+/*
+ * CRLs with an invalid Invalidity Date.
+ * https://github.com/openssl/openssl/issues/27445
+ */
+
+static const char *kInvalidDateMM[] = {
+    "-----BEGIN X509 CRL-----\n",
+    "MIICwDCCAagCAQEwDQYJKoZIhvcNAQELBQAweTELMAkGA1UEBhMCVVMxEzARBgNV\n",
+    "BAgMCkNhbGlmb3JuaWExFjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xEzARBgNVBAoM\n",
+    "Ck15IENvbXBhbnkxEzARBgNVBAMMCk15IFJvb3QgQ0ExEzARBgNVBAsMCk15IFJv\n",
+    "b3QgQ0EXDTI1MDEwMTAwMDAwMFoXDTI1MTIwMTAwMDAwMFowgbswJQIUHIACLvgf\n",
+    "JAXulqYS3LYf4KxwHl4XDTI1MDQxNzEwMTY1MVowSAIRAIy4GT7M5nHsAAAAAFgs\n",
+    "inoXDTI1MDMwNDAwMDAwMFowJDAKBgNVHRUEAwoBADAWBgNVHRgEDxgNMjAxMTEz\n",
+    "MTIyNDQ2WjBIAhEAjLgZPszmcewAAAAAWCyKehcNMjUwMzA0MDAwMDAwWjAkMAoG\n",
+    "A1UdFQQDCgEEMBYGA1UdGAQPGA0yMDEyMTMxMjI1NDdaoD0wOzAYBgNVHRQEEQIP\n",
+    "Gc//3tp07fL2pEYMzuFAMB8GA1UdIwQYMBaAFNdhiR+Tlot2VBbp5XfcfLdlG4Ak\n",
+    "MA0GCSqGSIb3DQEBCwUAA4IBAQCXPgi5aD+9nPVYmpebHQHeyZgyj5DWf+Jhb0iT\n",
+    "ljjOVLht83c59eCH2bsi+ZiGSI7d6nPdqP5PL0sX2Pp1NBEJk3LanlTXdmJbhEzV\n",
+    "uTEQPgtHt2fFHVLDbFatQhTpXt+wXTahogE1oRleunG2nYzSuDBUQHKj+2VEhPxh\n",
+    "ghMLkp3ZM59SJUp8MPWLLjoGtHsIYBHlw6clnq/7tmuzDYBZPerW2gMPjKuywSYj\n",
+    "pcWJOYTFzeOrEW5wRHVMs0jDwaOOeJNlRHEJ19SsGTDSNPTk8n3OwTKSOaJ+Y9M0\n",
+    "O2p9+7c2oIK6AnLuVNTyiBtEqMvukBkHT8PPPIpsJrzGUTj6\n",
+    "-----END X509 CRL-----\n",
+    NULL
+};
+
+static const char *kInvalidDateSS[] = {
+    "-----BEGIN X509 CRL-----\n",
+    "MIICdTCCAV0CAQEwDQYJKoZIhvcNAQELBQAweTELMAkGA1UEBhMCVVMxEzARBgNV\n",
+    "BAgMCkNhbGlmb3JuaWExFjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xEzARBgNVBAoM\n",
+    "Ck15IENvbXBhbnkxEzARBgNVBAMMCk15IFJvb3QgQ0ExEzARBgNVBAsMCk15IFJv\n",
+    "b3QgQ0EXDTI1MDEwMTAwMDAwMFoXDTI1MTIwMTAwMDAwMFowcTAlAhQcgAIu+B8k\n",
+    "Be6WphLcth/grHAeXhcNMjUwNDE3MTAxNjUxWjBIAhEAjLgZPszmcewAAAAAWCyK\n",
+    "ehcNMjUwMzA0MDAwMDAwWjAkMAoGA1UdFQQDCgEFMBYGA1UdGAQPGA0yMDI0MDgy\n",
+    "MTAwMDBaoD0wOzAYBgNVHRQEEQIPGc//3tp07fL2pEYMzuFAMB8GA1UdIwQYMBaA\n",
+    "FNdhiR+Tlot2VBbp5XfcfLdlG4AkMA0GCSqGSIb3DQEBCwUAA4IBAQCl9pd3BaSn\n",
+    "crbjvcjLZH0nomP8ipuez5+eTYSdb3Tpams7/70l/YrDZnR633LJLWKOTJpkP8DA\n",
+    "2e9FWVY086enUy3AxAzsAEpnFeuACPLqqGAAgOGy/Ad6gIwR3CK4vcF+SfSHNvh0\n",
+    "50305mFrur737C3yaC1MALqkMOPeZYIm+loKK8Q3qmk2dbt5Vj4hdi09tsti3Wl+\n",
+    "SoR94psjlmzgi3/+Wf5Ubdo9LhyXjjGlx/oZm+Y55Ti30NC4HuAA7UsWLwcaD23T\n",
+    "fLmUgatPdqozdGKtK0PsuxH2sfPaVnWQExkTBysZV4iQ7OvcadhShLyjwvGHT69D\n",
+    "EK028LrNrWTA\n",
+    "-----END X509 CRL-----\n",
+    NULL
+};
+
+static const char *kInvalidDateUTC[] = {
+
+    "-----BEGIN X509 CRL-----\n",
+    "MIICdTCCAV0CAQEwDQYJKoZIhvcNAQELBQAweTELMAkGA1UEBhMCVVMxEzARBgNV\n",
+    "BAgMCkNhbGlmb3JuaWExFjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xEzARBgNVBAoM\n",
+    "Ck15IENvbXBhbnkxEzARBgNVBAMMCk15IFJvb3QgQ0ExEzARBgNVBAsMCk15IFJv\n",
+    "b3QgQ0EXDTI1MDEwMTAwMDAwMFoXDTI1MTIwMTAwMDAwMFowcTAlAhQcgAIu+B8k\n",
+    "Be6WphLcth/grHAeXhcNMjUwNDE3MTAxNjUxWjBIAhEAjLgZPszmcewAAAAAWCyK\n",
+    "ehcNMjUwMzA0MDAwMDAwWjAkMAoGA1UdFQQDCgEEMBYGA1UdGAQPFw0yNDExMTQw\n",
+    "NjQ0MDBaoD0wOzAYBgNVHRQEEQIPGc//3tp07fL2pEYMzuFAMB8GA1UdIwQYMBaA\n",
+    "FNdhiR+Tlot2VBbp5XfcfLdlG4AkMA0GCSqGSIb3DQEBCwUAA4IBAQDKX5PynQJ8\n",
+    "EHENKO7avhGO2z/lz/7nU76tbkGVZHgS/Vufsr/x934sRTBxkGdE8COU67FiU+Yx\n",
+    "dO2yfPjHqgoxDlxXTrI71lElSCMURDY1vR/7cHhlbQlr/TXW4vLBnwAsXYx6gjV7\n",
+    "nHxvTwvb6DE5VXN7CrWfQ+UpVpE/OymjDVcPBBp5mMKvac4PaNdlGU3BcRGx+6iH\n",
+    "/CRNHU3fgOi37KqQ3rEZBRN1CI5JX7gFf6fCFRJNFnWez65FoHkA0L/J52y6QLdm\n",
+    "KPHBluIk4UD6eeZNDAC1keYDfIsY1fDvPm4W1Hd0J5QgjKcxFXK8qRi7BPy3UZjw\n",
+    "yYUQ4YV+e1Je\n",
     "-----END X509 CRL-----\n",
     NULL
 };
@@ -557,6 +623,56 @@ static int test_crl_cert_issuer_ext(void)
     return test;
 }
 
+/*
+ * This function clears the error stack before parsing and delegates the actual
+ * decoding to CRL_from_strings().
+ */
+static X509_CRL *crl_clear_err_parse(const char **pem)
+{
+    ERR_clear_error();
+    return CRL_from_strings(pem);
+}
+
+/*
+ * Checks whether a specific error reason is present in the error stack.
+ * This function iterates over the current thread's error queue using
+ * ERR_get_error_all(), extracting all pending errors. If any of them match
+ * the specified reason code (as returned by ERR_GET_REASON()), the function
+ * returns 1 to indicate that the corresponding error was found.
+ */
+static int err_chk(int lib, int reason)
+{
+#if defined(OPENSSL_NO_ERR) || defined(OPENSSL_SMALL_FOOTPRINT) || \
+    defined(OPENSSL_NO_DEPRECATED_3_0) || defined(OPENSSL_NO_HTTP)
+    return 1;
+#endif
+    unsigned long e;
+
+    while ((e = ERR_get_error_all(NULL, NULL, NULL, NULL, NULL)))
+        if (ERR_GET_LIB(e) == lib && ERR_GET_REASON(e) == reason)
+            return 1;
+
+    return 0;
+}
+
+static int test_crl_date_invalid(void)
+{
+    X509_CRL *tmm = NULL, *tss = NULL, *utc = NULL;
+    int test = 0;
+
+    test = TEST_ptr_null((tmm = crl_clear_err_parse(kInvalidDateMM)))
+        && TEST_true(err_chk(ERR_LIB_ASN1, ASN1_R_GENERALIZEDTIME_IS_TOO_SHORT))
+        && TEST_ptr_null((tss = crl_clear_err_parse(kInvalidDateSS)))
+        && TEST_true(err_chk(ERR_LIB_ASN1, ASN1_R_GENERALIZEDTIME_IS_TOO_SHORT))
+        && TEST_ptr_null((utc = crl_clear_err_parse(kInvalidDateUTC)))
+        && TEST_true(err_chk(ERR_LIB_ASN1, ASN1_R_WRONG_TAG));
+
+    X509_CRL_free(tmm);
+    X509_CRL_free(utc);
+    X509_CRL_free(tss);
+    return test;
+}
+
 int setup_tests(void)
 {
     if (!TEST_ptr(test_root = X509_from_strings(kCRLTestRoot))
@@ -571,6 +687,7 @@ int setup_tests(void)
     ADD_TEST(test_crl_empty_idp);
     ADD_TEST(test_known_critical_crl);
     ADD_TEST(test_crl_cert_issuer_ext);
+    ADD_TEST(test_crl_date_invalid);
     ADD_ALL_TESTS(test_unknown_critical_crl, OSSL_NELEM(unknown_critical_crls));
     ADD_ALL_TESTS(test_reuse_crl, 6);