Re: git: bafe0e7edaee - main - pam_ksu: Proactively address MIT KRB5 build failure
- In reply to: Cy Schubert : "Re: git: bafe0e7edaee - main - pam_ksu: Proactively address MIT KRB5 build failure"
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Mon, 16 Jun 2025 17:38:00 UTC
In message <20250616173436.68EA7AE@slippy.cwsent.com>, Cy Schubert writes:
> In message <36939c6e-1c9f-4e55-bd42-e759e6a37a2e@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=bafe0e7edaee75f3fcfe6bf6c3e7
> b1
> > e816361365
> > >
> > > 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 whil
> e
> > > 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/pa
> m_
> > 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 *t
> ar
> > 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, N
> ULL, cur
> > rent_user, NULL);
> > > +#endif
> >
> > At this point default_principal is always NULL, so you pass in a NULL point
> er
> > 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.
I should have said,
build_principal() references the pointer to the struct because the
principal is a struct while in MIT the principal is a pointer to
principal_data, the actual struct.
--
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
>
> >
> > Also, you then pass that NULL pointer to the following code which would als
> o
> > 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 clearl
> y
> > allocating
> > a principal, so the MIT version should also be doing that, and you should a
> lr
> > 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 cy@CWSENT.COM
> Account root: authorization for cy@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.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
>