| raw e-mail | index | archive | help
>>> + } >>> + 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?>