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.

(cherry picked from commit 3a1d2b59522163ebb83bb68e13c896188dc222c6)

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

diff --git a/crypto/asn1/bio_asn1.c b/crypto/asn1/bio_asn1.c
index 86ee566323052d5728e0dbd49780e68ae95d5a7f..7bb3c1fa16e26f82f8a1f251986d3c8a2047c921 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 d7d7d80eea912aabb74aa5f2b95443b138a3ea53..760e4846a474499866f21a7bb7ce47aac9451e52 100644 (file)
--- a/crypto/asn1/bio_ndef.c
+++ b/crypto/asn1/bio_ndef.c
@@ -142,6 +142,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 383c82d156c064a918330ef111134adc97df7baf..23a6e7e5ce616ea75440db148396484fd2ce338d 100644 (file)
--- a/test/bio_memleak_test.c
+++ b/test/bio_memleak_test.c
@@ -10,6 +10,8 @@
 #include <string.h>
 #include <openssl/buffer.h>
 #include <openssl/bio.h>
+#include <openssl/pkcs7.h>
+#include <openssl/obj_mac.h>
 
 #include "testutil.h"
 
@@ -35,7 +37,7 @@ static int test_bio_memleak(void)
         goto finish;
     ok = 1;
 
-finish:
+ finish:
     BIO_free(bio);
     return ok;
 }
@@ -62,7 +64,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 +100,7 @@ static int test_bio_new_mem_buf(void)
         goto finish;
     ok = 1;
 
-finish:
+ finish:
     BIO_free(bio);
     return ok;
 }
@@ -139,7 +141,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 +178,7 @@ static int test_bio_rdwr_rdonly(void)
 
     ok = 1;
 
-finish:
+ finish:
     BIO_free(bio);
     return ok;
 }
@@ -216,11 +218,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 global_init(void)
 {
     CRYPTO_set_mem_debug(1);
@@ -236,5 +299,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;
 }