BIO_NOCLOSE leads to memory leaks while Cert processing

[botanegg]

Description

BIO_NOCLOSE leads to memory leaks while Cert processing

Steps to reproduce

1. build with asan
2. run mumle
3. connect to server
4. quit
5. look inside asan logs

Mumble version

master (cb01bfa)

Mumble component

Both

OS

Linux

Reproducible?

Yes

Additional information

No response

Relevant log output

Direct leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x78fe050fd891 in malloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x78fe0417c4f1 in CRYPTO_malloc (/usr/lib/libcrypto.so.3+0x17c4f1) (BuildId: 47080a1667cb8fc00b7cb144d099a63942d6bee4)
    #2 0x78fe0417c555 in CRYPTO_zalloc (/usr/lib/libcrypto.so.3+0x17c555) (BuildId: 47080a1667cb8fc00b7cb144d099a63942d6bee4)
    #3 0x78fe04092f95 in BUF_MEM_new_ex (/usr/lib/libcrypto.so.3+0x92f95) (BuildId: 47080a1667cb8fc00b7cb144d099a63942d6bee4)
    #4 0x78fe040825af  (/usr/lib/libcrypto.so.3+0x825af) (BuildId: 47080a1667cb8fc00b7cb144d099a63942d6bee4)
    #5 0x78fe04076b5c in BIO_new_ex (/usr/lib/libcrypto.so.3+0x76b5c) (BuildId: 47080a1667cb8fc00b7cb144d099a63942d6bee4)
    #6 0x78fe0407cf81 in BIO_new_mem_buf (/usr/lib/libcrypto.so.3+0x7cf81) (BuildId: 47080a1667cb8fc00b7cb144d099a63942d6bee4)
    #7 0x5aa7560d3e0f in CertWizard::importCert(QByteArray, QString const&) .../mumble/src/mumble/Cert.cpp:439

Direct leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x7a432c2fd891 in malloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x7a432b77c4f1 in CRYPTO_malloc (/usr/lib/libcrypto.so.3+0x17c4f1) (BuildId: 47080a1667cb8fc00b7cb144d099a63942d6bee4)
    #2 0x7a432b77c555 in CRYPTO_zalloc (/usr/lib/libcrypto.so.3+0x17c555) (BuildId: 47080a1667cb8fc00b7cb144d099a63942d6bee4)
    #3 0x7a432b692f95 in BUF_MEM_new_ex (/usr/lib/libcrypto.so.3+0x92f95) (BuildId: 47080a1667cb8fc00b7cb144d099a63942d6bee4)
    #4 0x7a432b6825af  (/usr/lib/libcrypto.so.3+0x825af) (BuildId: 47080a1667cb8fc00b7cb144d099a63942d6bee4)
    #5 0x7a432b676b5c in BIO_new_ex (/usr/lib/libcrypto.so.3+0x76b5c) (BuildId: 47080a1667cb8fc00b7cb144d099a63942d6bee4)
    #6 0x7a432b67cf81 in BIO_new_mem_buf (/usr/lib/libcrypto.so.3+0x7cf81) (BuildId: 47080a1667cb8fc00b7cb144d099a63942d6bee4)
    #7 0x5eb5d31e51e5 in Server::isKeyForCert(QSslKey const&, QSslCertificate const&) .../mumble/src/murmur/Cert.cpp:40

Direct leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x7a432c2fd891 in malloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x7a432b77c4f1 in CRYPTO_malloc (/usr/lib/libcrypto.so.3+0x17c4f1) (BuildId: 47080a1667cb8fc00b7cb144d099a63942d6bee4)
    #2 0x7a432b77c555 in CRYPTO_zalloc (/usr/lib/libcrypto.so.3+0x17c555) (BuildId: 47080a1667cb8fc00b7cb144d099a63942d6bee4)
    #3 0x7a432b692f95 in BUF_MEM_new_ex (/usr/lib/libcrypto.so.3+0x92f95) (BuildId: 47080a1667cb8fc00b7cb144d099a63942d6bee4)
    #4 0x7a432b6825af  (/usr/lib/libcrypto.so.3+0x825af) (BuildId: 47080a1667cb8fc00b7cb144d099a63942d6bee4)
    #5 0x7a432b676b5c in BIO_new_ex (/usr/lib/libcrypto.so.3+0x76b5c) (BuildId: 47080a1667cb8fc00b7cb144d099a63942d6bee4)
    #6 0x7a432b67cf81 in BIO_new_mem_buf (/usr/lib/libcrypto.so.3+0x7cf81) (BuildId: 47080a1667cb8fc00b7cb144d099a63942d6bee4)
    #7 0x5eb5d31e5180 in Server::isKeyForCert(QSslKey const&, QSslCertificate const&) .../mumble/src/murmur/Cert.cpp:35

### Screenshots

_No response_

[botanegg]

Even seems like we don't need call BIO here

Simple call d2i_Private Key on qbaKey should return same result
Anyway it should be proved in other issue

[Hartmnt]

I am trying to follow the cause of the issue here. So the warnings you posted here complain about memory allocated by Cert.cpp:439 Cert.cpp:40 and Cert.cpp:35, the BIO_new_mem_buf call. Sounds reasonable.

But I struggle to see how Q_UNUSED(BIO_set_close(mem, BIO_NOCLOSE)); can cause a memory leak here unless there is something buggy in the openssl implementation. The man page reads like this just sets a flag in whatever is referencing the memory.

If you remove the Q_UNUSED(BIO_set_close(mem, BIO_NOCLOSE)); and the memory leak goes away, wouldn't the more plausible explanation be that the allocated memory is defaulting to BIO_CLOSE, the d2i_* methods do not magically set the BIO_NOCLOSE flag and our codebase simply does not deallocate the memory manually we used in BIO_new_mem_buf (which is still a bug but has nothing todo with BIO_set_close)?

@Krzmbrzl @davidebeatrici

[davidebeatrici]

You're right. From OpenSSL's documentation:

BIO_set_close() sets the BIO b close flag to flag. flag can take the value BIO_CLOSE or BIO_NOCLOSE. Typically BIO_CLOSE is used in a source/sink BIO to indicate that the underlying I/O stream should be closed when the BIO is freed.

[botanegg]

mumble/src/murmur/Cert.cpp

Lines 35 to 38 in cb01bfa

	mem = BIO_new_mem_buf(qbaKey.data(), static_cast< int >(qbaKey.size()));
	Q_UNUSED(BIO_set_close(mem, BIO_NOCLOSE));
	pkey = d2i_PrivateKey_bio(mem, nullptr);
	BIO_free(mem);

mumble/src/murmur/Cert.cpp

Lines 40 to 44 in cb01bfa

	mem = BIO_new_mem_buf(qbaCert.data(), static_cast< int >(qbaCert.size()));
	Q_UNUSED(BIO_set_close(mem, BIO_NOCLOSE));
	x509 = d2i_X509_bio(mem, nullptr);
	BIO_free(mem);
	mem = nullptr;

There is attempt to free memory, but flag prevents it.

[Hartmnt]

If I understand correctly, BIO_NOCLOSE and BIO_CLOSE just refer to the underlying buffer. In this case qbaKey.data. So the BIO_NOCLOSE should mean that qbaKey.data is not freed, which would be correct, if we assume it is freed by Qt.

It should not affect whether or not mem is freed by BIO_free. Why would this even be an option? It does not make any sense.

Looking at the bio_free documentation it clearly reads:

BIO_free() frees up a single BIO , BIO_vfree() also frees up a single BIO but it does not return a value. Calling BIO_free() may also have some effect on the underlying I/O structure, for example it may close the file being referred to under certain circumstances. For more details see the individual BIO_METHOD descriptions.

BIO_free_all() frees up an entire BIO chain, it does not halt if an error occurs freeing up an individual BIO in the chain.

If BIO_free() is called on a BIO chain it will only free one BIO resulting in a memory leak.

Maybe using d2i_X509_bio (and others) creates a chain and we need to use BIO_free_all()? But then again, why would removing the BIO_set_close fix the memory leak 🤔

[botanegg]

Still leak with BIO_free_all
Just because BIO_NOCLOSE prevents to "close" resource

[botanegg]

OKAY
I'll try to explain

Core files is bss_mem.c and bio_lib.c

We create a BIO struct with BIO_new_mem_buf using mem_method inside.
It means we use mem_new as new method, mem_ctrl as ctrl method and mem_free as free method
then we allocate bio_buf_mem_st with buf_mem_st inside, set data pointer to qbaKey.data and set shutdown = 1

using mem_ctrl with BIO_NOCLOSE we set shutdown = 0

Finally, we cannot deallocate the buf_mem_st here

If this explanation is not enough, please just close the issue and PR..
It seems like is too hard to save 32 bytes :D