In message <36939c6e-1c9f-4e55-bd42-e759e6a37...@freebsd.org>, John Baldwin
wri
tes:
> 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=bafe0e7edaee75f3fcfe6bf6c3e7b1
> e816361365
> >
> > commit bafe0e7edaee75f3fcfe6bf6c3e7b1e816361365
> > Author: Cy Schubert <c...@freebsd.org>
> > AuthorDate: 2025-06-05 17:09:57 +0000
> > Commit: Cy Schubert <c...@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 ch
> ar *, 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 *tar
> get_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, curr
> ent_user, NULL);
> > +#else
> > + /* For Heimdal. */
> > rv = krb5_make_principal(context, &default_principal, NULL, cur
> rent_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.
In Heimdal the principal argument is defined as
krb5_principal *principal
Whereas in MIT the principal argument is
krb5_principal princ
In Heimdal krb5_principal is a structure. In MIT it is a pointer to the
same structure.
build_principal() is defined as a struct in Heimdal and a pointer to a
struct in MIT. Therefore the function prototypes are different.
>
> 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.
> * `u...@rea.lm' -> `user/r...@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 alr
> eady be
> using krb5_build_prinpcial_alloc_va() _now_. Your comment claims that the ca
> lling code
> isn't using krb5_free_principal(), but the calling code quoted above in get_s
> u_principal()
> does call krb5_free_principal().
>
> Have you tested this at runtime?
Yes. It is running here.
slippy$ which ksu
/usr/bin/ksu
slippy$ /usr/bin/ksu
Authenticated c...@cwsent.com
Account root: authorization for c...@cwsent.com successful
Changing uid to root (0)
slippy$ id
uid=0(root) gid=0(wheel) groups=0(wheel),5(operator),920(vboxusers)
slippy$
--
Cheers,
Cy Schubert <cy.schub...@cschubert.com>
FreeBSD UNIX: <c...@freebsd.org> Web: https://FreeBSD.org
NTP: <c...@nwtime.org> Web: https://nwtime.org
e**(i*pi)+1=0
