From nobody Thu Jan 18 08:16:36 2024 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4TFwZH18DLz57nYc; Thu, 18 Jan 2024 08:16:39 +0000 (UTC) (envelope-from cy.schubert@cschubert.com) Received: from omta002.cacentral1.a.cloudfilter.net (omta002.cacentral1.a.cloudfilter.net [3.97.99.33]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "Client", Issuer "CA" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 4TFwZG5JjYz432d; Thu, 18 Jan 2024 08:16:38 +0000 (UTC) (envelope-from cy.schubert@cschubert.com) Authentication-Results: mx1.freebsd.org; none Received: from shw-obgw-4001a.ext.cloudfilter.net ([10.228.9.142]) by cmsmtp with ESMTPS id Q6oyrrcHoGAIJQNZlrfbAV; Thu, 18 Jan 2024 08:16:37 +0000 Received: from spqr.komquats.com ([70.66.152.170]) by cmsmtp with ESMTPSA id QNZkrxrVIZR3lQNZlr0Nhf; Thu, 18 Jan 2024 08:16:37 +0000 X-Authority-Analysis: v=2.4 cv=Lo2Bd1Rc c=1 sm=1 tr=0 ts=65a8de65 a=y8EK/9tc/U6QY+pUhnbtgQ==:117 a=y8EK/9tc/U6QY+pUhnbtgQ==:17 a=kj9zAlcOel0A:10 a=dEuoMetlWLkA:10 a=6I5d2MoRAAAA:8 a=YxBL1-UpAAAA:8 a=EkcXrb_YAAAA:8 a=hqGvTmuFqozcHbYXOTQA:9 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 a=Ia-lj3WSrqcvXOmTRaiG:22 a=LK5xJRSDVpKd5WXXoEvA:22 Received: from slippy.cwsent.com (slippy [10.1.1.91]) by spqr.komquats.com (Postfix) with ESMTP id 1F10CA2F; Thu, 18 Jan 2024 00:16:36 -0800 (PST) Received: by slippy.cwsent.com (Postfix, from userid 1000) id 15DB9178; Thu, 18 Jan 2024 00:16:36 -0800 (PST) X-Mailer: exmh version 2.9.0 11/07/2018 with nmh-1.8+dev Reply-to: Cy Schubert From: Cy Schubert X-os: FreeBSD X-Sender: cy@cwsent.com X-URL: http://www.cschubert.com/ To: Jessica Clarke cc: Cy Schubert , "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" Subject: Re: git: 476d63e091c2 - main - kerberos: Fix numerous segfaults when using weak crypto In-reply-to: References: <202401180748.40I7mt58008173@gitrepo.freebsd.org> Comments: In-reply-to Jessica Clarke message dated "Thu, 18 Jan 2024 08:03:10 +0000." List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Date: Thu, 18 Jan 2024 00:16:36 -0800 Message-Id: <20240118081636.15DB9178@slippy.cwsent.com> X-CMAE-Envelope: MS4xfHFLbXreRqcHwEzYKmc4fk6Fum6dkbyRfWbBX0eJqySn2Lq7JQChs+GYELpNZz8Ro1z3oBFsjRrX3aTmRYHzH9ak26WqLX7Rq9ZPzkT32illK/pOJfYR 5AHcZG2VfW9iUmeqH02r31o+vzB4y0zKsf0CdeBgJPQxIleCr53WkYTZJwismRzKk/eBDmlPqV6jz6jI7SQ1RJ13MTpb8DK3NiUxnkT/DdmPLMG6G8p2GZ+o n0xy3tocQGPZRPSln0topTxyDSIURTpsSIsKNC3TEn5NTtaV3zTpBzPxPUkl0QkBETiWo+Ny3UCOR3f4OXj8ESOoCRkyJgjmlxoYNMSRcaVtx+a6349wd08/ ftCmkdhx X-Rspamd-Queue-Id: 4TFwZG5JjYz432d X-Spamd-Bar: ---- X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:16509, ipnet:3.96.0.0/15, country:US] In message , Jessica Clarke w rites: > On 18 Jan 2024, at 07:48, Cy Schubert 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 > > AuthorDate: 2023-12-06 15:30:05 +0000 > > Commit: Cy Schubert > > 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 (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 > > #include > > #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 > > #include > > #include > > +#if defined(OPENSSL_VERSION_MAJOR) && (OPENSSL_VERSION_MAJOR >=3D 3) > > +#include > > +#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 > > +#include > > +#include > > +#include > > +#include > > + > > +#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 FreeBSD UNIX: Web: https://FreeBSD.org NTP: Web: https://nwtime.org e^(i*pi)+1=0