Commit bde5f49e9a for openssl.org
commit bde5f49e9a9a7bc0f825df42f5e3796bbecd48d0
Author: Viktor Dukhovni <openssl-users@dukhovni.org>
Date: Wed Mar 4 01:35:36 2026 +1100
Some more X509 extension add/del polish
- In various structures with optional X.509 extensions, deallocate and
NULL out the extensions pointer when the extensions become empty after
an extension is deleted. This uses a new X509v3_delete_extension()
helper function. Added corresponding docs.
- Do the same in X509V3_EXT_add_nconf_sk() if after processing all
the pending updates the stack becomes empty.
- Handle resulting NULL stack in X509V3_EXT_REQ_add_nconf() and
update_req_extensions().
- Improved testing of certificate SKID/AKID addition and implicit
removal via "none" value.
Reviewed-by: Saša Nedvědický <sashan@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
MergeDate: Thu Mar 5 17:40:10 2026
(Merged from https://github.com/openssl/openssl/pull/30252)
diff --git a/crypto/ocsp/ocsp_ext.c b/crypto/ocsp/ocsp_ext.c
index 81cc51fdc9..4142d0b029 100644
--- a/crypto/ocsp/ocsp_ext.c
+++ b/crypto/ocsp/ocsp_ext.c
@@ -48,7 +48,7 @@ const X509_EXTENSION *OCSP_REQUEST_get_ext(OCSP_REQUEST *x, int loc)
X509_EXTENSION *OCSP_REQUEST_delete_ext(OCSP_REQUEST *x, int loc)
{
- return X509v3_delete_ext(x->tbsRequest.requestExtensions, loc);
+ return X509v3_delete_extension(&x->tbsRequest.requestExtensions, loc);
}
void *OCSP_REQUEST_get1_ext_d2i(OCSP_REQUEST *x, int nid, int *crit, int *idx)
@@ -98,7 +98,7 @@ const X509_EXTENSION *OCSP_ONEREQ_get_ext(OCSP_ONEREQ *x, int loc)
X509_EXTENSION *OCSP_ONEREQ_delete_ext(OCSP_ONEREQ *x, int loc)
{
- return X509v3_delete_ext(x->singleRequestExtensions, loc);
+ return X509v3_delete_extension(&x->singleRequestExtensions, loc);
}
void *OCSP_ONEREQ_get1_ext_d2i(OCSP_ONEREQ *x, int nid, int *crit, int *idx)
@@ -149,7 +149,7 @@ const X509_EXTENSION *OCSP_BASICRESP_get_ext(OCSP_BASICRESP *x, int loc)
X509_EXTENSION *OCSP_BASICRESP_delete_ext(OCSP_BASICRESP *x, int loc)
{
- return X509v3_delete_ext(x->tbsResponseData.responseExtensions, loc);
+ return X509v3_delete_extension(&x->tbsResponseData.responseExtensions, loc);
}
void *OCSP_BASICRESP_get1_ext_d2i(OCSP_BASICRESP *x, int nid, int *crit,
@@ -203,7 +203,7 @@ const X509_EXTENSION *OCSP_SINGLERESP_get_ext(OCSP_SINGLERESP *x, int loc)
X509_EXTENSION *OCSP_SINGLERESP_delete_ext(OCSP_SINGLERESP *x, int loc)
{
- return X509v3_delete_ext(x->singleExtensions, loc);
+ return X509v3_delete_extension(&x->singleExtensions, loc);
}
void *OCSP_SINGLERESP_get1_ext_d2i(OCSP_SINGLERESP *x, int nid, int *crit,
diff --git a/crypto/ts/ts_req_utils.c b/crypto/ts/ts_req_utils.c
index a8818d56d6..00a1a681af 100644
--- a/crypto/ts/ts_req_utils.c
+++ b/crypto/ts/ts_req_utils.c
@@ -169,7 +169,7 @@ const X509_EXTENSION *TS_REQ_get_ext(TS_REQ *a, int loc)
X509_EXTENSION *TS_REQ_delete_ext(TS_REQ *a, int loc)
{
- return X509v3_delete_ext(a->extensions, loc);
+ return X509v3_delete_extension(&a->extensions, loc);
}
int TS_REQ_add_ext(TS_REQ *a, X509_EXTENSION *ex, int loc)
diff --git a/crypto/ts/ts_rsp_utils.c b/crypto/ts/ts_rsp_utils.c
index c83bedf79f..81b5e899c7 100644
--- a/crypto/ts/ts_rsp_utils.c
+++ b/crypto/ts/ts_rsp_utils.c
@@ -330,7 +330,7 @@ const X509_EXTENSION *TS_TST_INFO_get_ext(TS_TST_INFO *a, int loc)
X509_EXTENSION *TS_TST_INFO_delete_ext(TS_TST_INFO *a, int loc)
{
- return X509v3_delete_ext(a->extensions, loc);
+ return X509v3_delete_extension(&a->extensions, loc);
}
int TS_TST_INFO_add_ext(TS_TST_INFO *a, X509_EXTENSION *ex, int loc)
diff --git a/crypto/x509/v3_conf.c b/crypto/x509/v3_conf.c
index 6db7f37d68..848a4895c3 100644
--- a/crypto/x509/v3_conf.c
+++ b/crypto/x509/v3_conf.c
@@ -346,6 +346,10 @@ int X509V3_EXT_add_nconf_sk(CONF *conf, X509V3_CTX *ctx, const char *section,
}
X509_EXTENSION_free(ext);
}
+ if (sk != NULL && sk_X509_EXTENSION_num(*sk) == 0) {
+ sk_X509_EXTENSION_free(*sk);
+ *sk = NULL;
+ }
return 1;
}
@@ -357,6 +361,7 @@ int X509V3_EXT_add_nconf(CONF *conf, X509V3_CTX *ctx, const char *section,
X509 *cert)
{
STACK_OF(X509_EXTENSION) **sk = NULL;
+
if (cert != NULL)
sk = &cert->cert_info.extensions;
return X509V3_EXT_add_nconf_sk(conf, ctx, section, sk);
@@ -379,17 +384,19 @@ static int
update_req_extensions(X509_REQ *req, int *pnid, STACK_OF(X509_EXTENSION) *exts)
{
unsigned char *ext = NULL;
- int ret = 0, loc = -1, extlen;
+ int ret = 0, loc = -1, extlen = 0;
if (pnid == NULL || *pnid == NID_undef)
if ((pnid = X509_REQ_get_extension_nids()) == NULL)
return 0;
loc = X509at_get_attr_by_NID(req->req_info.attributes, *pnid, -1);
- extlen = ASN1_item_i2d((const ASN1_VALUE *)exts,
- &ext, ASN1_ITEM_rptr(X509_EXTENSIONS));
- if (extlen <= 0)
- return ret;
+ if (exts != NULL) {
+ extlen = ASN1_item_i2d((const ASN1_VALUE *)exts,
+ &ext, ASN1_ITEM_rptr(X509_EXTENSIONS));
+ if (extlen <= 0)
+ return ret;
+ }
if (loc != -1) {
X509_ATTRIBUTE *att = X509at_delete_attr(req->req_info.attributes, loc);
@@ -433,7 +440,7 @@ int X509V3_EXT_REQ_add_nconf(CONF *conf, X509V3_CTX *ctx, const char *section,
ret = X509V3_EXT_add_nconf_sk(conf, ctx, section, &exts);
/* Replace original extension list (stack) with updated stack */
- if (ret && req != NULL && exts != NULL)
+ if (ret && req != NULL)
ret = update_req_extensions(req, pnid, exts);
sk_X509_EXTENSION_pop_free(exts, X509_EXTENSION_free);
return ret;
diff --git a/crypto/x509/x509_ext.c b/crypto/x509/x509_ext.c
index 2b73e14a60..7f23f3a666 100644
--- a/crypto/x509/x509_ext.c
+++ b/crypto/x509/x509_ext.c
@@ -104,32 +104,28 @@ X509_EXTENSION *X509_delete_ext(X509 *x, int loc)
{
X509_EXTENSION *ret;
- if (x->cert_info.extensions == NULL)
- return NULL;
- if ((ret = delete_ext(&x->cert_info.extensions, loc)) != NULL)
+ ret = X509v3_delete_extension(&x->cert_info.extensions, loc);
+ if (ret != NULL)
x->cert_info.enc.modified = 1;
return ret;
}
int X509_add_ext(X509 *x, const X509_EXTENSION *ex, int loc)
{
- STACK_OF(X509_EXTENSION) *exts = x->cert_info.extensions;
+ STACK_OF(X509_EXTENSION) **exts = &x->cert_info.extensions;
- if (X509v3_add_ext(&exts, ex, loc) == NULL)
+ /* x->cert_info.extensions might initially be NULL */
+ if (X509v3_add_ext(exts, ex, loc) == NULL)
return 0;
/*
- * A duplicate empty SKID/AKID extension can displace a prior non-empty
- * one, but is then not itself added, so, somewhat counter-intutively, the
- * the extension list can become empty after an "add", in which case we must
- * drop the extension stack entirely, setting it to NULL. The extensions
- * list is either non-empty or absent.
+ * An ignored "empty" SKID or AKID extension will appear to be successfully
+ * added, even though nothing is pushed onto the resulting stack. However,
+ * if the stack was initially NULL or empty, it will now be non-NULL, but
+ * empty, deallocate and make it NULL in that case.
*/
- if (sk_X509_EXTENSION_num(exts) != 0) {
- x->cert_info.extensions = exts;
- } else {
- sk_X509_EXTENSION_free(exts);
- sk_X509_EXTENSION_pop_free(x->cert_info.extensions, X509_EXTENSION_free);
- x->cert_info.extensions = NULL;
+ if (sk_X509_EXTENSION_num(*exts) == 0) {
+ sk_X509_EXTENSION_free(*exts);
+ *exts = NULL;
}
x->cert_info.enc.modified = 1;
return 1;
diff --git a/crypto/x509/x509_v3.c b/crypto/x509/x509_v3.c
index f9ea5f75fe..60b18d2d13 100644
--- a/crypto/x509/x509_v3.c
+++ b/crypto/x509/x509_v3.c
@@ -100,6 +100,22 @@ X509_EXTENSION *X509v3_delete_ext(STACK_OF(X509_EXTENSION) *x, int loc)
return ret;
}
+X509_EXTENSION *X509v3_delete_extension(STACK_OF(X509_EXTENSION) **x, int loc)
+{
+ X509_EXTENSION *ext;
+
+ if (x == NULL)
+ return NULL;
+
+ /* Set extensions to NULL when last element dropped */
+ if ((ext = X509v3_delete_ext(*x, loc)) != NULL
+ && sk_X509_EXTENSION_num(*x) == 0) {
+ sk_X509_EXTENSION_free(*x);
+ *x = NULL;
+ }
+ return ext;
+}
+
STACK_OF(X509_EXTENSION) *X509v3_add_ext(STACK_OF(X509_EXTENSION) **x,
const X509_EXTENSION *ex, int loc)
{
diff --git a/doc/man3/X509v3_get_ext_by_NID.pod b/doc/man3/X509v3_get_ext_by_NID.pod
index d9e4257940..298afdbe57 100644
--- a/doc/man3/X509v3_get_ext_by_NID.pod
+++ b/doc/man3/X509v3_get_ext_by_NID.pod
@@ -4,14 +4,15 @@
X509v3_get_ext_count, X509v3_get_ext, X509v3_get_ext_by_NID,
X509v3_get_ext_by_OBJ, X509v3_get_ext_by_critical, X509v3_delete_ext,
-X509v3_add_ext, X509v3_add_extensions, X509_get_ext_count, X509_get_ext,
-X509_get_ext_by_NID, X509_get_ext_by_OBJ, X509_get_ext_by_critical,
-X509_delete_ext, X509_add_ext, X509_CRL_get_ext_count, X509_CRL_get_ext,
-X509_CRL_get_ext_by_NID, X509_CRL_get_ext_by_OBJ, X509_CRL_get_ext_by_critical,
-X509_CRL_delete_ext, X509_CRL_add_ext, X509_REVOKED_get_ext_count,
-X509_REVOKED_get_ext, X509_REVOKED_get_ext_by_NID, X509_REVOKED_get_ext_by_OBJ,
-X509_REVOKED_get_ext_by_critical, X509_REVOKED_delete_ext,
-X509_REVOKED_add_ext - extension stack utility functions
+X509v3_delete_extension, X509v3_add_ext, X509v3_add_extensions,
+X509_get_ext_count, X509_get_ext, X509_get_ext_by_NID, X509_get_ext_by_OBJ,
+X509_get_ext_by_critical, X509_delete_ext, X509_add_ext,
+X509_CRL_get_ext_count, X509_CRL_get_ext, X509_CRL_get_ext_by_NID,
+X509_CRL_get_ext_by_OBJ, X509_CRL_get_ext_by_critical, X509_CRL_delete_ext,
+X509_CRL_add_ext, X509_REVOKED_get_ext_count, X509_REVOKED_get_ext,
+X509_REVOKED_get_ext_by_NID, X509_REVOKED_get_ext_by_OBJ,
+X509_REVOKED_get_ext_by_critical, X509_REVOKED_delete_ext, X509_REVOKED_add_ext
+- extension stack utility functions
=head1 SYNOPSIS
@@ -27,6 +28,7 @@ X509_REVOKED_add_ext - extension stack utility functions
int X509v3_get_ext_by_critical(const STACK_OF(X509_EXTENSION) *x,
int crit, int lastpos);
X509_EXTENSION *X509v3_delete_ext(STACK_OF(X509_EXTENSION) *x, int loc);
+ X509_EXTENSION *X509v3_delete_extension(STACK_OF(X509_EXTENSION) **x, int loc);
STACK_OF(X509_EXTENSION) *X509v3_add_ext(STACK_OF(X509_EXTENSION) **x,
X509_EXTENSION *ex, int loc);
STACK_OF(X509_EXTENSION)
@@ -82,6 +84,12 @@ X509v3_delete_ext() deletes the extension with index I<loc> from I<x>.
The deleted extension is returned and must be freed by the caller.
If I<loc> is an invalid index value, NULL is returned.
+X509v3_delete_extension() extends X509v3_delete_ext() by deallocating the
+extension stack I<*x> if it becomes empty, and in that case also setting I<*x>
+to NULL.
+This is a convenience wrapper for cases in which extensions are optional and
+should be omitted if the stack becomes empty.
+
X509v3_add_ext() inserts extension I<ex> to STACK I<*x> at position I<loc>.
If I<loc> is -1, the new extension is added to the end.
A new STACK is allocated if I<*x> is NULL.
@@ -140,8 +148,9 @@ that case.
X509v3_get_ext_count() returns the extension count or 0 for failure.
-X509v3_get_ext(), X509v3_delete_ext() and X509_delete_ext() return an
-B<X509_EXTENSION> structure or NULL if an error occurs.
+X509v3_get_ext(), X509v3_delete_ext(), X509v3_delete_extension() and
+X509_delete_ext() return an B<X509_EXTENSION> structure or NULL if an error
+occurs.
X509v3_get_ext_by_OBJ() and X509v3_get_ext_by_critical() return
the extension index or -1 if an error occurs.
@@ -164,6 +173,8 @@ L<X509V3_get_d2i(3)>
X509v3_add_extensions() was added in OpenSSL 3.4.
+X509v3_delete_extension() was added in OpenSSL 4.0.
+
=head1 COPYRIGHT
Copyright 2015-2024 The OpenSSL Project Authors. All Rights Reserved.
diff --git a/include/openssl/x509.h.in b/include/openssl/x509.h.in
index 84d65269cc..578df3fa6c 100644
--- a/include/openssl/x509.h.in
+++ b/include/openssl/x509.h.in
@@ -905,6 +905,7 @@ int X509v3_get_ext_by_critical(const STACK_OF(X509_EXTENSION) *x,
int crit, int lastpos);
const X509_EXTENSION *X509v3_get_ext(const STACK_OF(X509_EXTENSION) *x, int loc);
X509_EXTENSION *X509v3_delete_ext(STACK_OF(X509_EXTENSION) *x, int loc);
+X509_EXTENSION *X509v3_delete_extension(STACK_OF(X509_EXTENSION) **x, int loc);
STACK_OF(X509_EXTENSION) *X509v3_add_ext(STACK_OF(X509_EXTENSION) **x,
const X509_EXTENSION *ex, int loc);
STACK_OF(X509_EXTENSION) *X509v3_add_extensions(STACK_OF(X509_EXTENSION) **target,
diff --git a/test/x509_test.c b/test/x509_test.c
index 1381a1d33d..30f4770650 100644
--- a/test/x509_test.c
+++ b/test/x509_test.c
@@ -288,10 +288,13 @@ err:
static int test_drop_empty_cert_keyids(void)
{
static const unsigned char commonName[] = "test";
+ BIO *bio = NULL;
+ CONF *conf = NULL;
X509 *x = NULL;
X509_NAME *subject = NULL;
X509_NAME_ENTRY *name_entry = NULL;
X509_EXTENSION *ext = NULL;
+ const STACK_OF(X509_EXTENSION) *exts;
X509V3_CTX ctx;
int ret = 0;
@@ -312,23 +315,59 @@ static int test_drop_empty_cert_keyids(void)
|| !TEST_int_eq(X509_set_pubkey(x, pubkey), 1))
goto err;
- X509V3_set_ctx(&ctx, x, x, NULL, NULL, 0);
- if (!TEST_ptr(ext = X509V3_EXT_conf(NULL, &ctx, "subjectKeyIdentifier",
- "none"))
+ /*
+ * Check that X509_add_ext() does not create non-NULL empty stack when
+ * adding an ignored extension (from initial NULL state).
+ */
+ X509V3_set_ctx(&ctx, x, x, NULL, NULL, X509V3_CTX_REPLACE);
+ if (!TEST_ptr(ext = X509V3_EXT_conf(NULL, &ctx, "subjectKeyIdentifier", "none"))
|| !TEST_int_eq(X509_add_ext(x, ext, -1), 1)
|| !TEST_ptr_null(X509_get0_extensions(x)))
goto err;
+ /* Add non-empty SKID */
+ if (!TEST_ptr(bio = BIO_new(BIO_s_mem()))
+ || !TEST_int_ge(BIO_printf(bio, "subjectKeyIdentifier = hash\n"), 0)
+ || !TEST_ptr(conf = NCONF_new(NULL))
+ || !TEST_int_gt(NCONF_load_bio(conf, bio, NULL), 0))
+ goto err;
+ (void)BIO_reset(bio);
+
+ X509V3_set_nconf(&ctx, conf);
+ if (!TEST_true(X509V3_EXT_add_nconf(conf, &ctx, "default", x))
+ || !TEST_ptr(exts = X509_get0_extensions(x))
+ || !TEST_int_eq(sk_X509_EXTENSION_num(exts), 1))
+ goto err;
+
+ /* Request "empty" SKID and AKID in order to drop any previous values */
+ NCONF_free(conf);
+ if (!TEST_ptr(conf = NCONF_new(NULL))
+ || !TEST_int_ge(BIO_printf(bio, "subjectKeyIdentifier = none\n"), 0)
+ || !TEST_int_gt(NCONF_load_bio(conf, bio, NULL), 0))
+ goto err;
+
+ X509V3_set_nconf(&ctx, conf);
+ if (!TEST_true(X509V3_EXT_add_nconf(conf, &ctx, "default", x))
+ || !TEST_int_gt(X509_sign(x, privkey, signmd), 0)
+ || !TEST_ptr_null(X509_get0_extensions(x)))
+ goto err;
+
+ /*
+ * Now check that a non-empty extension is actually added via
+ * X509_add_ext().
+ */
X509_EXTENSION_free(ext);
- if (!TEST_ptr(ext = X509V3_EXT_conf(NULL, &ctx, "authorityKeyIdentifier",
- "none"))
+ if (!TEST_ptr(ext = X509V3_EXT_conf(NULL, &ctx, "subjectKeyIdentifier", "hash"))
|| !TEST_int_eq(X509_add_ext(x, ext, -1), 1)
- || !TEST_ptr_null(X509_get0_extensions(x))
- || !TEST_int_gt(X509_sign(x, privkey, signmd), 0))
+ || !TEST_int_gt(X509_sign(x, privkey, signmd), 0)
+ || !TEST_ptr(exts = X509_get0_extensions(x))
+ || !TEST_int_eq(sk_X509_EXTENSION_num(exts), 1))
goto err;
ret = 1;
err:
+ BIO_free(bio);
+ NCONF_free(conf);
X509_NAME_ENTRY_free(name_entry);
X509_NAME_free(subject);
X509_EXTENSION_free(ext);
diff --git a/util/libcrypto.num b/util/libcrypto.num
index b694a335c5..b8dcd55a86 100644
--- a/util/libcrypto.num
+++ b/util/libcrypto.num
@@ -5709,6 +5709,7 @@ OSSL_ENCODER_CTX_ctrl_string ? 4_0_0 EXIST::FUNCTION:
OPENSSL_sk_set_cmp_thunks ? 4_0_0 EXIST::FUNCTION:
ASN1_BIT_STRING_set1 ? 4_0_0 EXIST::FUNCTION:
OSSL_ESS_check_signing_certs_ex ? 4_0_0 EXIST::FUNCTION:
+X509v3_delete_extension ? 4_0_0 EXIST::FUNCTION:
X509_new ? 4_0_0 EXIST::FUNCTION:
X509_free ? 4_0_0 EXIST::FUNCTION:
d2i_X509 ? 4_0_0 EXIST::FUNCTION: