* Re: [PATCH v2 3/3] nfsd: implement machine credential support for some operations
2016-01-18 20:08 ` [PATCH v2 3/3] nfsd: implement machine credential support for some operations Andrew Elble
@ 2016-01-20 22:53 ` J. Bruce Fields
2016-01-21 16:07 ` J. Bruce Fields
2016-01-21 19:01 ` J. Bruce Fields
2016-01-22 15:40 ` J. Bruce Fields
2 siblings, 1 reply; 17+ messages in thread
From: J. Bruce Fields @ 2016-01-20 22:53 UTC (permalink / raw)
To: Andrew Elble; +Cc: linux-nfs, dros
On Mon, Jan 18, 2016 at 03:08:22PM -0500, Andrew Elble wrote:
> Add server support for properly decoding and using spo_must_enforce
> and spo_must_allow bits. Add support for machine credentials to be
> used for CLOSE, OPEN_DOWNGRADE, LOCKU, DELEGRETURN,
> and TEST/FREE STATEID.
> Implement a check so as to not throw WRONGSEC errors when these
> operations are used if integrity/privacy isn't turned on.
I'm OK with supporting MACH_CRED on these additional operation, but
could you explain why it's necessary?
Rereading the spec.... Is it that you're hitting the "conundrum"
described in https://tools.ietf.org/html/rfc5661#page-504 ? I guess
that would explain the connection to KEYEXPIRED as well, OK. I think
it'd be worth an explanation in the changelog and maybe a comment in the
code referencing that bit of the spec.
I'm a little concerned that we're bypassing file permissions
completely--can any rogue client unlock another client's locks or return
their delegations regardless of any file or other permissions? (Looks
like that may be a preexisting problem, to some degree; e.g. does
nfsd4_locku() need more checks?)
--b.
>
> Signed-off-by: Andrew Elble <aweits@rit.edu>
> ---
> fs/nfsd/export.c | 4 ++++
> fs/nfsd/nfs4proc.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> fs/nfsd/nfs4state.c | 18 ++++++++++++++
> fs/nfsd/nfs4xdr.c | 51 ++++++++++++++++++---------------------
> fs/nfsd/nfsd.h | 5 ++++
> fs/nfsd/state.h | 1 +
> fs/nfsd/xdr4.h | 3 +++
> 7 files changed, 123 insertions(+), 28 deletions(-)
>
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index b4d84b579f20..0395e3e8fc3e 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -954,6 +954,10 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp)
> rqstp->rq_cred.cr_flavor == RPC_AUTH_UNIX)
> return 0;
> }
> +
> + if (nfsd4_spo_must_allow(rqstp))
> + return 0;
> +
> return nfserr_wrongsec;
> }
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index a9f096c7e99f..047d6662010b 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2285,6 +2285,75 @@ static struct nfsd4_operation nfsd4_ops[] = {
> },
> };
>
> +/**
> + * nfsd4_spo_must_allow - Determine if the compound op contains an
> + * operation that is allowed to be sent with machine credentials
> + *
> + * @rqstp: a pointer to the struct svc_rqst
> + *
> + * nfsd4_spo_must_allow() allows check_nfsd_access() to succeed
> + * when the operation and/or the FH+operation(s) is part of what the
> + * client negotiated to be able to send with machine credentials.
> + * We keep some state so that FH+operation(s) can succeed despite
> + * check_nfsd_access() being called from fh_verify() as well as
> + * nfsd4_proc_compound().
> + */
> +
> +bool nfsd4_spo_must_allow(struct svc_rqst *rqstp)
> +{
> + struct nfsd4_compoundres *resp = rqstp->rq_resp;
> + struct nfsd4_compoundargs *argp = rqstp->rq_argp;
> + struct nfsd4_op *this = &argp->ops[resp->opcnt - 1];
> + struct nfsd4_compound_state *cstate = &resp->cstate;
> + struct nfsd4_operation *thisd;
> + struct nfs4_op_map *allow = &cstate->clp->cl_spo_must_allow;
> + u32 opiter;
> +
> + if (!cstate->minorversion)
> + return false;
> +
> + thisd = OPDESC(this);
> +
> + if (!(thisd->op_flags & OP_IS_PUTFH_LIKE)) {
> + if (cstate->spo_must_allowed == true)
> + /*
> + * a prior putfh + op has set
> + * spo_must_allow conditions
> + */
> + return true;
> + /* evaluate op against spo_must_allow with no prior putfh */
> + if (test_bit(this->opnum, allow->u.longs) &&
> + cstate->clp->cl_mach_cred &&
> + nfsd4_mach_creds_match(cstate->clp, rqstp))
> + return true;
> + else
> + return false;
> + }
> + /*
> + * this->opnum has PUTFH ramifications
> + * scan forward to next putfh or end of compound op
> + */
> + opiter = resp->opcnt;
> + while (opiter < argp->opcnt) {
> + this = &argp->ops[opiter++];
> + thisd = OPDESC(this);
> + if (thisd->op_flags & OP_IS_PUTFH_LIKE)
> + break;
> + if (test_bit(this->opnum, allow->u.longs) &&
> + cstate->clp->cl_mach_cred &&
> + nfsd4_mach_creds_match(cstate->clp, rqstp)) {
> + /*
> + * the op covered by the fh is a
> + * spo_must_allow operation
> + */
> + cstate->spo_must_allowed = true;
> + return true;
> + }
> + }
> + cstate->spo_must_allowed = false;
> + return false;
> +}
> +
> int nfsd4_max_reply(struct svc_rqst *rqstp, struct nfsd4_op *op)
> {
> struct nfsd4_operation *opdesc;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 65efc900e97e..b28805519725 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2367,6 +2367,22 @@ nfsd4_exchange_id(struct svc_rqst *rqstp,
>
> switch (exid->spa_how) {
> case SP4_MACH_CRED:
> + exid->spo_must_enforce[0] = 0;
> + exid->spo_must_enforce[1] = (
> + 1 << (OP_BIND_CONN_TO_SESSION - 32) |
> + 1 << (OP_EXCHANGE_ID - 32) |
> + 1 << (OP_CREATE_SESSION - 32) |
> + 1 << (OP_DESTROY_SESSION - 32) |
> + 1 << (OP_DESTROY_CLIENTID - 32));
> +
> + exid->spo_must_allow[0] &= (1 << (OP_CLOSE) |
> + 1 << (OP_OPEN_DOWNGRADE) |
> + 1 << (OP_LOCKU) |
> + 1 << (OP_DELEGRETURN));
> +
> + exid->spo_must_allow[1] &= (
> + 1 << (OP_TEST_STATEID - 32) |
> + 1 << (OP_FREE_STATEID - 32));
> if (!svc_rqst_integrity_protected(rqstp))
> return nfserr_inval;
> case SP4_NONE:
> @@ -2443,6 +2459,8 @@ out_new:
> }
> new->cl_minorversion = cstate->minorversion;
> new->cl_mach_cred = (exid->spa_how == SP4_MACH_CRED);
> + new->cl_spo_must_allow.u.words[0] = exid->spo_must_allow[0];
> + new->cl_spo_must_allow.u.words[1] = exid->spo_must_allow[1];
>
> gen_clid(new, nn);
> add_to_unconfirmed(new);
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 51c9e9ca39a4..e2043aa95e27 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1297,16 +1297,14 @@ nfsd4_decode_exchange_id(struct nfsd4_compoundargs *argp,
> break;
> case SP4_MACH_CRED:
> /* spo_must_enforce */
> - READ_BUF(4);
> - dummy = be32_to_cpup(p++);
> - READ_BUF(dummy * 4);
> - p += dummy;
> -
> + status = nfsd4_decode_bitmap(argp,
> + exid->spo_must_enforce);
> + if (status)
> + goto out;
> /* spo_must_allow */
> - READ_BUF(4);
> - dummy = be32_to_cpup(p++);
> - READ_BUF(dummy * 4);
> - p += dummy;
> + status = nfsd4_decode_bitmap(argp, exid->spo_must_allow);
> + if (status)
> + goto out;
> break;
> case SP4_SSV:
> /* ssp_ops */
> @@ -3841,14 +3839,6 @@ nfsd4_encode_write(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_w
> return nfserr;
> }
>
> -static const u32 nfs4_minimal_spo_must_enforce[2] = {
> - [1] = 1 << (OP_BIND_CONN_TO_SESSION - 32) |
> - 1 << (OP_EXCHANGE_ID - 32) |
> - 1 << (OP_CREATE_SESSION - 32) |
> - 1 << (OP_DESTROY_SESSION - 32) |
> - 1 << (OP_DESTROY_CLIENTID - 32)
> -};
> -
> static __be32
> nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
> struct nfsd4_exchange_id *exid)
> @@ -3859,6 +3849,7 @@ nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
> char *server_scope;
> int major_id_sz;
> int server_scope_sz;
> + int status = 0;
> uint64_t minor_id = 0;
>
> if (nfserr)
> @@ -3887,18 +3878,20 @@ nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
> case SP4_NONE:
> break;
> case SP4_MACH_CRED:
> - /* spo_must_enforce, spo_must_allow */
> - p = xdr_reserve_space(xdr, 16);
> - if (!p)
> - return nfserr_resource;
> -
> /* spo_must_enforce bitmap: */
> - *p++ = cpu_to_be32(2);
> - *p++ = cpu_to_be32(nfs4_minimal_spo_must_enforce[0]);
> - *p++ = cpu_to_be32(nfs4_minimal_spo_must_enforce[1]);
> - /* empty spo_must_allow bitmap: */
> - *p++ = cpu_to_be32(0);
> -
> + status = nfsd4_encode_bitmap(xdr,
> + exid->spo_must_enforce[0],
> + exid->spo_must_enforce[1],
> + exid->spo_must_enforce[2]);
> + if (status)
> + goto out;
> + /* spo_must_allow bitmap: */
> + status = nfsd4_encode_bitmap(xdr,
> + exid->spo_must_allow[0],
> + exid->spo_must_allow[1],
> + exid->spo_must_allow[2]);
> + if (status)
> + goto out;
> break;
> default:
> WARN_ON_ONCE(1);
> @@ -3925,6 +3918,8 @@ nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
> /* Implementation id */
> *p++ = cpu_to_be32(0); /* zero length nfs_impl_id4 array */
> return 0;
> +out:
> + return status;
> }
>
> static __be32
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index cf980523898b..9446849888d5 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -124,6 +124,7 @@ void nfs4_state_shutdown_net(struct net *net);
> void nfs4_reset_lease(time_t leasetime);
> int nfs4_reset_recoverydir(char *recdir);
> char * nfs4_recoverydir(void);
> +bool nfsd4_spo_must_allow(struct svc_rqst *rqstp);
> #else
> static inline int nfsd4_init_slabs(void) { return 0; }
> static inline void nfsd4_free_slabs(void) { }
> @@ -134,6 +135,10 @@ static inline void nfs4_state_shutdown_net(struct net *net) { }
> static inline void nfs4_reset_lease(time_t leasetime) { }
> static inline int nfs4_reset_recoverydir(char *recdir) { return 0; }
> static inline char * nfs4_recoverydir(void) {return NULL; }
> +static inline bool nfsd4_spo_must_allow(struct svc_rqst *rqstp)
> +{
> + return false;
> +}
> #endif
>
> /*
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 77fdf4de91ba..2b59c74f098c 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -345,6 +345,7 @@ struct nfs4_client {
> u32 cl_exchange_flags;
> /* number of rpc's in progress over an associated session: */
> atomic_t cl_refcount;
> + struct nfs4_op_map cl_spo_must_allow;
>
> /* for nfs41 callbacks */
> /* We currently support a single back channel with a single slot */
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 25c9c79460f9..c88aca9c42d7 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -59,6 +59,7 @@ struct nfsd4_compound_state {
> struct nfsd4_session *session;
> struct nfsd4_slot *slot;
> int data_offset;
> + bool spo_must_allowed;
> size_t iovlen;
> u32 minorversion;
> __be32 status;
> @@ -403,6 +404,8 @@ struct nfsd4_exchange_id {
> clientid_t clientid;
> u32 seqid;
> int spa_how;
> + u32 spo_must_enforce[3];
> + u32 spo_must_allow[3];
> };
>
> struct nfsd4_sequence {
> --
> 2.6.3
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 3/3] nfsd: implement machine credential support for some operations
2016-01-20 22:53 ` J. Bruce Fields
@ 2016-01-21 16:07 ` J. Bruce Fields
0 siblings, 0 replies; 17+ messages in thread
From: J. Bruce Fields @ 2016-01-21 16:07 UTC (permalink / raw)
To: Andrew Elble; +Cc: linux-nfs, dros
On Wed, Jan 20, 2016 at 05:53:25PM -0500, J. Bruce Fields wrote:
> On Mon, Jan 18, 2016 at 03:08:22PM -0500, Andrew Elble wrote:
> > Add server support for properly decoding and using spo_must_enforce
> > and spo_must_allow bits. Add support for machine credentials to be
> > used for CLOSE, OPEN_DOWNGRADE, LOCKU, DELEGRETURN,
> > and TEST/FREE STATEID.
> > Implement a check so as to not throw WRONGSEC errors when these
> > operations are used if integrity/privacy isn't turned on.
>
> I'm OK with supporting MACH_CRED on these additional operation, but
> could you explain why it's necessary?
>
> Rereading the spec.... Is it that you're hitting the "conundrum"
> described in https://tools.ietf.org/html/rfc5661#page-504 ? I guess
> that would explain the connection to KEYEXPIRED as well, OK. I think
> it'd be worth an explanation in the changelog and maybe a comment in the
> code referencing that bit of the spec.
>
> I'm a little concerned that we're bypassing file permissions
> completely--can any rogue client unlock another client's locks or return
> their delegations regardless of any file or other permissions? (Looks
> like that may be a preexisting problem, to some degree; e.g. does
> nfsd4_locku() need more checks?)
Thinking about the LOCKU case some more: checking file permissions could
risk leaving us with unlockable locks if permissions change. User
permissions may be tough to use in general--locally it's OK to lock and
unlock as a different user, isn't it? So nfsd4_locku() may be right
just to give up and check nothing. With MACH_CRED we can at least check
that the client has the right to use the given lockowner. (Could we add
an nfsd4_match_creds() check to nfsd4_locku()?)
--b.
> > Signed-off-by: Andrew Elble <aweits@rit.edu>
> > ---
> > fs/nfsd/export.c | 4 ++++
> > fs/nfsd/nfs4proc.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > fs/nfsd/nfs4state.c | 18 ++++++++++++++
> > fs/nfsd/nfs4xdr.c | 51 ++++++++++++++++++---------------------
> > fs/nfsd/nfsd.h | 5 ++++
> > fs/nfsd/state.h | 1 +
> > fs/nfsd/xdr4.h | 3 +++
> > 7 files changed, 123 insertions(+), 28 deletions(-)
> >
> > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> > index b4d84b579f20..0395e3e8fc3e 100644
> > --- a/fs/nfsd/export.c
> > +++ b/fs/nfsd/export.c
> > @@ -954,6 +954,10 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp)
> > rqstp->rq_cred.cr_flavor == RPC_AUTH_UNIX)
> > return 0;
> > }
> > +
> > + if (nfsd4_spo_must_allow(rqstp))
> > + return 0;
> > +
> > return nfserr_wrongsec;
> > }
> >
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index a9f096c7e99f..047d6662010b 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -2285,6 +2285,75 @@ static struct nfsd4_operation nfsd4_ops[] = {
> > },
> > };
> >
> > +/**
> > + * nfsd4_spo_must_allow - Determine if the compound op contains an
> > + * operation that is allowed to be sent with machine credentials
> > + *
> > + * @rqstp: a pointer to the struct svc_rqst
> > + *
> > + * nfsd4_spo_must_allow() allows check_nfsd_access() to succeed
> > + * when the operation and/or the FH+operation(s) is part of what the
> > + * client negotiated to be able to send with machine credentials.
> > + * We keep some state so that FH+operation(s) can succeed despite
> > + * check_nfsd_access() being called from fh_verify() as well as
> > + * nfsd4_proc_compound().
> > + */
> > +
> > +bool nfsd4_spo_must_allow(struct svc_rqst *rqstp)
> > +{
> > + struct nfsd4_compoundres *resp = rqstp->rq_resp;
> > + struct nfsd4_compoundargs *argp = rqstp->rq_argp;
> > + struct nfsd4_op *this = &argp->ops[resp->opcnt - 1];
> > + struct nfsd4_compound_state *cstate = &resp->cstate;
> > + struct nfsd4_operation *thisd;
> > + struct nfs4_op_map *allow = &cstate->clp->cl_spo_must_allow;
> > + u32 opiter;
> > +
> > + if (!cstate->minorversion)
> > + return false;
> > +
> > + thisd = OPDESC(this);
> > +
> > + if (!(thisd->op_flags & OP_IS_PUTFH_LIKE)) {
> > + if (cstate->spo_must_allowed == true)
> > + /*
> > + * a prior putfh + op has set
> > + * spo_must_allow conditions
> > + */
> > + return true;
> > + /* evaluate op against spo_must_allow with no prior putfh */
> > + if (test_bit(this->opnum, allow->u.longs) &&
> > + cstate->clp->cl_mach_cred &&
> > + nfsd4_mach_creds_match(cstate->clp, rqstp))
> > + return true;
> > + else
> > + return false;
> > + }
> > + /*
> > + * this->opnum has PUTFH ramifications
> > + * scan forward to next putfh or end of compound op
> > + */
> > + opiter = resp->opcnt;
> > + while (opiter < argp->opcnt) {
> > + this = &argp->ops[opiter++];
> > + thisd = OPDESC(this);
> > + if (thisd->op_flags & OP_IS_PUTFH_LIKE)
> > + break;
> > + if (test_bit(this->opnum, allow->u.longs) &&
> > + cstate->clp->cl_mach_cred &&
> > + nfsd4_mach_creds_match(cstate->clp, rqstp)) {
> > + /*
> > + * the op covered by the fh is a
> > + * spo_must_allow operation
> > + */
> > + cstate->spo_must_allowed = true;
> > + return true;
> > + }
> > + }
> > + cstate->spo_must_allowed = false;
> > + return false;
> > +}
> > +
> > int nfsd4_max_reply(struct svc_rqst *rqstp, struct nfsd4_op *op)
> > {
> > struct nfsd4_operation *opdesc;
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 65efc900e97e..b28805519725 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -2367,6 +2367,22 @@ nfsd4_exchange_id(struct svc_rqst *rqstp,
> >
> > switch (exid->spa_how) {
> > case SP4_MACH_CRED:
> > + exid->spo_must_enforce[0] = 0;
> > + exid->spo_must_enforce[1] = (
> > + 1 << (OP_BIND_CONN_TO_SESSION - 32) |
> > + 1 << (OP_EXCHANGE_ID - 32) |
> > + 1 << (OP_CREATE_SESSION - 32) |
> > + 1 << (OP_DESTROY_SESSION - 32) |
> > + 1 << (OP_DESTROY_CLIENTID - 32));
> > +
> > + exid->spo_must_allow[0] &= (1 << (OP_CLOSE) |
> > + 1 << (OP_OPEN_DOWNGRADE) |
> > + 1 << (OP_LOCKU) |
> > + 1 << (OP_DELEGRETURN));
> > +
> > + exid->spo_must_allow[1] &= (
> > + 1 << (OP_TEST_STATEID - 32) |
> > + 1 << (OP_FREE_STATEID - 32));
> > if (!svc_rqst_integrity_protected(rqstp))
> > return nfserr_inval;
> > case SP4_NONE:
> > @@ -2443,6 +2459,8 @@ out_new:
> > }
> > new->cl_minorversion = cstate->minorversion;
> > new->cl_mach_cred = (exid->spa_how == SP4_MACH_CRED);
> > + new->cl_spo_must_allow.u.words[0] = exid->spo_must_allow[0];
> > + new->cl_spo_must_allow.u.words[1] = exid->spo_must_allow[1];
> >
> > gen_clid(new, nn);
> > add_to_unconfirmed(new);
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index 51c9e9ca39a4..e2043aa95e27 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -1297,16 +1297,14 @@ nfsd4_decode_exchange_id(struct nfsd4_compoundargs *argp,
> > break;
> > case SP4_MACH_CRED:
> > /* spo_must_enforce */
> > - READ_BUF(4);
> > - dummy = be32_to_cpup(p++);
> > - READ_BUF(dummy * 4);
> > - p += dummy;
> > -
> > + status = nfsd4_decode_bitmap(argp,
> > + exid->spo_must_enforce);
> > + if (status)
> > + goto out;
> > /* spo_must_allow */
> > - READ_BUF(4);
> > - dummy = be32_to_cpup(p++);
> > - READ_BUF(dummy * 4);
> > - p += dummy;
> > + status = nfsd4_decode_bitmap(argp, exid->spo_must_allow);
> > + if (status)
> > + goto out;
> > break;
> > case SP4_SSV:
> > /* ssp_ops */
> > @@ -3841,14 +3839,6 @@ nfsd4_encode_write(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_w
> > return nfserr;
> > }
> >
> > -static const u32 nfs4_minimal_spo_must_enforce[2] = {
> > - [1] = 1 << (OP_BIND_CONN_TO_SESSION - 32) |
> > - 1 << (OP_EXCHANGE_ID - 32) |
> > - 1 << (OP_CREATE_SESSION - 32) |
> > - 1 << (OP_DESTROY_SESSION - 32) |
> > - 1 << (OP_DESTROY_CLIENTID - 32)
> > -};
> > -
> > static __be32
> > nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
> > struct nfsd4_exchange_id *exid)
> > @@ -3859,6 +3849,7 @@ nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
> > char *server_scope;
> > int major_id_sz;
> > int server_scope_sz;
> > + int status = 0;
> > uint64_t minor_id = 0;
> >
> > if (nfserr)
> > @@ -3887,18 +3878,20 @@ nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
> > case SP4_NONE:
> > break;
> > case SP4_MACH_CRED:
> > - /* spo_must_enforce, spo_must_allow */
> > - p = xdr_reserve_space(xdr, 16);
> > - if (!p)
> > - return nfserr_resource;
> > -
> > /* spo_must_enforce bitmap: */
> > - *p++ = cpu_to_be32(2);
> > - *p++ = cpu_to_be32(nfs4_minimal_spo_must_enforce[0]);
> > - *p++ = cpu_to_be32(nfs4_minimal_spo_must_enforce[1]);
> > - /* empty spo_must_allow bitmap: */
> > - *p++ = cpu_to_be32(0);
> > -
> > + status = nfsd4_encode_bitmap(xdr,
> > + exid->spo_must_enforce[0],
> > + exid->spo_must_enforce[1],
> > + exid->spo_must_enforce[2]);
> > + if (status)
> > + goto out;
> > + /* spo_must_allow bitmap: */
> > + status = nfsd4_encode_bitmap(xdr,
> > + exid->spo_must_allow[0],
> > + exid->spo_must_allow[1],
> > + exid->spo_must_allow[2]);
> > + if (status)
> > + goto out;
> > break;
> > default:
> > WARN_ON_ONCE(1);
> > @@ -3925,6 +3918,8 @@ nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
> > /* Implementation id */
> > *p++ = cpu_to_be32(0); /* zero length nfs_impl_id4 array */
> > return 0;
> > +out:
> > + return status;
> > }
> >
> > static __be32
> > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> > index cf980523898b..9446849888d5 100644
> > --- a/fs/nfsd/nfsd.h
> > +++ b/fs/nfsd/nfsd.h
> > @@ -124,6 +124,7 @@ void nfs4_state_shutdown_net(struct net *net);
> > void nfs4_reset_lease(time_t leasetime);
> > int nfs4_reset_recoverydir(char *recdir);
> > char * nfs4_recoverydir(void);
> > +bool nfsd4_spo_must_allow(struct svc_rqst *rqstp);
> > #else
> > static inline int nfsd4_init_slabs(void) { return 0; }
> > static inline void nfsd4_free_slabs(void) { }
> > @@ -134,6 +135,10 @@ static inline void nfs4_state_shutdown_net(struct net *net) { }
> > static inline void nfs4_reset_lease(time_t leasetime) { }
> > static inline int nfs4_reset_recoverydir(char *recdir) { return 0; }
> > static inline char * nfs4_recoverydir(void) {return NULL; }
> > +static inline bool nfsd4_spo_must_allow(struct svc_rqst *rqstp)
> > +{
> > + return false;
> > +}
> > #endif
> >
> > /*
> > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > index 77fdf4de91ba..2b59c74f098c 100644
> > --- a/fs/nfsd/state.h
> > +++ b/fs/nfsd/state.h
> > @@ -345,6 +345,7 @@ struct nfs4_client {
> > u32 cl_exchange_flags;
> > /* number of rpc's in progress over an associated session: */
> > atomic_t cl_refcount;
> > + struct nfs4_op_map cl_spo_must_allow;
> >
> > /* for nfs41 callbacks */
> > /* We currently support a single back channel with a single slot */
> > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > index 25c9c79460f9..c88aca9c42d7 100644
> > --- a/fs/nfsd/xdr4.h
> > +++ b/fs/nfsd/xdr4.h
> > @@ -59,6 +59,7 @@ struct nfsd4_compound_state {
> > struct nfsd4_session *session;
> > struct nfsd4_slot *slot;
> > int data_offset;
> > + bool spo_must_allowed;
> > size_t iovlen;
> > u32 minorversion;
> > __be32 status;
> > @@ -403,6 +404,8 @@ struct nfsd4_exchange_id {
> > clientid_t clientid;
> > u32 seqid;
> > int spa_how;
> > + u32 spo_must_enforce[3];
> > + u32 spo_must_allow[3];
> > };
> >
> > struct nfsd4_sequence {
> > --
> > 2.6.3
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] nfsd: implement machine credential support for some operations
2016-01-18 20:08 ` [PATCH v2 3/3] nfsd: implement machine credential support for some operations Andrew Elble
2016-01-20 22:53 ` J. Bruce Fields
@ 2016-01-21 19:01 ` J. Bruce Fields
2016-01-21 19:30 ` Andrew W Elble
2016-01-22 15:40 ` J. Bruce Fields
2 siblings, 1 reply; 17+ messages in thread
From: J. Bruce Fields @ 2016-01-21 19:01 UTC (permalink / raw)
To: Andrew Elble; +Cc: linux-nfs, dros
On Mon, Jan 18, 2016 at 03:08:22PM -0500, Andrew Elble wrote:
> Add server support for properly decoding and using spo_must_enforce
> and spo_must_allow bits. Add support for machine credentials to be
> used for CLOSE, OPEN_DOWNGRADE, LOCKU, DELEGRETURN,
> and TEST/FREE STATEID.
> Implement a check so as to not throw WRONGSEC errors when these
> operations are used if integrity/privacy isn't turned on.
>
> Signed-off-by: Andrew Elble <aweits@rit.edu>
> ---
> fs/nfsd/export.c | 4 ++++
> fs/nfsd/nfs4proc.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> fs/nfsd/nfs4state.c | 18 ++++++++++++++
> fs/nfsd/nfs4xdr.c | 51 ++++++++++++++++++---------------------
> fs/nfsd/nfsd.h | 5 ++++
> fs/nfsd/state.h | 1 +
> fs/nfsd/xdr4.h | 3 +++
> 7 files changed, 123 insertions(+), 28 deletions(-)
>
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index b4d84b579f20..0395e3e8fc3e 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -954,6 +954,10 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp)
> rqstp->rq_cred.cr_flavor == RPC_AUTH_UNIX)
> return 0;
> }
> +
> + if (nfsd4_spo_must_allow(rqstp))
> + return 0;
> +
> return nfserr_wrongsec;
> }
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index a9f096c7e99f..047d6662010b 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2285,6 +2285,75 @@ static struct nfsd4_operation nfsd4_ops[] = {
> },
> };
>
> +/**
> + * nfsd4_spo_must_allow - Determine if the compound op contains an
> + * operation that is allowed to be sent with machine credentials
> + *
> + * @rqstp: a pointer to the struct svc_rqst
> + *
> + * nfsd4_spo_must_allow() allows check_nfsd_access() to succeed
> + * when the operation and/or the FH+operation(s) is part of what the
> + * client negotiated to be able to send with machine credentials.
> + * We keep some state so that FH+operation(s) can succeed despite
> + * check_nfsd_access() being called from fh_verify() as well as
> + * nfsd4_proc_compound().
> + */
> +
> +bool nfsd4_spo_must_allow(struct svc_rqst *rqstp)
> +{
> + struct nfsd4_compoundres *resp = rqstp->rq_resp;
> + struct nfsd4_compoundargs *argp = rqstp->rq_argp;
> + struct nfsd4_op *this = &argp->ops[resp->opcnt - 1];
> + struct nfsd4_compound_state *cstate = &resp->cstate;
> + struct nfsd4_operation *thisd;
> + struct nfs4_op_map *allow = &cstate->clp->cl_spo_must_allow;
> + u32 opiter;
> +
> + if (!cstate->minorversion)
> + return false;
> +
> + thisd = OPDESC(this);
> +
> + if (!(thisd->op_flags & OP_IS_PUTFH_LIKE)) {
> + if (cstate->spo_must_allowed == true)
> + /*
> + * a prior putfh + op has set
> + * spo_must_allow conditions
> + */
> + return true;
> + /* evaluate op against spo_must_allow with no prior putfh */
> + if (test_bit(this->opnum, allow->u.longs) &&
> + cstate->clp->cl_mach_cred &&
> + nfsd4_mach_creds_match(cstate->clp, rqstp))
> + return true;
> + else
> + return false;
> + }
> + /*
> + * this->opnum has PUTFH ramifications
> + * scan forward to next putfh or end of compound op
You also return early if you get a must_allow'd op:
> + */
> + opiter = resp->opcnt;
> + while (opiter < argp->opcnt) {
> + this = &argp->ops[opiter++];
> + thisd = OPDESC(this);
> + if (thisd->op_flags & OP_IS_PUTFH_LIKE)
> + break;
> + if (test_bit(this->opnum, allow->u.longs) &&
> + cstate->clp->cl_mach_cred &&
> + nfsd4_mach_creds_match(cstate->clp, rqstp)) {
> + /*
> + * the op covered by the fh is a
> + * spo_must_allow operation
> + */
> + cstate->spo_must_allowed = true;
> + return true;
Doesn't this mean that a compound like e.g.:
PUTFH
CLOSE
OPEN
would result in a return of true on the OPEN, if CLOSE was in must_allow
but OPEN wasn't? (Because the above loop sets spo_must_allowed as soon
as it hits the CLOSE.)
I wonder if you could actually do this more easily in
need_wrongsec_check()? Of the new ops you're allowing must_allow for, I
believe only nfsd4_delegreturn() calls fh_verify(), and I suspect that's
actually wrong.
In which case the only nfsd_check_access() call we care about is
actually the one gated by need_wrongsec_check().
? But I haven't completely thought this through.
--b.
> + }
> + }
> + cstate->spo_must_allowed = false;
> + return false;
> +}
> +
> int nfsd4_max_reply(struct svc_rqst *rqstp, struct nfsd4_op *op)
> {
> struct nfsd4_operation *opdesc;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 65efc900e97e..b28805519725 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2367,6 +2367,22 @@ nfsd4_exchange_id(struct svc_rqst *rqstp,
>
> switch (exid->spa_how) {
> case SP4_MACH_CRED:
> + exid->spo_must_enforce[0] = 0;
> + exid->spo_must_enforce[1] = (
> + 1 << (OP_BIND_CONN_TO_SESSION - 32) |
> + 1 << (OP_EXCHANGE_ID - 32) |
> + 1 << (OP_CREATE_SESSION - 32) |
> + 1 << (OP_DESTROY_SESSION - 32) |
> + 1 << (OP_DESTROY_CLIENTID - 32));
> +
> + exid->spo_must_allow[0] &= (1 << (OP_CLOSE) |
> + 1 << (OP_OPEN_DOWNGRADE) |
> + 1 << (OP_LOCKU) |
> + 1 << (OP_DELEGRETURN));
> +
> + exid->spo_must_allow[1] &= (
> + 1 << (OP_TEST_STATEID - 32) |
> + 1 << (OP_FREE_STATEID - 32));
> if (!svc_rqst_integrity_protected(rqstp))
> return nfserr_inval;
> case SP4_NONE:
> @@ -2443,6 +2459,8 @@ out_new:
> }
> new->cl_minorversion = cstate->minorversion;
> new->cl_mach_cred = (exid->spa_how == SP4_MACH_CRED);
> + new->cl_spo_must_allow.u.words[0] = exid->spo_must_allow[0];
> + new->cl_spo_must_allow.u.words[1] = exid->spo_must_allow[1];
>
> gen_clid(new, nn);
> add_to_unconfirmed(new);
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 51c9e9ca39a4..e2043aa95e27 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1297,16 +1297,14 @@ nfsd4_decode_exchange_id(struct nfsd4_compoundargs *argp,
> break;
> case SP4_MACH_CRED:
> /* spo_must_enforce */
> - READ_BUF(4);
> - dummy = be32_to_cpup(p++);
> - READ_BUF(dummy * 4);
> - p += dummy;
> -
> + status = nfsd4_decode_bitmap(argp,
> + exid->spo_must_enforce);
> + if (status)
> + goto out;
> /* spo_must_allow */
> - READ_BUF(4);
> - dummy = be32_to_cpup(p++);
> - READ_BUF(dummy * 4);
> - p += dummy;
> + status = nfsd4_decode_bitmap(argp, exid->spo_must_allow);
> + if (status)
> + goto out;
> break;
> case SP4_SSV:
> /* ssp_ops */
> @@ -3841,14 +3839,6 @@ nfsd4_encode_write(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_w
> return nfserr;
> }
>
> -static const u32 nfs4_minimal_spo_must_enforce[2] = {
> - [1] = 1 << (OP_BIND_CONN_TO_SESSION - 32) |
> - 1 << (OP_EXCHANGE_ID - 32) |
> - 1 << (OP_CREATE_SESSION - 32) |
> - 1 << (OP_DESTROY_SESSION - 32) |
> - 1 << (OP_DESTROY_CLIENTID - 32)
> -};
> -
> static __be32
> nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
> struct nfsd4_exchange_id *exid)
> @@ -3859,6 +3849,7 @@ nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
> char *server_scope;
> int major_id_sz;
> int server_scope_sz;
> + int status = 0;
> uint64_t minor_id = 0;
>
> if (nfserr)
> @@ -3887,18 +3878,20 @@ nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
> case SP4_NONE:
> break;
> case SP4_MACH_CRED:
> - /* spo_must_enforce, spo_must_allow */
> - p = xdr_reserve_space(xdr, 16);
> - if (!p)
> - return nfserr_resource;
> -
> /* spo_must_enforce bitmap: */
> - *p++ = cpu_to_be32(2);
> - *p++ = cpu_to_be32(nfs4_minimal_spo_must_enforce[0]);
> - *p++ = cpu_to_be32(nfs4_minimal_spo_must_enforce[1]);
> - /* empty spo_must_allow bitmap: */
> - *p++ = cpu_to_be32(0);
> -
> + status = nfsd4_encode_bitmap(xdr,
> + exid->spo_must_enforce[0],
> + exid->spo_must_enforce[1],
> + exid->spo_must_enforce[2]);
> + if (status)
> + goto out;
> + /* spo_must_allow bitmap: */
> + status = nfsd4_encode_bitmap(xdr,
> + exid->spo_must_allow[0],
> + exid->spo_must_allow[1],
> + exid->spo_must_allow[2]);
> + if (status)
> + goto out;
> break;
> default:
> WARN_ON_ONCE(1);
> @@ -3925,6 +3918,8 @@ nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
> /* Implementation id */
> *p++ = cpu_to_be32(0); /* zero length nfs_impl_id4 array */
> return 0;
> +out:
> + return status;
> }
>
> static __be32
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index cf980523898b..9446849888d5 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -124,6 +124,7 @@ void nfs4_state_shutdown_net(struct net *net);
> void nfs4_reset_lease(time_t leasetime);
> int nfs4_reset_recoverydir(char *recdir);
> char * nfs4_recoverydir(void);
> +bool nfsd4_spo_must_allow(struct svc_rqst *rqstp);
> #else
> static inline int nfsd4_init_slabs(void) { return 0; }
> static inline void nfsd4_free_slabs(void) { }
> @@ -134,6 +135,10 @@ static inline void nfs4_state_shutdown_net(struct net *net) { }
> static inline void nfs4_reset_lease(time_t leasetime) { }
> static inline int nfs4_reset_recoverydir(char *recdir) { return 0; }
> static inline char * nfs4_recoverydir(void) {return NULL; }
> +static inline bool nfsd4_spo_must_allow(struct svc_rqst *rqstp)
> +{
> + return false;
> +}
> #endif
>
> /*
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 77fdf4de91ba..2b59c74f098c 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -345,6 +345,7 @@ struct nfs4_client {
> u32 cl_exchange_flags;
> /* number of rpc's in progress over an associated session: */
> atomic_t cl_refcount;
> + struct nfs4_op_map cl_spo_must_allow;
>
> /* for nfs41 callbacks */
> /* We currently support a single back channel with a single slot */
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 25c9c79460f9..c88aca9c42d7 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -59,6 +59,7 @@ struct nfsd4_compound_state {
> struct nfsd4_session *session;
> struct nfsd4_slot *slot;
> int data_offset;
> + bool spo_must_allowed;
> size_t iovlen;
> u32 minorversion;
> __be32 status;
> @@ -403,6 +404,8 @@ struct nfsd4_exchange_id {
> clientid_t clientid;
> u32 seqid;
> int spa_how;
> + u32 spo_must_enforce[3];
> + u32 spo_must_allow[3];
> };
>
> struct nfsd4_sequence {
> --
> 2.6.3
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 3/3] nfsd: implement machine credential support for some operations
2016-01-21 19:01 ` J. Bruce Fields
@ 2016-01-21 19:30 ` Andrew W Elble
2016-01-21 19:50 ` J. Bruce Fields
0 siblings, 1 reply; 17+ messages in thread
From: Andrew W Elble @ 2016-01-21 19:30 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs, dros
> Doesn't this mean that a compound like e.g.:
>
> PUTFH
> CLOSE
> OPEN
>
> would result in a return of true on the OPEN, if CLOSE was in must_allow
> but OPEN wasn't? (Because the above loop sets spo_must_allowed as soon
> as it hits the CLOSE.)
Yes. A real-world example is DELEGRETURN with the Linux NFS client:
PUTFH
GETATTR
DELEGRETURN
GETATTR isn't in spo_must_allowed, but the whole compound request looks like
krb5i in a krb5 setting. Still digesting the rest of your replies...
Thanks,
Andy
--
Andrew W. Elble
aweits@discipline.rit.edu
Infrastructure Engineer, Communications Technical Lead
Rochester Institute of Technology
PGP: BFAD 8461 4CCF DC95 DA2C B0EB 965B 082E 863E C912
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] nfsd: implement machine credential support for some operations
2016-01-21 19:30 ` Andrew W Elble
@ 2016-01-21 19:50 ` J. Bruce Fields
2016-01-22 0:01 ` Andrew W Elble
0 siblings, 1 reply; 17+ messages in thread
From: J. Bruce Fields @ 2016-01-21 19:50 UTC (permalink / raw)
To: Andrew W Elble; +Cc: linux-nfs, dros
On Thu, Jan 21, 2016 at 02:30:42PM -0500, Andrew W Elble wrote:
>
> > Doesn't this mean that a compound like e.g.:
> >
> > PUTFH
> > CLOSE
> > OPEN
> >
> > would result in a return of true on the OPEN, if CLOSE was in must_allow
> > but OPEN wasn't? (Because the above loop sets spo_must_allowed as soon
> > as it hits the CLOSE.)
>
> Yes. A real-world example is DELEGRETURN with the Linux NFS client:
>
> PUTFH
> GETATTR
> DELEGRETURN
>
> GETATTR isn't in spo_must_allowed, but the whole compound request looks like
> krb5i in a krb5 setting. Still digesting the rest of your replies...
Ugh. So the client actually needs to allow random other ops in any
compound containing an spo_must_allow'd operation? That doesn't seem
right to me.
--b.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] nfsd: implement machine credential support for some operations
2016-01-21 19:50 ` J. Bruce Fields
@ 2016-01-22 0:01 ` Andrew W Elble
2016-01-22 15:24 ` J. Bruce Fields
0 siblings, 1 reply; 17+ messages in thread
From: Andrew W Elble @ 2016-01-22 0:01 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs, dros
> Ugh. So the client actually needs to allow random other ops in any
> compound containing an spo_must_allow'd operation? That doesn't seem
> right to me.
Well, that's most certainly my fault. Seems like I should
submit a patch to have the client ask for GETATTR if it's going to send
it as a tag-along to DELEGRETURN. Is WRONGSEC really the correct way
to enforce appropriate use of spo_must_allow here?
For instance, the client could ask for just DELEGRETURN:
PUTFH
GETATTR
DELEGRETURN
...would be successful as long as the export was done with krb5i/krb5p.
Thanks,
Andy
--
Andrew W. Elble
aweits@discipline.rit.edu
Infrastructure Engineer, Communications Technical Lead
Rochester Institute of Technology
PGP: BFAD 8461 4CCF DC95 DA2C B0EB 965B 082E 863E C912
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] nfsd: implement machine credential support for some operations
2016-01-22 0:01 ` Andrew W Elble
@ 2016-01-22 15:24 ` J. Bruce Fields
2016-01-22 16:06 ` Trond Myklebust
0 siblings, 1 reply; 17+ messages in thread
From: J. Bruce Fields @ 2016-01-22 15:24 UTC (permalink / raw)
To: Andrew W Elble; +Cc: linux-nfs, dros
On Thu, Jan 21, 2016 at 07:01:31PM -0500, Andrew W Elble wrote:
>
> > Ugh. So the client actually needs to allow random other ops in any
> > compound containing an spo_must_allow'd operation? That doesn't seem
> > right to me.
>
> Well, that's most certainly my fault. Seems like I should
> submit a patch to have the client ask for GETATTR if it's going to send
> it as a tag-along to DELEGRETURN. Is WRONGSEC really the correct way
> to enforce appropriate use of spo_must_allow here?
>
> For instance, the client could ask for just DELEGRETURN:
>
> PUTFH
> GETATTR
> DELEGRETURN
>
> ...would be successful as long as the export was done with krb5i/krb5p.
I don't know what the right thing to do is here.
I wonder what the GETATTR's for? I guess if any changes are flushed
before sending this compound then this is a good chance to get a
changeattr for a known state. For that you need the GETATTR to be
sequenced before the DELEGRETURN, so that it happens before any other
clients start writing, and the only other way to do that is to send the
GETATTR separately and wait for the response. Which would be annoying.
You could add GETATTR to must_allow. But then the GETATTR could in
theory be denied. I think that would only happen in the case of servers
that enforce ACE4_READ_ATTRIBUTES. I seem to recall seeing such at
testing events, but maybe they're rare. I guess you could handle that
rare case by resending the DELEGRETURN without the GETATTR. Also kind
of annoying.
--b.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] nfsd: implement machine credential support for some operations
2016-01-22 15:24 ` J. Bruce Fields
@ 2016-01-22 16:06 ` Trond Myklebust
0 siblings, 0 replies; 17+ messages in thread
From: Trond Myklebust @ 2016-01-22 16:06 UTC (permalink / raw)
To: J. Bruce Fields
Cc: Andrew W Elble, Linux NFS Mailing List, Weston Andros Adamson
On Fri, Jan 22, 2016 at 10:24 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Thu, Jan 21, 2016 at 07:01:31PM -0500, Andrew W Elble wrote:
>>
>> > Ugh. So the client actually needs to allow random other ops in any
>> > compound containing an spo_must_allow'd operation? That doesn't seem
>> > right to me.
>>
>> Well, that's most certainly my fault. Seems like I should
>> submit a patch to have the client ask for GETATTR if it's going to send
>> it as a tag-along to DELEGRETURN. Is WRONGSEC really the correct way
>> to enforce appropriate use of spo_must_allow here?
>>
>> For instance, the client could ask for just DELEGRETURN:
>>
>> PUTFH
>> GETATTR
>> DELEGRETURN
>>
>> ...would be successful as long as the export was done with krb5i/krb5p.
>
> I don't know what the right thing to do is here.
>
> I wonder what the GETATTR's for?
Close to open cache consistency.
Cheers
Trond
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] nfsd: implement machine credential support for some operations
2016-01-18 20:08 ` [PATCH v2 3/3] nfsd: implement machine credential support for some operations Andrew Elble
2016-01-20 22:53 ` J. Bruce Fields
2016-01-21 19:01 ` J. Bruce Fields
@ 2016-01-22 15:40 ` J. Bruce Fields
2016-01-22 16:09 ` Andrew W Elble
2 siblings, 1 reply; 17+ messages in thread
From: J. Bruce Fields @ 2016-01-22 15:40 UTC (permalink / raw)
To: Andrew Elble; +Cc: linux-nfs, dros
On Mon, Jan 18, 2016 at 03:08:22PM -0500, Andrew Elble wrote:
> Add server support for properly decoding and using spo_must_enforce
> and spo_must_allow bits. Add support for machine credentials to be
> used for CLOSE, OPEN_DOWNGRADE, LOCKU, DELEGRETURN,
> and TEST/FREE STATEID.
> Implement a check so as to not throw WRONGSEC errors when these
> operations are used if integrity/privacy isn't turned on.
By the way, is the only problem is that the client is trying to do
krb5i/krb5p on an export exported only with sec=sys or sec=krb5?
We could almost just decide to allow krb5i/krb5p in such cases, would
anyone really mind?
The only reasons I can think of that a user would object to "stronger"
security levels:
- they don't trust the more complicated krb5i/krb5p code, and
want to avoid exposing possible bugs there to malicious
clients--but clients can already send EXCHANGE_ID and other
non-filehandle-based operations with krb5i/krb5p, so stopping
this at the export level seems too late.
- perhaps they want to turn off krb5i/krb5p at the server for
performance reasons.
So we're not making the first problem any worse here. For the second
problem, as long as the sec= option is correctly enforced on some subset
of the operations, the client will negotiate down quickly.
So for example we could allow krb5i/krb5p on any compound containing an
so_must_allow op?
--b.
>
> Signed-off-by: Andrew Elble <aweits@rit.edu>
> ---
> fs/nfsd/export.c | 4 ++++
> fs/nfsd/nfs4proc.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> fs/nfsd/nfs4state.c | 18 ++++++++++++++
> fs/nfsd/nfs4xdr.c | 51 ++++++++++++++++++---------------------
> fs/nfsd/nfsd.h | 5 ++++
> fs/nfsd/state.h | 1 +
> fs/nfsd/xdr4.h | 3 +++
> 7 files changed, 123 insertions(+), 28 deletions(-)
>
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index b4d84b579f20..0395e3e8fc3e 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -954,6 +954,10 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp)
> rqstp->rq_cred.cr_flavor == RPC_AUTH_UNIX)
> return 0;
> }
> +
> + if (nfsd4_spo_must_allow(rqstp))
> + return 0;
> +
> return nfserr_wrongsec;
> }
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index a9f096c7e99f..047d6662010b 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2285,6 +2285,75 @@ static struct nfsd4_operation nfsd4_ops[] = {
> },
> };
>
> +/**
> + * nfsd4_spo_must_allow - Determine if the compound op contains an
> + * operation that is allowed to be sent with machine credentials
> + *
> + * @rqstp: a pointer to the struct svc_rqst
> + *
> + * nfsd4_spo_must_allow() allows check_nfsd_access() to succeed
> + * when the operation and/or the FH+operation(s) is part of what the
> + * client negotiated to be able to send with machine credentials.
> + * We keep some state so that FH+operation(s) can succeed despite
> + * check_nfsd_access() being called from fh_verify() as well as
> + * nfsd4_proc_compound().
> + */
> +
> +bool nfsd4_spo_must_allow(struct svc_rqst *rqstp)
> +{
> + struct nfsd4_compoundres *resp = rqstp->rq_resp;
> + struct nfsd4_compoundargs *argp = rqstp->rq_argp;
> + struct nfsd4_op *this = &argp->ops[resp->opcnt - 1];
> + struct nfsd4_compound_state *cstate = &resp->cstate;
> + struct nfsd4_operation *thisd;
> + struct nfs4_op_map *allow = &cstate->clp->cl_spo_must_allow;
> + u32 opiter;
> +
> + if (!cstate->minorversion)
> + return false;
> +
> + thisd = OPDESC(this);
> +
> + if (!(thisd->op_flags & OP_IS_PUTFH_LIKE)) {
> + if (cstate->spo_must_allowed == true)
> + /*
> + * a prior putfh + op has set
> + * spo_must_allow conditions
> + */
> + return true;
> + /* evaluate op against spo_must_allow with no prior putfh */
> + if (test_bit(this->opnum, allow->u.longs) &&
> + cstate->clp->cl_mach_cred &&
> + nfsd4_mach_creds_match(cstate->clp, rqstp))
> + return true;
> + else
> + return false;
> + }
> + /*
> + * this->opnum has PUTFH ramifications
> + * scan forward to next putfh or end of compound op
> + */
> + opiter = resp->opcnt;
> + while (opiter < argp->opcnt) {
> + this = &argp->ops[opiter++];
> + thisd = OPDESC(this);
> + if (thisd->op_flags & OP_IS_PUTFH_LIKE)
> + break;
> + if (test_bit(this->opnum, allow->u.longs) &&
> + cstate->clp->cl_mach_cred &&
> + nfsd4_mach_creds_match(cstate->clp, rqstp)) {
> + /*
> + * the op covered by the fh is a
> + * spo_must_allow operation
> + */
> + cstate->spo_must_allowed = true;
> + return true;
> + }
> + }
> + cstate->spo_must_allowed = false;
> + return false;
> +}
> +
> int nfsd4_max_reply(struct svc_rqst *rqstp, struct nfsd4_op *op)
> {
> struct nfsd4_operation *opdesc;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 65efc900e97e..b28805519725 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2367,6 +2367,22 @@ nfsd4_exchange_id(struct svc_rqst *rqstp,
>
> switch (exid->spa_how) {
> case SP4_MACH_CRED:
> + exid->spo_must_enforce[0] = 0;
> + exid->spo_must_enforce[1] = (
> + 1 << (OP_BIND_CONN_TO_SESSION - 32) |
> + 1 << (OP_EXCHANGE_ID - 32) |
> + 1 << (OP_CREATE_SESSION - 32) |
> + 1 << (OP_DESTROY_SESSION - 32) |
> + 1 << (OP_DESTROY_CLIENTID - 32));
> +
> + exid->spo_must_allow[0] &= (1 << (OP_CLOSE) |
> + 1 << (OP_OPEN_DOWNGRADE) |
> + 1 << (OP_LOCKU) |
> + 1 << (OP_DELEGRETURN));
> +
> + exid->spo_must_allow[1] &= (
> + 1 << (OP_TEST_STATEID - 32) |
> + 1 << (OP_FREE_STATEID - 32));
> if (!svc_rqst_integrity_protected(rqstp))
> return nfserr_inval;
> case SP4_NONE:
> @@ -2443,6 +2459,8 @@ out_new:
> }
> new->cl_minorversion = cstate->minorversion;
> new->cl_mach_cred = (exid->spa_how == SP4_MACH_CRED);
> + new->cl_spo_must_allow.u.words[0] = exid->spo_must_allow[0];
> + new->cl_spo_must_allow.u.words[1] = exid->spo_must_allow[1];
>
> gen_clid(new, nn);
> add_to_unconfirmed(new);
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 51c9e9ca39a4..e2043aa95e27 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1297,16 +1297,14 @@ nfsd4_decode_exchange_id(struct nfsd4_compoundargs *argp,
> break;
> case SP4_MACH_CRED:
> /* spo_must_enforce */
> - READ_BUF(4);
> - dummy = be32_to_cpup(p++);
> - READ_BUF(dummy * 4);
> - p += dummy;
> -
> + status = nfsd4_decode_bitmap(argp,
> + exid->spo_must_enforce);
> + if (status)
> + goto out;
> /* spo_must_allow */
> - READ_BUF(4);
> - dummy = be32_to_cpup(p++);
> - READ_BUF(dummy * 4);
> - p += dummy;
> + status = nfsd4_decode_bitmap(argp, exid->spo_must_allow);
> + if (status)
> + goto out;
> break;
> case SP4_SSV:
> /* ssp_ops */
> @@ -3841,14 +3839,6 @@ nfsd4_encode_write(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_w
> return nfserr;
> }
>
> -static const u32 nfs4_minimal_spo_must_enforce[2] = {
> - [1] = 1 << (OP_BIND_CONN_TO_SESSION - 32) |
> - 1 << (OP_EXCHANGE_ID - 32) |
> - 1 << (OP_CREATE_SESSION - 32) |
> - 1 << (OP_DESTROY_SESSION - 32) |
> - 1 << (OP_DESTROY_CLIENTID - 32)
> -};
> -
> static __be32
> nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
> struct nfsd4_exchange_id *exid)
> @@ -3859,6 +3849,7 @@ nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
> char *server_scope;
> int major_id_sz;
> int server_scope_sz;
> + int status = 0;
> uint64_t minor_id = 0;
>
> if (nfserr)
> @@ -3887,18 +3878,20 @@ nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
> case SP4_NONE:
> break;
> case SP4_MACH_CRED:
> - /* spo_must_enforce, spo_must_allow */
> - p = xdr_reserve_space(xdr, 16);
> - if (!p)
> - return nfserr_resource;
> -
> /* spo_must_enforce bitmap: */
> - *p++ = cpu_to_be32(2);
> - *p++ = cpu_to_be32(nfs4_minimal_spo_must_enforce[0]);
> - *p++ = cpu_to_be32(nfs4_minimal_spo_must_enforce[1]);
> - /* empty spo_must_allow bitmap: */
> - *p++ = cpu_to_be32(0);
> -
> + status = nfsd4_encode_bitmap(xdr,
> + exid->spo_must_enforce[0],
> + exid->spo_must_enforce[1],
> + exid->spo_must_enforce[2]);
> + if (status)
> + goto out;
> + /* spo_must_allow bitmap: */
> + status = nfsd4_encode_bitmap(xdr,
> + exid->spo_must_allow[0],
> + exid->spo_must_allow[1],
> + exid->spo_must_allow[2]);
> + if (status)
> + goto out;
> break;
> default:
> WARN_ON_ONCE(1);
> @@ -3925,6 +3918,8 @@ nfsd4_encode_exchange_id(struct nfsd4_compoundres *resp, __be32 nfserr,
> /* Implementation id */
> *p++ = cpu_to_be32(0); /* zero length nfs_impl_id4 array */
> return 0;
> +out:
> + return status;
> }
>
> static __be32
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index cf980523898b..9446849888d5 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -124,6 +124,7 @@ void nfs4_state_shutdown_net(struct net *net);
> void nfs4_reset_lease(time_t leasetime);
> int nfs4_reset_recoverydir(char *recdir);
> char * nfs4_recoverydir(void);
> +bool nfsd4_spo_must_allow(struct svc_rqst *rqstp);
> #else
> static inline int nfsd4_init_slabs(void) { return 0; }
> static inline void nfsd4_free_slabs(void) { }
> @@ -134,6 +135,10 @@ static inline void nfs4_state_shutdown_net(struct net *net) { }
> static inline void nfs4_reset_lease(time_t leasetime) { }
> static inline int nfs4_reset_recoverydir(char *recdir) { return 0; }
> static inline char * nfs4_recoverydir(void) {return NULL; }
> +static inline bool nfsd4_spo_must_allow(struct svc_rqst *rqstp)
> +{
> + return false;
> +}
> #endif
>
> /*
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 77fdf4de91ba..2b59c74f098c 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -345,6 +345,7 @@ struct nfs4_client {
> u32 cl_exchange_flags;
> /* number of rpc's in progress over an associated session: */
> atomic_t cl_refcount;
> + struct nfs4_op_map cl_spo_must_allow;
>
> /* for nfs41 callbacks */
> /* We currently support a single back channel with a single slot */
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 25c9c79460f9..c88aca9c42d7 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -59,6 +59,7 @@ struct nfsd4_compound_state {
> struct nfsd4_session *session;
> struct nfsd4_slot *slot;
> int data_offset;
> + bool spo_must_allowed;
> size_t iovlen;
> u32 minorversion;
> __be32 status;
> @@ -403,6 +404,8 @@ struct nfsd4_exchange_id {
> clientid_t clientid;
> u32 seqid;
> int spa_how;
> + u32 spo_must_enforce[3];
> + u32 spo_must_allow[3];
> };
>
> struct nfsd4_sequence {
> --
> 2.6.3
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 3/3] nfsd: implement machine credential support for some operations
2016-01-22 15:40 ` J. Bruce Fields
@ 2016-01-22 16:09 ` Andrew W Elble
2016-01-22 16:36 ` J. Bruce Fields
0 siblings, 1 reply; 17+ messages in thread
From: Andrew W Elble @ 2016-01-22 16:09 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs, dros
> By the way, is the only problem is that the client is trying to do
> krb5i/krb5p on an export exported only with sec=sys or sec=krb5?
Barring anything else I missed, yes.
> So for example we could allow krb5i/krb5p on any compound containing an
> so_must_allow op?
This was roughly my reasoning/question...
Thanks,
Andy
--
Andrew W. Elble
aweits@discipline.rit.edu
Infrastructure Engineer, Communications Technical Lead
Rochester Institute of Technology
PGP: BFAD 8461 4CCF DC95 DA2C B0EB 965B 082E 863E C912
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] nfsd: implement machine credential support for some operations
2016-01-22 16:09 ` Andrew W Elble
@ 2016-01-22 16:36 ` J. Bruce Fields
0 siblings, 0 replies; 17+ messages in thread
From: J. Bruce Fields @ 2016-01-22 16:36 UTC (permalink / raw)
To: Andrew W Elble; +Cc: linux-nfs, dros
On Fri, Jan 22, 2016 at 11:09:15AM -0500, Andrew W Elble wrote:
>
> > By the way, is the only problem is that the client is trying to do
> > krb5i/krb5p on an export exported only with sec=sys or sec=krb5?
>
> Barring anything else I missed, yes.
>
> > So for example we could allow krb5i/krb5p on any compound containing an
> > so_must_allow op?
>
> This was roughly my reasoning/question...
Right, so I guess I've convinced myself to stop worrying as much about
whether your nfsd4_spo_must_allow allows too much.
In fact I wonder if it'd be simpler just to skip the OP_IS_PUTFH_LIKE
checks and just set spo_must_allowed on any compound with any must_allow
op in it.
At worst we've allowed use of krb5p/krb5i for a few ops on filesystems
that don't allow those, but who cares.
It doesn't bypass filesystem permission checks on operations that do
permission checks, and you still might consider removing that fh_verify
from DELEGRETURN in a separate patch. And the client may still have
some trouble with filesystems that do permission checks on GETATTR.
--b.
^ permalink raw reply [flat|nested] 17+ messages in thread