Re: patch: make CRYPTO_get_ex_new_index not return 0
On Wed, May 24, 2023 at 05:06:04PM -0400, Marc Aldorasi wrote: > On Wed, May 24, 2023 at 2:16 PM Theo Buehler 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=142421750627504=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: Thanks for the explanation and the test case. Committed as-is, sorry for the wait. We should rework this code, but for now your diff is the way to go.
Re: patch: make CRYPTO_get_ex_new_index not return 0
On Wed, May 24, 2023 at 2:16 PM Theo Buehler 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=142421750627504=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
#include
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 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);
>
Re: patch: make CRYPTO_get_ex_new_index not return 0
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=142421750627504=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.
> 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);
patch: make CRYPTO_get_ex_new_index not return 0
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=142421750627504=2
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);
