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=bafe0e7edaee75f3fcfe6bf6c3e7b1e816361365
commit bafe0e7edaee75f3fcfe6bf6c3e7b1e816361365
Author: Cy Schubert <c...@freebsd.org>
AuthorDate: 2025-06-05 17:09:57 +0000
Commit: Cy Schubert <c...@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 char *,
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);
+ }
+ va_start(ap, realm);
+ /*
+ * 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);
+ 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
*target_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,
current_user, NULL);
+#else
+ /* For Heimdal. */
rv = krb5_make_principal(context, &default_principal, NULL,
current_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.
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.
* `u...@rea.lm' -> `user/r...@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
already be
using krb5_build_prinpcial_alloc_va() _now_. Your comment claims that the
calling code
isn't using krb5_free_principal(), but the calling code quoted above in
get_su_principal()
does call krb5_free_principal().
Have you tested this at runtime?
--
John Baldwin
