Re: git: 476d63e091c2 - main - kerberos: Fix numerous segfaults when using weak crypto
Date: Thu, 18 Jan 2024 08:16:36 UTC
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