BIO_NOCLOSE leads to memory leaks while Cert processing #6603

botanegg · 2024-10-11T12:46:38Z

Description

BIO_NOCLOSE leads to memory leaks while Cert processing

Steps to reproduce

build with asan
run mumle
connect to server
quit
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_

The text was updated successfully, but these errors were encountered:

BIO_set_close with BIO_NOCLOSE argument leads to memory leaks here

BIO_set_close with BIO_NOCLOSE argument leads to memory leaks here Closes mumble-voip#6603

botanegg · 2024-10-11T13:19:30Z

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 · 2024-10-14T17:57:10Z

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 · 2024-10-15T16:06:10Z

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 · 2024-10-16T12:57:26Z

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 · 2024-10-16T15:37:50Z

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 · 2024-10-25T18:14:22Z

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

botanegg · 2024-10-25T19:37:01Z

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

Krzmbrzl · 2025-01-12T17:51:50Z

After having tried to read through the OpenSSL documentation, I conclude that

OpenSSL docs are horrible. They are very sparse on actual details
What we are creating are so-called memory-BIOs. So their only purpose is to wrap pre-existing memory regions into a BIO object so that OpenSSL can work with them.

From 2. I deduce that it would be pretty silly if these BIOs would, under any circumstances delete the buffer they are wrapping. By design, OpenSSL does not own the wrapped buffer and therefore (probably!) doesn't try to delete them, no matter what.

If this is true, then we indeed don't need any special precautions, i.e. the BIO_set_close call is unnecessary.

The truthfulness of this assumption is supported by

having a memory leak when using BIO_set_close which goes away when not using that function
All examples that I could find that make use of BIO_new_mem_buf (such as https://stackoverflow.com/a/53608170) simply call BIO_free on the BIO object, without using BIO_set_close.
The few examples I could find that did set the BIO_NOCLOSE flag were extracting part of the internal BIO structure and in order to take ownership of this extracted part, the BIO_NOCLOSE flag needs to be set. We don't do this in our case though.

While I don't claim to 100% understand the OpenSSL API (not even the little bit that we are using), I am confident enough with my observations to agree with @botanegg's proposed changes.

BIO_set_close with BIO_NOCLOSE argument leads to OpenSSL not (fully) deleting the allocated BIO struct under the assumption that the user code has taken ownership of it. However, in our case, this is not the case and therefore OpenSSL should do the deletion as usual. The flag was probably introduced under the assumption that the component that either is or isn't deleted by OpenSSL was the externally provided buffer that is wrapped into a BIO object via BIO_new_mem_buf. However, this is not the case. OpenSSL doesn't take ownership of the provided buffer and therefore also doesn't delete it. Closes mumble-voip#6603

botanegg added bug A bug (error) in the software triage This issue is waiting to be triaged by one of the project members labels Oct 11, 2024

botanegg added a commit to botanegg/mumble that referenced this issue Oct 11, 2024

FIX(client): Fix memory leak in CertWizard::importCert mumble-voip#6603

a428773

BIO_set_close with BIO_NOCLOSE argument leads to memory leaks here

botanegg added a commit to botanegg/mumble that referenced this issue Oct 11, 2024

FIX(server): Fix memory leak in Server::isKeyForCert mumble-voip#6603

8ce2972

BIO_set_close with BIO_NOCLOSE argument leads to memory leaks here

botanegg added a commit to botanegg/mumble that referenced this issue Oct 11, 2024

FIX(client): Fix memory leak in CertWizard::importCert

c02eab3

BIO_set_close with BIO_NOCLOSE argument leads to memory leaks here Closes mumble-voip#6603

botanegg added a commit to botanegg/mumble that referenced this issue Oct 11, 2024

FIX(server): Fix memory leak in Server::isKeyForCert

138f3a4

BIO_set_close with BIO_NOCLOSE argument leads to memory leaks here Closes mumble-voip#6603

Hartmnt linked a pull request Oct 14, 2024 that will close this issue

FIX(client): Fix memory leaks due to BIO_NOCLOSE flag #6604

Merged

1 task

Krzmbrzl mentioned this issue Jan 12, 2025

FIX(client): Fix memory leaks due to BIO_NOCLOSE flag #6604

Merged

1 task

Krzmbrzl closed this as completed in #6604 Jan 13, 2025

Krzmbrzl closed this as completed in 1fcb27f Jan 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BIO_NOCLOSE leads to memory leaks while Cert processing #6603

BIO_NOCLOSE leads to memory leaks while Cert processing #6603

botanegg commented Oct 11, 2024

botanegg commented Oct 11, 2024

Hartmnt commented Oct 14, 2024

davidebeatrici commented Oct 15, 2024

botanegg commented Oct 16, 2024 •

edited

Loading

Hartmnt commented Oct 16, 2024

botanegg commented Oct 25, 2024 •

edited

Loading

botanegg commented Oct 25, 2024

Krzmbrzl commented Jan 12, 2025

BIO_NOCLOSE leads to memory leaks while Cert processing #6603

BIO_NOCLOSE leads to memory leaks while Cert processing #6603

Comments

botanegg commented Oct 11, 2024

Description

Steps to reproduce

Mumble version

Mumble component

OS

Reproducible?

Additional information

Relevant log output

botanegg commented Oct 11, 2024

Hartmnt commented Oct 14, 2024

davidebeatrici commented Oct 15, 2024

botanegg commented Oct 16, 2024 • edited Loading

Hartmnt commented Oct 16, 2024

botanegg commented Oct 25, 2024 • edited Loading

botanegg commented Oct 25, 2024

Krzmbrzl commented Jan 12, 2025

botanegg commented Oct 16, 2024 •

edited

Loading

botanegg commented Oct 25, 2024 •

edited

Loading