From: Daniel Kubec Date: Sat, 24 Jan 2026 19:50:42 +0000 (+0100) Subject: ASN.1: Raise additional errors in crl_set_issuers() X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=dc01d6cfa1211a2ca9ff793e4381c9ad268ec996;p=thirdparty%2Fopenssl.git ASN.1: Raise additional errors in crl_set_issuers() Additional ASN.1 parsing errors are now raised to the error stack, allowing invalid CRLs to be rejected early with detailed error messages. Reviewed-by: Neil Horman Reviewed-by: Tomas Mraz MergeDate: Tue Feb 3 09:02:15 2026 (Merged from https://github.com/openssl/openssl/pull/29750) --- diff --git a/crypto/x509/x_crl.c b/crypto/x509/x_crl.c index 7314d30dbd7..a0c93a750c5 100644 --- a/crypto/x509/x_crl.c +++ b/crypto/x509/x_crl.c @@ -8,7 +8,6 @@ */ #include -#include "internal/cryptlib.h" #include #include #include "crypto/x509.h" @@ -80,7 +79,6 @@ ASN1_SEQUENCE_enc(X509_CRL_INFO, enc, crl_inf_cb) = { static int crl_set_issuers(X509_CRL *crl) { - int i, j; GENERAL_NAMES *most_recent_issuer, *gtmp; STACK_OF(X509_REVOKED) *revoked; @@ -91,6 +89,8 @@ static int crl_set_issuers(X509_CRL *crl) */ if (crl->crl.lastUpdate == NULL) { crl->flags |= EXFLAG_INVALID; + ERR_raise_data(ERR_LIB_ASN1, ASN1_R_ILLEGAL_TIME_VALUE, + "lastUpdate in CRL is not well-formed"); return 0; } @@ -121,6 +121,8 @@ static int crl_set_issuers(X509_CRL *crl) */ if ((rev_date = X509_REVOKED_get0_revocationDate(rev)) == NULL) { crl->flags |= EXFLAG_INVALID; + ERR_raise_data(ERR_LIB_ASN1, ASN1_R_ILLEGAL_TIME_VALUE, + "revocationDate in CRL is not well-formed"); return 0; } @@ -139,6 +141,8 @@ static int crl_set_issuers(X509_CRL *crl) */ if (crl->idp == NULL || !crl->idp->indirectCRL) { crl->flags |= EXFLAG_INVALID; + ERR_raise_data(ERR_LIB_ASN1, ASN1_R_INVALID_VALUE, + "CRL Certificate Issuer extension requires Indirect CRL flag to be set"); GENERAL_NAMES_free(gtmp); return 0; } @@ -171,7 +175,6 @@ static int crl_set_issuers(X509_CRL *crl) rev->reason = CRL_REASON_NONE; /* Check for critical CRL entry extensions and validate time. */ - exts = rev->extensions; for (j = 0; j < sk_X509_EXTENSION_num(exts); j++) { @@ -186,6 +189,8 @@ static int crl_set_issuers(X509_CRL *crl) if (nid == NID_invalidity_date) { if ((inv_date = X509V3_EXT_d2i(ext)) == NULL) { crl->flags |= EXFLAG_INVALID; + ERR_raise_data(ERR_LIB_ASN1, ASN1_R_ILLEGAL_TIME_VALUE, + "invalidityDate in CRL is not well-formed"); return 0; } ASN1_GENERALIZEDTIME_free(inv_date); diff --git a/test/crltest.c b/test/crltest.c index 0d81ac69dbf..efcb7692ea8 100644 --- a/test/crltest.c +++ b/test/crltest.c @@ -628,27 +628,23 @@ 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); -} - 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)); + test = TEST_ptr_null((tmm = CRL_from_strings(kInvalidDateMM))) + && TEST_err_r(ERR_LIB_ASN1, ASN1_R_GENERALIZEDTIME_IS_TOO_SHORT) + && TEST_err_r(ERR_LIB_ASN1, ASN1_R_ILLEGAL_TIME_VALUE) + && TEST_err_s("invalidityDate in CRL is not well-formed") + && TEST_ptr_null((tss = CRL_from_strings(kInvalidDateSS))) + && TEST_err_r(ERR_LIB_ASN1, ASN1_R_GENERALIZEDTIME_IS_TOO_SHORT) + && TEST_err_r(ERR_LIB_ASN1, ASN1_R_ILLEGAL_TIME_VALUE) + && TEST_err_s("invalidityDate in CRL is not well-formed") + && TEST_ptr_null((utc = CRL_from_strings(kInvalidDateUTC))) + && TEST_err_r(ERR_LIB_ASN1, ASN1_R_WRONG_TAG) + && TEST_err_r(ERR_LIB_ASN1, ASN1_R_ILLEGAL_TIME_VALUE) + && TEST_err_s("invalidityDate in CRL is not well-formed"); X509_CRL_free(tmm); X509_CRL_free(utc); diff --git a/test/errtest.c b/test/errtest.c index 323184d93bd..a6c27ef3ab0 100644 --- a/test/errtest.c +++ b/test/errtest.c @@ -420,6 +420,36 @@ err: return res; } +static int test_error_reason(void) +{ + int test; + + ERR_raise(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE); + ERR_raise(ERR_LIB_CRYPTO, ERR_R_INTERNAL_ERROR); + ERR_raise(ERR_LIB_CRYPTO, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); + + test = TEST_err_r(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE) + && TEST_err_r(ERR_LIB_CRYPTO, ERR_R_INTERNAL_ERROR) + && TEST_err_r(ERR_LIB_CRYPTO, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); + + return test; +} + +static int test_error_string(void) +{ + int test; + + ERR_raise_data(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE, "malloc failure"); + ERR_raise_data(ERR_LIB_CRYPTO, ERR_R_INTERNAL_ERROR, "internal error"); + + test = TEST_err_r(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE) + && TEST_err_r(ERR_LIB_CRYPTO, ERR_R_INTERNAL_ERROR) + && TEST_err_s("malloc failure") + && TEST_err_s("internal error"); + + return test; +} + int setup_tests(void) { ADD_TEST(preserves_system_error); @@ -431,5 +461,7 @@ int setup_tests(void) ADD_TEST(test_marks); ADD_ALL_TESTS(test_save_restore, 2); ADD_TEST(test_clear_error); + ADD_TEST(test_error_reason); + ADD_TEST(test_error_string); return 1; } diff --git a/test/testutil.h b/test/testutil.h index de4023f761e..1a43a96faf4 100644 --- a/test/testutil.h +++ b/test/testutil.h @@ -379,6 +379,23 @@ int test_mem_ne(const char *, int, const char *, const char *, int test_true(const char *file, int line, const char *s, int b); int test_false(const char *file, int line, const char *s, int b); +/* + * Checks whether a specific error reason is present in the error stack. + * This function iterates over the current thread's error queue 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. + */ +int test_err_r(const char *file, int line, int lib, int reason); + +/* + * Checks whether a specific string is present in the error data stack. + * This function iterates over the current thread's error queue extracting all + * pending errors. If any of them match the specified string the function + * returns 1 to indicate that the corresponding error was found. + */ +int test_err_s(const char *file, int line, const char *data); + /* * Comparisons between BIGNUMs. * BIGNUMS can be compared against other BIGNUMs or zero. @@ -520,6 +537,9 @@ void test_perror(const char *s); #define TEST_true(a) test_true(__FILE__, __LINE__, #a, (a) != 0) #define TEST_false(a) test_false(__FILE__, __LINE__, #a, (a) != 0) +#define TEST_err_r(a, b) test_err_r(__FILE__, __LINE__, a, b) +#define TEST_err_s(a) test_err_s(__FILE__, __LINE__, a) + #define TEST_BN_eq(a, b) test_BN_eq(__FILE__, __LINE__, #a, #b, a, b) #define TEST_BN_ne(a, b) test_BN_ne(__FILE__, __LINE__, #a, #b, a, b) #define TEST_BN_lt(a, b) test_BN_lt(__FILE__, __LINE__, #a, #b, a, b) @@ -661,13 +681,5 @@ X509_CRL *CRL_from_strings(const char **pem); * into |*out| so we can free it. */ BIO *glue2bio(const char **pem, char **out); -/* - * 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. - */ -int err_chk(int lib, int reason); #endif /* OSSL_TESTUTIL_H */ diff --git a/test/testutil/load.c b/test/testutil/load.c index d75e14983e3..c3ab1d2700b 100644 --- a/test/testutil/load.c +++ b/test/testutil/load.c @@ -130,6 +130,7 @@ X509_CRL *CRL_from_strings(const char **pem) return NULL; } + ERR_clear_error(); crl = PEM_read_bio_X509_CRL(b, NULL, NULL, NULL); OPENSSL_free(p); @@ -151,28 +152,10 @@ X509 *X509_from_strings(const char **pem) return NULL; } + ERR_clear_error(); x = PEM_read_bio_X509(b, NULL, NULL, NULL); OPENSSL_free(p); BIO_free(b); return x; } - -int err_chk(int lib, int reason) -{ -#if defined(OPENSSL_NO_ERR) || defined(OPENSSL_SMALL_FOOTPRINT) - return 1; -#endif -#if 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; -} diff --git a/test/testutil/tests.c b/test/testutil/tests.c index ee83f8a4a96..7299726e79f 100644 --- a/test/testutil/tests.c +++ b/test/testutil/tests.c @@ -357,6 +357,97 @@ int test_mem_ne(const char *file, int line, const char *st1, const char *st2, return 1; } +#if defined(OPENSSL_NO_ERR) || defined(OPENSSL_NO_DEPRECATED_3_0) || defined(OPENSSL_SMALL_FOOTPRINT) + +int test_err_r(const char *file, int line, int lib, int reason) +{ + return 1; +} + +int test_err_s(const char *file, int line, const char *data) +{ + return 1; +} + +#else + +struct test_err_expect_ctx { + struct test_err_expect { + char *file, *fn, *data; + unsigned long code; + int line; + } errs[ERR_NUM_ERRORS]; + int err_count; +}; + +static int err_r_cb(int lib, int reason, struct test_err_expect *e) +{ + if (ERR_GET_LIB(e->code) == lib && ERR_GET_REASON(e->code) == reason) + return 1; + return 0; +} + +static int err_s_cb(const char *data, struct test_err_expect *e) +{ + if (e->data != NULL && strcmp(e->data, data) == 0) + return 1; + return 0; +} + +static int test_err_helper(int lib, int reason, const char *str) +{ + struct test_err_expect_ctx ctx; + int result = 0; + + for (ctx.err_count = 0;; ctx.err_count++) { + struct test_err_expect *e = &ctx.errs[ctx.err_count]; + const char *file = NULL, *fn = NULL, *data = NULL; + + if ((e->code = ERR_get_error_all(&file, &e->line, &fn, &data, NULL)) == 0) + break; + + /* + * Usage after free won't actually happen in tests, but for greater + * robustness, paying for 3 allocations per record in the test is not a + * problem and we also gain robustness against future changes in the + * error stack. + */ + + e->file = file != NULL ? OPENSSL_strdup(file) : NULL; + e->fn = fn != NULL ? OPENSSL_strdup(fn) : NULL; + e->data = data != NULL ? OPENSSL_strdup(data) : NULL; + } + + for (int i = 0; i < ctx.err_count; i++) { + struct test_err_expect *e = &ctx.errs[i]; + + if ((str != NULL ? err_s_cb(str, e) : err_r_cb(lib, reason, e)) == 1) + result = 1; + + ERR_new(); + ERR_set_debug(e->file, e->line, e->fn); + ERR_set_error(ERR_GET_LIB(e->code), ERR_GET_REASON(e->code), e->data); + + OPENSSL_free(e->file); + OPENSSL_free(e->fn); + OPENSSL_free(e->data); + } + + return result; +} + +int test_err_r(const char *file, int line, int lib, int reason) +{ + return test_err_helper(lib, reason, NULL); +} + +int test_err_s(const char *file, int line, const char *data) +{ + return test_err_helper(0, 0, data); +} + +#endif + #define DEFINE_BN_COMPARISONS(opname, op, zero_cond) \ int test_BN_##opname(const char *file, int line, \ const char *s1, const char *s2, \ diff --git a/test/x509_internal_test.c b/test/x509_internal_test.c index ecd6d7651ea..155a5662635 100644 --- a/test/x509_internal_test.c +++ b/test/x509_internal_test.c @@ -519,13 +519,54 @@ static int tests_X509_check_crypto(void) test = TEST_ptr((rsa_n_neg = X509_from_strings(kRSAModulusNeg))) && TEST_ptr_null((pub = X509_get_pubkey(rsa_n_neg))) - && TEST_true(err_chk(ERR_LIB_EVP, EVP_R_DECODE_ERROR)); + && TEST_err_r(ERR_LIB_EVP, EVP_R_DECODE_ERROR); EVP_PKEY_free(pub); X509_free(rsa_n_neg); return test; } +/* https://github.com/openssl/openssl/issues/11722 */ +static const char *kDistributionPointWrongTag[] = { + "-----BEGIN CERTIFICATE-----\n", + "MIIDiTCCAnGgAwIBAgIFAO3UVNIwDQYJKoZIhvcNAQELBQAwfTEWMBQGA1UEBhMN\n", + "VW5pdGVkTmF0aW9uczEQMA4GA1UECAwHTmV3WW9yazEQMA4GA1UEBwwHTmV3WW9y\n", + "azEcMBoGA1UECgwTU29mdHdhcmVFbmdpbmVlcmluZzEQMA4GA1UECwwHVGVzdGlu\n", + "ZzEPMA0GA1UEAwwGdW4ub3JnMCIYDzIwMTcwMTIzMDkzMDAwWhgPMjAxODEyMjMw\n", + "OTMwMDBaMGwxCzAJBgNVBAYTAlVOMRAwDgYDVQQIEwdOZXdZb3JrMRAwDgYDVQQH\n", + "EwdOZXdZb3JrMQ8wDQYDVQQKEwZzdWJPcmcxEzARBgNVBAsTCnN1Yk9yZ1VuaXQx\n", + "EzARBgNVBAMTCnN1Yi51bi5vcmcwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEK\n", + "AoIBAQDBpo9Viz3OQa6vmsyULUFLgznPkB3OX5Yf5XFBnZWoAu2d9qNypidARWUs\n", + "DomjtevXKrQApnmuWlxqLV5Q4JGbggeq6UsAwnoXCnH7zhQqInczr0U7rzlUYsZq\n", + "H+5haEfV9OewOIFHStAVXyTOqJuEfZOQKZbhhx7TDYg9IpoQisvKB4HbPfi6a6BV\n", + "YPAqNFlSfnYsF7sHNztTJXxyRY/KjrBeKVWs3n2+QQaydI+seiDD1GKBhApHrrWo\n", + "XtaP4VFbSyPszlRnW0ICAVrMItmt1rJBJlARVRq+gpU0gifmMheNBTWt8js6Ms/i\n", + "XeSzBkrQtFsnbVE75qeTrybOxqTXAgMBAAGjHTAbMBkGA1UdHwEB/wQPMA2BCwBS\n", + "ZWFzb24uLi4AMA0GCSqGSIb3DQEBCwUAA4IBAQCzwoxTrHgICZeYE7owZxV39BZh\n", + "MAHYYzS16/EXdXPZvZFQkL+wMBGkPC82s/3D/4kjHUwDxmmu2jBR8k+vEiV5VMnw\n", + "ZcoS22KFNVskk+CBfP0G5/d+ZfFMuW1tE3B1sO7RvYT1MtYt+DryRZ7vvLv7MlQb\n", + "sE+le0VjCfZHAZ+D3GqhYNNy+qhKYaHQDg/tfA/J28yyYm1EMzUd//Bao9BbnRxi\n", + "p4x2WfCFGB/ZP9BV0VA2KH3qF5M1RAETch/YbWqOIn+LxKomhvQSwQ4DEmRHRu69\n", + "loi+aH8qoQ4hb91EeaNb3OCV3azSH8I8RGGZDM2I2fZmgFwZ+5w7rgFjKe6b\n", + "-----END CERTIFICATE-----\n", + NULL +}; + +static int tests_x509_check_dpn(void) +{ + X509 *cert = NULL; + STACK_OF(DIST_POINT) *crls = NULL; + int test, nid = NID_crl_distribution_points; + + test = TEST_ptr((cert = X509_from_strings(kDistributionPointWrongTag))) + && TEST_ptr_null((crls = X509_get_ext_d2i(cert, nid, NULL, NULL))) + && TEST_err_r(ERR_LIB_ASN1, ASN1_R_WRONG_TAG); + + sk_DIST_POINT_pop_free(crls, DIST_POINT_free); + X509_free(cert); + return test; +} + int setup_tests(void) { ADD_TEST(test_standard_exts); @@ -533,5 +574,6 @@ int setup_tests(void) ADD_TEST(tests_X509_PURPOSE); ADD_TEST(tests_X509_check_time); ADD_TEST(tests_X509_check_crypto); + ADD_TEST(tests_x509_check_dpn); return 1; }