org; s=dkim; t=1750097929; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=0jOvyCVrtolyApCQzDyUaZS8Si+UET/uO6lNM42qbfM=; b=ZKdPWCsLw/od1jYY9gPC7vRzq3NfUjOBgC5/gY101ag993dMQ/MI3K4hPWR1J0DVkl98Ao /5jR7APVUTw0+EeY4R+PzRcz1tqaV3Z9tv7QNLtjwmwSYNRIqaQ3/MBXMn0/AOO0Qsn+JV arNCyVEU7d37bgSe32Z59Xvy4DlLNnon42Qh22EEykZYp1fav58DI5IJSRcqFSrLhdvMcD g4aQ4eTtO2EDeyCHTcbxcrjG+H8FQu+/A9moZnPs3k3JGw9jjqqGAFPBi7LV9zfw8ZK8kv bCdNrJqiHjx/wBi3/eLaKtjXHE/4LE1Wy7Uf2De1nvmBYPsFlLMad3kofVdO6Q== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1750097929; a=rsa-sha256; cv=none; b=BjmgU+8abaYTxW8f257UB1YJuMpDMR5vUzEEqmOIDRXN7DRPyS2+sQasrzc61n3piGllks pK0occoE13BjwOD+pQbn3fNcAHVQbPYCo815l8J7GKjro7H/gHmQOiFsMQJBcD9CwhDHMZ 1aX1FWNU7ndMBOyoOcXpNh07IQtY2DOcKzqiBhzeUEKNTfyzvR65zvz5hgwA9FXeRSFZNC eizdtNRkB+bdjhL3nzR74AM4O77aVsvNg5712UurpywyDFQef4b+BQOC0CaRtSUj4KGeZy peXOXkTJGg4LjWh3GxG6ubCSbN4JPCA4HUPXW6u2sqm9njBis/0t08qQlH99YQ== Received: from [IPV6:2601:5c0:4200:b830:1556:8d92:a58f:7b21] (unknown [IPv6:2601:5c0:4200:b830:1556:8d92:a58f:7b21]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 4bLdYP3dpDzppM; Mon, 16 Jun 2025 18:18:49 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Message-ID: Date: Mon, 16 Jun 2025 14:18:48 -0400 List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: git: bafe0e7edaee - main - pam_ksu: Proactively address MIT KRB5 build failure Content-Language: en-US To: Cy Schubert Cc: Cy Schubert , src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org References: <202506160251.55G2plI3062868@gitrepo.freebsd.org> <36939c6e-1c9f-4e55-bd42-e759e6a37a2e@FreeBSD.org> <20250616173436.68EA7AE@slippy.cwsent.com> From: John Baldwin In-Reply-To: <20250616173436.68EA7AE@slippy.cwsent.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 6/16/25 13:34, Cy Schubert wrote: > In message <36939c6e-1c9f-4e55-bd42-e759e6a37a2e@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 >>> AuthorDate: 2025-06-05 17:09:57 +0000 >>> Commit: Cy Schubert >>> 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. >> * `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