Date: Thu, 18 Jan 2024 00:16:36 -0800 From: Cy Schubert <Cy.Schubert@cschubert.com> To: Jessica Clarke <jrtc27@freebsd.org> Cc: Cy Schubert <cy@FreeBSD.org>, "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: Re: git: 476d63e091c2 - main - kerberos: Fix numerous segfaults when using weak crypto Message-ID: <20240118081636.15DB9178@slippy.cwsent.com> In-Reply-To: <EAC6D4E3-09FD-4ED0-B15D-6155431628E3@freebsd.org> References: <202401180748.40I7mt58008173@gitrepo.freebsd.org> <EAC6D4E3-09FD-4ED0-B15D-6155431628E3@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
In message <EAC6D4E3-09FD-4ED0-B15D-6155431628E3@freebsd.org>, Jessica Clarke w rites: > On 18 Jan 2024, at 07:48, Cy Schubert <cy@FreeBSD.org> wrote: > >=20 > > The branch main has been updated by cy: > >=20 > > URL: = > https://cgit.FreeBSD.org/src/commit/?id=3D476d63e091c2e663b51d18acf6acb282= > e1f22bbc > >=20 > > commit 476d63e091c2e663b51d18acf6acb282e1f22bbc > > Author: Cy Schubert <cy@FreeBSD.org> > > AuthorDate: 2023-12-06 15:30:05 +0000 > > Commit: Cy Schubert <cy@FreeBSD.org> > > CommitDate: 2024-01-18 07:46:57 +0000 > >=20 > > kerberos: Fix numerous segfaults when using weak crypto > >=20 > > Weak crypto is provided by the openssl legacy provider which is > > not load by default. Load the legacy providers as needed. > >=20 > > When the legacy provider is loaded into the default context the = > default > > provider will no longer be automatically loaded. Without the = > default > > provider the various kerberos applicaions and functions will = > abort(). > >=20 > > This is the second attempt at this patch. Instead of linking > > secure/lib/libcrypto at build time we now link it at runtime, = > avoiding > > buildworld failures under Linux and MacOS. This is because > > TARGET_ENDIANNESS is undefined at pre-build time. > >=20 > > PR: 272835 > > MFC after: 3 days > > X-MFC: only to stable/14 > > Tested by: netchild > > Joerg Pulz <Joerg.Pulz@frm2.tum.de> (previous = > version) > > --- > > crypto/heimdal/lib/kadm5/create_s.c | 4 ++ > > crypto/heimdal/lib/kadm5/kadm5_locl.h | 1 + > > crypto/heimdal/lib/krb5/context.c | 4 ++ > > crypto/heimdal/lib/krb5/crypto.c | 3 + > > crypto/heimdal/lib/krb5/salt.c | 5 ++ > > crypto/heimdal/lib/roken/version-script.map | 1 + > > kerberos5/include/crypto-headers.h | 4 ++ > > kerberos5/include/fbsd_ossl_provider.h | 4 ++ > > kerberos5/lib/libroken/Makefile | 8 ++- > > kerberos5/lib/libroken/fbsd_ossl_provider_load.c | 77 = > ++++++++++++++++++++++++ > > 10 files changed, 109 insertions(+), 2 deletions(-) > >=20 > > diff --git a/crypto/heimdal/lib/kadm5/create_s.c = > b/crypto/heimdal/lib/kadm5/create_s.c > > index 1033ca103239..267e9bbda2a0 100644 > > --- a/crypto/heimdal/lib/kadm5/create_s.c > > +++ b/crypto/heimdal/lib/kadm5/create_s.c > > @@ -169,6 +169,10 @@ kadm5_s_create_principal(void *server_handle, > > ent.entry.keys.len =3D 0; > > ent.entry.keys.val =3D NULL; > >=20 > > + ret =3D fbsd_ossl_provider_load(); > > + if (ret) > > + goto out; > > + > > ret =3D _kadm5_set_keys(context, &ent.entry, password); > > if (ret) > > goto out; > > diff --git a/crypto/heimdal/lib/kadm5/kadm5_locl.h = > b/crypto/heimdal/lib/kadm5/kadm5_locl.h > > index 68b6a5ebf024..63b367ab7e21 100644 > > --- a/crypto/heimdal/lib/kadm5/kadm5_locl.h > > +++ b/crypto/heimdal/lib/kadm5/kadm5_locl.h > > @@ -79,5 +79,6 @@ > > #include <der.h> > > #include <parse_units.h> > > #include "private.h" > > +#include "fbsd_ossl_provider.h" > >=20 > > #endif /* __KADM5_LOCL_H__ */ > > diff --git a/crypto/heimdal/lib/krb5/context.c = > b/crypto/heimdal/lib/krb5/context.c > > index 86bfe539b974..681bc9a0982f 100644 > > --- a/crypto/heimdal/lib/krb5/context.c > > +++ b/crypto/heimdal/lib/krb5/context.c > > @@ -392,6 +392,10 @@ krb5_init_context(krb5_context *context) > > } > > HEIMDAL_MUTEX_init(p->mutex); > >=20 > > + ret =3D fbsd_ossl_provider_load(); > > + if(ret) > > + goto out; > > + > > p->flags |=3D KRB5_CTX_F_HOMEDIR_ACCESS; > >=20 > > ret =3D krb5_get_default_config_files(&files); > > diff --git a/crypto/heimdal/lib/krb5/crypto.c = > b/crypto/heimdal/lib/krb5/crypto.c > > index 67ecef62e875..6ee22609a4d5 100644 > > --- a/crypto/heimdal/lib/krb5/crypto.c > > +++ b/crypto/heimdal/lib/krb5/crypto.c > > @@ -2054,6 +2054,9 @@ krb5_crypto_init(krb5_context context, > > *crypto =3D NULL; > > return ret; > > } > > + ret =3D fbsd_ossl_provider_load(); > > + if (ret) > > + return ret; > > (*crypto)->key.schedule =3D NULL; > > (*crypto)->num_key_usage =3D 0; > > (*crypto)->key_usage =3D NULL; > > diff --git a/crypto/heimdal/lib/krb5/salt.c = > b/crypto/heimdal/lib/krb5/salt.c > > index 5e4c8a1c8572..2b1fbee80ab6 100644 > > --- a/crypto/heimdal/lib/krb5/salt.c > > +++ b/crypto/heimdal/lib/krb5/salt.c > > @@ -43,6 +43,8 @@ krb5_salttype_to_string (krb5_context context, > > struct _krb5_encryption_type *e; > > struct salt_type *st; > >=20 > > + (void) fbsd_ossl_provider_load(); > > + > > e =3D _krb5_find_enctype (etype); > > if (e =3D=3D NULL) { > > krb5_set_error_message(context, KRB5_PROG_ETYPE_NOSUPP, > > @@ -75,6 +77,8 @@ krb5_string_to_salttype (krb5_context context, > > struct _krb5_encryption_type *e; > > struct salt_type *st; > >=20 > > + (void) fbsd_ossl_provider_load(); > > + > > e =3D _krb5_find_enctype (etype); > > if (e =3D=3D NULL) { > > krb5_set_error_message(context, KRB5_PROG_ETYPE_NOSUPP, > > @@ -196,6 +200,7 @@ krb5_string_to_key_data_salt_opaque (krb5_context = > context, > > enctype); > > return KRB5_PROG_ETYPE_NOSUPP; > > } > > + (void) fbsd_ossl_provider_load(); > > for(st =3D et->keytype->string_to_key; st && st->type; st++) > > if(st->type =3D=3D salt.salttype) > > return (*st->string_to_key)(context, enctype, password, > > diff --git a/crypto/heimdal/lib/roken/version-script.map = > b/crypto/heimdal/lib/roken/version-script.map > > index 72d2ea7e4f7c..bb2139ed74cc 100644 > > --- a/crypto/heimdal/lib/roken/version-script.map > > +++ b/crypto/heimdal/lib/roken/version-script.map > > @@ -13,6 +13,7 @@ HEIMDAL_ROKEN_1.0 { > > ct_memcmp; > > err; > > errx; > > + fbsd_ossl_provider_load; > > free_getarg_strings; > > get_default_username; > > get_window_size; > > diff --git a/kerberos5/include/crypto-headers.h = > b/kerberos5/include/crypto-headers.h > > index 3ae0d9624ffd..2cc870642964 100644 > > --- a/kerberos5/include/crypto-headers.h > > +++ b/kerberos5/include/crypto-headers.h > > @@ -17,5 +17,9 @@ > > #include <openssl/ec.h> > > #include <openssl/ecdsa.h> > > #include <openssl/ecdh.h> > > +#if defined(OPENSSL_VERSION_MAJOR) && (OPENSSL_VERSION_MAJOR >=3D 3) > > +#include <openssl/provider.h> > > +#include "fbsd_ossl_provider.h" > > +#endif > >=20 > > #endif /* __crypto_headers_h__ */ > > diff --git a/kerberos5/include/fbsd_ossl_provider.h = > b/kerberos5/include/fbsd_ossl_provider.h > > new file mode 100644 > > index 000000000000..013983ca9f83 > > --- /dev/null > > +++ b/kerberos5/include/fbsd_ossl_provider.h > > @@ -0,0 +1,4 @@ > > +#ifndef __fbsd_ossl_provider_h > > +#define __fbsd_ossl_provider_h > > +int fbsd_ossl_provider_load(void); > > +#endif > > diff --git a/kerberos5/lib/libroken/Makefile = > b/kerberos5/lib/libroken/Makefile > > index 0c46ba6c4cb5..ca6d090e64f0 100644 > > --- a/kerberos5/lib/libroken/Makefile > > +++ b/kerberos5/lib/libroken/Makefile > > @@ -74,9 +74,13 @@ SRCS=3D base64.c \ > > vis.c \ > > warnerr.c \ > > write_pid.c \ > > - xfree.c > > + xfree.c \ > > + fbsd_ossl_provider_load.c > >=20 > > -CFLAGS+=3D-I${KRB5DIR}/lib/roken -I. > > +CFLAGS+=3D-I${KRB5DIR}/lib/roken \ > > + -I${SRCTOP}/kerberos5/include \ > > + -I${KRB5DIR}/lib/krb5 \ > > + -I${SRCTOP}/crypto/openssl/include -I. > >=20 > > CLEANFILES=3D roken.h > >=20 > > diff --git a/kerberos5/lib/libroken/fbsd_ossl_provider_load.c = > b/kerberos5/lib/libroken/fbsd_ossl_provider_load.c > > new file mode 100644 > > index 000000000000..497b32124f96 > > --- /dev/null > > +++ b/kerberos5/lib/libroken/fbsd_ossl_provider_load.c > > @@ -0,0 +1,77 @@ > > +#include <dlfcn.h> > > +#include <errno.h> > > +#include <stdio.h> > > +#include <stdlib.h> > > +#include <openssl/provider.h> > > + > > +#if defined(OPENSSL_VERSION_MAJOR) && (OPENSSL_VERSION_MAJOR >=3D 3) > > +static void fbsd_ossl_provider_unload(void); > > +static void print_dlerror(char *); > > +static OSSL_PROVIDER *legacy; > > +static OSSL_PROVIDER *deflt; > > +static int providers_loaded =3D 0; > > +static OSSL_PROVIDER * (*ossl_provider_load)(OSSL_LIB_CTX *, const = > char*) =3D NULL; > > +static int (*ossl_provider_unload)(OSSL_PROVIDER *) =3D NULL; > > +static void *crypto_lib_handle =3D NULL; > > + > > +static void > > +fbsd_ossl_provider_unload(void) > > +{ > > + if (ossl_provider_unload =3D=3D NULL) { > > + if (!(ossl_provider_unload =3D (int (*)(OSSL_PROVIDER*)) = > dlsym(crypto_lib_handle, "OSSL_PROVIDER_unload"))) { > > + print_dlerror("Unable to link OSSL_PROVIDER_unload"); > > + return; > > + } > > + } > > + if (providers_loaded =3D=3D 1) { > > + (*ossl_provider_unload)(legacy); > > + (*ossl_provider_unload)(deflt); > > + providers_loaded =3D 0; > > + } > > +} > > + > > +static void > > +print_dlerror(char *message) > > +{ > > + char *errstr; > > + > > + if ((errstr =3D dlerror()) !=3D NULL) > > + fprintf(stderr, "%s: %s\n", > > + message, errstr); > > +} > > +#endif > > + > > +int > > +fbsd_ossl_provider_load(void) > > +{ > > +#if defined(OPENSSL_VERSION_MAJOR) && (OPENSSL_VERSION_MAJOR >=3D 3) > > + if (crypto_lib_handle =3D=3D NULL) { > > + if (!(crypto_lib_handle =3D dlopen("/usr/lib/libcrypto.so", > > Doesn=E2=80=99t this need to account for libcompat? And probably best to = > use > the full soname here so old binaries don=E2=80=99t use newer = > incompatible > libraries (or new use old)? At which point can you not just drop the > path and use the file name on its own? That is, I would expect this to > be just "libcrypto.so.30". Probably yes. As long as we remember to update this when the library version number changes. > > Also you can direct link just fine so long as bootstrapping turns it > off, so you can simplify this unless there are other motivating reasons > for using dlopen. That would require some kind of bootstrap flag to be applied to libroken Makefile not to link libcrypto when bootstrapping. That would certainly be a messier proposition. > > Jess > I will add the version number for the reason you have pointed out. -- Cheers, Cy Schubert <Cy.Schubert@cschubert.com> FreeBSD UNIX: <cy@FreeBSD.org> Web: https://FreeBSD.org NTP: <cy@nwtime.org> Web: https://nwtime.org e^(i*pi)+1=0
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20240118081636.15DB9178>