Date: Mon, 16 Jun 2025 12:31:54 -0400 From: John Baldwin <jhb@FreeBSD.org> To: Cy Schubert <cy@FreeBSD.org>, src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: git: bafe0e7edaee - main - pam_ksu: Proactively address MIT KRB5 build failure Message-ID: <36939c6e-1c9f-4e55-bd42-e759e6a37a2e@FreeBSD.org> In-Reply-To: <202506160251.55G2plI3062868@gitrepo.freebsd.org> References: <202506160251.55G2plI3062868@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 6/15/25 22:51, Cy Schubert wrote: > The branch main has been updated by cy: > > URL: https://cgit.FreeBSD.org/src/commit/?id=bafe0e7edaee75f3fcfe6bf6c3e7b1e816361365 > > commit bafe0e7edaee75f3fcfe6bf6c3e7b1e816361365 > Author: Cy Schubert <cy@FreeBSD.org> > AuthorDate: 2025-06-05 17:09:57 +0000 > Commit: Cy Schubert <cy@FreeBSD.org> > CommitDate: 2025-06-16 02:49:35 +0000 > > pam_ksu: Proactively address MIT KRB5 build failure > > MIT KRB5 does not provide a krb5_make_principal() function. We need to > provide this ourselves for now. We provide the function for now while > MIT and Heimdal are both in the tree. When Heimdal is removed we can > inline the calls to krb5_get_default_realm() and > krb5_build_principal_va(). krb5_build_principal_va() is > deprecated in MIT KRB5. Its replacement, krb5_build_principal_alloc_va() > will be used instead at that time. > > Sponsored by: The FreeBSD Foundation > Differential revision: https://reviews.freebsd.org/D50808 I still don't understand how this can work instead of instantly segfaulting as I said in the review. > diff --git a/lib/libpam/modules/pam_ksu/pam_ksu.c b/lib/libpam/modules/pam_ksu/pam_ksu.c > index 47362c835c12..a6b3f043d3f4 100644 > --- a/lib/libpam/modules/pam_ksu/pam_ksu.c > +++ b/lib/libpam/modules/pam_ksu/pam_ksu.c > @@ -48,6 +48,61 @@ static long get_su_principal(krb5_context, const char *, const char *, > static int auth_krb5(pam_handle_t *, krb5_context, const char *, > krb5_principal); > > +#ifdef MK_MITKRB5 > +/* For MIT KRB5 only. */ > + > +/* > + * XXX This entire module will need to be rewritten when heimdal > + * XXX compatidibility is no longer needed. > + */ > +#define KRB5_DEFAULT_CCFILE_ROOT "/tmp/krb5cc_" > +#define KRB5_DEFAULT_CCROOT "FILE:" KRB5_DEFAULT_CCFILE_ROOT > + > +/* > + * XXX We will replace krb5_build_principal_va() with > + * XXX krb5_build_principal_alloc_va() when Heimdal is finally > + * XXX removed. > + */ > +krb5_error_code KRB5_CALLCONV > +krb5_build_principal_va(krb5_context context, > + krb5_principal princ, > + unsigned int rlen, > + const char *realm, > + va_list ap); > +typedef char *heim_general_string; > +typedef heim_general_string Realm; > +typedef Realm krb5_realm; > +typedef const char *krb5_const_realm; > + > +static krb5_error_code > +krb5_make_principal(krb5_context context, krb5_principal principal, > + krb5_const_realm realm, ...) > +{ > + krb5_error_code rc; > + va_list ap; > + if (realm == NULL) { > + krb5_realm temp_realm = NULL; > + if ((rc = krb5_get_default_realm(context, &temp_realm))) > + return (rc); > + realm=temp_realm; > + if (temp_realm) > + free(temp_realm); > + } > + va_start(ap, realm); > + /* > + * XXX Ideally we should be using krb5_build_principal_alloc_va() > + * XXX here because krb5_build_principal_va() is deprecated. But, > + * XXX this would require changes elsewhere in the calling code > + * XXX to call krb5_free_principal() elsewhere to free the > + * XXX principal. We can do that after Heimdal is removed from > + * XXX our tree. > + */ > + rc = krb5_build_principal_va(context, principal, strlen(realm), realm, ap); > + va_end(ap); > + return (rc); > +} > +#endif > + > PAM_EXTERN int > pam_sm_authenticate(pam_handle_t *pamh, int flags __unused, > int argc __unused, const char *argv[] __unused) > @@ -217,7 +272,13 @@ get_su_principal(krb5_context context, const char *target_user, const char *curr > if (rv != 0) > return (errno); > if (default_principal == NULL) { > +#ifdef MK_MITKRB5 > + /* For MIT KRB5. */ > + rv = krb5_make_principal(context, default_principal, NULL, current_user, NULL); > +#else > + /* For Heimdal. */ > rv = krb5_make_principal(context, &default_principal, NULL, current_user, NULL); > +#endif At this point default_principal is always NULL, so you pass in a NULL pointer to krb5_build_princpal_va. That will surely crash. Also, you then pass that NULL pointer to the following code which would also surely crash: /* Now that we have some principal, if the target account is * `root', then transform it into a `root' instance, e.g. * `user@REA.LM' -> `user/root@REA.LM'. */ rv = krb5_unparse_name(context, default_principal, &principal_name); krb5_free_principal(context, default_principal); This is why I said your comment seems wrong. The Heimdal version is clearly allocating a principal, so the MIT version should also be doing that, and you should already be using krb5_build_prinpcial_alloc_va() _now_. Your comment claims that the calling code isn't using krb5_free_principal(), but the calling code quoted above in get_su_principal() does call krb5_free_principal(). Have you tested this at runtime? -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?36939c6e-1c9f-4e55-bd42-e759e6a37a2e>