Page MenuHomeFreeBSD

pam_krb5: Stitch pam-krb5 plumbing into libpam
ClosedPublic

Authored by cy on Jun 5 2025, 5:58 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Oct 15, 6:18 AM
Unknown Object (File)
Tue, Oct 14, 11:56 AM
Unknown Object (File)
Tue, Oct 14, 11:56 AM
Unknown Object (File)
Tue, Oct 14, 11:56 AM
Unknown Object (File)
Tue, Oct 14, 11:56 AM
Unknown Object (File)
Tue, Oct 14, 11:56 AM
Unknown Object (File)
Tue, Oct 14, 11:56 AM
Unknown Object (File)
Mon, Oct 13, 9:52 PM
Subscribers
None

Details

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

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

cy requested review of this revision.Jun 5 2025, 5:58 AM
cy created this revision.
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.

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.

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

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

This revision was not accepted when it landed; it landed in state Needs Review.Jun 16 2025, 2:52 AM
This revision was automatically updated to reflect the committed changes.