search

mumble-voip / mumble

Mumble is an open-source, low-latency, high quality voice chat software.
https://www.mumble.info
Other
6.37k stars 1.12k forks source link

BIO_NOCLOSE leads to memory leaks while Cert processing #6603

Open botanegg opened 6 days ago

botanegg commented 6 days ago

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 (cb01bfa5200fce53db68b769d05995c999e7cdd8)

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 commented 6 days ago

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 commented 3 days ago

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 commented 2 days ago

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 commented 1 day ago

https://github.com/mumble-voip/mumble/blob/cb01bfa5200fce53db68b769d05995c999e7cdd8/src/murmur/Cert.cpp#L35-L38

https://github.com/mumble-voip/mumble/blob/cb01bfa5200fce53db68b769d05995c999e7cdd8/src/murmur/Cert.cpp#L40-L44

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

Hartmnt commented 1 day ago

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 :thinking: