From e32015b8f4e43c7223a87dd49d2d19fedce2e6b4 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 31 Aug 2025 17:25:40 -0400 Subject: [PATCH] Clear the extension list when removing the last extension The extensions list in a certificate, CRL, and CRL entry is defined as: ... extensions [3] EXPLICIT Extensions OPTIONAL ... ... crlEntryExtensions Extensions OPTIONAL ... ... crlExtensions [0] EXPLICIT Extensions OPTIONAL ... Extensions ::= SEQUENCE SIZE (1..MAX) OF Extension This means that a present but empty extensions list is actually invalid. Rather, if you have no extensions to encode, you are meant to omit the list altogether. Fix the delete_ext functions to handle this correctly. This would mostly be moot, as an application adding extensions only to delete them all would be unusual. However, #13658 implemented a slightly roundabout design where, to omit SKID/AKID, the library first puts them in and then the command-line tool detects some placeholder values and deletes the extension again. Fixes #28397 Reviewed-by: Paul Dale Reviewed-by: Tim Hudson Reviewed-by: Matt Caswell Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/28398) (cherry picked from commit 9a8d7dc14201aeeed1e77d54208e4af96916fc4f) --- crypto/x509/x509_ext.c | 31 ++++++++++-- test/x509_test.c | 109 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 3 deletions(-) diff --git a/crypto/x509/x509_ext.c b/crypto/x509/x509_ext.c index a7b85857bda..bed20bd5aeb 100644 --- a/crypto/x509/x509_ext.c +++ b/crypto/x509/x509_ext.c @@ -44,7 +44,15 @@ X509_EXTENSION *X509_CRL_get_ext(const X509_CRL *x, int loc) X509_EXTENSION *X509_CRL_delete_ext(X509_CRL *x, int loc) { - return X509v3_delete_ext(x->crl.extensions, loc); + X509_EXTENSION *ret = X509v3_delete_ext(x->crl.extensions, loc); + + /* Empty extension lists are omitted. */ + if (x->crl.extensions != NULL && + sk_X509_EXTENSION_num(x->crl.extensions) == 0) { + sk_X509_EXTENSION_pop_free(x->crl.extensions, X509_EXTENSION_free); + x->crl.extensions = NULL; + } + return ret; } void *X509_CRL_get_ext_d2i(const X509_CRL *x, int nid, int *crit, int *idx) @@ -91,7 +99,16 @@ X509_EXTENSION *X509_get_ext(const X509 *x, int loc) X509_EXTENSION *X509_delete_ext(X509 *x, int loc) { - return X509v3_delete_ext(x->cert_info.extensions, loc); + X509_EXTENSION *ret = X509v3_delete_ext(x->cert_info.extensions, loc); + + /* Empty extension lists are omitted. */ + if (x->cert_info.extensions != NULL && + sk_X509_EXTENSION_num(x->cert_info.extensions) == 0) { + sk_X509_EXTENSION_pop_free(x->cert_info.extensions, + X509_EXTENSION_free); + x->cert_info.extensions = NULL; + } + return ret; } int X509_add_ext(X509 *x, X509_EXTENSION *ex, int loc) @@ -139,7 +156,15 @@ X509_EXTENSION *X509_REVOKED_get_ext(const X509_REVOKED *x, int loc) X509_EXTENSION *X509_REVOKED_delete_ext(X509_REVOKED *x, int loc) { - return X509v3_delete_ext(x->extensions, loc); + X509_EXTENSION *ret = X509v3_delete_ext(x->extensions, loc); + + /* Empty extension lists are omitted. */ + if (x->extensions != NULL && + sk_X509_EXTENSION_num(x->extensions) == 0) { + sk_X509_EXTENSION_pop_free(x->extensions, X509_EXTENSION_free); + x->extensions = NULL; + } + return ret; } int X509_REVOKED_add_ext(X509_REVOKED *x, X509_EXTENSION *ex, int loc) diff --git a/test/x509_test.c b/test/x509_test.c index 1c6e569a4c4..1deef93ff4e 100644 --- a/test/x509_test.c +++ b/test/x509_test.c @@ -176,6 +176,112 @@ static int test_asn1_item_verify(void) return ret; } +static int test_x509_delete_last_extension(void) +{ + int ret = 0; + X509 *x509 = NULL; + X509_EXTENSION *ext = NULL; + ASN1_OBJECT *obj = NULL; + + if (!TEST_ptr((x509 = X509_new())) + /* Initially, there are no extensions and thus no extension list. */ + || !TEST_ptr_null(X509_get0_extensions(x509)) + /* Add an extension. */ + || !TEST_ptr((ext = X509_EXTENSION_new())) + || !TEST_ptr((obj = OBJ_nid2obj(NID_subject_key_identifier))) + || !TEST_int_eq(X509_EXTENSION_set_object(ext, obj), 1) + || !TEST_int_eq(X509_add_ext(x509, ext, -1), 1) + /* There should now be an extension list. */ + || !TEST_ptr(X509_get0_extensions(x509)) + || !TEST_int_eq(sk_X509_EXTENSION_num(X509_get0_extensions(x509)), 1)) + goto err; + + /* Delete the extension. */ + X509_EXTENSION_free(X509_delete_ext(x509, 0)); + + /* The extension list should be NULL again. */ + if (!TEST_ptr_null(X509_get0_extensions(x509))) + goto err; + + ret = 1; + +err: + X509_free(x509); + X509_EXTENSION_free(ext); + return ret; +} + +static int test_x509_crl_delete_last_extension(void) +{ + int ret = 0; + X509_CRL *crl = NULL; + X509_EXTENSION *ext = NULL; + ASN1_OBJECT *obj = NULL; + + if (!TEST_ptr((crl = X509_CRL_new())) + /* Initially, there are no extensions and thus no extension list. */ + || !TEST_ptr_null(X509_CRL_get0_extensions(crl)) + /* Add an extension. */ + || !TEST_ptr((ext = X509_EXTENSION_new())) + || !TEST_ptr((obj = OBJ_nid2obj(NID_subject_key_identifier))) + || !TEST_int_eq(X509_EXTENSION_set_object(ext, obj), 1) + || !TEST_int_eq(X509_CRL_add_ext(crl, ext, -1), 1) + /* There should now be an extension list. */ + || !TEST_ptr(X509_CRL_get0_extensions(crl)) + || !TEST_int_eq(sk_X509_EXTENSION_num(X509_CRL_get0_extensions(crl)), + 1)) + goto err; + + /* Delete the extension. */ + X509_EXTENSION_free(X509_CRL_delete_ext(crl, 0)); + + /* The extension list should be NULL again. */ + if (!TEST_ptr_null(X509_CRL_get0_extensions(crl))) + goto err; + + ret = 1; + +err: + X509_CRL_free(crl); + X509_EXTENSION_free(ext); + return ret; +} + +static int test_x509_revoked_delete_last_extension(void) +{ + int ret = 0; + X509_REVOKED *rev = NULL; + X509_EXTENSION *ext = NULL; + ASN1_OBJECT *obj = NULL; + + if (!TEST_ptr((rev = X509_REVOKED_new())) + /* Initially, there are no extensions and thus no extension list. */ + || !TEST_ptr_null(X509_REVOKED_get0_extensions(rev)) + /* Add an extension. */ + || !TEST_ptr((ext = X509_EXTENSION_new())) + || !TEST_ptr((obj = OBJ_nid2obj(NID_subject_key_identifier))) + || !TEST_int_eq(X509_EXTENSION_set_object(ext, obj), 1) + || !TEST_int_eq(X509_REVOKED_add_ext(rev, ext, -1), 1) + /* There should now be an extension list. */ + || !TEST_ptr(X509_REVOKED_get0_extensions(rev)) + || !TEST_int_eq(sk_X509_EXTENSION_num(X509_REVOKED_get0_extensions(rev)), 1)) + goto err; + + /* Delete the extension. */ + X509_EXTENSION_free(X509_REVOKED_delete_ext(rev, 0)); + + /* The extension list should be NULL again. */ + if (!TEST_ptr_null(X509_REVOKED_get0_extensions(rev))) + goto err; + + ret = 1; + +err: + X509_REVOKED_free(rev); + X509_EXTENSION_free(ext); + return ret; +} + OPT_TEST_DECLARE_USAGE("\n") int setup_tests(void) @@ -210,6 +316,9 @@ int setup_tests(void) ADD_TEST(test_x509_tbs_cache); ADD_TEST(test_x509_crl_tbs_cache); ADD_TEST(test_asn1_item_verify); + ADD_TEST(test_x509_delete_last_extension); + ADD_TEST(test_x509_crl_delete_last_extension); + ADD_TEST(test_x509_revoked_delete_last_extension); return 1; } -- 2.47.3