From nobody Fri Nov 15 13:00:50 2024 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4XqcZp5knpz5d6gT; Fri, 15 Nov 2024 13:00:50 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R10" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4XqcZp23bhz4pdF; Fri, 15 Nov 2024 13:00:50 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1731675650; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=Uhdvajkd0f2A8DpQv32Amr+gd/NCW7jGfbwTEF9vR04=; b=JFeRFqr1FOJM481lraIBXyuMikU4K+eIgMbHZ1sVg5gKMm31VyXUwM4NPDYvMuD6AZqFVO TZk+qllUI9D6QSEgydRTcd9ea+j66ZBeFTr61hkq/KImEsMfmQO1QatWEU2JJ/xrDVefHy Aiyr6GHTqz4zg3O1VVVeoJy9aKKW1LoSgwZseLxJmngkZvt2hnun3ZVkZjJGTqoqB1/B6I hb2ye+L5GvuKjHKjoSJi81RW+M5jbfb2VEf27cxp+aqf+GWImkxkKuaa4UQozSAXsS1cv8 4DZQHOdgEUngwN3JGOxv9AF9h6JFvZy+6e/pZXQiUe+gPEeMZ30+iAJ2kGA3nA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1731675650; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=Uhdvajkd0f2A8DpQv32Amr+gd/NCW7jGfbwTEF9vR04=; b=AwaMGqoSmxLSdtbaJ0z+1V1zp/o04AYyhCs1WW55BrvukVXhQd1MnN0spJRzSOlY2plue7 66AvaV/E58ibTpf66MiPisV4+L1nti/BlVkW+Zh5cukJHapRZ9DNph6SZzZlVsSBgWDGXQ kjwRMh3gGSeVkEiWyxeic2pUxzuy8psYPYDoAwOlyi60E2Z58xo9JWl6GsbGDmlUyRPmIw rOCfeb4MVgDdzObTFTuHSZHai2TsHdl+v3tyywgm7gtCusSvYwEEkMIdq9jI+rx5Zuc9KN NNno1mFz8vGH59dCndhSUYh6bPQbQxVoubmJHA8n5mWpjMS9qz/btbqo6BLIKQ== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1731675650; a=rsa-sha256; cv=none; b=a8QX/GVy8WA5Fc+H579IFBbf/u9U+M51pJN37UvzzYotSzP8H/5yMrluN7b660oieoPa/0 qC/SDYpUvAOJ6JJhdIRHK+TT5unTJDJPdyISYmu1cNGCeu2T4wzo3qZ/Xz1zx1l0acqubn r200Ls4aV0BZSoy6ZeOXoWQmBO0sl3eQPo1cJ0WP12kU9Z/tT7aTvinVoE9RW/Oboz0Z56 uxTz+JMr08QrO6LgcvWzFyz+SpSS+sh6AQA2k7thBQXrGy6Mq1IGAEwA92+twKxGZ2PIGO paYua3unX3iMtcES5M1yAdLbIr07aPRnqH8OugZ/+UVmEREyfZcz12J6mEIIqg== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4XqcZp1bs2zP5c; Fri, 15 Nov 2024 13:00:50 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.18.1/8.18.1) with ESMTP id 4AFD0onJ071185; Fri, 15 Nov 2024 13:00:50 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 4AFD0oJq071180; Fri, 15 Nov 2024 13:00:50 GMT (envelope-from git) Date: Fri, 15 Nov 2024 13:00:50 GMT Message-Id: <202411151300.4AFD0oJq071180@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Olivier Certner Subject: git: bd3813a8e1cf - stable/13 - cred: crsetgroups(): Improve and factor out groups normalization 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 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: olce X-Git-Repository: src X-Git-Refname: refs/heads/stable/13 X-Git-Reftype: branch X-Git-Commit: bd3813a8e1cf3e1766283f4f9e7fe085222cd32a Auto-Submitted: auto-generated The branch stable/13 has been updated by olce: URL: https://cgit.FreeBSD.org/src/commit/?id=bd3813a8e1cf3e1766283f4f9e7fe085222cd32a commit bd3813a8e1cf3e1766283f4f9e7fe085222cd32a Author: Olivier Certner AuthorDate: 2024-10-02 12:40:27 +0000 Commit: Olivier Certner CommitDate: 2024-11-15 12:59:08 +0000 cred: crsetgroups(): Improve and factor out groups normalization The groups array has been sorted (not including the first element, which is always the effective GID) to enable performing a binary search for determining if some group is part of the supplementary groups set. Factor out this sorting operation into an internal normalization function (groups_normalize()), adding to it the removal of duplicates after the sort. Separating groups normalization code allows to perform it in advance, and in particular before calling MAC hooks which need the groups array to be sorted to perform better. This also enables sorting input arrays ahead of acquiring the process lock (which is not necessary for this operation). kern_setgroups() has been changed accordingly, so MAC modules implementing the mac_cred_check_setgroups() hook now can assume a normalized groups array (and also that it has at least one element, as if kern_setgroups() is passed no groups, the hook is called with an array of one element being the current effective GID, as this is effectively the effect of such a call to kern_setgroups()). Further commits introducing the setcred() system call and associated MAC hooks will also guarantee a normalized groups array to MAC modules implementing these hooks. Rename crsetgroups_locked() into crsetgroups_internal(), as it is no more "locked" than crsetgroups() itself. However, it can be called under any lock (as needed), whereas the second may sleep to allocate memory. Update their herald comments to make that explicit. In passing, using qsort() instead of the old open-coded insertion sort (in crsetgroups_locked()) fixes the performance concern about the latter when using a large number of groups. Also, our qsort() falls back to insertion sort for small arrays and in case the array is likely to be mostly sorted, so this shouldn't cause concerns for the small number of groups common case. While here, add assertions in inner modification routines to check that the passed credentials object has a reference count of exactly 1 (in particular, it must not be shared). Remove a redundant one from some outer routine. Reviewed by: mhorne Approved by: markj (mentor) MFC after: 3 days Differential Revision: https://reviews.freebsd.org/D46914 (cherry picked from commit 6d2efbb34fdb59facbe6d83374ef4ab69d395866) Approved by: markj (mentor) --- sys/kern/kern_prot.c | 168 ++++++++++++++++++++++++++++++++++++++------------ sys/sys/syscallsubr.h | 2 +- sys/sys/ucred.h | 2 +- 3 files changed, 131 insertions(+), 41 deletions(-) diff --git a/sys/kern/kern_prot.c b/sys/kern/kern_prot.c index 901753f1e5b7..6cdfea2774c0 100644 --- a/sys/kern/kern_prot.c +++ b/sys/kern/kern_prot.c @@ -89,8 +89,22 @@ SYSCTL_NODE(_security, OID_AUTO, bsd, CTLFLAG_RW | CTLFLAG_MPSAFE, 0, "BSD security policy"); static void crfree_final(struct ucred *cr); -static void crsetgroups_locked(struct ucred *cr, int ngrp, - gid_t *groups); + +static inline void +groups_check_positive_len(int ngrp) +{ + MPASS2(ngrp >= 0, "negative number of groups"); + MPASS2(ngrp != 0, "at least one group expected (effective GID)"); +} +static inline void +groups_check_max_len(int ngrp) +{ + MPASS2(ngrp <= ngroups_max + 1, "too many groups"); +} + +static void groups_normalize(int *ngrp, gid_t *groups); +static void crsetgroups_internal(struct ucred *cr, int ngrp, + const gid_t *groups); #ifndef _SYS_SYSPROTO_H_ struct getpid_args { @@ -831,9 +845,9 @@ sys_setgroups(struct thread *td, struct setgroups_args *uap) error = copyin(uap->gidset, groups, gidsetsize * sizeof(gid_t)); if (error == 0) - error = kern_setgroups(td, gidsetsize, groups); + error = kern_setgroups(td, &gidsetsize, groups); - if (gidsetsize > CRED_SMALLGROUPS_NB) + if (groups != smallgroups) free(groups, M_TEMP); return (error); } @@ -847,25 +861,38 @@ gidp_cmp(const void *p1, const void *p2) return ((g1 > g2) - (g1 < g2)); } +/* + * CAUTION: This function normalizes 'groups', possibly also changing the value + * of '*ngrpp' as a consequence. + */ int -kern_setgroups(struct thread *td, int ngrp, gid_t *groups) +kern_setgroups(struct thread *td, int *ngrpp, gid_t *groups) { struct proc *p = td->td_proc; struct ucred *newcred, *oldcred; - int error; + int ngrp, error; + ngrp = *ngrpp; /* Sanity check size. */ if (ngrp < 0 || ngrp > ngroups_max + 1) return (EINVAL); AUDIT_ARG_GROUPSET(groups, ngrp); + if (ngrp != 0) { + /* We allow and treat 0 specially below. */ + groups_normalize(ngrpp, groups); + ngrp = *ngrpp; + } newcred = crget(); crextend(newcred, ngrp); PROC_LOCK(p); oldcred = crcopysafe(p, newcred); #ifdef MAC - error = mac_cred_check_setgroups(oldcred, ngrp, groups); + error = ngrp == 0 ? + /* If 'ngrp' is 0, we'll keep just the current effective GID. */ + mac_cred_check_setgroups(oldcred, 1, oldcred->cr_groups) : + mac_cred_check_setgroups(oldcred, ngrp, groups); if (error) goto fail; #endif @@ -882,9 +909,9 @@ kern_setgroups(struct thread *td, int ngrp, gid_t *groups) * when running non-BSD software if we do not do the same. */ newcred->cr_ngroups = 1; - } else { - crsetgroups_locked(newcred, ngrp, groups); - } + } else + crsetgroups_internal(newcred, ngrp, groups); + setsugid(p); proc_set_cred(p, newcred); PROC_UNLOCK(p); @@ -1294,6 +1321,32 @@ sys___setugid(struct thread *td, struct __setugid_args *uap) #endif /* REGRESSION */ } +#ifdef INVARIANTS +static void +groups_check_normalized(int ngrp, const gid_t *groups) +{ + gid_t prev_g; + + groups_check_positive_len(ngrp); + groups_check_max_len(ngrp); + + if (ngrp == 1) + return; + + prev_g = groups[1]; + for (int i = 2; i < ngrp; ++i) { + const gid_t g = groups[i]; + + if (prev_g >= g) + panic("%s: groups[%d] (%u) >= groups[%d] (%u)", + __func__, i - 1, prev_g, i, g); + prev_g = g; + } +} +#else +#define groups_check_normalized(...) +#endif + /* * Returns whether gid designates a supplementary group in cred. */ @@ -2156,7 +2209,6 @@ void crcopy(struct ucred *dest, struct ucred *src) { - KASSERT(dest->cr_ref == 1, ("crcopy of shared ucred")); bcopy(&src->cr_startcopy, &dest->cr_startcopy, (unsigned)((caddr_t)&src->cr_endcopy - (caddr_t)&src->cr_startcopy)); @@ -2301,6 +2353,8 @@ crextend(struct ucred *cr, int n) { int cnt; + MPASS2(cr->cr_ref == 1, "'cr_ref' must be 1 (referenced, unshared)"); + /* Truncate? */ if (n <= cr->cr_agroups) return; @@ -2334,52 +2388,88 @@ crextend(struct ucred *cr, int n) } /* - * Copy groups in to a credential, preserving any necessary invariants. - * Currently this includes the sorting of all supplementary gids. - * crextend() must have been called before hand to ensure sufficient - * space is available. + * Normalizes a set of groups to be applied to a 'struct ucred'. + * + * The set of groups is an array that must comprise the effective GID as its + * first element (so its length cannot be 0). + * + * Normalization ensures that elements after the first, which stand for the + * supplementary groups, are sorted in ascending order and do not contain + * duplicates. */ static void -crsetgroups_locked(struct ucred *cr, int ngrp, gid_t *groups) +groups_normalize(int *ngrp, gid_t *groups) { - int i; - int j; - gid_t g; + gid_t prev_g; + int ins_idx; - KASSERT(cr->cr_agroups >= ngrp, ("cr_ngroups is too small")); + groups_check_positive_len(*ngrp); + groups_check_max_len(*ngrp); - bcopy(groups, cr->cr_groups, ngrp * sizeof(gid_t)); - cr->cr_ngroups = ngrp; + if (*ngrp == 1) + return; - /* - * Sort all groups except cr_groups[0] to allow groupmember to - * perform a binary search. - * - * XXX: If large numbers of groups become common this should - * be replaced with shell sort like linux uses or possibly - * heap sort. - */ - for (i = 2; i < ngrp; i++) { - g = cr->cr_groups[i]; - for (j = i-1; j >= 1 && g < cr->cr_groups[j]; j--) - cr->cr_groups[j + 1] = cr->cr_groups[j]; - cr->cr_groups[j + 1] = g; + qsort(groups + 1, *ngrp - 1, sizeof(*groups), gidp_cmp); + + /* Remove duplicates. */ + prev_g = groups[1]; + ins_idx = 2; + for (int i = 2; i < *ngrp; ++i) { + const gid_t g = groups[i]; + + if (g != prev_g) { + if (i != ins_idx) + groups[ins_idx] = g; + ++ins_idx; + prev_g = g; + } } + *ngrp = ins_idx; + + groups_check_normalized(*ngrp, groups); +} + +/* + * Internal function copying groups into a credential. + * + * 'ngrp' must be strictly positive. Either the passed 'groups' array must have + * been normalized in advance (see groups_normalize()), else it must be so + * before the structure is to be used again. + * + * This function is suitable to be used under any lock (it doesn't take any lock + * itself nor sleep, and in particular doesn't allocate memory). crextend() + * must have been called beforehand to ensure sufficient space is available. + * See also crsetgroups(), which handles that. + */ +static void +crsetgroups_internal(struct ucred *cr, int ngrp, const gid_t *groups) +{ + + MPASS2(cr->cr_ref == 1, "'cr_ref' must be 1 (referenced, unshared)"); + MPASS2(cr->cr_agroups >= ngrp, "'cr_agroups' too small"); + groups_check_positive_len(ngrp); + + bcopy(groups, cr->cr_groups, ngrp * sizeof(gid_t)); + cr->cr_ngroups = ngrp; } /* * Copy groups in to a credential after expanding it if required. - * Truncate the list to (ngroups_max + 1) if it is too large. + * + * May sleep in order to allocate memory (except if, e.g., crextend() was called + * before with 'ngrp' or greater). Truncates the list to (ngroups_max + 1) if + * it is too large. Array 'groups' doesn't need to be sorted. 'ngrp' must be + * strictly positive. */ void -crsetgroups(struct ucred *cr, int ngrp, gid_t *groups) +crsetgroups(struct ucred *cr, int ngrp, const gid_t *groups) { if (ngrp > ngroups_max + 1) ngrp = ngroups_max + 1; - crextend(cr, ngrp); - crsetgroups_locked(cr, ngrp, groups); + crsetgroups_internal(cr, ngrp, groups); + groups_normalize(&cr->cr_ngroups, cr->cr_groups); } /* diff --git a/sys/sys/syscallsubr.h b/sys/sys/syscallsubr.h index 9edd62729c38..78de8876e51b 100644 --- a/sys/sys/syscallsubr.h +++ b/sys/sys/syscallsubr.h @@ -287,7 +287,7 @@ int kern_select(struct thread *td, int nd, fd_set *fd_in, fd_set *fd_ou, fd_set *fd_ex, struct timeval *tvp, int abi_nfdbits); int kern_sendit(struct thread *td, int s, struct msghdr *mp, int flags, struct mbuf *control, enum uio_seg segflg); -int kern_setgroups(struct thread *td, int ngrp, gid_t *groups); +int kern_setgroups(struct thread *td, int *ngrpp, gid_t *groups); int kern_setitimer(struct thread *, u_int, struct itimerval *, struct itimerval *); int kern_setpriority(struct thread *td, int which, int who, int prio); diff --git a/sys/sys/ucred.h b/sys/sys/ucred.h index 243a2431bd0b..8152f962f3cb 100644 --- a/sys/sys/ucred.h +++ b/sys/sys/ucred.h @@ -165,7 +165,7 @@ struct ucred *crcowget(struct ucred *cr); void crcowfree(struct thread *td); void cru2x(struct ucred *cr, struct xucred *xcr); void cru2xt(struct thread *td, struct xucred *xcr); -void crsetgroups(struct ucred *cr, int n, gid_t *groups); +void crsetgroups(struct ucred *cr, int ngrp, const gid_t *groups); /* * Returns whether gid designates a primary group in cred.