Fix memory leak in i2d_ASN1_bio_stream

When creating a signed S/MIME message using SMIME_write_CMS()
if the reading from the bio fails, the state is therefore
still ASN1_STATE_START when BIO_flush() is called by i2d_ASN1_bio_stream().
This results in calling asn1_bio_flush_ex cleanup but will only
reset retry flags as the state is not ASN1_STATE_POST_COPY.
Therefore 48 bytes (Linux x86_64) leaked since the
ndef_prefix_free / ndef_suffix_free callbacks are not executed
and the ndef_aux structure is not freed.

By always calling free function callback in asn1_bio_free() the
memory leak is fixed.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/14844)

diff --git a/crypto/asn1/bio_asn1.c b/crypto/asn1/bio_asn1.c
index fa81b3a28ad589edc2f5d0a3954c96fa65ef9c5c..f792c08806624e2288675472276447ccbf2b2527 100644 (file)
--- a/crypto/asn1/bio_asn1.c
+++ b/crypto/asn1/bio_asn1.c
@@ -138,6 +138,11 @@ static int asn1_bio_free(BIO *b)
     if (ctx == NULL)
         return 0;
 
+    if (ctx->prefix_free != NULL)
+        ctx->prefix_free(b, &ctx->ex_buf, &ctx->ex_len, &ctx->ex_arg);
+    if (ctx->suffix_free != NULL)
+        ctx->suffix_free(b, &ctx->ex_buf, &ctx->ex_len, &ctx->ex_arg);
+
     OPENSSL_free(ctx->buf);
     OPENSSL_free(ctx);
     BIO_set_data(b, NULL);

diff --git a/crypto/asn1/bio_ndef.c b/crypto/asn1/bio_ndef.c
index df462d741a31ec7db8b64099f7195db1d76c7d91..d94e3a364497b69180045a0926e08f3d91a7f98d 100644 (file)
--- a/crypto/asn1/bio_ndef.c
+++ b/crypto/asn1/bio_ndef.c
@@ -143,6 +143,9 @@ static int ndef_prefix_free(BIO *b, unsigned char **pbuf, int *plen,
 
     ndef_aux = *(NDEF_SUPPORT **)parg;
 
+    if (ndef_aux == NULL)
+        return 0;
+
     OPENSSL_free(ndef_aux->derbuf);
 
     ndef_aux->derbuf = NULL;

diff --git a/test/bio_memleak_test.c b/test/bio_memleak_test.c
index cafc60e7b7ce1adabd71423b821deb91540a9a9d..518e7dd982be7a089cd307983ab1c23c183237ab 100644 (file)
--- a/test/bio_memleak_test.c
+++ b/test/bio_memleak_test.c
@@ -35,7 +35,7 @@ static int test_bio_memleak(void)
         goto finish;
     ok = 1;
 
-finish:
+ finish:
     BIO_free(bio);
     return ok;
 }
@@ -62,7 +62,7 @@ static int test_bio_get_mem(void)
         goto finish;
     ok = 1;
 
-finish:
+ finish:
     BIO_free(bio);
     BUF_MEM_free(bufmem);
     return ok;
@@ -98,7 +98,7 @@ static int test_bio_new_mem_buf(void)
         goto finish;
     ok = 1;
 
-finish:
+ finish:
     BIO_free(bio);
     return ok;
 }
@@ -139,7 +139,7 @@ static int test_bio_rdonly_mem_buf(void)
         goto finish;
     ok = 1;
 
-finish:
+ finish:
     BIO_free(bio);
     BIO_free(bio2);
     return ok;
@@ -176,7 +176,7 @@ static int test_bio_rdwr_rdonly(void)
 
     ok = 1;
 
-finish:
+ finish:
     BIO_free(bio);
     return ok;
 }
@@ -216,11 +216,72 @@ static int test_bio_nonclear_rst(void)
 
     ok = 1;
 
-finish:
+ finish:
     BIO_free(bio);
     return ok;
 }
 
+static int error_callback_fired;
+static long BIO_error_callback(BIO *bio, int cmd, const char *argp,
+                               size_t len, int argi,
+                               long argl, int ret, size_t *processed)
+{
+    if ((cmd & (BIO_CB_READ | BIO_CB_RETURN)) != 0) {
+        error_callback_fired = 1;
+        ret = 0;  /* fail for read operations to simulate error in input BIO */
+    }
+    return ret;
+}
+
+/* Checks i2d_ASN1_bio_stream() is freeing all memory when input BIO ends unexpectedly. */
+static int test_bio_i2d_ASN1_mime(void)
+{
+    int ok = 0;
+    BIO *bio = NULL, *out = NULL;
+    BUF_MEM bufmem;
+    static const char str[] = "BIO mime test\n";
+    PKCS7 *p7 = NULL;
+
+    if (!TEST_ptr(bio = BIO_new(BIO_s_mem())))
+        goto finish;
+
+    bufmem.length = sizeof(str);
+    bufmem.data = (char *) str;
+    bufmem.max = bufmem.length;
+    BIO_set_mem_buf(bio, &bufmem, BIO_NOCLOSE);
+    BIO_set_flags(bio, BIO_FLAGS_MEM_RDONLY);
+    BIO_set_callback_ex(bio, BIO_error_callback);
+
+    if (!TEST_ptr(out = BIO_new(BIO_s_mem())))
+        goto finish;
+    if (!TEST_ptr(p7 = PKCS7_new()))
+        goto finish;
+    if (!TEST_true(PKCS7_set_type(p7, NID_pkcs7_data)))
+        goto finish;
+
+    error_callback_fired = 0;
+
+    /*
+     * The call succeeds even if the input stream ends unexpectedly as
+     * there is no handling for this case in SMIME_crlf_copy().
+     */
+    if (!TEST_true(i2d_ASN1_bio_stream(out, (ASN1_VALUE*) p7, bio,
+                                       SMIME_STREAM | SMIME_BINARY,
+                                       ASN1_ITEM_rptr(PKCS7))))
+        goto finish;
+
+    if (!TEST_int_eq(error_callback_fired, 1))
+        goto finish;
+
+    ok = 1;
+
+ finish:
+    BIO_free(bio);
+    BIO_free(out);
+    PKCS7_free(p7);
+    return ok;
+}
+
 int setup_tests(void)
 {
     ADD_TEST(test_bio_memleak);
@@ -229,5 +290,6 @@ int setup_tests(void)
     ADD_TEST(test_bio_rdonly_mem_buf);
     ADD_TEST(test_bio_rdwr_rdonly);
     ADD_TEST(test_bio_nonclear_rst);
+    ADD_TEST(test_bio_i2d_ASN1_mime);
     return 1;
 }