pam_krb5: Stitch pam-krb5 plumbing into libpam

[cy]

Summary

The eyeire.org pam-krb5 supports MIT KRB5 and Heimdal. FreeBSD will use
it to implement pam_krb5 for MIT KRB5. The existing libpam pam_krb5
only supports Heimdal and therefore cannot be used with the MIT KRB5
import.

Sponsored by: The FreeBSD Foundation

Test Plan

Already running here

[jhb]

lib/libpam/modules/pam_krb5/pam_krb5.c

63

Why is this diff here if you aren't using this file with MIT?

lib/libpam/modules/pam_ksu/pam_ksu.c

85 ↗

(On Diff #156568)

I don't understand how this can work though as you are freeing the realm right away? Doesn't that mean you get a use after free below when you call krb5_build_principal_va? That is, should this be more like:

krb5_error_code rc;
va_list ap;
krb5_realm tem_realm = NULL;

if (realm == NULL) {
    rc = krb5_get_default_realm(context, &temp_realm);
    if (rc != 0)
        return (rc);
}

va_start(ap, realm);
rc = krb5_build_principal_va(...);
va_end(ap);
free(temp_realm);
return (rc);

91 ↗

(On Diff #156568)

I don't understand this comment at all. The calling code does indeed call krb5_free_principal, and this function is only called with principal == NULL, so this seems like it would always fault immediately? Have you tested this case? It seems to me that this should be taking a pointer to principal just as in the KTH case, and then this code should indeed be calling the alloc version, and this whole comment should go away.

288 ↗

(On Diff #156568)

Call to krb5_free_principal that the comment seems to claim doesn't exist.

[cy]

I will regen this patch to exclude pam_ksu. This review and commit should only touch pam_krb5. A new review will be submitted for pam_ksu.

lib/libpam/modules/pam_ksu/pam_ksu.c

85 ↗

(On Diff #156568)

I don't understand how this can work though as you are freeing the realm right away? Doesn't that mean you get a use after free below when you call krb5_build_principal_va? That is, should this be more like:

krb5_error_code rc;
va_list ap;
krb5_realm tem_realm = NULL;

if (realm == NULL) {
    rc = krb5_get_default_realm(context, &temp_realm);
    if (rc != 0)
        return (rc);
}

va_start(ap, realm);
rc = krb5_build_principal_va(...);
va_end(ap);
free(temp_realm);
return (rc);

You are correct. I can't say what I was thinking at the time (a year ago). Embarrassing.

85 ↗

(On Diff #156568)

I don't understand how this can work though as you are freeing the realm right away? Doesn't that mean you get a use after free below when you call krb5_build_principal_va? That is, should this be more like:

krb5_error_code rc;
va_list ap;
krb5_realm tem_realm = NULL;

if (realm == NULL) {
    rc = krb5_get_default_realm(context, &temp_realm);
    if (rc != 0)
        return (rc);
}

va_start(ap, realm);
rc = krb5_build_principal_va(...);
va_end(ap);
free(temp_realm);
return (rc);

91 ↗

(On Diff #156568)

The comment is correct. We are using krb5_build_principal_va() but it is deprecated in MIT. See https://web.mit.edu/kerberos/krb5-devel/doc/appdev/refs/api/krb5_build_principal_va.html, This is a mental note to me to use krb5_build_principal_alloc_va() instead. We have some time to replace the call. Just a reminder.

[cy]

My mistake for including pam_ksu in this review/commit. My fault.

[cy]

-U99999 version.

[cy]

Updated patch based on a comment left by markj@ in another review.