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

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