Re: git: bafe0e7edaee - main - pam_ksu: Proactively address MIT KRB5 build failure

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=bafe0e7edaee75f3fcfe6bf6c3e7b1
> 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 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.
> 	 * `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 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 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