On Wed, May 24, 2023 at 2:16 PM Theo Buehler <t...@theobuehler.org> wrote:
> On Tue, May 23, 2023 at 12:40:40PM -0400, Marc Aldorasi wrote:
> > The man page for CRYPTO_get_ex_new_index says that "the value 0 is
> > reserved for the legacy "app_data" APIs", but the function can still
> > return 0, which can cause issues for programs that use both APIs. The
> > attached patch causes the returned indices to start at 1 instead.
> >
> > See also the corresponding OpenSSL bug report:
> > https://marc.info/?l=openssl-dev&m=142421750627504&w=2
>
> Thanks for the diff. This makes some sense, but I need to look closer to
> understand the full ramifications. Unfortunately, OpenSSL's rt is now
> lost to the Internet Dark Ages, so information is a bit lacking.
>
> What made you write this patch? Is there a real-world issue you ran
> into? If so, could you point me at it?
>
> Thanks.
The issue is that I have an application that uses SSL_get_ex_data and
SSL_set_ex_data, but also uses a library (Boost Asio) that uses
SSL_get_app_data and SSL_set_app_data. When using OpenSSL this works
fine; SSL_get_ex_new_index returns 1, so the main program uses index 1,
the library uses index 0, and there's no conflict. With LibreSSL,
SSL_get_ex_new_index returns 0, so both the main program and the library
use index 0 and conflict with each other. Specifically, Boost Asio is
trying to call a virtual function on the data that my application set,
which fails because my data has the wrong type. The application is
proprietary, but here's a reduced example that exhibits the problem:
#include <boost/asio/ssl.hpp>
#include <boost/asio.hpp>
using namespace boost::asio;
int new_data(void *, void *, CRYPTO_EX_DATA *cd, int idx, long, void *)
{
CRYPTO_set_ex_data(cd, idx, strdup("some custom data"));
return 0;
}
int main()
{
int custom_data_index = SSL_get_ex_new_index(0, nullptr,
new_data, nullptr, nullptr);
io_context ctx;
ssl::context ssl_ctx(ssl::context::tls);
ssl::stream<ip::tcp::socket> ssl_sock(ctx, ssl_ctx);
}
When ssl_sock is destroyed, boost calls SSL_get_app_data, interprets the
non-null return to mean that it had previously set that data, and so
calls delete on it, which tries to read the object's destructor out of
its vtable. Since the data is actually a string set by new_data and not
an object with a vtable, this causes a segfault.
> > diff --git a/src/lib/libcrypto/ex_data.c b/src/lib/libcrypto/ex_data.c
> > index b1e391366..d9c39b2c4 100644
> > --- a/src/lib/libcrypto/ex_data.c
> > +++ b/src/lib/libcrypto/ex_data.c
> > @@ -320,7 +320,7 @@ def_get_class(int class_index)
> > gen = malloc(sizeof(EX_CLASS_ITEM));
> > if (gen) {
> > gen->class_index = class_index;
> > - gen->meth_num = 0;
> > + gen->meth_num = 1;
> > gen->meth = sk_CRYPTO_EX_DATA_FUNCS_new_null();
> > if (!gen->meth)
> > free(gen);
>
