Re: cfbe7a62dc62 - main - nfs, rpc: Ensure kernel credentials have at least one group

Hi Olivier,

This appears to break amd64.MINIMAL and amd64.MINIMALUP:

================================================================
% less _.amd64.MINIMAL
linking kernel.full
ld: error: undefined symbol: vnet_entry_nfsrv_defaultgid
>>> referenced by vfs_export.c:141 (sys/kern/vfs_export.c:141)
>>>               vfs_export.o:(vfs_export)
>>> referenced by vfs_export.c:220 (sys/kern/vfs_export.c:220)
>>>               vfs_export.o:(vfs_export)
--- kernel.full ---
*** [kernel.full] Error code 1

make[5]: stopped making "all" in amd64.amd64/sys/MINIMAL
================================================================

Thanks,

Ravi (rpokala@)

-----Original Message-----
From: <owner-src-committers@freebsd.org <mailto:owner-src-committers@freebsd.org>> on behalf of Olivier Certner <olce@FreeBSD.org <mailto:olce@FreeBSD.org>>
Date: Saturday, November 2, 2024 at 13:39
To: <src-committers@FreeBSD.org <mailto:src-committers@FreeBSD.org>>, <dev-commits-src-all@FreeBSD.org <mailto:dev-commits-src-all@FreeBSD.org>>, <dev-commits-src-main@FreeBSD.org <mailto:dev-commits-src-main@FreeBSD.org>>
Subject: git: cfbe7a62dc62 - main - nfs, rpc: Ensure kernel credentials have at least one group


The branch main has been updated by olce:


URL: https://cgit.FreeBSD.org/src/commit/?id=cfbe7a62dc62e8a5d7520cb5eb8ad7c4a9418e26 <https://cgit.FreeBSD.org/src/commit/?id=cfbe7a62dc62e8a5d7520cb5eb8ad7c4a9418e26>


commit cfbe7a62dc62e8a5d7520cb5eb8ad7c4a9418e26
Author: Olivier Certner <olce@FreeBSD.org <mailto:olce@FreeBSD.org>>
AuthorDate: 2024-10-02 14:28:59 +0000
Commit: Olivier Certner <olce@FreeBSD.org <mailto:olce@FreeBSD.org>>
CommitDate: 2024-11-02 20:37:42 +0000


nfs, rpc: Ensure kernel credentials have at least one group


This fixes several bugs where some 'struct ucred' in the kernel,
constructed from user input (via nmount(2)) or obtained from other
servers (e.g., gssd(8)), could have an unfilled 'cr_groups' field and
whose 'cr_groups[0]' (or 'cr_gid', which is an alias) was later
accessed, causing an uninitialized access giving random access rights.


Use crsetgroups_fallback() to enforce a fallback group when possible.
For NFS, the chosen fallback group is that of the NFS server in the
current VNET (NFSD_VNET(nfsrv_defaultgid)).


There does not seem to be any sensible fallback available in rpc code
(sys/rpc/svc_auth.c, svc_getcred()) on AUTH_UNIX (TLS or not), so just
fail credential retrieval there. Stock NSS sources, rpc.tlsservd(8) or
rpc.tlsclntd(8) provide non-empty group lists, so will not be impacted.


Discussed with: rmacklem (by mail)
Approved by: markj (mentor)
MFC after: 3 days
Differential Revision: https://reviews.freebsd.org/D46918 <https://reviews.freebsd.org/D46918>
---
sys/fs/nfs/nfs_commonport.c | 4 +++-
sys/fs/nfs/nfs_commonsubs.c | 5 +++--
sys/fs/nfsserver/nfs_nfsdport.c | 6 +++++-
sys/fs/nfsserver/nfs_nfsdsocket.c | 6 ++----
sys/kern/vfs_export.c | 12 ++++++++----
sys/rpc/rpcsec_gss/svc_rpcsec_gss.c | 2 +-
sys/rpc/svc_auth.c | 8 ++++++--
7 files changed, 28 insertions(+), 15 deletions(-)


diff --git a/sys/fs/nfs/nfs_commonport.c b/sys/fs/nfs/nfs_commonport.c
index 2db9af5b9ea9..11f31d1a0e9f 100644
--- a/sys/fs/nfs/nfs_commonport.c
+++ b/sys/fs/nfs/nfs_commonport.c
@@ -75,6 +75,7 @@ NFSD_VNET_DEFINE(struct nfsstatsv1 *, nfsstatsv1_p);


NFSD_VNET_DECLARE(struct nfssockreq, nfsrv_nfsuserdsock);
NFSD_VNET_DECLARE(nfsuserd_state, nfsrv_nfsuserd);
+NFSD_VNET_DECLARE(gid_t, nfsrv_defaultgid);


int nfs_pnfsio(task_fn_t *, void *);


@@ -258,7 +259,8 @@ newnfs_copycred(struct nfscred *nfscr, struct ucred *cr)
KASSERT(nfscr->nfsc_ngroups >= 0,
("newnfs_copycred: negative nfsc_ngroups"));
cr->cr_uid = nfscr->nfsc_uid;
- crsetgroups(cr, nfscr->nfsc_ngroups, nfscr->nfsc_groups);
+ crsetgroups_fallback(cr, nfscr->nfsc_ngroups, nfscr->nfsc_groups,
+ NFSD_VNET(nfsrv_defaultgid));
}


/*
diff --git a/sys/fs/nfs/nfs_commonsubs.c b/sys/fs/nfs/nfs_commonsubs.c
index 90b30f462106..ce4b0052714e 100644
--- a/sys/fs/nfs/nfs_commonsubs.c
+++ b/sys/fs/nfs/nfs_commonsubs.c
@@ -4051,8 +4051,9 @@ nfssvc_idname(struct nfsd_idargs *nidp)
*/
cr = crget();
cr->cr_uid = cr->cr_ruid = cr->cr_svuid = nidp->nid_uid;
- crsetgroups(cr, nidp->nid_ngroup, grps);
- cr->cr_rgid = cr->cr_svgid = cr->cr_groups[0];
+ crsetgroups_fallback(cr, nidp->nid_ngroup, grps,
+ NFSD_VNET(nfsrv_defaultgid));
+ cr->cr_rgid = cr->cr_svgid = cr->cr_gid;
cr->cr_prison = curthread->td_ucred->cr_prison;
prison_hold(cr->cr_prison);
#ifdef MAC
diff --git a/sys/fs/nfsserver/nfs_nfsdport.c b/sys/fs/nfsserver/nfs_nfsdport.c
index 8a2a39052506..5160645ad73c 100644
--- a/sys/fs/nfsserver/nfs_nfsdport.c
+++ b/sys/fs/nfsserver/nfs_nfsdport.c
@@ -3311,7 +3311,11 @@ nfsd_excred(struct nfsrv_descript *nd, struct nfsexstuff *exp,
NFSVNO_EXPORTANON(exp) ||
(nd->nd_flag & ND_AUTHNONE) != 0) {
nd->nd_cred->cr_uid = credanon->cr_uid;
- nd->nd_cred->cr_gid = credanon->cr_gid;
+ /*
+ * 'credanon' is already a 'struct ucred' that was built
+ * internally with calls to crsetgroups_fallback(), so
+ * we don't need a fallback here.
+ */
crsetgroups(nd->nd_cred, credanon->cr_ngroups,
credanon->cr_groups);
} else if ((nd->nd_flag & ND_GSS) == 0) {
diff --git a/sys/fs/nfsserver/nfs_nfsdsocket.c b/sys/fs/nfsserver/nfs_nfsdsocket.c
index df0c0edd1b59..d1b6198ba0e1 100644
--- a/sys/fs/nfsserver/nfs_nfsdsocket.c
+++ b/sys/fs/nfsserver/nfs_nfsdsocket.c
@@ -1422,13 +1422,11 @@ static struct ucred *
nfsrv_createrootcred(void)
{
struct ucred *cr;
- gid_t grp;


cr = crget();
cr->cr_uid = cr->cr_ruid = cr->cr_svuid = UID_ROOT;
- grp = GID_WHEEL;
- crsetgroups(cr, 1, &grp);
- cr->cr_rgid = cr->cr_svgid = cr->cr_groups[0];
+ crsetgroups_fallback(cr, 0, NULL, GID_WHEEL);
+ cr->cr_rgid = cr->cr_svgid = cr->cr_gid;
cr->cr_prison = curthread->td_ucred->cr_prison;
prison_hold(cr->cr_prison);
#ifdef MAC
diff --git a/sys/kern/vfs_export.c b/sys/kern/vfs_export.c
index 996f3f74193f..c0337b1fe858 100644
--- a/sys/kern/vfs_export.c
+++ b/sys/kern/vfs_export.c
@@ -61,6 +61,10 @@
#include <rpc/types.h>
#include <rpc/auth.h>


+#include <fs/nfs/nfsport.h>
+
+NFSD_VNET_DECLARE(gid_t, nfsrv_defaultgid);
+
static MALLOC_DEFINE(M_NETADDR, "export_host", "Export host address structure");


#if defined(INET) || defined(INET6)
@@ -133,8 +137,8 @@ vfs_hang_addrlist(struct mount *mp, struct netexport *nep,
np->netc_exflags = argp->ex_flags;
np->netc_anon = crget();
np->netc_anon->cr_uid = argp->ex_uid;
- crsetgroups(np->netc_anon, argp->ex_ngroups,
- argp->ex_groups);
+ crsetgroups_fallback(np->netc_anon, argp->ex_ngroups,
+ argp->ex_groups, NFSD_VNET(nfsrv_defaultgid));
np->netc_anon->cr_prison = &prison0;
prison_hold(np->netc_anon->cr_prison);
np->netc_numsecflavors = argp->ex_numsecflavors;
@@ -212,8 +216,8 @@ vfs_hang_addrlist(struct mount *mp, struct netexport *nep,
np->netc_exflags = argp->ex_flags;
np->netc_anon = crget();
np->netc_anon->cr_uid = argp->ex_uid;
- crsetgroups(np->netc_anon, argp->ex_ngroups,
- argp->ex_groups);
+ crsetgroups_fallback(np->netc_anon, argp->ex_ngroups, argp->ex_groups,
+ NFSD_VNET(nfsrv_defaultgid));
np->netc_anon->cr_prison = &prison0;
prison_hold(np->netc_anon->cr_prison);
np->netc_numsecflavors = argp->ex_numsecflavors;
diff --git a/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c b/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c
index 1e6e71fa10ac..b1790dd167d5 100644
--- a/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c
+++ b/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c
@@ -537,7 +537,7 @@ rpc_gss_svc_getcred(struct svc_req *req, struct ucred **crp, int *flavorp)
cr = client->cl_cred = crget();
cr->cr_uid = cr->cr_ruid = cr->cr_svuid = uc->uid;
cr->cr_rgid = cr->cr_svgid = uc->gid;
- crsetgroups(cr, uc->gidlen, uc->gidlist);
+ crsetgroups_fallback(cr, uc->gidlen, uc->gidlist, uc->gid);
cr->cr_prison = curthread->td_ucred->cr_prison;
prison_hold(cr->cr_prison);
*crp = crhold(cr);
diff --git a/sys/rpc/svc_auth.c b/sys/rpc/svc_auth.c
index 6acb1fb0d4b9..92f1ee0f2844 100644
--- a/sys/rpc/svc_auth.c
+++ b/sys/rpc/svc_auth.c
@@ -187,10 +187,12 @@ svc_getcred(struct svc_req *rqst, struct ucred **crp, int *flavorp)
if ((xprt->xp_tls & (RPCTLS_FLAGS_CERTUSER |
RPCTLS_FLAGS_DISABLED)) == RPCTLS_FLAGS_CERTUSER &&
flavor == AUTH_UNIX) {
+ if (xprt->xp_ngrps <= 0)
+ return (FALSE);
cr = crget();
cr->cr_uid = cr->cr_ruid = cr->cr_svuid = xprt->xp_uid;
crsetgroups(cr, xprt->xp_ngrps, xprt->xp_gidp);
- cr->cr_rgid = cr->cr_svgid = xprt->xp_gidp[0];
+ cr->cr_rgid = cr->cr_svgid = cr->cr_gid;
cr->cr_prison = curthread->td_ucred->cr_prison;
prison_hold(cr->cr_prison);
*crp = cr;
@@ -200,10 +202,12 @@ svc_getcred(struct svc_req *rqst, struct ucred **crp, int *flavorp)
switch (flavor) {
case AUTH_UNIX:
xcr = (struct xucred *) rqst->rq_clntcred;
+ if (xcr->cr_ngroups <= 0)
+ return (FALSE);
cr = crget();
cr->cr_uid = cr->cr_ruid = cr->cr_svuid = xcr->cr_uid;
crsetgroups(cr, xcr->cr_ngroups, xcr->cr_groups);
- cr->cr_rgid = cr->cr_svgid = cr->cr_groups[0];
+ cr->cr_rgid = cr->cr_svgid = cr->cr_gid;
cr->cr_prison = curthread->td_ucred->cr_prison;
prison_hold(cr->cr_prison);
*crp = cr;