Date: Sat, 02 Nov 2024 20:58:12 -0700 From: Ravi Pokala <rpokala@freebsd.org> To: Olivier Certner <olce@FreeBSD.org>, <src-committers@FreeBSD.org>, <dev-commits-src-all@FreeBSD.org>, <dev-commits-src-main@FreeBSD.org> Subject: Re: cfbe7a62dc62 - main - nfs, rpc: Ensure kernel credentials have at least one group Message-ID: <9307D0CC-6D10-4F86-AE3B-43E7D6DA19A9@panasas.com> In-Reply-To: <202411022039.4A2KdbAE046580@gitrepo.freebsd.org> References: <202411022039.4A2KdbAE046580@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Olivier, This appears to break amd64.MINIMAL and amd64.MINIMALUP: =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D % 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 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D Thanks, Ravi (rpokala@) =EF=BB=BF-----Original Message----- From: <owner-src-committers@freebsd.org <mailto:owner-src-committers@freebs= d.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 hav= e at least one group The branch main has been updated by olce: URL: https://cgit.FreeBSD.org/src/commit/?id=3Dcfbe7a62dc62e8a5d7520cb5eb8ad7= c4a9418e26 <https://cgit.FreeBSD.org/src/commit/?id=3Dcfbe7a62dc62e8a5d7520cb5= eb8ad7c4a9418e26> 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 >=3D 0, ("newnfs_copycred: negative nfsc_ngroups")); cr->cr_uid =3D 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 =3D crget(); cr->cr_uid =3D cr->cr_ruid =3D cr->cr_svuid =3D nidp->nid_uid; - crsetgroups(cr, nidp->nid_ngroup, grps); - cr->cr_rgid =3D cr->cr_svgid =3D cr->cr_groups[0]; + crsetgroups_fallback(cr, nidp->nid_ngroup, grps, + NFSD_VNET(nfsrv_defaultgid)); + cr->cr_rgid =3D cr->cr_svgid =3D cr->cr_gid; cr->cr_prison =3D 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_nfsdpor= t.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 nfsexs= tuff *exp, NFSVNO_EXPORTANON(exp) || (nd->nd_flag & ND_AUTHNONE) !=3D 0) { nd->nd_cred->cr_uid =3D credanon->cr_uid; - nd->nd_cred->cr_gid =3D 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) =3D=3D 0) { diff --git a/sys/fs/nfsserver/nfs_nfsdsocket.c b/sys/fs/nfsserver/nfs_nfsds= ocket.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 =3D crget(); cr->cr_uid =3D cr->cr_ruid =3D cr->cr_svuid =3D UID_ROOT; - grp =3D GID_WHEEL; - crsetgroups(cr, 1, &grp); - cr->cr_rgid =3D cr->cr_svgid =3D cr->cr_groups[0]; + crsetgroups_fallback(cr, 0, NULL, GID_WHEEL); + cr->cr_rgid =3D cr->cr_svgid =3D cr->cr_gid; cr->cr_prison =3D 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 structu= re"); #if defined(INET) || defined(INET6) @@ -133,8 +137,8 @@ vfs_hang_addrlist(struct mount *mp, struct netexport *n= ep, np->netc_exflags =3D argp->ex_flags; np->netc_anon =3D crget(); np->netc_anon->cr_uid =3D 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 =3D &prison0; prison_hold(np->netc_anon->cr_prison); np->netc_numsecflavors =3D argp->ex_numsecflavors; @@ -212,8 +216,8 @@ vfs_hang_addrlist(struct mount *mp, struct netexport *n= ep, np->netc_exflags =3D argp->ex_flags; np->netc_anon =3D crget(); np->netc_anon->cr_uid =3D 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 =3D &prison0; prison_hold(np->netc_anon->cr_prison); np->netc_numsecflavors =3D argp->ex_numsecflavors; diff --git a/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c b/sys/rpc/rpcsec_gss/svc_r= pcsec_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 =3D client->cl_cred =3D crget(); cr->cr_uid =3D cr->cr_ruid =3D cr->cr_svuid =3D uc->uid; cr->cr_rgid =3D cr->cr_svgid =3D uc->gid; - crsetgroups(cr, uc->gidlen, uc->gidlist); + crsetgroups_fallback(cr, uc->gidlen, uc->gidlist, uc->gid); cr->cr_prison =3D curthread->td_ucred->cr_prison; prison_hold(cr->cr_prison); *crp =3D 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)) =3D=3D RPCTLS_FLAGS_CERTUSER && flavor =3D=3D AUTH_UNIX) { + if (xprt->xp_ngrps <=3D 0) + return (FALSE); cr =3D crget(); cr->cr_uid =3D cr->cr_ruid =3D cr->cr_svuid =3D xprt->xp_uid; crsetgroups(cr, xprt->xp_ngrps, xprt->xp_gidp); - cr->cr_rgid =3D cr->cr_svgid =3D xprt->xp_gidp[0]; + cr->cr_rgid =3D cr->cr_svgid =3D cr->cr_gid; cr->cr_prison =3D curthread->td_ucred->cr_prison; prison_hold(cr->cr_prison); *crp =3D cr; @@ -200,10 +202,12 @@ svc_getcred(struct svc_req *rqst, struct ucred **crp,= int *flavorp) switch (flavor) { case AUTH_UNIX: xcr =3D (struct xucred *) rqst->rq_clntcred; + if (xcr->cr_ngroups <=3D 0) + return (FALSE); cr =3D crget(); cr->cr_uid =3D cr->cr_ruid =3D cr->cr_svuid =3D xcr->cr_uid; crsetgroups(cr, xcr->cr_ngroups, xcr->cr_groups); - cr->cr_rgid =3D cr->cr_svgid =3D cr->cr_groups[0]; + cr->cr_rgid =3D cr->cr_svgid =3D cr->cr_gid; cr->cr_prison =3D curthread->td_ucred->cr_prison; prison_hold(cr->cr_prison); *crp =3D cr;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?9307D0CC-6D10-4F86-AE3B-43E7D6DA19A9>