Re: [PATCH] nfsd: Fix NFSD_MAY_BYPASS_GSS and NFSD_MAY_BYPASS_GSS_ON_ROOT

From: NeilBrown
Date: Sun Oct 06 2024 - 18:13:41 EST

Next message: Lorenz Brun: "[PATCH] net: atlantic: support reading SFP module info"
Previous message: Artem Chernyshev: "[PATCH net] pktgen: Avoid out-of-range in get_imix_entries"
In reply to: Chuck Lever: "Re: [PATCH] nfsd: Fix NFSD_MAY_BYPASS_GSS and NFSD_MAY_BYPASS_GSS_ON_ROOT"
Next in thread: Pali Rohár: "Re: [PATCH] nfsd: Fix NFSD_MAY_BYPASS_GSS and NFSD_MAY_BYPASS_GSS_ON_ROOT"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, 07 Oct 2024, Chuck Lever wrote:
> On Fri, Sep 13, 2024 at 08:52:20AM +1000, NeilBrown wrote:
> > On Fri, 13 Sep 2024, Pali Rohár wrote:
> > > Currently NFSD_MAY_BYPASS_GSS and NFSD_MAY_BYPASS_GSS_ON_ROOT do not bypass
> > > only GSS, but bypass any authentication method. This is problem specially
> > > for NFS3 AUTH_NULL-only exports.
> > >
> > > The purpose of NFSD_MAY_BYPASS_GSS_ON_ROOT is described in RFC 2623,
> > > section 2.3.2, to allow mounting NFS2/3 GSS-only export without
> > > authentication. So few procedures which do not expose security risk used
> > > during mount time can be called also with AUTH_NONE or AUTH_SYS, to allow
> > > client mount operation to finish successfully.
> > >
> > > The problem with current implementation is that for AUTH_NULL-only exports,
> > > the NFSD_MAY_BYPASS_GSS_ON_ROOT is active also for NFS3 AUTH_UNIX mount
> > > attempts which confuse NFS3 clients, and make them think that AUTH_UNIX is
> > > enabled and is working. Linux NFS3 client never switches from AUTH_UNIX to
> > > AUTH_NONE on active mount, which makes the mount inaccessible.
> > >
> > > Fix the NFSD_MAY_BYPASS_GSS and NFSD_MAY_BYPASS_GSS_ON_ROOT implementation
> > > and really allow to bypass only exports which have some GSS auth flavor
> > > enabled.
> > >
> > > The result would be: For AUTH_NULL-only export if client attempts to do
> > > mount with AUTH_UNIX flavor then it will receive access errors, which
> > > instruct client that AUTH_UNIX flavor is not usable and will either try
> > > other auth flavor (AUTH_NULL if enabled) or fails mount procedure.
> > >
> > > This should fix problems with AUTH_NULL-only or AUTH_UNIX-only exports if
> > > client attempts to mount it with other auth flavor (e.g. with AUTH_NULL for
> > > AUTH_UNIX-only export, or with AUTH_UNIX for AUTH_NULL-only export).
> >
> > The MAY_BYPASS_GSS flag currently also bypasses TLS restrictions. With
> > your change it doesn't. I don't think we want to make that change.
>
> Neil, I'm not seeing this, I must be missing something.
>
> RPC_AUTH_TLS is used only on NULL procedures.
>
> The export's xprtsec= setting determines whether a TLS session must
> be present to access the files on the export. If the TLS session
> meets the xprtsec= policy, then the normal user authentication
> settings apply. In other words, I don't think execution gets close
> to check_nfsd_access() unless the xprtsec policy setting is met.

check_nfsd_access() is literally the ONLY place that ->ex_xprtsec_modes
is tested and that seems to be where xprtsec= export settings are stored.

>
> I'm not convinced check_nfsd_access() needs to care about
> RPC_AUTH_TLS. Can you expand a little on your concern?

Probably it doesn't care about RPC_AUTH_TLS which as you say is only
used on NULL procedures when setting up the TLS connection.

But it *does* care about NFS_XPRTSEC_MTLS etc.

But I now see that RPC_AUTH_TLS is never reported by OP_SECINFO as an
acceptable flavour, so the client cannot dynamically determine that TLS
is required. So there is no value in giving non-tls clients access to
xprtsec=mtls exports so they can discover that for themselves. The
client needs to explicitly mount with tls, or possibly the client can
opportunistically try TLS in every case, and call back.

So the original patch is OK.

NeilBrown

>
>
> > I think that what you want to do makes sense. Higher security can be
> > downgraded to AUTH_UNIX, but AUTH_NULL mustn't be upgraded to to
> > AUTH_UNIX.
> >
> > Maybe that needs to be explicit in the code. The bypass is ONLY allowed
> > for AUTH_UNIX and only if something other than AUTH_NULL is allowed.
> >
> > Thanks,
> > NeilBrown
> >
> >
> >
> > >
> > > Signed-off-by: Pali Rohár <pali@xxxxxxxxxx>
> > > ---
> > > fs/nfsd/export.c | 19 ++++++++++++++++++-
> > > fs/nfsd/export.h | 2 +-
> > > fs/nfsd/nfs4proc.c | 2 +-
> > > fs/nfsd/nfs4xdr.c | 2 +-
> > > fs/nfsd/nfsfh.c | 12 +++++++++---
> > > fs/nfsd/vfs.c | 2 +-
> > > 6 files changed, 31 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> > > index 50b3135d07ac..eb11d3fdffe1 100644
> > > --- a/fs/nfsd/export.c
> > > +++ b/fs/nfsd/export.c
> > > @@ -1074,7 +1074,7 @@ static struct svc_export *exp_find(struct cache_detail *cd,
> > > return exp;
> > > }
> > >
> > > -__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp)
> > > +__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp, bool may_bypass_gss)
> > > {
> > > struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
> > > struct svc_xprt *xprt = rqstp->rq_xprt;
> > > @@ -1120,6 +1120,23 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp)
> > > if (nfsd4_spo_must_allow(rqstp))
> > > return 0;
> > >
> > > + /* Some calls may be processed without authentication
> > > + * on GSS exports. For example NFS2/3 calls on root
> > > + * directory, see section 2.3.2 of rfc 2623.
> > > + * For "may_bypass_gss" check that export has really
> > > + * enabled some GSS flavor and also check that the
> > > + * used auth flavor is without auth (none or sys).
> > > + */
> > > + if (may_bypass_gss && (
> > > + rqstp->rq_cred.cr_flavor == RPC_AUTH_NULL ||
> > > + rqstp->rq_cred.cr_flavor == RPC_AUTH_UNIX)) {
> > > + for (f = exp->ex_flavors; f < end; f++) {
> > > + if (f->pseudoflavor == RPC_AUTH_GSS ||
> > > + f->pseudoflavor >= RPC_AUTH_GSS_KRB5)
> > > + return 0;
> > > + }
> > > + }
> > > +
> > > denied:
> > > return rqstp->rq_vers < 4 ? nfserr_acces : nfserr_wrongsec;
> > > }
> > > diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h
> > > index ca9dc230ae3d..dc7cf4f6ac53 100644
> > > --- a/fs/nfsd/export.h
> > > +++ b/fs/nfsd/export.h
> > > @@ -100,7 +100,7 @@ struct svc_expkey {
> > > #define EX_WGATHER(exp) ((exp)->ex_flags & NFSEXP_GATHERED_WRITES)
> > >
> > > int nfsexp_flags(struct svc_rqst *rqstp, struct svc_export *exp);
> > > -__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp);
> > > +__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp, bool may_bypass_gss);
> > >
> > > /*
> > > * Function declarations
> > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > index 2e39cf2e502a..0f67f4a7b8b2 100644
> > > --- a/fs/nfsd/nfs4proc.c
> > > +++ b/fs/nfsd/nfs4proc.c
> > > @@ -2791,7 +2791,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
> > >
> > > if (current_fh->fh_export &&
> > > need_wrongsec_check(rqstp))
> > > - op->status = check_nfsd_access(current_fh->fh_export, rqstp);
> > > + op->status = check_nfsd_access(current_fh->fh_export, rqstp, false);
> > > }
> > > encode_op:
> > > if (op->status == nfserr_replay_me) {
> > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > > index 97f583777972..b45ea5757652 100644
> > > --- a/fs/nfsd/nfs4xdr.c
> > > +++ b/fs/nfsd/nfs4xdr.c
> > > @@ -3775,7 +3775,7 @@ nfsd4_encode_entry4_fattr(struct nfsd4_readdir *cd, const char *name,
> > > nfserr = nfserrno(err);
> > > goto out_put;
> > > }
> > > - nfserr = check_nfsd_access(exp, cd->rd_rqstp);
> > > + nfserr = check_nfsd_access(exp, cd->rd_rqstp, false);
> > > if (nfserr)
> > > goto out_put;
> > >
> > > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> > > index dd4e11a703aa..ed0eabfa3cb0 100644
> > > --- a/fs/nfsd/nfsfh.c
> > > +++ b/fs/nfsd/nfsfh.c
> > > @@ -329,6 +329,7 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access)
> > > {
> > > struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> > > struct svc_export *exp = NULL;
> > > + bool may_bypass_gss = false;
> > > struct dentry *dentry;
> > > __be32 error;
> > >
> > > @@ -375,8 +376,13 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access)
> > > * which clients virtually always use auth_sys for,
> > > * even while using RPCSEC_GSS for NFS.
> > > */
> > > - if (access & NFSD_MAY_LOCK || access & NFSD_MAY_BYPASS_GSS)
> > > + if (access & NFSD_MAY_LOCK)
> > > goto skip_pseudoflavor_check;
> > > + /*
> > > + * NFS4 PUTFH may bypass GSS (see nfsd4_putfh() in nfs4proc.c).
> > > + */
> > > + if (access & NFSD_MAY_BYPASS_GSS)
> > > + may_bypass_gss = true;
> > > /*
> > > * Clients may expect to be able to use auth_sys during mount,
> > > * even if they use gss for everything else; see section 2.3.2
> > > @@ -384,9 +390,9 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access)
> > > */
> > > if (access & NFSD_MAY_BYPASS_GSS_ON_ROOT
> > > && exp->ex_path.dentry == dentry)
> > > - goto skip_pseudoflavor_check;
> > > + may_bypass_gss = true;
> > >
> > > - error = check_nfsd_access(exp, rqstp);
> > > + error = check_nfsd_access(exp, rqstp, may_bypass_gss);
> > > if (error)
> > > goto out;
> > >
> > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > > index 29b1f3613800..b2f5ea7c2187 100644
> > > --- a/fs/nfsd/vfs.c
> > > +++ b/fs/nfsd/vfs.c
> > > @@ -320,7 +320,7 @@ nfsd_lookup(struct svc_rqst *rqstp, struct svc_fh *fhp, const char *name,
> > > err = nfsd_lookup_dentry(rqstp, fhp, name, len, &exp, &dentry);
> > > if (err)
> > > return err;
> > > - err = check_nfsd_access(exp, rqstp);
> > > + err = check_nfsd_access(exp, rqstp, false);
> > > if (err)
> > > goto out;
> > > /*
> > > --
> > > 2.20.1
> > >
> > >
> >
>
> --
> Chuck Lever
>

Next message: Lorenz Brun: "[PATCH] net: atlantic: support reading SFP module info"
Previous message: Artem Chernyshev: "[PATCH net] pktgen: Avoid out-of-range in get_imix_entries"
In reply to: Chuck Lever: "Re: [PATCH] nfsd: Fix NFSD_MAY_BYPASS_GSS and NFSD_MAY_BYPASS_GSS_ON_ROOT"
Next in thread: Pali Rohár: "Re: [PATCH] nfsd: Fix NFSD_MAY_BYPASS_GSS and NFSD_MAY_BYPASS_GSS_ON_ROOT"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]