Re: git: bafe0e7edaee - main - pam_ksu: Proactively address MIT KRB5 build failure
Date: Mon, 16 Jun 2025 18:18:48 UTC
On 6/16/25 13:34, Cy Schubert wrote:
> 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);
This still has the use-after-free of `temp_realm` I pointed out in the review
since you are going to use it below.
>>> + }
>>> + va_start(ap, realm);
Also, this seems quite sketchy if realm was NULL. You are starting the
va_list with the value of temp_realm which is not an on-stack argument?
>>> + /*
>>> + * 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);
You need to wait to free temp_realm until here?
>>> + 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.
This doesn't make sense. The code assumes it is a pointer:
if (default_principal == NULL) {
#ifdef MK_MITKRB5
/* For MIT KRB5. */
rv = krb5_make_principal(context, default_principal, NULL, current_user, NULL);
#else
How are we comparing a structure to NULL?
>>
>> 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$
Did you test without a valid credential cache to exercise the default_principal == NULL
path?
--
John Baldwin