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

It also causes a panic when mountd is started. I'll post details later.


-- 
Cheers,
Cy Schubert <Cy.Schubert@cschubert.com>
FreeBSD UNIX:  <cy@FreeBSD.org>   Web:  https://FreeBSD.org
NTP:           <cy@nwtime.org>    Web:  https://nwtime.org

			e^(i*pi)+1=0


In message <9307D0CC-6D10-4F86-AE3B-43E7D6DA19A9@panasas.com>, Ravi Pokala 
writ
es:
> 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=3
> D=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=3
> D=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=3Dcfbe7a62dc62e8a5d7520cb
> 5=
> 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;
>
>
>
>