On 6/16/25 13:34, Cy Schubert wrote:
In message <36939c6e-1c9f-4e55-bd42-e759e6a37...@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=bafe0e7edaee75f3fcfe6bf6c3e7b1
e816361365
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 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.
+ }
+ 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.
* `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 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 c...@cwsent.com
Account root: authorization for c...@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
