Repository navigation
Incorrect calls to OPENSSL_malloc_init() can lead to an endless loop. #2865
Description
The following code snippet triggers either an endless loop or a stack overflow (depending on compiler settings) in OpenSSL 1.1.0c. Please note that the call order in this example is actually violating the API (i.e. SSL_library_init() should be called before OPENSSL_malloc_init()).
OPENSSL_malloc_init()
SSL_library_init() // should have been called prior to OPENSSL_malloc_init()
The problem occurs due to the fact, that malloc_impl would be set to the CRYPTO_malloc-function which is linked into the 3rd-party application while CRYPTO_malloc being actually called from the DLL. Therefore, the check in the function which seems to be designed to prevent the recursive call fails and will recursively call CRYPTO_malloc().
Note: The issue was discussed as a potential security issue with Rich Salz by mail and assessed to not be a security issue. He suggested to eventually raise it as a feature request to strengthen OpenSSL against such an incorrect usage. After reading up on the design goals for OpenSSL 1.1.0 (i.e. simplifying/strengthening OpenSSL's initialization behavior), I thought I'd give it a shot and raise it as an issue for you to decide whether you feel like something on OpenSSL's side should be done about this.
Please be aware that I spotted at least one case of such an incorrect usage of the API in the wild in Apache Serf (details are available here: http://www.luke1410.de:8090/browse/MAXSVN-83 - Issue 4).
Activity
rhuijben commentedon Nov 22, 2017
I reproduced this, this morning. This happens for me when apr-util initializes OpenSSL from a separate DLL, while it is also used from the main executable. Comparing global function pointers to see if they match some specific function has more than a few issues on Windows.
mattcaswell commentedon Apr 26, 2018
I'm not really sure why we need the
OPENSSL_malloc_init()macro at all. There is a comment in the header next to it:But that doesn't really serve as a satisfactory explanation. The API docs aren't particularly enlightening either:
What happens if you remove the call to this macro altogether?
Note that
SSL_library_init()is just a compatibility macro that calls the 1.1.0 functionOPENSSL_init_ssl(). Normally you don't need to call that either in 1.1.0.richsalz commentedon Jun 27, 2018
This is not a 1.1.1-specific issue, so removing that milestone. (It was reported against 1.1.0)
Jan-E commentedon Feb 2, 2019
@nono303 discovered in nono303/win-svn#3 that this issue was causing a subversion test to run into an endless loop. Another incorrect usage of the API in the wild can be found here:
https://svn.apache.org/viewvc/apr/apr-util/branches/1.6.x/crypto/apr_crypto_openssl.c?view=markup#l127
The patch: nono303/win-svn#3 (comment)
BZ-63139: crypto_openssl endless loop OpenSSL 1.1
BZ-63139: endless loop in apr_crypto_openssl.dll
mattcaswell commentedon Feb 4, 2019
@richsalz AFAICT you wrote the docs for
OPENSSL_malloc_init():Do you know what the "certain shared-library situations" are? I can't think of a good reason for this macro and I am tempted to just make it a noop. Or perphaps @levitte knows.
richsalz commentedon Feb 4, 2019
I looked at 0.9.8 and a recent 1.0.2 and could not find any related documentation (see INSTALL.NW in 0.9.8, maybe that was it). My recollection is that there might be something in RT, or email discussion on the team list around the time I did this, that described the rare corner case. I didn't want to put details in the manpage, so I just put that text there. Barring a compelling reason, making it a no-op seems safe to me.
Make OPENSSL_malloc_init() a no-op
mattcaswell commentedon Feb 5, 2019
A proposed fix for this issue is in #8167.
Make OPENSSL_malloc_init() a no-op
openssl patched for [#2865](openssl/openssl#2865)
openssl patched for [#2865](openssl/openssl#2865)
FS#76001 Remove openssl-malloc-init.patch
FS#76001 Remove openssl-malloc-init.patch