From d550d2aae531c6fa2e10b1a30d2acdf373663889 Mon Sep 17 00:00:00 2001 From: Bernd Edlinger Date: Tue, 12 Mar 2024 20:04:56 +0100 Subject: Fix unpredictible refcount handling of d2i functions The passed in reference of a ref-counted object is free'd by d2i functions in the error handling. However if it is not the last reference, the in/out reference variable is not set to null here. This makes it impossible for the caller to handle the error correctly, because there are numerous cases where the passed in reference is free'd and set to null, while in other cases, where the passed in reference is not free'd, the reference is left untouched. Therefore the passed in reference must be set to NULL even when it was not the last reference. Fixes #23713 Reviewed-by: Dmitry Belyavskiy Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/22809) --- crypto/asn1/tasn_fre.c | 6 +++++- test/crltest.c | 23 ++++++++++++++++++----- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/crypto/asn1/tasn_fre.c b/crypto/asn1/tasn_fre.c index 13aa6a728e..9035bc1b5c 100644 --- a/crypto/asn1/tasn_fre.c +++ b/crypto/asn1/tasn_fre.c @@ -85,8 +85,12 @@ void ossl_asn1_item_embed_free(ASN1_VALUE **pval, const ASN1_ITEM *it, int embed case ASN1_ITYPE_NDEF_SEQUENCE: case ASN1_ITYPE_SEQUENCE: - if (ossl_asn1_do_lock(pval, -1, it) != 0) /* if error or ref-counter > 0 */ + if (ossl_asn1_do_lock(pval, -1, it) != 0) { + /* if error or ref-counter > 0 */ + OPENSSL_assert(embed == 0); + *pval = NULL; return; + } if (asn1_cb) { i = asn1_cb(ASN1_OP_FREE_PRE, pval, it, NULL); if (i == 2) diff --git a/test/crltest.c b/test/crltest.c index 76b53f46dc..fe686946ba 100644 --- a/test/crltest.c +++ b/test/crltest.c @@ -381,12 +381,24 @@ static int test_unknown_critical_crl(int n) static int test_reuse_crl(int idx) { X509_CRL *result, *reused_crl = CRL_from_strings(kBasicCRL); - char *p; - BIO *b = glue2bio(idx == 2 ? kRevokedCRL : kInvalidCRL + idx, &p); + X509_CRL *addref_crl = NULL; + char *p = NULL; + BIO *b = NULL; int r = 0; - if (!TEST_ptr(reused_crl) - || !TEST_ptr(b)) + if (!TEST_ptr(reused_crl)) + goto err; + + if (idx & 1) { + if (!TEST_true(X509_CRL_up_ref(reused_crl))) + goto err; + addref_crl = reused_crl; + } + + idx >>= 1; + b = glue2bio(idx == 2 ? kRevokedCRL : kInvalidCRL + idx, &p); + + if (!TEST_ptr(b)) goto err; result = PEM_read_bio_X509_CRL(b, &reused_crl, NULL, NULL); @@ -416,6 +428,7 @@ static int test_reuse_crl(int idx) OPENSSL_free(p); BIO_free(b); X509_CRL_free(reused_crl); + X509_CRL_free(addref_crl); return r; } @@ -430,7 +443,7 @@ int setup_tests(void) ADD_TEST(test_bad_issuer_crl); ADD_TEST(test_known_critical_crl); ADD_ALL_TESTS(test_unknown_critical_crl, OSSL_NELEM(unknown_critical_crls)); - ADD_ALL_TESTS(test_reuse_crl, 3); + ADD_ALL_TESTS(test_reuse_crl, 6); return 1; } -- cgit v1.2.3-70-g09d2