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 <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/22809)

(cherry picked from commit d550d2aae531c6fa2e10b1a30d2acdf373663889)

diff --git a/crypto/asn1/tasn_fre.c b/crypto/asn1/tasn_fre.c
index 13aa6a728e2cce88ca3418afd07a395fb8758004..9035bc1b5c300ee3c54f7e75ce6e95d3f0faa1a8 100644 (file)
--- 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 8af5e03a0926dc6fc7fb342a626fc0678ecbd35f..d659e96cc83f2634b32309702d3f0cd1c34d0036 100644 (file)
--- 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;
 }