Re: git: bafe0e7edaee - main - pam_ksu: Proactively address MIT KRB5 build failure
- In reply to: John Baldwin : "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 18:54:42 UTC
In message <c13f9326-a1ca-4293-86d4-1c2353e30b03@FreeBSD.org>, John Baldwin
wri
tes:
> 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=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 whi
> le
> >>> MIT and Heimdal are both in the tree. When Heimdal is removed we ca
> n
> >>> 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 segfaultin
> g
> >> 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);
>
> This still has the use-after-free of `temp_realm` I pointed out in the review
> since you are going to use it below.
I'm not sure what happened I recall moving this. I did have an accident
with the repo (a reset --hard in the wrong repo) requiring a recovery from
backup. I may have missed reapplying the change.
>
> >>> + }
> >>> + 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?
I'm sure I'd already moved it here. Apparently not.
>
> >>> + 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 poin
> ter
> >> 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, curr
> ent_user, NULL);
default_principal is a pointer under MIT. In Heimdal it is a structure.
If we assume MIT defines default_principal a structure (like Heimdal) we
get,
/opt/src/git-src/lib/libpam/modules/pam_ksu/pam_ksu.c:277:37: error:
incompatible pointer types passing 'krb5_principal *' (aka 'struct
krb5_principal_data **') to parameter of type 'krb5_principal' (aka 'struct
krb5_principal_data *'); remove & [-Werror,-Wincompatible-pointer-types]
277 | rv = krb5_make_principal(context,
&default_principal, NULL, current_user, NULL);
| ^~~~~~~~~~~~~~~~~~
default_principal in MIT is a pointer to a struct. Not so in Heimdal.
> #else
>
> How are we comparing a structure to NULL?
To reiterate, principal is not a structure in MIT. It is a pointer to a
structure. Passing it as a pointer results in a pointer to a pointer being
passed.
In Heimdal principal is a structure therefore it must be passed as
&principal.
>
> >>
> >> Also, you then pass that NULL pointer to the following code which would al
> so
> >> 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 clear
> ly
> >> 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 ge
> t_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_princip
> al == NULL
> path?
Yes.
slippy$ kdestroy
slippy$ ksu
cy@CWSENT.COM does not have any appropriate tickets in the cache.
Authentication failed.
slippy$
And let's show an expired ticket too:
slippy$ kinit -l 00:01
Password for cy@CWSENT.COM:
slippy$ klist -l
Principal name Cache name
-------------- ----------
cy@CWSENT.COM FILE:/tmp/krb5cc_1000_RqXUEA
slippy$ klist -l
Principal name Cache name
-------------- ----------
cy@CWSENT.COM FILE:/tmp/krb5cc_1000_RqXUEA (Expired)
slippy$ ksu
ksu: Ticket expired while verifying ticket for server
Authentication failed.
slippy$
Remember, the difference between Heimdal and MIT in this case is Heimdal's
principal is the structure while MIT's principal is a pointer to a
structure.
--
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