[13/14] NFSD: Server implementation of MAC Labeling

Commit Message

Steve Dickson March 28, 2013, 1:54 p.m. UTC

From: David Quigley <[e-mail]>

This patch adds the ability to encode and decode file labels on the server for
the purpose of sending them to the client and also to process label change
requests from the client.

Signed-off-by: Matthew N. Dodd <[e-mail]>
Signed-off-by: Miguel Rodel Felipe <[e-mail]>
Signed-off-by: Phua Eu Gene <[e-mail]>
Signed-off-by: Khin Mi Mi Aung <[e-mail]>
---
 fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++
 fs/nfsd/nfs4xdr.c  | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 fs/nfsd/nfsd.h     |   6 ++-
 fs/nfsd/vfs.c      |  29 ++++++++++++++
 fs/nfsd/vfs.h      |   2 +
 fs/nfsd/xdr4.h     |   3 ++
 6 files changed, 191 insertions(+), 6 deletions(-)

Comments

J. Bruce Fields March 28, 2013, 4:10 p.m. UTC | #1

On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
> @@ -888,6 +924,11 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  					    setattr->sa_acl);
>  	if (status)
>  		goto out;
> +	if (setattr->sa_label != NULL)
> +		status = nfsd4_set_nfs4_label(rqstp, &cstate->current_fh,
> +				setattr->sa_label);

I'm getting an error from setattr over NFS4.0, even when I don't have
nfs4_label support compiled in, I'm not sure why....  Maybe sa_label
isn't being initialized to NULL in that case?

(Reproduceable with pynfs 4.0 SATT13.)

...
> index 2b2e239..b2b2e5a 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -28,6 +28,7 @@
>  #include <asm/uaccess.h>
>  #include <linux/exportfs.h>
>  #include <linux/writeback.h>
> +#include <linux/security.h>
>  
>  #ifdef CONFIG_NFSD_V3
>  #include "xdr3.h"
> @@ -621,6 +622,34 @@ int nfsd4_is_junction(struct dentry *dentry)
>  		return 0;
>  	return 1;
>  }
> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> +__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
> +		struct nfs4_label *label)
> +{
> +	__be32 error;
> +	int host_error;
> +	struct dentry *dentry;
> +
> +	/* XXX: should we have a MAY_SSECCTX? */
> +	error = fh_verify(rqstp, fhp, 0 /* S_IFREG */, NFSD_MAY_SATTR);
> +	if (error)
> +		return error;
> +
> +	dentry = fhp->fh_dentry;
> +
> +	mutex_lock(&dentry->d_inode->i_mutex);
> +	host_error = security_inode_setsecctx(dentry, label->label, label->len);
> +	mutex_unlock(&dentry->d_inode->i_mutex);
> +	return nfserrno(host_error);
> +}
> +#else
> +__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
> +		struct nfs4_label *label)
> +{
> +	return -EOPNOTSUPP;

That's returning a kernel error number to an nfs client.  You want
something else, maybe nfserr_attrnotsupp?

But this actually should have been caught earlier by
check_attr_support().

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

J. Bruce Fields March 28, 2013, 4:14 p.m. UTC | #2

On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
> From: David Quigley <[e-mail]>
> 
> This patch adds the ability to encode and decode file labels on the server for
> the purpose of sending them to the client and also to process label change
> requests from the client.
> 
> Signed-off-by: Matthew N. Dodd <[e-mail]>
> Signed-off-by: Miguel Rodel Felipe <[e-mail]>
> Signed-off-by: Phua Eu Gene <[e-mail]>
> Signed-off-by: Khin Mi Mi Aung <[e-mail]>
> ---
>  fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++
>  fs/nfsd/nfs4xdr.c  | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  fs/nfsd/nfsd.h     |   6 ++-
>  fs/nfsd/vfs.c      |  29 ++++++++++++++
>  fs/nfsd/vfs.h      |   2 +
>  fs/nfsd/xdr4.h     |   3 ++
>  6 files changed, 191 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index ae73175..bb17589 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -42,6 +42,36 @@
>  #include "current_stateid.h"
>  #include "netns.h"
>  
> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> +#include <linux/security.h>
> +
> +static inline void
> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval)
> +{
> +	struct inode *inode = resfh->fh_dentry->d_inode;
> +	int status;
> +
> +	mutex_lock(&inode->i_mutex);
> +	status = security_inode_setsecctx(resfh->fh_dentry,
> +		label->label, label->len);
> +	mutex_unlock(&inode->i_mutex);
> +
> +	if (status)
> +		/*
> +		 * We should probably fail the whole open at this point,
> +		 * but we've already created or opened the file, so it's 
> +		 * too late; So this seems the least of evils:
> +		 */
> +		bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;

Is there any way to avoid this?

How does the vfs open code handle this?  It has to be able to set a
security contexts atomically on open(O_CREAT), doesn't it?

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

J. Bruce Fields March 28, 2013, 6:58 p.m. UTC | #3

On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
> From: David Quigley <[e-mail]>
> 
> This patch adds the ability to encode and decode file labels on the server for
> the purpose of sending them to the client and also to process label change
> requests from the client.
> 
> Signed-off-by: Matthew N. Dodd <[e-mail]>
> Signed-off-by: Miguel Rodel Felipe <[e-mail]>
> Signed-off-by: Phua Eu Gene <[e-mail]>
> Signed-off-by: Khin Mi Mi Aung <[e-mail]>
> ---
>  fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++
>  fs/nfsd/nfs4xdr.c  | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  fs/nfsd/nfsd.h     |   6 ++-
>  fs/nfsd/vfs.c      |  29 ++++++++++++++
>  fs/nfsd/vfs.h      |   2 +
>  fs/nfsd/xdr4.h     |   3 ++
>  6 files changed, 191 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index ae73175..bb17589 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -42,6 +42,36 @@
>  #include "current_stateid.h"
>  #include "netns.h"
>  
> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> +#include <linux/security.h>
> +
> +static inline void
> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval)
> +{
> +	struct inode *inode = resfh->fh_dentry->d_inode;
> +	int status;
> +
> +	mutex_lock(&inode->i_mutex);
> +	status = security_inode_setsecctx(resfh->fh_dentry,
> +		label->label, label->len);
> +	mutex_unlock(&inode->i_mutex);
> +
> +	if (status)
> +		/*
> +		 * We should probably fail the whole open at this point,
> +		 * but we've already created or opened the file, so it's 
> +		 * too late; So this seems the least of evils:
> +		 */
> +		bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> +		
> +	return;
> +}
> +#else 
> +static inline void 
> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval)
> +{ }
> +#endif
> +
>  #define NFSDDBG_FACILITY		NFSDDBG_PROC
>  
>  static u32 nfsd_attrmask[] = {
> @@ -230,6 +260,9 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o
>  					(u32 *)open->op_verf.data,
>  					&open->op_truncate, &open->op_created);
>  
> +		if (!status && open->op_label != NULL) 
> +			nfsd4_security_inode_setsecctx(resfh, open->op_label, open->op_bmval);
> +
>  		/*
>  		 * Following rfc 3530 14.2.16, use the returned bitmask
>  		 * to indicate which attributes we used to store the
> @@ -599,6 +632,9 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	if (status)
>  		goto out;
>  
> +	if (create->cr_label != NULL)
> +		nfsd4_security_inode_setsecctx(&resfh, create->cr_label, create->cr_bmval);
> +
>  	if (create->cr_acl != NULL)
>  		do_set_nfs4_acl(rqstp, &resfh, create->cr_acl,
>  				create->cr_bmval);
> @@ -888,6 +924,11 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  					    setattr->sa_acl);
>  	if (status)
>  		goto out;
> +	if (setattr->sa_label != NULL)
> +		status = nfsd4_set_nfs4_label(rqstp, &cstate->current_fh,
> +				setattr->sa_label);
> +	if (status)
> +		goto out;
>  	status = nfsd_setattr(rqstp, &cstate->current_fh, &setattr->sa_iattr,
>  				0, (time_t)0);
>  out:
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 0116886..52e219c 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -55,6 +55,11 @@
>  #include "cache.h"
>  #include "netns.h"
>  
> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> +#include <linux/security.h>
> +#endif
> +
> +
>  #define NFSDDBG_FACILITY		NFSDDBG_XDR
>  
>  /*
> @@ -79,6 +84,24 @@ check_filename(char *str, int len)
>  			return nfserr_badname;
>  	return 0;
>  } 
> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> +static struct nfs4_label *nfsd4_label_alloc(u32 len)
> +{
> +	struct nfs4_label *label = NULL;
> +
> +	if (len > NFS4_MAXLABELLEN)
> +		return ERR_PTR(-ENAMETOOLONG);

This is returned as NFS4ERR_NAMETOOLONG.

The 4.2 spec should note this case, if it's the right error.  rfc 5661
says that error's only for filenames, and doesn't allow it for setattr
(which doesn't take a filename).

> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> +	if (bmval[2] & FATTR4_WORD2_SECURITY_LABEL) {
> +		uint32_t pi;
> +		uint32_t lfs;
> +
> +		READ_BUF(4);
> +		len += 4;
> +		READ32(lfs);
> +		READ_BUF(4);
> +		len += 4;
> +		READ32(pi);
> +		READ_BUF(4);
> +		len += 4;
> +		READ32(dummy32);
> +		READ_BUF(dummy32);
> +		len += (XDR_QUADLEN(dummy32) << 2);
> +		READMEM(buf, dummy32);
> +
> +		*label = nfsd4_label_alloc(dummy32);
> +		if (*label == NULL) {

Careful!  That should be IS_ERR(*label).

> +			host_err = PTR_ERR(label);

And that should be *label.

>  static __be32
> @@ -1988,6 +2044,50 @@ nfsd4_encode_aclname(struct svc_rqst *rqstp, struct nfs4_ace *ace,
>  			      FATTR4_WORD0_RDATTR_ERROR)
>  #define WORD1_ABSENT_FS_ATTRS FATTR4_WORD1_MOUNTED_ON_FILEID
>  
> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> +static inline __be32
> +nfsd4_encode_security_label(struct svc_rqst *rqstp, struct dentry *dentry, __be32 **pp, int *buflen)
> +{
> +	void *context;
> +	int err;
> +	int len;
> +	uint32_t pi = 0;
> +	uint32_t lfs = 0;
> +	__be32 *p = *pp;
> +
> +	err = 0;
> +	(void)security_inode_getsecctx(dentry->d_inode, &context, &len);
> +	if (len < 0)
> +		return nfserrno(len);
> +
> +	if (*buflen < ((XDR_QUADLEN(len) << 2) + 4 + 4 + 4)) {
> +		err = nfserr_resource;
> +		goto out;
> +	}
> +
> +	/* XXX: A call to the translation code should be placed here
> +	 * for now send 0  until we have that to indicate the null
> +	 * translation */ 

Could we better a better comment here?

> +
> +	if ((*buflen -= 4) < 0)

Looks like that should be 8.

> +		return nfserr_resource;
> +
> +	WRITE32(lfs);	
> +	WRITE32(pi);
> +	p = xdr_encode_opaque(p, context, len);
> +	*buflen -= (XDR_QUADLEN(len) << 2) + 4;
> +
> +	*pp = p;
> +out:
> +	security_release_secctx(context, len);
> +	return err;
> +}

...
> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> +__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
> +		struct nfs4_label *label)
> +{
> +	__be32 error;
> +	int host_error;
> +	struct dentry *dentry;
> +
> +	/* XXX: should we have a MAY_SSECCTX? */

Again: could we get an answer to this question?

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

J. Bruce Fields March 28, 2013, 7:19 p.m. UTC | #4

Dave, I think these are both questions for you?

--b.

On Thu, Mar 28, 2013 at 02:58:45PM -0400, bfields wrote:
> On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
> > @@ -1988,6 +2044,50 @@ nfsd4_encode_aclname(struct svc_rqst *rqstp, struct nfs4_ace *ace,
> >  			      FATTR4_WORD0_RDATTR_ERROR)
> >  #define WORD1_ABSENT_FS_ATTRS FATTR4_WORD1_MOUNTED_ON_FILEID
> >  
> > +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> > +static inline __be32
> > +nfsd4_encode_security_label(struct svc_rqst *rqstp, struct dentry *dentry, __be32 **pp, int *buflen)
> > +{
> > +	void *context;
> > +	int err;
> > +	int len;
> > +	uint32_t pi = 0;
> > +	uint32_t lfs = 0;
> > +	__be32 *p = *pp;
> > +
> > +	err = 0;
> > +	(void)security_inode_getsecctx(dentry->d_inode, &context, &len);
> > +	if (len < 0)
> > +		return nfserrno(len);
> > +
> > +	if (*buflen < ((XDR_QUADLEN(len) << 2) + 4 + 4 + 4)) {
> > +		err = nfserr_resource;
> > +		goto out;
> > +	}
> > +
> > +	/* XXX: A call to the translation code should be placed here
> > +	 * for now send 0  until we have that to indicate the null
> > +	 * translation */ 
> 
> Could we better a better comment here?
...
> > +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> > +__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > +		struct nfs4_label *label)
> > +{
> > +	__be32 error;
> > +	int host_error;
> > +	struct dentry *dentry;
> > +
> > +	/* XXX: should we have a MAY_SSECCTX? */
> 
> Again: could we get an answer to this question?
> 
> --b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Quigley March 29, 2013, 3:32 a.m. UTC | #5

On 3/28/2013 3:19 PM, J. Bruce Fields wrote:
> Dave, I think these are both questions for you?
>
> --b.
>
> On Thu, Mar 28, 2013 at 02:58:45PM -0400, bfields wrote:
>> On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
>>> @@ -1988,6 +2044,50 @@ nfsd4_encode_aclname(struct svc_rqst *rqstp, struct nfs4_ace *ace,
>>>   			      FATTR4_WORD0_RDATTR_ERROR)
>>>   #define WORD1_ABSENT_FS_ATTRS FATTR4_WORD1_MOUNTED_ON_FILEID
>>>
>>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>>> +static inline __be32
>>> +nfsd4_encode_security_label(struct svc_rqst *rqstp, struct dentry *dentry, __be32 **pp, int *buflen)
>>> +{
>>> +	void *context;
>>> +	int err;
>>> +	int len;
>>> +	uint32_t pi = 0;
>>> +	uint32_t lfs = 0;
>>> +	__be32 *p = *pp;
>>> +
>>> +	err = 0;
>>> +	(void)security_inode_getsecctx(dentry->d_inode, &context, &len);
>>> +	if (len < 0)
>>> +		return nfserrno(len);
>>> +
>>> +	if (*buflen < ((XDR_QUADLEN(len) << 2) + 4 + 4 + 4)) {
>>> +		err = nfserr_resource;
>>> +		goto out;
>>> +	}
>>> +
>>> +	/* XXX: A call to the translation code should be placed here
>>> +	 * for now send 0  until we have that to indicate the null
>>> +	 * translation */
>>
>> Could we better a better comment here?

We could remove this comment all together if you want. We're trying to 
indicate that at some point in the future there will be a label 
translation facility like idmapd but for labels instead. This is where 
the call would go if we had it. I had a basic version of the code a long 
time ago and that is where I had the call but there was way too much 
work to make it usable for now. Instead I just pass the null translation 
indicator on the wire and we wave our hands and say don't look at the 
man behind the curtain.

> ...
>>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>>> +__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>> +		struct nfs4_label *label)
>>> +{
>>> +	__be32 error;
>>> +	int host_error;
>>> +	struct dentry *dentry;
>>> +
>>> +	/* XXX: should we have a MAY_SSECCTX? */
>>
>> Again: could we get an answer to this question?

I'm pretty sure we can just remove that comment. It was a question from 
about 5 years ago now and I never found a compelling reason to say yes 
to it.

>>
>> --b.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Quigley March 29, 2013, 3:35 a.m. UTC | #6

On 3/28/2013 12:14 PM, J. Bruce Fields wrote:
> On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
>> From: David Quigley <[e-mail]>
>>
>> This patch adds the ability to encode and decode file labels on the server for
>> the purpose of sending them to the client and also to process label change
>> requests from the client.
>>
>> Signed-off-by: Matthew N. Dodd <[e-mail]>
>> Signed-off-by: Miguel Rodel Felipe <[e-mail]>
>> Signed-off-by: Phua Eu Gene <[e-mail]>
>> Signed-off-by: Khin Mi Mi Aung <[e-mail]>
>> ---
>>   fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++
>>   fs/nfsd/nfs4xdr.c  | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>   fs/nfsd/nfsd.h     |   6 ++-
>>   fs/nfsd/vfs.c      |  29 ++++++++++++++
>>   fs/nfsd/vfs.h      |   2 +
>>   fs/nfsd/xdr4.h     |   3 ++
>>   6 files changed, 191 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index ae73175..bb17589 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -42,6 +42,36 @@
>>   #include "current_stateid.h"
>>   #include "netns.h"
>>
>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>> +#include <linux/security.h>
>> +
>> +static inline void
>> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval)
>> +{
>> +	struct inode *inode = resfh->fh_dentry->d_inode;
>> +	int status;
>> +
>> +	mutex_lock(&inode->i_mutex);
>> +	status = security_inode_setsecctx(resfh->fh_dentry,
>> +		label->label, label->len);
>> +	mutex_unlock(&inode->i_mutex);
>> +
>> +	if (status)
>> +		/*
>> +		 * We should probably fail the whole open at this point,
>> +		 * but we've already created or opened the file, so it's
>> +		 * too late; So this seems the least of evils:
>> +		 */
>> +		bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
>
> Is there any way to avoid this?
>
> How does the vfs open code handle this?  It has to be able to set a
> security contexts atomically on open(O_CREAT), doesn't it?
>

I believe the way this is handled is that the inode is created and 
labeled and then only after that is it bound to the namespace. Because 
of that ordering we can fail and release the inode without it ever 
having a dentry in the namespace.


> --b.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

J. Bruce Fields March 29, 2013, 2:23 p.m. UTC | #7

On Thu, Mar 28, 2013 at 11:32:55PM -0400, Dave Quigley wrote:
> On 3/28/2013 3:19 PM, J. Bruce Fields wrote:
> >Dave, I think these are both questions for you?
> >
> >--b.
> >
> >On Thu, Mar 28, 2013 at 02:58:45PM -0400, bfields wrote:
> >>On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
> >>>@@ -1988,6 +2044,50 @@ nfsd4_encode_aclname(struct svc_rqst *rqstp, struct nfs4_ace *ace,
> >>>  			      FATTR4_WORD0_RDATTR_ERROR)
> >>>  #define WORD1_ABSENT_FS_ATTRS FATTR4_WORD1_MOUNTED_ON_FILEID
> >>>
> >>>+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> >>>+static inline __be32
> >>>+nfsd4_encode_security_label(struct svc_rqst *rqstp, struct dentry *dentry, __be32 **pp, int *buflen)
> >>>+{
> >>>+	void *context;
> >>>+	int err;
> >>>+	int len;
> >>>+	uint32_t pi = 0;
> >>>+	uint32_t lfs = 0;
> >>>+	__be32 *p = *pp;
> >>>+
> >>>+	err = 0;
> >>>+	(void)security_inode_getsecctx(dentry->d_inode, &context, &len);
> >>>+	if (len < 0)
> >>>+		return nfserrno(len);
> >>>+
> >>>+	if (*buflen < ((XDR_QUADLEN(len) << 2) + 4 + 4 + 4)) {
> >>>+		err = nfserr_resource;
> >>>+		goto out;
> >>>+	}
> >>>+
> >>>+	/* XXX: A call to the translation code should be placed here
> >>>+	 * for now send 0  until we have that to indicate the null
> >>>+	 * translation */
> >>
> >>Could we better a better comment here?
> 
> We could remove this comment all together if you want. We're trying
> to indicate that at some point in the future there will be a label
> translation facility like idmapd but for labels instead. This is
> where the call would go if we had it. I had a basic version of the
> code a long time ago and that is where I had the call but there was
> way too much work to make it usable for now. Instead I just pass the
> null translation indicator on the wire and we wave our hands and say
> don't look at the man behind the curtain.

So how about just:

	/*
	 * For now we use a 0 here to indicate the null translation; in
	 * the future we may place a call to translation code here.
	 */

> 
> >...
> >>>+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> >>>+__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >>>+		struct nfs4_label *label)
> >>>+{
> >>>+	__be32 error;
> >>>+	int host_error;
> >>>+	struct dentry *dentry;
> >>>+
> >>>+	/* XXX: should we have a MAY_SSECCTX? */
> >>
> >>Again: could we get an answer to this question?
> 
> I'm pretty sure we can just remove that comment. It was a question
> from about 5 years ago now and I never found a compelling reason to
> say yes to it.

OK, let's remove the comment.

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Quigley March 29, 2013, 2:38 p.m. UTC | #8

On 03/29/2013 10:23, J. Bruce Fields wrote:
> On Thu, Mar 28, 2013 at 11:32:55PM -0400, Dave Quigley wrote:
>> On 3/28/2013 3:19 PM, J. Bruce Fields wrote:
>> >Dave, I think these are both questions for you?
>> >
>> >--b.
>> >
>> >On Thu, Mar 28, 2013 at 02:58:45PM -0400, bfields wrote:
>> >>On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
>> >>>@@ -1988,6 +2044,50 @@ nfsd4_encode_aclname(struct svc_rqst 
>> *rqstp, struct nfs4_ace *ace,
>> >>>  			      FATTR4_WORD0_RDATTR_ERROR)
>> >>>  #define WORD1_ABSENT_FS_ATTRS FATTR4_WORD1_MOUNTED_ON_FILEID
>> >>>
>> >>>+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>> >>>+static inline __be32
>> >>>+nfsd4_encode_security_label(struct svc_rqst *rqstp, struct 
>> dentry *dentry, __be32 **pp, int *buflen)
>> >>>+{
>> >>>+	void *context;
>> >>>+	int err;
>> >>>+	int len;
>> >>>+	uint32_t pi = 0;
>> >>>+	uint32_t lfs = 0;
>> >>>+	__be32 *p = *pp;
>> >>>+
>> >>>+	err = 0;
>> >>>+	(void)security_inode_getsecctx(dentry->d_inode, &context, 
>> &len);
>> >>>+	if (len < 0)
>> >>>+		return nfserrno(len);
>> >>>+
>> >>>+	if (*buflen < ((XDR_QUADLEN(len) << 2) + 4 + 4 + 4)) {
>> >>>+		err = nfserr_resource;
>> >>>+		goto out;
>> >>>+	}
>> >>>+
>> >>>+	/* XXX: A call to the translation code should be placed here
>> >>>+	 * for now send 0  until we have that to indicate the null
>> >>>+	 * translation */
>> >>
>> >>Could we better a better comment here?
>>
>> We could remove this comment all together if you want. We're trying
>> to indicate that at some point in the future there will be a label
>> translation facility like idmapd but for labels instead. This is
>> where the call would go if we had it. I had a basic version of the
>> code a long time ago and that is where I had the call but there was
>> way too much work to make it usable for now. Instead I just pass the
>> null translation indicator on the wire and we wave our hands and say
>> don't look at the man behind the curtain.
>
> So how about just:
>
> 	/*
> 	 * For now we use a 0 here to indicate the null translation; in
> 	 * the future we may place a call to translation code here.
> 	 */
>

Works for me.

>>
>> >...
>> >>>+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>> >>>+__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct 
>> svc_fh *fhp,
>> >>>+		struct nfs4_label *label)
>> >>>+{
>> >>>+	__be32 error;
>> >>>+	int host_error;
>> >>>+	struct dentry *dentry;
>> >>>+
>> >>>+	/* XXX: should we have a MAY_SSECCTX? */
>> >>
>> >>Again: could we get an answer to this question?
>>
>> I'm pretty sure we can just remove that comment. It was a question
>> from about 5 years ago now and I never found a compelling reason to
>> say yes to it.
>
> OK, let's remove the comment.
>

Agreed
> --b.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

J. Bruce Fields March 29, 2013, 2:40 p.m. UTC | #9

On Thu, Mar 28, 2013 at 11:35:31PM -0400, Dave Quigley wrote:
> On 3/28/2013 12:14 PM, J. Bruce Fields wrote:
> >On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
> >>From: David Quigley <[e-mail]>
> >>
> >>This patch adds the ability to encode and decode file labels on the server for
> >>the purpose of sending them to the client and also to process label change
> >>requests from the client.
> >>
> >>Signed-off-by: Matthew N. Dodd <[e-mail]>
> >>Signed-off-by: Miguel Rodel Felipe <[e-mail]>
> >>Signed-off-by: Phua Eu Gene <[e-mail]>
> >>Signed-off-by: Khin Mi Mi Aung <[e-mail]>
> >>---
> >>  fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++
> >>  fs/nfsd/nfs4xdr.c  | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> >>  fs/nfsd/nfsd.h     |   6 ++-
> >>  fs/nfsd/vfs.c      |  29 ++++++++++++++
> >>  fs/nfsd/vfs.h      |   2 +
> >>  fs/nfsd/xdr4.h     |   3 ++
> >>  6 files changed, 191 insertions(+), 6 deletions(-)
> >>
> >>diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >>index ae73175..bb17589 100644
> >>--- a/fs/nfsd/nfs4proc.c
> >>+++ b/fs/nfsd/nfs4proc.c
> >>@@ -42,6 +42,36 @@
> >>  #include "current_stateid.h"
> >>  #include "netns.h"
> >>
> >>+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> >>+#include <linux/security.h>
> >>+
> >>+static inline void
> >>+nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval)
> >>+{
> >>+	struct inode *inode = resfh->fh_dentry->d_inode;
> >>+	int status;
> >>+
> >>+	mutex_lock(&inode->i_mutex);
> >>+	status = security_inode_setsecctx(resfh->fh_dentry,
> >>+		label->label, label->len);
> >>+	mutex_unlock(&inode->i_mutex);
> >>+
> >>+	if (status)
> >>+		/*
> >>+		 * We should probably fail the whole open at this point,
> >>+		 * but we've already created or opened the file, so it's
> >>+		 * too late; So this seems the least of evils:
> >>+		 */
> >>+		bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> >
> >Is there any way to avoid this?
> >
> >How does the vfs open code handle this?  It has to be able to set a
> >security contexts atomically on open(O_CREAT), doesn't it?
> >
> 
> I believe the way this is handled is that the inode is created and
> labeled and then only after that is it bound to the namespace.
> Because of that ordering we can fail and release the inode without
> it ever having a dentry in the namespace.

Grepping around....  Looks like that's done by
security_inode_init_security(), from the filesystem's create method.

So we'd need to be able to pass something down to there.

Is the current client actually expected to use this?  (So are we going
to see a lot of opens that set the label?)

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Quigley March 29, 2013, 2:49 p.m. UTC | #10

On 03/29/2013 10:40, J. Bruce Fields wrote:
> On Thu, Mar 28, 2013 at 11:35:31PM -0400, Dave Quigley wrote:
>> On 3/28/2013 12:14 PM, J. Bruce Fields wrote:
>> >On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
>> >>From: David Quigley <[e-mail]>
>> >>
>> >>This patch adds the ability to encode and decode file labels on 
>> the server for
>> >>the purpose of sending them to the client and also to process 
>> label change
>> >>requests from the client.
>> >>
>> >>Signed-off-by: Matthew N. Dodd <[e-mail]>
>> >>Signed-off-by: Miguel Rodel Felipe <[e-mail]>
>> >>Signed-off-by: Phua Eu Gene <[e-mail]>
>> >>Signed-off-by: Khin Mi Mi Aung <[e-mail]>
>> >>---
>> >>  fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++
>> >>  fs/nfsd/nfs4xdr.c  | 116 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++---
>> >>  fs/nfsd/nfsd.h     |   6 ++-
>> >>  fs/nfsd/vfs.c      |  29 ++++++++++++++
>> >>  fs/nfsd/vfs.h      |   2 +
>> >>  fs/nfsd/xdr4.h     |   3 ++
>> >>  6 files changed, 191 insertions(+), 6 deletions(-)
>> >>
>> >>diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> >>index ae73175..bb17589 100644
>> >>--- a/fs/nfsd/nfs4proc.c
>> >>+++ b/fs/nfsd/nfs4proc.c
>> >>@@ -42,6 +42,36 @@
>> >>  #include "current_stateid.h"
>> >>  #include "netns.h"
>> >>
>> >>+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>> >>+#include <linux/security.h>
>> >>+
>> >>+static inline void
>> >>+nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct 
>> nfs4_label *label, u32 *bmval)
>> >>+{
>> >>+	struct inode *inode = resfh->fh_dentry->d_inode;
>> >>+	int status;
>> >>+
>> >>+	mutex_lock(&inode->i_mutex);
>> >>+	status = security_inode_setsecctx(resfh->fh_dentry,
>> >>+		label->label, label->len);
>> >>+	mutex_unlock(&inode->i_mutex);
>> >>+
>> >>+	if (status)
>> >>+		/*
>> >>+		 * We should probably fail the whole open at this point,
>> >>+		 * but we've already created or opened the file, so it's
>> >>+		 * too late; So this seems the least of evils:
>> >>+		 */
>> >>+		bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
>> >
>> >Is there any way to avoid this?
>> >
>> >How does the vfs open code handle this?  It has to be able to set a
>> >security contexts atomically on open(O_CREAT), doesn't it?
>> >
>>
>> I believe the way this is handled is that the inode is created and
>> labeled and then only after that is it bound to the namespace.
>> Because of that ordering we can fail and release the inode without
>> it ever having a dentry in the namespace.
>
> Grepping around....  Looks like that's done by
> security_inode_init_security(), from the filesystem's create method.
>
> So we'd need to be able to pass something down to there.
>
> Is the current client actually expected to use this?  (So are we 
> going
> to see a lot of opens that set the label?)
>
> --b.

I don't have all the code infront of me but we have a different hook to 
do that. The call to nfsd4_security_inode_setsecctx is supposed to be 
used from the setattr handlers to do the equivalent of a setxattr on the 
file over NFS. The actual creation gets done with something like 
security_dentry_init_security which should be in an earlier patch. I'd 
have to look more clearly at the code to find out. Also where did we 
come up with 128 for label length? The SELinux code assumes a starting 
point of 255 and goes up from there as needed. The MLS policies will 
definitely exceed a 128 byte label.

Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Casey Schaufler March 29, 2013, 3:18 p.m. UTC | #11

On 3/29/2013 7:49 AM, David Quigley wrote:
> On 03/29/2013 10:40, J. Bruce Fields wrote:
>> On Thu, Mar 28, 2013 at 11:35:31PM -0400, Dave Quigley wrote:
>>> On 3/28/2013 12:14 PM, J. Bruce Fields wrote:
>>> >On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
>>> >>From: David Quigley <[e-mail]>
>>> >>
>>> >>This patch adds the ability to encode and decode file labels on
>>> the server for
>>> >>the purpose of sending them to the client and also to process
>>> label change
>>> >>requests from the client.
>>> >>
>>> >>Signed-off-by: Matthew N. Dodd <[e-mail]>
>>> >>Signed-off-by: Miguel Rodel Felipe <[e-mail]>
>>> >>Signed-off-by: Phua Eu Gene <[e-mail]>
>>> >>Signed-off-by: Khin Mi Mi Aung <[e-mail]>
>>> >>---
>>> >>  fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++
>>> >>  fs/nfsd/nfs4xdr.c  | 116
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>> >>  fs/nfsd/nfsd.h     |   6 ++-
>>> >>  fs/nfsd/vfs.c      |  29 ++++++++++++++
>>> >>  fs/nfsd/vfs.h      |   2 +
>>> >>  fs/nfsd/xdr4.h     |   3 ++
>>> >>  6 files changed, 191 insertions(+), 6 deletions(-)
>>> >>
>>> >>diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>> >>index ae73175..bb17589 100644
>>> >>--- a/fs/nfsd/nfs4proc.c
>>> >>+++ b/fs/nfsd/nfs4proc.c
>>> >>@@ -42,6 +42,36 @@
>>> >>  #include "current_stateid.h"
>>> >>  #include "netns.h"
>>> >>
>>> >>+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>>> >>+#include <linux/security.h>
>>> >>+
>>> >>+static inline void
>>> >>+nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct
>>> nfs4_label *label, u32 *bmval)
>>> >>+{
>>> >>+    struct inode *inode = resfh->fh_dentry->d_inode;
>>> >>+    int status;
>>> >>+
>>> >>+    mutex_lock(&inode->i_mutex);
>>> >>+    status = security_inode_setsecctx(resfh->fh_dentry,
>>> >>+        label->label, label->len);
>>> >>+    mutex_unlock(&inode->i_mutex);
>>> >>+
>>> >>+    if (status)
>>> >>+        /*
>>> >>+         * We should probably fail the whole open at this point,
>>> >>+         * but we've already created or opened the file, so it's
>>> >>+         * too late; So this seems the least of evils:
>>> >>+         */
>>> >>+        bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
>>> >
>>> >Is there any way to avoid this?
>>> >
>>> >How does the vfs open code handle this?  It has to be able to set a
>>> >security contexts atomically on open(O_CREAT), doesn't it?
>>> >
>>>
>>> I believe the way this is handled is that the inode is created and
>>> labeled and then only after that is it bound to the namespace.
>>> Because of that ordering we can fail and release the inode without
>>> it ever having a dentry in the namespace.
>>
>> Grepping around....  Looks like that's done by
>> security_inode_init_security(), from the filesystem's create method.
>>
>> So we'd need to be able to pass something down to there.
>>
>> Is the current client actually expected to use this?  (So are we going
>> to see a lot of opens that set the label?)
>>
>> --b.
>
> I don't have all the code infront of me but we have a different hook
> to do that. The call to nfsd4_security_inode_setsecctx is supposed to
> be used from the setattr handlers to do the equivalent of a setxattr
> on the file over NFS. The actual creation gets done with something
> like security_dentry_init_security which should be in an earlier
> patch. I'd have to look more clearly at the code to find out. Also
> where did we come up with 128 for label length? The SELinux code
> assumes a starting point of 255 and goes up from there as needed. The
> MLS policies will definitely exceed a 128 byte label.

If anyone cares, Smack labels can (now) be 255 characters.
Also, when (if) we get multiple concurrent LSMs the
"security context" may include information for more than
one LSM. 128 bytes is going to look pretty tiny then.

smack='com.corportation.service'selinux='system_u:object_r:bin_t:s0'



>
> Dave
> -- 
> To unsubscribe from this list: send the line "unsubscribe
> linux-security-module" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

J. Bruce Fields March 29, 2013, 6:42 p.m. UTC | #12

On Fri, Mar 29, 2013 at 08:18:59AM -0700, Casey Schaufler wrote:
> On 3/29/2013 7:49 AM, David Quigley wrote:
> > On 03/29/2013 10:40, J. Bruce Fields wrote:
> >> On Thu, Mar 28, 2013 at 11:35:31PM -0400, Dave Quigley wrote:
> >>> On 3/28/2013 12:14 PM, J. Bruce Fields wrote:
> >>> >On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
> >>> >>From: David Quigley <[e-mail]>
> >>> >>
> >>> >>This patch adds the ability to encode and decode file labels on
> >>> the server for
> >>> >>the purpose of sending them to the client and also to process
> >>> label change
> >>> >>requests from the client.
> >>> >>
> >>> >>Signed-off-by: Matthew N. Dodd <[e-mail]>
> >>> >>Signed-off-by: Miguel Rodel Felipe <[e-mail]>
> >>> >>Signed-off-by: Phua Eu Gene <[e-mail]>
> >>> >>Signed-off-by: Khin Mi Mi Aung <[e-mail]>
> >>> >>---
> >>> >>  fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++
> >>> >>  fs/nfsd/nfs4xdr.c  | 116
> >>> ++++++++++++++++++++++++++++++++++++++++++++++++++---
> >>> >>  fs/nfsd/nfsd.h     |   6 ++-
> >>> >>  fs/nfsd/vfs.c      |  29 ++++++++++++++
> >>> >>  fs/nfsd/vfs.h      |   2 +
> >>> >>  fs/nfsd/xdr4.h     |   3 ++
> >>> >>  6 files changed, 191 insertions(+), 6 deletions(-)
> >>> >>
> >>> >>diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >>> >>index ae73175..bb17589 100644
> >>> >>--- a/fs/nfsd/nfs4proc.c
> >>> >>+++ b/fs/nfsd/nfs4proc.c
> >>> >>@@ -42,6 +42,36 @@
> >>> >>  #include "current_stateid.h"
> >>> >>  #include "netns.h"
> >>> >>
> >>> >>+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> >>> >>+#include <linux/security.h>
> >>> >>+
> >>> >>+static inline void
> >>> >>+nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct
> >>> nfs4_label *label, u32 *bmval)
> >>> >>+{
> >>> >>+    struct inode *inode = resfh->fh_dentry->d_inode;
> >>> >>+    int status;
> >>> >>+
> >>> >>+    mutex_lock(&inode->i_mutex);
> >>> >>+    status = security_inode_setsecctx(resfh->fh_dentry,
> >>> >>+        label->label, label->len);
> >>> >>+    mutex_unlock(&inode->i_mutex);
> >>> >>+
> >>> >>+    if (status)
> >>> >>+        /*
> >>> >>+         * We should probably fail the whole open at this point,
> >>> >>+         * but we've already created or opened the file, so it's
> >>> >>+         * too late; So this seems the least of evils:
> >>> >>+         */
> >>> >>+        bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> >>> >
> >>> >Is there any way to avoid this?
> >>> >
> >>> >How does the vfs open code handle this?  It has to be able to set a
> >>> >security contexts atomically on open(O_CREAT), doesn't it?
> >>> >
> >>>
> >>> I believe the way this is handled is that the inode is created and
> >>> labeled and then only after that is it bound to the namespace.
> >>> Because of that ordering we can fail and release the inode without
> >>> it ever having a dentry in the namespace.
> >>
> >> Grepping around....  Looks like that's done by
> >> security_inode_init_security(), from the filesystem's create method.
> >>
> >> So we'd need to be able to pass something down to there.
> >>
> >> Is the current client actually expected to use this?  (So are we going
> >> to see a lot of opens that set the label?)
> >>
> >> --b.
> >
> > I don't have all the code infront of me but we have a different hook
> > to do that. The call to nfsd4_security_inode_setsecctx is supposed to
> > be used from the setattr handlers to do the equivalent of a setxattr
> > on the file over NFS. The actual creation gets done with something
> > like security_dentry_init_security which should be in an earlier
> > patch. I'd have to look more clearly at the code to find out. Also
> > where did we come up with 128 for label length? The SELinux code
> > assumes a starting point of 255 and goes up from there as needed. The
> > MLS policies will definitely exceed a 128 byte label.
> 
> If anyone cares, Smack labels can (now) be 255 characters.
> Also, when (if) we get multiple concurrent LSMs the
> "security context" may include information for more than
> one LSM. 128 bytes is going to look pretty tiny then.
> 
> smack='com.corportation.service'selinux='system_u:object_r:bin_t:s0'

OK.  We need a number.  2k?

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Casey Schaufler March 29, 2013, 8:10 p.m. UTC | #13

On 3/29/2013 11:42 AM, J. Bruce Fields wrote:
> On Fri, Mar 29, 2013 at 08:18:59AM -0700, Casey Schaufler wrote:
>> On 3/29/2013 7:49 AM, David Quigley wrote:
>>> On 03/29/2013 10:40, J. Bruce Fields wrote:
>>>> On Thu, Mar 28, 2013 at 11:35:31PM -0400, Dave Quigley wrote:
>>>>> On 3/28/2013 12:14 PM, J. Bruce Fields wrote:
>>>>>> On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
>>>>>>> From: David Quigley <[e-mail]>
>>>>>>>
>>>>>>> This patch adds the ability to encode and decode file labels on
>>>>> the server for
>>>>>>> the purpose of sending them to the client and also to process
>>>>> label change
>>>>>>> requests from the client.
>>>>>>>
>>>>>>> Signed-off-by: Matthew N. Dodd <[e-mail]>
>>>>>>> Signed-off-by: Miguel Rodel Felipe <[e-mail]>
>>>>>>> Signed-off-by: Phua Eu Gene <[e-mail]>
>>>>>>> Signed-off-by: Khin Mi Mi Aung <[e-mail]>
>>>>>>> ---
>>>>>>>  fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++
>>>>>>>  fs/nfsd/nfs4xdr.c  | 116
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>>>>>  fs/nfsd/nfsd.h     |   6 ++-
>>>>>>>  fs/nfsd/vfs.c      |  29 ++++++++++++++
>>>>>>>  fs/nfsd/vfs.h      |   2 +
>>>>>>>  fs/nfsd/xdr4.h     |   3 ++
>>>>>>>  6 files changed, 191 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>>>>> index ae73175..bb17589 100644
>>>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>>>> @@ -42,6 +42,36 @@
>>>>>>>  #include "current_stateid.h"
>>>>>>>  #include "netns.h"
>>>>>>>
>>>>>>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>>>>>>> +#include <linux/security.h>
>>>>>>> +
>>>>>>> +static inline void
>>>>>>> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct
>>>>> nfs4_label *label, u32 *bmval)
>>>>>>> +{
>>>>>>> +    struct inode *inode = resfh->fh_dentry->d_inode;
>>>>>>> +    int status;
>>>>>>> +
>>>>>>> +    mutex_lock(&inode->i_mutex);
>>>>>>> +    status = security_inode_setsecctx(resfh->fh_dentry,
>>>>>>> +        label->label, label->len);
>>>>>>> +    mutex_unlock(&inode->i_mutex);
>>>>>>> +
>>>>>>> +    if (status)
>>>>>>> +        /*
>>>>>>> +         * We should probably fail the whole open at this point,
>>>>>>> +         * but we've already created or opened the file, so it's
>>>>>>> +         * too late; So this seems the least of evils:
>>>>>>> +         */
>>>>>>> +        bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
>>>>>> Is there any way to avoid this?
>>>>>>
>>>>>> How does the vfs open code handle this?  It has to be able to set a
>>>>>> security contexts atomically on open(O_CREAT), doesn't it?
>>>>>>
>>>>> I believe the way this is handled is that the inode is created and
>>>>> labeled and then only after that is it bound to the namespace.
>>>>> Because of that ordering we can fail and release the inode without
>>>>> it ever having a dentry in the namespace.
>>>> Grepping around....  Looks like that's done by
>>>> security_inode_init_security(), from the filesystem's create method.
>>>>
>>>> So we'd need to be able to pass something down to there.
>>>>
>>>> Is the current client actually expected to use this?  (So are we going
>>>> to see a lot of opens that set the label?)
>>>>
>>>> --b.
>>> I don't have all the code infront of me but we have a different hook
>>> to do that. The call to nfsd4_security_inode_setsecctx is supposed to
>>> be used from the setattr handlers to do the equivalent of a setxattr
>>> on the file over NFS. The actual creation gets done with something
>>> like security_dentry_init_security which should be in an earlier
>>> patch. I'd have to look more clearly at the code to find out. Also
>>> where did we come up with 128 for label length? The SELinux code
>>> assumes a starting point of 255 and goes up from there as needed. The
>>> MLS policies will definitely exceed a 128 byte label.
>> If anyone cares, Smack labels can (now) be 255 characters.
>> Also, when (if) we get multiple concurrent LSMs the
>> "security context" may include information for more than
>> one LSM. 128 bytes is going to look pretty tiny then.
>>
>> smack='com.corportation.service'selinux='system_u:object_r:bin_t:s0'
> OK.  We need a number.  2k?

Consider some of the kinds of attributes we are likely to
see in the not to distant future:

Permitted access times, at around 100 bytes each.
Bell & LaPadula labels at 500 bytes each.
Signatures of various sizes and flavors.
HEPA restrictions

I think 2k will do for a while. I think that in the long run
it will come up short. I think the real question is whether
NFS will remain viable long enough for it to matter.

>
> --b.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Steve Dickson March 30, 2013, 9:09 p.m. UTC | #14

On 29/03/13 16:10, Casey Schaufler wrote:
> On 3/29/2013 11:42 AM, J. Bruce Fields wrote:
>> On Fri, Mar 29, 2013 at 08:18:59AM -0700, Casey Schaufler wrote:
>>> On 3/29/2013 7:49 AM, David Quigley wrote:
>>>> On 03/29/2013 10:40, J. Bruce Fields wrote:
>>>>> On Thu, Mar 28, 2013 at 11:35:31PM -0400, Dave Quigley wrote:
>>>>>> On 3/28/2013 12:14 PM, J. Bruce Fields wrote:
>>>>>>> On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
>>>>>>>> From: David Quigley <[e-mail]>
>>>>>>>>
>>>>>>>> This patch adds the ability to encode and decode file labels on
>>>>>> the server for
>>>>>>>> the purpose of sending them to the client and also to process
>>>>>> label change
>>>>>>>> requests from the client.
>>>>>>>>
>>>>>>>> Signed-off-by: Matthew N. Dodd <[e-mail]>
>>>>>>>> Signed-off-by: Miguel Rodel Felipe <[e-mail]>
>>>>>>>> Signed-off-by: Phua Eu Gene <[e-mail]>
>>>>>>>> Signed-off-by: Khin Mi Mi Aung <[e-mail]>
>>>>>>>> ---
>>>>>>>>  fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++
>>>>>>>>  fs/nfsd/nfs4xdr.c  | 116
>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>>>>>>  fs/nfsd/nfsd.h     |   6 ++-
>>>>>>>>  fs/nfsd/vfs.c      |  29 ++++++++++++++
>>>>>>>>  fs/nfsd/vfs.h      |   2 +
>>>>>>>>  fs/nfsd/xdr4.h     |   3 ++
>>>>>>>>  6 files changed, 191 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>>>>>> index ae73175..bb17589 100644
>>>>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>>>>> @@ -42,6 +42,36 @@
>>>>>>>>  #include "current_stateid.h"
>>>>>>>>  #include "netns.h"
>>>>>>>>
>>>>>>>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>>>>>>>> +#include <linux/security.h>
>>>>>>>> +
>>>>>>>> +static inline void
>>>>>>>> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct
>>>>>> nfs4_label *label, u32 *bmval)
>>>>>>>> +{
>>>>>>>> +    struct inode *inode = resfh->fh_dentry->d_inode;
>>>>>>>> +    int status;
>>>>>>>> +
>>>>>>>> +    mutex_lock(&inode->i_mutex);
>>>>>>>> +    status = security_inode_setsecctx(resfh->fh_dentry,
>>>>>>>> +        label->label, label->len);
>>>>>>>> +    mutex_unlock(&inode->i_mutex);
>>>>>>>> +
>>>>>>>> +    if (status)
>>>>>>>> +        /*
>>>>>>>> +         * We should probably fail the whole open at this point,
>>>>>>>> +         * but we've already created or opened the file, so it's
>>>>>>>> +         * too late; So this seems the least of evils:
>>>>>>>> +         */
>>>>>>>> +        bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
>>>>>>> Is there any way to avoid this?
>>>>>>>
>>>>>>> How does the vfs open code handle this?  It has to be able to set a
>>>>>>> security contexts atomically on open(O_CREAT), doesn't it?
>>>>>>>
>>>>>> I believe the way this is handled is that the inode is created and
>>>>>> labeled and then only after that is it bound to the namespace.
>>>>>> Because of that ordering we can fail and release the inode without
>>>>>> it ever having a dentry in the namespace.
>>>>> Grepping around....  Looks like that's done by
>>>>> security_inode_init_security(), from the filesystem's create method.
>>>>>
>>>>> So we'd need to be able to pass something down to there.
>>>>>
>>>>> Is the current client actually expected to use this?  (So are we going
>>>>> to see a lot of opens that set the label?)
>>>>>
>>>>> --b.
>>>> I don't have all the code infront of me but we have a different hook
>>>> to do that. The call to nfsd4_security_inode_setsecctx is supposed to
>>>> be used from the setattr handlers to do the equivalent of a setxattr
>>>> on the file over NFS. The actual creation gets done with something
>>>> like security_dentry_init_security which should be in an earlier
>>>> patch. I'd have to look more clearly at the code to find out. Also
>>>> where did we come up with 128 for label length? The SELinux code
>>>> assumes a starting point of 255 and goes up from there as needed. The
>>>> MLS policies will definitely exceed a 128 byte label.
>>> If anyone cares, Smack labels can (now) be 255 characters.
>>> Also, when (if) we get multiple concurrent LSMs the
>>> "security context" may include information for more than
>>> one LSM. 128 bytes is going to look pretty tiny then.
>>>
>>> smack='com.corportation.service'selinux='system_u:object_r:bin_t:s0'
>> OK.  We need a number.  2k?
> 
> Consider some of the kinds of attributes we are likely to
> see in the not to distant future:
> 
> Permitted access times, at around 100 bytes each.
> Bell & LaPadula labels at 500 bytes each.
> Signatures of various sizes and flavors.
> HEPA restrictions
> 
> I think 2k will do for a while. I think that in the long run
> it will come up short. I think the real question is whether
> NFS will remain viable long enough for it to matter.

2k it is! 

steved.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Steve Dickson March 30, 2013, 9:50 p.m. UTC | #15

On 28/03/13 14:58, J. Bruce Fields wrote:
> On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
>> From: David Quigley <[e-mail]>
>>
>> This patch adds the ability to encode and decode file labels on the server for
>> the purpose of sending them to the client and also to process label change
>> requests from the client.
>>
>> Signed-off-by: Matthew N. Dodd <[e-mail]>
>> Signed-off-by: Miguel Rodel Felipe <[e-mail]>
>> Signed-off-by: Phua Eu Gene <[e-mail]>
>> Signed-off-by: Khin Mi Mi Aung <[e-mail]>
>> ---
>>  fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++
>>  fs/nfsd/nfs4xdr.c  | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>  fs/nfsd/nfsd.h     |   6 ++-
>>  fs/nfsd/vfs.c      |  29 ++++++++++++++
>>  fs/nfsd/vfs.h      |   2 +
>>  fs/nfsd/xdr4.h     |   3 ++
>>  6 files changed, 191 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index ae73175..bb17589 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -42,6 +42,36 @@
>>  #include "current_stateid.h"
>>  #include "netns.h"
>>  
>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>> +#include <linux/security.h>
>> +
>> +static inline void
>> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval)
>> +{
>> +	struct inode *inode = resfh->fh_dentry->d_inode;
>> +	int status;
>> +
>> +	mutex_lock(&inode->i_mutex);
>> +	status = security_inode_setsecctx(resfh->fh_dentry,
>> +		label->label, label->len);
>> +	mutex_unlock(&inode->i_mutex);
>> +
>> +	if (status)
>> +		/*
>> +		 * We should probably fail the whole open at this point,
>> +		 * but we've already created or opened the file, so it's 
>> +		 * too late; So this seems the least of evils:
>> +		 */
>> +		bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
>> +		
>> +	return;
>> +}
>> +#else 
>> +static inline void 
>> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval)
>> +{ }
>> +#endif
>> +
>>  #define NFSDDBG_FACILITY		NFSDDBG_PROC
>>  
>>  static u32 nfsd_attrmask[] = {
>> @@ -230,6 +260,9 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o
>>  					(u32 *)open->op_verf.data,
>>  					&open->op_truncate, &open->op_created);
>>  
>> +		if (!status && open->op_label != NULL) 
>> +			nfsd4_security_inode_setsecctx(resfh, open->op_label, open->op_bmval);
>> +
>>  		/*
>>  		 * Following rfc 3530 14.2.16, use the returned bitmask
>>  		 * to indicate which attributes we used to store the
>> @@ -599,6 +632,9 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>  	if (status)
>>  		goto out;
>>  
>> +	if (create->cr_label != NULL)
>> +		nfsd4_security_inode_setsecctx(&resfh, create->cr_label, create->cr_bmval);
>> +
>>  	if (create->cr_acl != NULL)
>>  		do_set_nfs4_acl(rqstp, &resfh, create->cr_acl,
>>  				create->cr_bmval);
>> @@ -888,6 +924,11 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>  					    setattr->sa_acl);
>>  	if (status)
>>  		goto out;
>> +	if (setattr->sa_label != NULL)
>> +		status = nfsd4_set_nfs4_label(rqstp, &cstate->current_fh,
>> +				setattr->sa_label);
>> +	if (status)
>> +		goto out;
>>  	status = nfsd_setattr(rqstp, &cstate->current_fh, &setattr->sa_iattr,
>>  				0, (time_t)0);
>>  out:
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index 0116886..52e219c 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -55,6 +55,11 @@
>>  #include "cache.h"
>>  #include "netns.h"
>>  
>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>> +#include <linux/security.h>
>> +#endif
>> +
>> +
>>  #define NFSDDBG_FACILITY		NFSDDBG_XDR
>>  
>>  /*
>> @@ -79,6 +84,24 @@ check_filename(char *str, int len)
>>  			return nfserr_badname;
>>  	return 0;
>>  } 
>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>> +static struct nfs4_label *nfsd4_label_alloc(u32 len)
>> +{
>> +	struct nfs4_label *label = NULL;
>> +
>> +	if (len > NFS4_MAXLABELLEN)
>> +		return ERR_PTR(-ENAMETOOLONG);
> 
> This is returned as NFS4ERR_NAMETOOLONG.
> 
> The 4.2 spec should note this case, if it's the right error.  rfc 5661
> says that error's only for filenames, and doesn't allow it for setattr
> (which doesn't take a filename).
The 4.2 spec (draft-ietf-nfsv4-minorversion2-19.txt) defines two Labeled 
NFS errors:
NFS4ERR_BADLABEL  (Error Code 10093)
NFS4ERR_WRONG_LFS (Error Code 10092)

and we currently don't define either one of them... So I guess that
will have to change... 

So what should NFS4ERR_BADLABEL map to in nfserrno()?

> 
>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>> +	if (bmval[2] & FATTR4_WORD2_SECURITY_LABEL) {
>> +		uint32_t pi;
>> +		uint32_t lfs;
>> +
>> +		READ_BUF(4);
>> +		len += 4;
>> +		READ32(lfs);
>> +		READ_BUF(4);
>> +		len += 4;
>> +		READ32(pi);
>> +		READ_BUF(4);
>> +		len += 4;
>> +		READ32(dummy32);
>> +		READ_BUF(dummy32);
>> +		len += (XDR_QUADLEN(dummy32) << 2);
>> +		READMEM(buf, dummy32);
>> +
>> +		*label = nfsd4_label_alloc(dummy32);
>> +		if (*label == NULL) {
> 
> Careful!  That should be IS_ERR(*label).
> 
>> +			host_err = PTR_ERR(label);
> 
> And that should be *label.
Gotta it... 

> 
>>  static __be32
>> @@ -1988,6 +2044,50 @@ nfsd4_encode_aclname(struct svc_rqst *rqstp, struct nfs4_ace *ace,
>>  			      FATTR4_WORD0_RDATTR_ERROR)
>>  #define WORD1_ABSENT_FS_ATTRS FATTR4_WORD1_MOUNTED_ON_FILEID
>>  
>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>> +static inline __be32
>> +nfsd4_encode_security_label(struct svc_rqst *rqstp, struct dentry *dentry, __be32 **pp, int *buflen)
>> +{
>> +	void *context;
>> +	int err;
>> +	int len;
>> +	uint32_t pi = 0;
>> +	uint32_t lfs = 0;
>> +	__be32 *p = *pp;
>> +
>> +	err = 0;
>> +	(void)security_inode_getsecctx(dentry->d_inode, &context, &len);
>> +	if (len < 0)
>> +		return nfserrno(len);
>> +
>> +	if (*buflen < ((XDR_QUADLEN(len) << 2) + 4 + 4 + 4)) {
>> +		err = nfserr_resource;
>> +		goto out;
>> +	}
>> +
>> +	/* XXX: A call to the translation code should be placed here
>> +	 * for now send 0  until we have that to indicate the null
>> +	 * translation */ 
> 
> Could we better a better comment here?
Changed to:
/*
 * For now we use a 0 here to indicate the null translation; in
 * the future we may place a call to translation code here.
 */
> 
>> +
>> +	if ((*buflen -= 4) < 0)
> 
> Looks like that should be 8.
Right...

> 
>> +		return nfserr_resource;
>> +
>> +	WRITE32(lfs);	
>> +	WRITE32(pi);
>> +	p = xdr_encode_opaque(p, context, len);
>> +	*buflen -= (XDR_QUADLEN(len) << 2) + 4;
>> +
>> +	*pp = p;
>> +out:
>> +	security_release_secctx(context, len);
>> +	return err;
>> +}
> 
> ...
>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>> +__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
>> +		struct nfs4_label *label)
>> +{
>> +	__be32 error;
>> +	int host_error;
>> +	struct dentry *dentry;
>> +
>> +	/* XXX: should we have a MAY_SSECCTX? */
> 
> Again: could we get an answer to this question?
Gone... just a bad dream! ;-) 

Thank you the cycles! 

steved.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

J. Bruce Fields March 30, 2013, 10:08 p.m. UTC | #16

On Sat, Mar 30, 2013 at 05:50:21PM -0400, Steve Dickson wrote:
> 
> 
> On 28/03/13 14:58, J. Bruce Fields wrote:
> > On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
> >> From: David Quigley <[e-mail]>
> >>
> >> This patch adds the ability to encode and decode file labels on the server for
> >> the purpose of sending them to the client and also to process label change
> >> requests from the client.
> >>
> >> Signed-off-by: Matthew N. Dodd <[e-mail]>
> >> Signed-off-by: Miguel Rodel Felipe <[e-mail]>
> >> Signed-off-by: Phua Eu Gene <[e-mail]>
> >> Signed-off-by: Khin Mi Mi Aung <[e-mail]>
> >> ---
> >>  fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++
> >>  fs/nfsd/nfs4xdr.c  | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> >>  fs/nfsd/nfsd.h     |   6 ++-
> >>  fs/nfsd/vfs.c      |  29 ++++++++++++++
> >>  fs/nfsd/vfs.h      |   2 +
> >>  fs/nfsd/xdr4.h     |   3 ++
> >>  6 files changed, 191 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >> index ae73175..bb17589 100644
> >> --- a/fs/nfsd/nfs4proc.c
> >> +++ b/fs/nfsd/nfs4proc.c
> >> @@ -42,6 +42,36 @@
> >>  #include "current_stateid.h"
> >>  #include "netns.h"
> >>  
> >> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> >> +#include <linux/security.h>
> >> +
> >> +static inline void
> >> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval)
> >> +{
> >> +	struct inode *inode = resfh->fh_dentry->d_inode;
> >> +	int status;
> >> +
> >> +	mutex_lock(&inode->i_mutex);
> >> +	status = security_inode_setsecctx(resfh->fh_dentry,
> >> +		label->label, label->len);
> >> +	mutex_unlock(&inode->i_mutex);
> >> +
> >> +	if (status)
> >> +		/*
> >> +		 * We should probably fail the whole open at this point,
> >> +		 * but we've already created or opened the file, so it's 
> >> +		 * too late; So this seems the least of evils:
> >> +		 */
> >> +		bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> >> +		
> >> +	return;
> >> +}
> >> +#else 
> >> +static inline void 
> >> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval)
> >> +{ }
> >> +#endif
> >> +
> >>  #define NFSDDBG_FACILITY		NFSDDBG_PROC
> >>  
> >>  static u32 nfsd_attrmask[] = {
> >> @@ -230,6 +260,9 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o
> >>  					(u32 *)open->op_verf.data,
> >>  					&open->op_truncate, &open->op_created);
> >>  
> >> +		if (!status && open->op_label != NULL) 
> >> +			nfsd4_security_inode_setsecctx(resfh, open->op_label, open->op_bmval);
> >> +
> >>  		/*
> >>  		 * Following rfc 3530 14.2.16, use the returned bitmask
> >>  		 * to indicate which attributes we used to store the
> >> @@ -599,6 +632,9 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >>  	if (status)
> >>  		goto out;
> >>  
> >> +	if (create->cr_label != NULL)
> >> +		nfsd4_security_inode_setsecctx(&resfh, create->cr_label, create->cr_bmval);
> >> +
> >>  	if (create->cr_acl != NULL)
> >>  		do_set_nfs4_acl(rqstp, &resfh, create->cr_acl,
> >>  				create->cr_bmval);
> >> @@ -888,6 +924,11 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >>  					    setattr->sa_acl);
> >>  	if (status)
> >>  		goto out;
> >> +	if (setattr->sa_label != NULL)
> >> +		status = nfsd4_set_nfs4_label(rqstp, &cstate->current_fh,
> >> +				setattr->sa_label);
> >> +	if (status)
> >> +		goto out;
> >>  	status = nfsd_setattr(rqstp, &cstate->current_fh, &setattr->sa_iattr,
> >>  				0, (time_t)0);
> >>  out:
> >> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> >> index 0116886..52e219c 100644
> >> --- a/fs/nfsd/nfs4xdr.c
> >> +++ b/fs/nfsd/nfs4xdr.c
> >> @@ -55,6 +55,11 @@
> >>  #include "cache.h"
> >>  #include "netns.h"
> >>  
> >> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> >> +#include <linux/security.h>
> >> +#endif
> >> +
> >> +
> >>  #define NFSDDBG_FACILITY		NFSDDBG_XDR
> >>  
> >>  /*
> >> @@ -79,6 +84,24 @@ check_filename(char *str, int len)
> >>  			return nfserr_badname;
> >>  	return 0;
> >>  } 
> >> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> >> +static struct nfs4_label *nfsd4_label_alloc(u32 len)
> >> +{
> >> +	struct nfs4_label *label = NULL;
> >> +
> >> +	if (len > NFS4_MAXLABELLEN)
> >> +		return ERR_PTR(-ENAMETOOLONG);
> > 
> > This is returned as NFS4ERR_NAMETOOLONG.
> > 
> > The 4.2 spec should note this case, if it's the right error.  rfc 5661
> > says that error's only for filenames, and doesn't allow it for setattr
> > (which doesn't take a filename).
> The 4.2 spec (draft-ietf-nfsv4-minorversion2-19.txt) defines two Labeled 
> NFS errors:
> NFS4ERR_BADLABEL  (Error Code 10093)
> NFS4ERR_WRONG_LFS (Error Code 10092)
> 
> and we currently don't define either one of them... So I guess that
> will have to change... 

http://tools.ietf.org/html/draft-ietf-nfsv4-minorversion2-19#section-11.1.3.1
just says "The label specified is invalid in some manner" for
NFS4ERR_BADLABEL.

Anyway, that sounds like the right one for this case....

> So what should NFS4ERR_BADLABEL map to in nfserrno()?

I don't know; I guess I'd check out what errors can be returned from the
code that sets a context.

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Steve Dickson March 30, 2013, 10:49 p.m. UTC | #17

On 30/03/13 18:08, J. Bruce Fields wrote:
> On Sat, Mar 30, 2013 at 05:50:21PM -0400, Steve Dickson wrote:
>>
>>
>> On 28/03/13 14:58, J. Bruce Fields wrote:
>>> On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
>>>> From: David Quigley <[e-mail]>
>>>>
>>>> This patch adds the ability to encode and decode file labels on the server for
>>>> the purpose of sending them to the client and also to process label change
>>>> requests from the client.
>>>>
>>>> Signed-off-by: Matthew N. Dodd <[e-mail]>
>>>> Signed-off-by: Miguel Rodel Felipe <[e-mail]>
>>>> Signed-off-by: Phua Eu Gene <[e-mail]>
>>>> Signed-off-by: Khin Mi Mi Aung <[e-mail]>
>>>> ---
>>>>  fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++
>>>>  fs/nfsd/nfs4xdr.c  | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>>  fs/nfsd/nfsd.h     |   6 ++-
>>>>  fs/nfsd/vfs.c      |  29 ++++++++++++++
>>>>  fs/nfsd/vfs.h      |   2 +
>>>>  fs/nfsd/xdr4.h     |   3 ++
>>>>  6 files changed, 191 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>> index ae73175..bb17589 100644
>>>> --- a/fs/nfsd/nfs4proc.c
>>>> +++ b/fs/nfsd/nfs4proc.c
>>>> @@ -42,6 +42,36 @@
>>>>  #include "current_stateid.h"
>>>>  #include "netns.h"
>>>>  
>>>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>>>> +#include <linux/security.h>
>>>> +
>>>> +static inline void
>>>> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval)
>>>> +{
>>>> +	struct inode *inode = resfh->fh_dentry->d_inode;
>>>> +	int status;
>>>> +
>>>> +	mutex_lock(&inode->i_mutex);
>>>> +	status = security_inode_setsecctx(resfh->fh_dentry,
>>>> +		label->label, label->len);
>>>> +	mutex_unlock(&inode->i_mutex);
>>>> +
>>>> +	if (status)
>>>> +		/*
>>>> +		 * We should probably fail the whole open at this point,
>>>> +		 * but we've already created or opened the file, so it's 
>>>> +		 * too late; So this seems the least of evils:
>>>> +		 */
>>>> +		bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
>>>> +		
>>>> +	return;
>>>> +}
>>>> +#else 
>>>> +static inline void 
>>>> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval)
>>>> +{ }
>>>> +#endif
>>>> +
>>>>  #define NFSDDBG_FACILITY		NFSDDBG_PROC
>>>>  
>>>>  static u32 nfsd_attrmask[] = {
>>>> @@ -230,6 +260,9 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o
>>>>  					(u32 *)open->op_verf.data,
>>>>  					&open->op_truncate, &open->op_created);
>>>>  
>>>> +		if (!status && open->op_label != NULL) 
>>>> +			nfsd4_security_inode_setsecctx(resfh, open->op_label, open->op_bmval);
>>>> +
>>>>  		/*
>>>>  		 * Following rfc 3530 14.2.16, use the returned bitmask
>>>>  		 * to indicate which attributes we used to store the
>>>> @@ -599,6 +632,9 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>>  	if (status)
>>>>  		goto out;
>>>>  
>>>> +	if (create->cr_label != NULL)
>>>> +		nfsd4_security_inode_setsecctx(&resfh, create->cr_label, create->cr_bmval);
>>>> +
>>>>  	if (create->cr_acl != NULL)
>>>>  		do_set_nfs4_acl(rqstp, &resfh, create->cr_acl,
>>>>  				create->cr_bmval);
>>>> @@ -888,6 +924,11 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>>  					    setattr->sa_acl);
>>>>  	if (status)
>>>>  		goto out;
>>>> +	if (setattr->sa_label != NULL)
>>>> +		status = nfsd4_set_nfs4_label(rqstp, &cstate->current_fh,
>>>> +				setattr->sa_label);
>>>> +	if (status)
>>>> +		goto out;
>>>>  	status = nfsd_setattr(rqstp, &cstate->current_fh, &setattr->sa_iattr,
>>>>  				0, (time_t)0);
>>>>  out:
>>>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>>>> index 0116886..52e219c 100644
>>>> --- a/fs/nfsd/nfs4xdr.c
>>>> +++ b/fs/nfsd/nfs4xdr.c
>>>> @@ -55,6 +55,11 @@
>>>>  #include "cache.h"
>>>>  #include "netns.h"
>>>>  
>>>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>>>> +#include <linux/security.h>
>>>> +#endif
>>>> +
>>>> +
>>>>  #define NFSDDBG_FACILITY		NFSDDBG_XDR
>>>>  
>>>>  /*
>>>> @@ -79,6 +84,24 @@ check_filename(char *str, int len)
>>>>  			return nfserr_badname;
>>>>  	return 0;
>>>>  } 
>>>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>>>> +static struct nfs4_label *nfsd4_label_alloc(u32 len)
>>>> +{
>>>> +	struct nfs4_label *label = NULL;
>>>> +
>>>> +	if (len > NFS4_MAXLABELLEN)
>>>> +		return ERR_PTR(-ENAMETOOLONG);
>>>
>>> This is returned as NFS4ERR_NAMETOOLONG.
>>>
>>> The 4.2 spec should note this case, if it's the right error.  rfc 5661
>>> says that error's only for filenames, and doesn't allow it for setattr
>>> (which doesn't take a filename).
>> The 4.2 spec (draft-ietf-nfsv4-minorversion2-19.txt) defines two Labeled 
>> NFS errors:
>> NFS4ERR_BADLABEL  (Error Code 10093)
>> NFS4ERR_WRONG_LFS (Error Code 10092)
>>
>> and we currently don't define either one of them... So I guess that
>> will have to change... 
> 
> http://tools.ietf.org/html/draft-ietf-nfsv4-minorversion2-19#section-11.1.3.1
> just says "The label specified is invalid in some manner" for
> NFS4ERR_BADLABEL.
> 
> Anyway, that sounds like the right one for this case....
> 
>> So what should NFS4ERR_BADLABEL map to in nfserrno()?
> 
> I don't know; I guess I'd check out what errors can be returned from the
> code that sets a context.
Looking looking at the code under security/selinux, the popular returns 
seem to be are
-EOPNOTSUPP, -EACCES, -EPERM and -EINVAL. 

I say we go with -EINVAL....

steved.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Steve Dickson March 30, 2013, 11:13 p.m. UTC | #18

On 28/03/13 12:14, J. Bruce Fields wrote:
> On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
>> From: David Quigley <[e-mail]>
>>
>> This patch adds the ability to encode and decode file labels on the server for
>> the purpose of sending them to the client and also to process label change
>> requests from the client.
>>
>> Signed-off-by: Matthew N. Dodd <[e-mail]>
>> Signed-off-by: Miguel Rodel Felipe <[e-mail]>
>> Signed-off-by: Phua Eu Gene <[e-mail]>
>> Signed-off-by: Khin Mi Mi Aung <[e-mail]>
>> ---
>>  fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++
>>  fs/nfsd/nfs4xdr.c  | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>  fs/nfsd/nfsd.h     |   6 ++-
>>  fs/nfsd/vfs.c      |  29 ++++++++++++++
>>  fs/nfsd/vfs.h      |   2 +
>>  fs/nfsd/xdr4.h     |   3 ++
>>  6 files changed, 191 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index ae73175..bb17589 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -42,6 +42,36 @@
>>  #include "current_stateid.h"
>>  #include "netns.h"
>>  
>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>> +#include <linux/security.h>
>> +
>> +static inline void
>> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval)
>> +{
>> +	struct inode *inode = resfh->fh_dentry->d_inode;
>> +	int status;
>> +
>> +	mutex_lock(&inode->i_mutex);
>> +	status = security_inode_setsecctx(resfh->fh_dentry,
>> +		label->label, label->len);
>> +	mutex_unlock(&inode->i_mutex);
>> +
>> +	if (status)
>> +		/*
>> +		 * We should probably fail the whole open at this point,
>> +		 * but we've already created or opened the file, so it's 
>> +		 * too late; So this seems the least of evils:
>> +		 */
>> +		bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> 
> Is there any way to avoid this?
> 
> How does the vfs open code handle this?  It has to be able to set a
> security contexts atomically on open(O_CREAT), doesn't it?

The security guys are going have to answer this definitely but from
greping through the code, it appears the permission checking on 
the path to the object is done before the object is created. 

Stephen, Eric?? 

steved.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

J. Bruce Fields March 31, 2013, 12:02 a.m. UTC | #19

On Sat, Mar 30, 2013 at 06:49:19PM -0400, Steve Dickson wrote:
> 
> 
> On 30/03/13 18:08, J. Bruce Fields wrote:
> > On Sat, Mar 30, 2013 at 05:50:21PM -0400, Steve Dickson wrote:
> >>
> >>
> >> On 28/03/13 14:58, J. Bruce Fields wrote:
> >>> On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
> >>>> From: David Quigley <[e-mail]>
> >>>>
> >>>> This patch adds the ability to encode and decode file labels on the server for
> >>>> the purpose of sending them to the client and also to process label change
> >>>> requests from the client.
> >>>>
> >>>> Signed-off-by: Matthew N. Dodd <[e-mail]>
> >>>> Signed-off-by: Miguel Rodel Felipe <[e-mail]>
> >>>> Signed-off-by: Phua Eu Gene <[e-mail]>
> >>>> Signed-off-by: Khin Mi Mi Aung <[e-mail]>
> >>>> ---
> >>>>  fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++
> >>>>  fs/nfsd/nfs4xdr.c  | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> >>>>  fs/nfsd/nfsd.h     |   6 ++-
> >>>>  fs/nfsd/vfs.c      |  29 ++++++++++++++
> >>>>  fs/nfsd/vfs.h      |   2 +
> >>>>  fs/nfsd/xdr4.h     |   3 ++
> >>>>  6 files changed, 191 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >>>> index ae73175..bb17589 100644
> >>>> --- a/fs/nfsd/nfs4proc.c
> >>>> +++ b/fs/nfsd/nfs4proc.c
> >>>> @@ -42,6 +42,36 @@
> >>>>  #include "current_stateid.h"
> >>>>  #include "netns.h"
> >>>>  
> >>>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> >>>> +#include <linux/security.h>
> >>>> +
> >>>> +static inline void
> >>>> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval)
> >>>> +{
> >>>> +	struct inode *inode = resfh->fh_dentry->d_inode;
> >>>> +	int status;
> >>>> +
> >>>> +	mutex_lock(&inode->i_mutex);
> >>>> +	status = security_inode_setsecctx(resfh->fh_dentry,
> >>>> +		label->label, label->len);
> >>>> +	mutex_unlock(&inode->i_mutex);
> >>>> +
> >>>> +	if (status)
> >>>> +		/*
> >>>> +		 * We should probably fail the whole open at this point,
> >>>> +		 * but we've already created or opened the file, so it's 
> >>>> +		 * too late; So this seems the least of evils:
> >>>> +		 */
> >>>> +		bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> >>>> +		
> >>>> +	return;
> >>>> +}
> >>>> +#else 
> >>>> +static inline void 
> >>>> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval)
> >>>> +{ }
> >>>> +#endif
> >>>> +
> >>>>  #define NFSDDBG_FACILITY		NFSDDBG_PROC
> >>>>  
> >>>>  static u32 nfsd_attrmask[] = {
> >>>> @@ -230,6 +260,9 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o
> >>>>  					(u32 *)open->op_verf.data,
> >>>>  					&open->op_truncate, &open->op_created);
> >>>>  
> >>>> +		if (!status && open->op_label != NULL) 
> >>>> +			nfsd4_security_inode_setsecctx(resfh, open->op_label, open->op_bmval);
> >>>> +
> >>>>  		/*
> >>>>  		 * Following rfc 3530 14.2.16, use the returned bitmask
> >>>>  		 * to indicate which attributes we used to store the
> >>>> @@ -599,6 +632,9 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >>>>  	if (status)
> >>>>  		goto out;
> >>>>  
> >>>> +	if (create->cr_label != NULL)
> >>>> +		nfsd4_security_inode_setsecctx(&resfh, create->cr_label, create->cr_bmval);
> >>>> +
> >>>>  	if (create->cr_acl != NULL)
> >>>>  		do_set_nfs4_acl(rqstp, &resfh, create->cr_acl,
> >>>>  				create->cr_bmval);
> >>>> @@ -888,6 +924,11 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >>>>  					    setattr->sa_acl);
> >>>>  	if (status)
> >>>>  		goto out;
> >>>> +	if (setattr->sa_label != NULL)
> >>>> +		status = nfsd4_set_nfs4_label(rqstp, &cstate->current_fh,
> >>>> +				setattr->sa_label);
> >>>> +	if (status)
> >>>> +		goto out;
> >>>>  	status = nfsd_setattr(rqstp, &cstate->current_fh, &setattr->sa_iattr,
> >>>>  				0, (time_t)0);
> >>>>  out:
> >>>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> >>>> index 0116886..52e219c 100644
> >>>> --- a/fs/nfsd/nfs4xdr.c
> >>>> +++ b/fs/nfsd/nfs4xdr.c
> >>>> @@ -55,6 +55,11 @@
> >>>>  #include "cache.h"
> >>>>  #include "netns.h"
> >>>>  
> >>>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> >>>> +#include <linux/security.h>
> >>>> +#endif
> >>>> +
> >>>> +
> >>>>  #define NFSDDBG_FACILITY		NFSDDBG_XDR
> >>>>  
> >>>>  /*
> >>>> @@ -79,6 +84,24 @@ check_filename(char *str, int len)
> >>>>  			return nfserr_badname;
> >>>>  	return 0;
> >>>>  } 
> >>>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> >>>> +static struct nfs4_label *nfsd4_label_alloc(u32 len)
> >>>> +{
> >>>> +	struct nfs4_label *label = NULL;
> >>>> +
> >>>> +	if (len > NFS4_MAXLABELLEN)
> >>>> +		return ERR_PTR(-ENAMETOOLONG);
> >>>
> >>> This is returned as NFS4ERR_NAMETOOLONG.
> >>>
> >>> The 4.2 spec should note this case, if it's the right error.  rfc 5661
> >>> says that error's only for filenames, and doesn't allow it for setattr
> >>> (which doesn't take a filename).
> >> The 4.2 spec (draft-ietf-nfsv4-minorversion2-19.txt) defines two Labeled 
> >> NFS errors:
> >> NFS4ERR_BADLABEL  (Error Code 10093)
> >> NFS4ERR_WRONG_LFS (Error Code 10092)
> >>
> >> and we currently don't define either one of them... So I guess that
> >> will have to change... 
> > 
> > http://tools.ietf.org/html/draft-ietf-nfsv4-minorversion2-19#section-11.1.3.1
> > just says "The label specified is invalid in some manner" for
> > NFS4ERR_BADLABEL.
> > 
> > Anyway, that sounds like the right one for this case....
> > 
> >> So what should NFS4ERR_BADLABEL map to in nfserrno()?
> > 
> > I don't know; I guess I'd check out what errors can be returned from the
> > code that sets a context.
> Looking looking at the code under security/selinux, the popular returns 
> seem to be are
> -EOPNOTSUPP, -EACCES, -EPERM and -EINVAL. 
> 
> I say we go with -EINVAL....

Note nfserrno() converts from -ERR to nfs4 errors.  You may be thinking
of the client side nfs4_stat_to_errno().

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Steve Dickson March 31, 2013, 12:23 a.m. UTC | #20

On 30/03/13 20:02, J. Bruce Fields wrote:
> On Sat, Mar 30, 2013 at 06:49:19PM -0400, Steve Dickson wrote:
>>
>>
>> On 30/03/13 18:08, J. Bruce Fields wrote:
>>> On Sat, Mar 30, 2013 at 05:50:21PM -0400, Steve Dickson wrote:
>>>>
>>>>
>>>> On 28/03/13 14:58, J. Bruce Fields wrote:
>>>>> On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
>>>>>> From: David Quigley <[e-mail]>
>>>>>>
>>>>>> This patch adds the ability to encode and decode file labels on the server for
>>>>>> the purpose of sending them to the client and also to process label change
>>>>>> requests from the client.
>>>>>>
>>>>>> Signed-off-by: Matthew N. Dodd <[e-mail]>
>>>>>> Signed-off-by: Miguel Rodel Felipe <[e-mail]>
>>>>>> Signed-off-by: Phua Eu Gene <[e-mail]>
>>>>>> Signed-off-by: Khin Mi Mi Aung <[e-mail]>
>>>>>> ---
>>>>>>  fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++
>>>>>>  fs/nfsd/nfs4xdr.c  | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>>>>  fs/nfsd/nfsd.h     |   6 ++-
>>>>>>  fs/nfsd/vfs.c      |  29 ++++++++++++++
>>>>>>  fs/nfsd/vfs.h      |   2 +
>>>>>>  fs/nfsd/xdr4.h     |   3 ++
>>>>>>  6 files changed, 191 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>>>> index ae73175..bb17589 100644
>>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>>> @@ -42,6 +42,36 @@
>>>>>>  #include "current_stateid.h"
>>>>>>  #include "netns.h"
>>>>>>  
>>>>>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>>>>>> +#include <linux/security.h>
>>>>>> +
>>>>>> +static inline void
>>>>>> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval)
>>>>>> +{
>>>>>> +	struct inode *inode = resfh->fh_dentry->d_inode;
>>>>>> +	int status;
>>>>>> +
>>>>>> +	mutex_lock(&inode->i_mutex);
>>>>>> +	status = security_inode_setsecctx(resfh->fh_dentry,
>>>>>> +		label->label, label->len);
>>>>>> +	mutex_unlock(&inode->i_mutex);
>>>>>> +
>>>>>> +	if (status)
>>>>>> +		/*
>>>>>> +		 * We should probably fail the whole open at this point,
>>>>>> +		 * but we've already created or opened the file, so it's 
>>>>>> +		 * too late; So this seems the least of evils:
>>>>>> +		 */
>>>>>> +		bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
>>>>>> +		
>>>>>> +	return;
>>>>>> +}
>>>>>> +#else 
>>>>>> +static inline void 
>>>>>> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval)
>>>>>> +{ }
>>>>>> +#endif
>>>>>> +
>>>>>>  #define NFSDDBG_FACILITY		NFSDDBG_PROC
>>>>>>  
>>>>>>  static u32 nfsd_attrmask[] = {
>>>>>> @@ -230,6 +260,9 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o
>>>>>>  					(u32 *)open->op_verf.data,
>>>>>>  					&open->op_truncate, &open->op_created);
>>>>>>  
>>>>>> +		if (!status && open->op_label != NULL) 
>>>>>> +			nfsd4_security_inode_setsecctx(resfh, open->op_label, open->op_bmval);
>>>>>> +
>>>>>>  		/*
>>>>>>  		 * Following rfc 3530 14.2.16, use the returned bitmask
>>>>>>  		 * to indicate which attributes we used to store the
>>>>>> @@ -599,6 +632,9 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>>>>  	if (status)
>>>>>>  		goto out;
>>>>>>  
>>>>>> +	if (create->cr_label != NULL)
>>>>>> +		nfsd4_security_inode_setsecctx(&resfh, create->cr_label, create->cr_bmval);
>>>>>> +
>>>>>>  	if (create->cr_acl != NULL)
>>>>>>  		do_set_nfs4_acl(rqstp, &resfh, create->cr_acl,
>>>>>>  				create->cr_bmval);
>>>>>> @@ -888,6 +924,11 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>>>>  					    setattr->sa_acl);
>>>>>>  	if (status)
>>>>>>  		goto out;
>>>>>> +	if (setattr->sa_label != NULL)
>>>>>> +		status = nfsd4_set_nfs4_label(rqstp, &cstate->current_fh,
>>>>>> +				setattr->sa_label);
>>>>>> +	if (status)
>>>>>> +		goto out;
>>>>>>  	status = nfsd_setattr(rqstp, &cstate->current_fh, &setattr->sa_iattr,
>>>>>>  				0, (time_t)0);
>>>>>>  out:
>>>>>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>>>>>> index 0116886..52e219c 100644
>>>>>> --- a/fs/nfsd/nfs4xdr.c
>>>>>> +++ b/fs/nfsd/nfs4xdr.c
>>>>>> @@ -55,6 +55,11 @@
>>>>>>  #include "cache.h"
>>>>>>  #include "netns.h"
>>>>>>  
>>>>>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>>>>>> +#include <linux/security.h>
>>>>>> +#endif
>>>>>> +
>>>>>> +
>>>>>>  #define NFSDDBG_FACILITY		NFSDDBG_XDR
>>>>>>  
>>>>>>  /*
>>>>>> @@ -79,6 +84,24 @@ check_filename(char *str, int len)
>>>>>>  			return nfserr_badname;
>>>>>>  	return 0;
>>>>>>  } 
>>>>>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>>>>>> +static struct nfs4_label *nfsd4_label_alloc(u32 len)
>>>>>> +{
>>>>>> +	struct nfs4_label *label = NULL;
>>>>>> +
>>>>>> +	if (len > NFS4_MAXLABELLEN)
>>>>>> +		return ERR_PTR(-ENAMETOOLONG);
>>>>>
>>>>> This is returned as NFS4ERR_NAMETOOLONG.
>>>>>
>>>>> The 4.2 spec should note this case, if it's the right error.  rfc 5661
>>>>> says that error's only for filenames, and doesn't allow it for setattr
>>>>> (which doesn't take a filename).
>>>> The 4.2 spec (draft-ietf-nfsv4-minorversion2-19.txt) defines two Labeled 
>>>> NFS errors:
>>>> NFS4ERR_BADLABEL  (Error Code 10093)
>>>> NFS4ERR_WRONG_LFS (Error Code 10092)
>>>>
>>>> and we currently don't define either one of them... So I guess that
>>>> will have to change... 
>>>
>>> http://tools.ietf.org/html/draft-ietf-nfsv4-minorversion2-19#section-11.1.3.1
>>> just says "The label specified is invalid in some manner" for
>>> NFS4ERR_BADLABEL.
>>>
>>> Anyway, that sounds like the right one for this case....
>>>
>>>> So what should NFS4ERR_BADLABEL map to in nfserrno()?
>>>
>>> I don't know; I guess I'd check out what errors can be returned from the
>>> code that sets a context.
>> Looking looking at the code under security/selinux, the popular returns 
>> seem to be are
>> -EOPNOTSUPP, -EACCES, -EPERM and -EINVAL. 
>>
>> I say we go with -EINVAL....
> 
> Note nfserrno() converts from -ERR to nfs4 errors.  You may be thinking
> of the client side nfs4_stat_to_errno().
I was looking at nfsd4_decode_fattr()... If nfsd4_label_alloc() does fail,
host_err is set and we jump to out_nfserr:. Then status is then set by the
return value of nfserrno(host_err); I'm thinking that status should be set 
to -EINVAL

steved.




--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mimi Zohar March 31, 2013, 2:47 a.m. UTC | #21

On Fri, 2013-03-29 at 10:49 -0400, David Quigley wrote:
> On 03/29/2013 10:40, J. Bruce Fields wrote:
> > On Thu, Mar 28, 2013 at 11:35:31PM -0400, Dave Quigley wrote:
> >> On 3/28/2013 12:14 PM, J. Bruce Fields wrote:
> >> >On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
> >> >>From: David Quigley <[e-mail]>
> >> >>
> >> >>This patch adds the ability to encode and decode file labels on 
> >> the server for
> >> >>the purpose of sending them to the client and also to process 
> >> label change
> >> >>requests from the client.
> >> >>
> >> >>Signed-off-by: Matthew N. Dodd <[e-mail]>
> >> >>Signed-off-by: Miguel Rodel Felipe <[e-mail]>
> >> >>Signed-off-by: Phua Eu Gene <[e-mail]>
> >> >>Signed-off-by: Khin Mi Mi Aung <[e-mail]>
> >> >>---
> >> >>  fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++
> >> >>  fs/nfsd/nfs4xdr.c  | 116 
> >> ++++++++++++++++++++++++++++++++++++++++++++++++++---
> >> >>  fs/nfsd/nfsd.h     |   6 ++-
> >> >>  fs/nfsd/vfs.c      |  29 ++++++++++++++
> >> >>  fs/nfsd/vfs.h      |   2 +
> >> >>  fs/nfsd/xdr4.h     |   3 ++
> >> >>  6 files changed, 191 insertions(+), 6 deletions(-)
> >> >>
> >> >>diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >> >>index ae73175..bb17589 100644
> >> >>--- a/fs/nfsd/nfs4proc.c
> >> >>+++ b/fs/nfsd/nfs4proc.c
> >> >>@@ -42,6 +42,36 @@
> >> >>  #include "current_stateid.h"
> >> >>  #include "netns.h"
> >> >>
> >> >>+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> >> >>+#include <linux/security.h>
> >> >>+
> >> >>+static inline void
> >> >>+nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct 
> >> nfs4_label *label, u32 *bmval)
> >> >>+{
> >> >>+	struct inode *inode = resfh->fh_dentry->d_inode;
> >> >>+	int status;
> >> >>+
> >> >>+	mutex_lock(&inode->i_mutex);
> >> >>+	status = security_inode_setsecctx(resfh->fh_dentry,
> >> >>+		label->label, label->len);
> >> >>+	mutex_unlock(&inode->i_mutex);
> >> >>+
> >> >>+	if (status)
> >> >>+		/*
> >> >>+		 * We should probably fail the whole open at this point,
> >> >>+		 * but we've already created or opened the file, so it's
> >> >>+		 * too late; So this seems the least of evils:
> >> >>+		 */
> >> >>+		bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> >> >
> >> >Is there any way to avoid this?
> >> >
> >> >How does the vfs open code handle this?  It has to be able to set a
> >> >security contexts atomically on open(O_CREAT), doesn't it?
> >> >
> >>
> >> I believe the way this is handled is that the inode is created and
> >> labeled and then only after that is it bound to the namespace.
> >> Because of that ordering we can fail and release the inode without
> >> it ever having a dentry in the namespace.
> >
> > Grepping around....  Looks like that's done by
> > security_inode_init_security(), from the filesystem's create method.
> >
> > So we'd need to be able to pass something down to there.
> >
> > Is the current client actually expected to use this?  (So are we 
> > going
> > to see a lot of opens that set the label?)
> >
> > --b.
> 
> I don't have all the code infront of me but we have a different hook to 
> do that. The call to nfsd4_security_inode_setsecctx is supposed to be 
> used from the setattr handlers to do the equivalent of a setxattr on the 
> file over NFS. The actual creation gets done with something like 
> security_dentry_init_security which should be in an earlier patch. I'd 
> have to look more clearly at the code to find out. 

Anytime an EVM 'protected' xattr is created/modified/deleted, EVM needs
to be called to calculate and write the 'security.evm' hmac.  It looks
like the security_inode_setsecctx() hook is missing the EVM call.
Instead of writing the LSM label, immediately followed by the EVM label,
security_inode_init_security() creates the list of xattrs to be written.
Only then, it calls the fs to write them.  I would think that
security_inode_setsecctx() would need to do something similar.

thanks,

Mimi

> Also where did we 
> come up with 128 for label length? The SELinux code assumes a starting 
> point of 255 and goes up from there as needed. The MLS policies will 
> definitely exceed a 128 byte label.


--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Vu, Joseph April 1, 2013, 12:54 p.m. UTC | #22

I think 2k will do for a while. I think that in the long run it will come up short. I think the real question is whether NFS will remain viable long enough for it to matter. 

What is a good, and working alternative for NFS in term of SE label?


-----Original Message-----
From: [e-mail] [mailto:[e-mail]] On Behalf Of Casey Schaufler
Sent: Friday, March 29, 2013 3:10 PM
To: J. Bruce Fields
Cc: David Quigley; Steve Dickson; Trond Myklebust; J. Bruce Fields; David P. Quigley; Linux NFS list; Linux Security List; SELinux List; Casey Schaufler
Subject: Re: [PATCH 13/14] NFSD: Server implementation of MAC Labeling

On 3/29/2013 11:42 AM, J. Bruce Fields wrote:
> On Fri, Mar 29, 2013 at 08:18:59AM -0700, Casey Schaufler wrote:
>> On 3/29/2013 7:49 AM, David Quigley wrote:
>>> On 03/29/2013 10:40, J. Bruce Fields wrote:
>>>> On Thu, Mar 28, 2013 at 11:35:31PM -0400, Dave Quigley wrote:
>>>>> On 3/28/2013 12:14 PM, J. Bruce Fields wrote:
>>>>>> On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
>>>>>>> From: David Quigley <[e-mail]>
>>>>>>>
>>>>>>> This patch adds the ability to encode and decode file labels on
>>>>> the server for
>>>>>>> the purpose of sending them to the client and also to process
>>>>> label change
>>>>>>> requests from the client.
>>>>>>>
>>>>>>> Signed-off-by: Matthew N. Dodd <[e-mail]>
>>>>>>> Signed-off-by: Miguel Rodel Felipe <[e-mail]>
>>>>>>> Signed-off-by: Phua Eu Gene <[e-mail]>
>>>>>>> Signed-off-by: Khin Mi Mi Aung <[e-mail]>
>>>>>>> ---
>>>>>>>  fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++  fs/nfsd/nfs4xdr.c  
>>>>>>> | 116
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>>>>>  fs/nfsd/nfsd.h     |   6 ++-
>>>>>>>  fs/nfsd/vfs.c      |  29 ++++++++++++++
>>>>>>>  fs/nfsd/vfs.h      |   2 +
>>>>>>>  fs/nfsd/xdr4.h     |   3 ++
>>>>>>>  6 files changed, 191 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c index 
>>>>>>> ae73175..bb17589 100644
>>>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>>>> @@ -42,6 +42,36 @@
>>>>>>>  #include "current_stateid.h"
>>>>>>>  #include "netns.h"
>>>>>>>
>>>>>>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL #include 
>>>>>>> +<linux/security.h>
>>>>>>> +
>>>>>>> +static inline void
>>>>>>> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct
>>>>> nfs4_label *label, u32 *bmval)
>>>>>>> +{
>>>>>>> +    struct inode *inode = resfh->fh_dentry->d_inode;
>>>>>>> +    int status;
>>>>>>> +
>>>>>>> +    mutex_lock(&inode->i_mutex);
>>>>>>> +    status = security_inode_setsecctx(resfh->fh_dentry,
>>>>>>> +        label->label, label->len);
>>>>>>> +    mutex_unlock(&inode->i_mutex);
>>>>>>> +
>>>>>>> +    if (status)
>>>>>>> +        /*
>>>>>>> +         * We should probably fail the whole open at this point,
>>>>>>> +         * but we've already created or opened the file, so it's
>>>>>>> +         * too late; So this seems the least of evils:
>>>>>>> +         */
>>>>>>> +        bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
>>>>>> Is there any way to avoid this?
>>>>>>
>>>>>> How does the vfs open code handle this?  It has to be able to set 
>>>>>> a security contexts atomically on open(O_CREAT), doesn't it?
>>>>>>
>>>>> I believe the way this is handled is that the inode is created and 
>>>>> labeled and then only after that is it bound to the namespace.
>>>>> Because of that ordering we can fail and release the inode without 
>>>>> it ever having a dentry in the namespace.
>>>> Grepping around....  Looks like that's done by 
>>>> security_inode_init_security(), from the filesystem's create method.
>>>>
>>>> So we'd need to be able to pass something down to there.
>>>>
>>>> Is the current client actually expected to use this?  (So are we 
>>>> going to see a lot of opens that set the label?)
>>>>
>>>> --b.
>>> I don't have all the code infront of me but we have a different hook 
>>> to do that. The call to nfsd4_security_inode_setsecctx is supposed 
>>> to be used from the setattr handlers to do the equivalent of a 
>>> setxattr on the file over NFS. The actual creation gets done with 
>>> something like security_dentry_init_security which should be in an 
>>> earlier patch. I'd have to look more clearly at the code to find 
>>> out. Also where did we come up with 128 for label length? The 
>>> SELinux code assumes a starting point of 255 and goes up from there 
>>> as needed. The MLS policies will definitely exceed a 128 byte label.
>> If anyone cares, Smack labels can (now) be 255 characters.
>> Also, when (if) we get multiple concurrent LSMs the "security 
>> context" may include information for more than one LSM. 128 bytes is 
>> going to look pretty tiny then.
>>
>> smack='com.corportation.service'selinux='system_u:object_r:bin_t:s0'
> OK.  We need a number.  2k?

Consider some of the kinds of attributes we are likely to see in the not to distant future:

Permitted access times, at around 100 bytes each.
Bell & LaPadula labels at 500 bytes each.
Signatures of various sizes and flavors.
HEPA restrictions

I think 2k will do for a while. I think that in the long run it will come up short. I think the real question is whether NFS will remain viable long enough for it to matter.

>
> --b.
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-security-module" in the body of a message to 
> [e-mail] More majordomo info at  
> http://vger.kernel.org/majordomo-info.html
>


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to [e-mail] with the words "unsubscribe selinux" without quotes as the message.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Quigley April 1, 2013, 3:55 p.m. UTC | #23

On 04/01/2013 08:54, Vu, Joseph wrote:

> What is a good, and working alternative for NFS in term of SE label?

There isn't any unless you want to start a labeled cifs project. We 
looked at CIFS and NFSv4 back when I started this project and from what 
we saw NFS had the more open community. There are other solutions but 
they are not ideal. I believe someone did SELinux labels on network 
attached storage by treating the NAS as an iSCSI device. This isn't 
ideal because it has concurrency issues. Someone proposed xattr for 
NFSv4/NFSv3 support and that was shot down as well (and for good 
reason). I don't share Casey's skepticism about the long term importance 
of NFS. I think with NFSv4 and all the work that has gone into it we'll 
see NFS being important in Linux and enterprises for a very long time to 
come.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Casey Schaufler April 1, 2013, 4:53 p.m. UTC | #24

On 4/1/2013 5:54 AM, Vu, Joseph wrote:
> I think 2k will do for a while. I think that in the long run it will come up short. I think the real question is whether NFS will remain viable long enough for it to matter. 
>
> What is a good, and working alternative for NFS in term of SE label?

Answering this simple question is complicated.
For the short term, I don't think there is one.
For the long term I think that there has to be a
resource oriented shared storage mechanism that
will be very different from anything we have today.
We are on the cusp of the transition from the short
term to the long term. NFS, CIFS and the like are
old school solutions to file sharing, just as mode
bits and SELinux are old school security paradigms.

I am on record as not favoring this implementation
of security labeling in NFS. I believe that I
understand the rationale for the approach, and as
I don't have an alternative that hasn't been shot
down three ways from Sunday I hope to make the
best of it.
 

>
>
> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]] On Behalf Of Casey Schaufler
> Sent: Friday, March 29, 2013 3:10 PM
> To: J. Bruce Fields
> Cc: David Quigley; Steve Dickson; Trond Myklebust; J. Bruce Fields; David P. Quigley; Linux NFS list; Linux Security List; SELinux List; Casey Schaufler
> Subject: Re: [PATCH 13/14] NFSD: Server implementation of MAC Labeling
>
> On 3/29/2013 11:42 AM, J. Bruce Fields wrote:
>> On Fri, Mar 29, 2013 at 08:18:59AM -0700, Casey Schaufler wrote:
>>> On 3/29/2013 7:49 AM, David Quigley wrote:
>>>> On 03/29/2013 10:40, J. Bruce Fields wrote:
>>>>> On Thu, Mar 28, 2013 at 11:35:31PM -0400, Dave Quigley wrote:
>>>>>> On 3/28/2013 12:14 PM, J. Bruce Fields wrote:
>>>>>>> On Thu, Mar 28, 2013 at 09:54:04AM -0400, Steve Dickson wrote:
>>>>>>>> From: David Quigley <[e-mail]>
>>>>>>>>
>>>>>>>> This patch adds the ability to encode and decode file labels on
>>>>>> the server for
>>>>>>>> the purpose of sending them to the client and also to process
>>>>>> label change
>>>>>>>> requests from the client.
>>>>>>>>
>>>>>>>> Signed-off-by: Matthew N. Dodd <[e-mail]>
>>>>>>>> Signed-off-by: Miguel Rodel Felipe <[e-mail]>
>>>>>>>> Signed-off-by: Phua Eu Gene <[e-mail]>
>>>>>>>> Signed-off-by: Khin Mi Mi Aung <[e-mail]>
>>>>>>>> ---
>>>>>>>>  fs/nfsd/nfs4proc.c |  41 +++++++++++++++++++  fs/nfsd/nfs4xdr.c  
>>>>>>>> | 116
>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>>>>>>  fs/nfsd/nfsd.h     |   6 ++-
>>>>>>>>  fs/nfsd/vfs.c      |  29 ++++++++++++++
>>>>>>>>  fs/nfsd/vfs.h      |   2 +
>>>>>>>>  fs/nfsd/xdr4.h     |   3 ++
>>>>>>>>  6 files changed, 191 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c index 
>>>>>>>> ae73175..bb17589 100644
>>>>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>>>>> @@ -42,6 +42,36 @@
>>>>>>>>  #include "current_stateid.h"
>>>>>>>>  #include "netns.h"
>>>>>>>>
>>>>>>>> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL #include 
>>>>>>>> +<linux/security.h>
>>>>>>>> +
>>>>>>>> +static inline void
>>>>>>>> +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct
>>>>>> nfs4_label *label, u32 *bmval)
>>>>>>>> +{
>>>>>>>> +    struct inode *inode = resfh->fh_dentry->d_inode;
>>>>>>>> +    int status;
>>>>>>>> +
>>>>>>>> +    mutex_lock(&inode->i_mutex);
>>>>>>>> +    status = security_inode_setsecctx(resfh->fh_dentry,
>>>>>>>> +        label->label, label->len);
>>>>>>>> +    mutex_unlock(&inode->i_mutex);
>>>>>>>> +
>>>>>>>> +    if (status)
>>>>>>>> +        /*
>>>>>>>> +         * We should probably fail the whole open at this point,
>>>>>>>> +         * but we've already created or opened the file, so it's
>>>>>>>> +         * too late; So this seems the least of evils:
>>>>>>>> +         */
>>>>>>>> +        bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
>>>>>>> Is there any way to avoid this?
>>>>>>>
>>>>>>> How does the vfs open code handle this?  It has to be able to set 
>>>>>>> a security contexts atomically on open(O_CREAT), doesn't it?
>>>>>>>
>>>>>> I believe the way this is handled is that the inode is created and 
>>>>>> labeled and then only after that is it bound to the namespace.
>>>>>> Because of that ordering we can fail and release the inode without 
>>>>>> it ever having a dentry in the namespace.
>>>>> Grepping around....  Looks like that's done by 
>>>>> security_inode_init_security(), from the filesystem's create method.
>>>>>
>>>>> So we'd need to be able to pass something down to there.
>>>>>
>>>>> Is the current client actually expected to use this?  (So are we 
>>>>> going to see a lot of opens that set the label?)
>>>>>
>>>>> --b.
>>>> I don't have all the code infront of me but we have a different hook 
>>>> to do that. The call to nfsd4_security_inode_setsecctx is supposed 
>>>> to be used from the setattr handlers to do the equivalent of a 
>>>> setxattr on the file over NFS. The actual creation gets done with 
>>>> something like security_dentry_init_security which should be in an 
>>>> earlier patch. I'd have to look more clearly at the code to find 
>>>> out. Also where did we come up with 128 for label length? The 
>>>> SELinux code assumes a starting point of 255 and goes up from there 
>>>> as needed. The MLS policies will definitely exceed a 128 byte label.
>>> If anyone cares, Smack labels can (now) be 255 characters.
>>> Also, when (if) we get multiple concurrent LSMs the "security 
>>> context" may include information for more than one LSM. 128 bytes is 
>>> going to look pretty tiny then.
>>>
>>> smack='com.corportation.service'selinux='system_u:object_r:bin_t:s0'
>> OK.  We need a number.  2k?
> Consider some of the kinds of attributes we are likely to see in the not to distant future:
>
> Permitted access times, at around 100 bytes each.
> Bell & LaPadula labels at 500 bytes each.
> Signatures of various sizes and flavors.
> HEPA restrictions
>
> I think 2k will do for a while. I think that in the long run it will come up short. I think the real question is whether NFS will remain viable long enough for it to matter.
>
>> --b.
>> --
>> To unsubscribe from this list: send the line "unsubscribe 
>> linux-security-module" in the body of a message to 
>> [e-mail] More majordomo info at  
>> http://vger.kernel.org/majordomo-info.html
>>
>
> --
> This message was distributed to subscribers of the selinux mailing list.
> If you no longer wish to subscribe, send mail to [e-mail] with the words "unsubscribe selinux" without quotes as the message.
>

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Vu, Joseph April 2, 2013, 1:01 p.m. UTC | #25

Thank you David.

It is good that the community at least support some short term solution. 
Labeled NFS has been working hard to get the community acceptance.

Thanks. 
 

-----Original Message-----
From: David Quigley [mailto:[e-mail]] 
Sent: Monday, April 01, 2013 10:55 AM
To: Vu, Joseph
Cc: Casey Schaufler; J. Bruce Fields; Steve Dickson; Trond Myklebust; J. Bruce Fields; David P. Quigley; Linux NFS list; Linux Security List; SELinux List
Subject: RE: [PATCH 13/14] NFSD: Server implementation of MAC Labeling

On 04/01/2013 08:54, Vu, Joseph wrote:

> What is a good, and working alternative for NFS in term of SE label?

There isn't any unless you want to start a labeled cifs project. We looked at CIFS and NFSv4 back when I started this project and from what we saw NFS had the more open community. There are other solutions but they are not ideal. I believe someone did SELinux labels on network attached storage by treating the NAS as an iSCSI device. This isn't ideal because it has concurrency issues. Someone proposed xattr for
NFSv4/NFSv3 support and that was shot down as well (and for good reason). I don't share Casey's skepticism about the long term importance of NFS. I think with NFSv4 and all the work that has gone into it we'll see NFS being important in Linux and enterprises for a very long time to come.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Quigley April 2, 2013, 1:15 p.m. UTC | #26

On 04/02/2013 09:01, Vu, Joseph wrote:
> Thank you David.
>
> It is good that the community at least support some short term 
> solution.
> Labeled NFS has been working hard to get the community acceptance.
>
> Thanks.
>
>
> -----Original Message-----
> From: David Quigley [mailto:[e-mail]]
> Sent: Monday, April 01, 2013 10:55 AM
> To: Vu, Joseph
> Cc: Casey Schaufler; J. Bruce Fields; Steve Dickson; Trond Myklebust;
> J. Bruce Fields; David P. Quigley; Linux NFS list; Linux Security
> List; SELinux List
> Subject: RE: [PATCH 13/14] NFSD: Server implementation of MAC 
> Labeling
>
> On 04/01/2013 08:54, Vu, Joseph wrote:
>
>> What is a good, and working alternative for NFS in term of SE label?
>
> There isn't any unless you want to start a labeled cifs project. We
> looked at CIFS and NFSv4 back when I started this project and from
> what we saw NFS had the more open community. There are other 
> solutions
> but they are not ideal. I believe someone did SELinux labels on
> network attached storage by treating the NAS as an iSCSI device. This
> isn't ideal because it has concurrency issues. Someone proposed xattr
> for
> NFSv4/NFSv3 support and that was shot down as well (and for good
> reason). I don't share Casey's skepticism about the long term
> importance of NFS. I think with NFSv4 and all the work that has gone
> into it we'll see NFS being important in Linux and enterprises for a
> very long time to come.


I don't consider this a short term solution. Labeled NFS is a long term 
solution with short term milestones that get us something working fairly 
quickly and I mean fairly quickly in IETF terms (about 5 years). I don't 
buy Casey's assessment that network file-system protocols are old school 
and on the way out. A number of storage vendors are doing lots of real 
work into new versions of NFS and CIFS and they are major technologies 
in enterprise storage. To be honest I can't even figure out what sort of 
"long term" solutions Casey is talking about. It looks like he strung 
together a bunch a buzz words together into some vague ephemeral 
concept. Typing his idea of future storage into Google doesn't really 
come up with anything substantive either.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index ae73175..bb17589 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -42,6 +42,36 @@ 
 #include "current_stateid.h"
 #include "netns.h"
 
+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+#include <linux/security.h>
+
+static inline void
+nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval)
+{
+	struct inode *inode = resfh->fh_dentry->d_inode;
+	int status;
+
+	mutex_lock(&inode->i_mutex);
+	status = security_inode_setsecctx(resfh->fh_dentry,
+		label->label, label->len);
+	mutex_unlock(&inode->i_mutex);
+
+	if (status)
+		/*
+		 * We should probably fail the whole open at this point,
+		 * but we've already created or opened the file, so it's 
+		 * too late; So this seems the least of evils:
+		 */
+		bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
+		
+	return;
+}
+#else 
+static inline void 
+nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval)
+{ }
+#endif
+
 #define NFSDDBG_FACILITY		NFSDDBG_PROC
 
 static u32 nfsd_attrmask[] = {
@@ -230,6 +260,9 @@  do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o
 					(u32 *)open->op_verf.data,
 					&open->op_truncate, &open->op_created);
 
+		if (!status && open->op_label != NULL) 
+			nfsd4_security_inode_setsecctx(resfh, open->op_label, open->op_bmval);
+
 		/*
 		 * Following rfc 3530 14.2.16, use the returned bitmask
 		 * to indicate which attributes we used to store the
@@ -599,6 +632,9 @@  nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (status)
 		goto out;
 
+	if (create->cr_label != NULL)
+		nfsd4_security_inode_setsecctx(&resfh, create->cr_label, create->cr_bmval);
+
 	if (create->cr_acl != NULL)
 		do_set_nfs4_acl(rqstp, &resfh, create->cr_acl,
 				create->cr_bmval);
@@ -888,6 +924,11 @@  nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 					    setattr->sa_acl);
 	if (status)
 		goto out;
+	if (setattr->sa_label != NULL)
+		status = nfsd4_set_nfs4_label(rqstp, &cstate->current_fh,
+				setattr->sa_label);
+	if (status)
+		goto out;
 	status = nfsd_setattr(rqstp, &cstate->current_fh, &setattr->sa_iattr,
 				0, (time_t)0);
 out:
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 0116886..52e219c 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -55,6 +55,11 @@ 
 #include "cache.h"
 #include "netns.h"
 
+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+#include <linux/security.h>
+#endif
+
+
 #define NFSDDBG_FACILITY		NFSDDBG_XDR
 
 /*
@@ -79,6 +84,24 @@  check_filename(char *str, int len)
 			return nfserr_badname;
 	return 0;
 }
+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+static struct nfs4_label *nfsd4_label_alloc(u32 len)
+{
+	struct nfs4_label *label = NULL;
+
+	if (len > NFS4_MAXLABELLEN)
+		return ERR_PTR(-ENAMETOOLONG);
+
+	label = kzalloc(len + sizeof(struct nfs4_label), GFP_KERNEL);
+	if (label == NULL)
+		return ERR_PTR(-ENOMEM);
+
+	label->label = (char *)(label + 1);
+	label->len = len;
+
+	return label;
+}
+#endif
 
 #define DECODE_HEAD				\
 	__be32 *p;				\
@@ -242,7 +265,8 @@  nfsd4_decode_bitmap(struct nfsd4_compoundargs *argp, u32 *bmval)
 
 static __be32
 nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
-		   struct iattr *iattr, struct nfs4_acl **acl)
+		   struct iattr *iattr, struct nfs4_acl **acl,
+		   struct nfs4_label **label)
 {
 	int expected_len, len = 0;
 	u32 dummy32;
@@ -386,6 +410,38 @@  nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
 			goto xdr_error;
 		}
 	}
+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+	if (bmval[2] & FATTR4_WORD2_SECURITY_LABEL) {
+		uint32_t pi;
+		uint32_t lfs;
+
+		READ_BUF(4);
+		len += 4;
+		READ32(lfs);
+		READ_BUF(4);
+		len += 4;
+		READ32(pi);
+		READ_BUF(4);
+		len += 4;
+		READ32(dummy32);
+		READ_BUF(dummy32);
+		len += (XDR_QUADLEN(dummy32) << 2);
+		READMEM(buf, dummy32);
+
+		*label = nfsd4_label_alloc(dummy32);
+		if (*label == NULL) {
+			host_err = PTR_ERR(label);
+			goto out_nfserr;
+		}
+
+		memcpy((*label)->label, buf, (*label)->len);
+		((char *)(*label)->label)[(*label)->len] = '\0';
+		(*label)->pi = pi;
+		(*label)->lfs = lfs;
+
+		defer_free(argp, kfree, *label);
+	}
+#endif
 	if (bmval[0] & ~NFSD_WRITEABLE_ATTRS_WORD0
 	    || bmval[1] & ~NFSD_WRITEABLE_ATTRS_WORD1
 	    || bmval[2] & ~NFSD_WRITEABLE_ATTRS_WORD2)
@@ -582,7 +638,7 @@  nfsd4_decode_create(struct nfsd4_compoundargs *argp, struct nfsd4_create *create
 		return status;
 
 	status = nfsd4_decode_fattr(argp, create->cr_bmval, &create->cr_iattr,
-				    &create->cr_acl);
+				    &create->cr_acl, &create->cr_label);
 	if (status)
 		goto out;
 
@@ -832,7 +888,7 @@  nfsd4_decode_open(struct nfsd4_compoundargs *argp, struct nfsd4_open *open)
 		case NFS4_CREATE_UNCHECKED:
 		case NFS4_CREATE_GUARDED:
 			status = nfsd4_decode_fattr(argp, open->op_bmval,
-				&open->op_iattr, &open->op_acl);
+				&open->op_iattr, &open->op_acl, &open->op_label);
 			if (status)
 				goto out;
 			break;
@@ -846,7 +902,7 @@  nfsd4_decode_open(struct nfsd4_compoundargs *argp, struct nfsd4_open *open)
 			READ_BUF(NFS4_VERIFIER_SIZE);
 			COPYMEM(open->op_verf.data, NFS4_VERIFIER_SIZE);
 			status = nfsd4_decode_fattr(argp, open->op_bmval,
-				&open->op_iattr, &open->op_acl);
+				&open->op_iattr, &open->op_acl, &open->op_label);
 			if (status)
 				goto out;
 			break;
@@ -1068,7 +1124,7 @@  nfsd4_decode_setattr(struct nfsd4_compoundargs *argp, struct nfsd4_setattr *seta
 	if (status)
 		return status;
 	return nfsd4_decode_fattr(argp, setattr->sa_bmval, &setattr->sa_iattr,
-				  &setattr->sa_acl);
+				  &setattr->sa_acl, &setattr->sa_label);
 }
 
 static __be32
@@ -1988,6 +2044,50 @@  nfsd4_encode_aclname(struct svc_rqst *rqstp, struct nfs4_ace *ace,
 			      FATTR4_WORD0_RDATTR_ERROR)
 #define WORD1_ABSENT_FS_ATTRS FATTR4_WORD1_MOUNTED_ON_FILEID
 
+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+static inline __be32
+nfsd4_encode_security_label(struct svc_rqst *rqstp, struct dentry *dentry, __be32 **pp, int *buflen)
+{
+	void *context;
+	int err;
+	int len;
+	uint32_t pi = 0;
+	uint32_t lfs = 0;
+	__be32 *p = *pp;
+
+	err = 0;
+	(void)security_inode_getsecctx(dentry->d_inode, &context, &len);
+	if (len < 0)
+		return nfserrno(len);
+
+	if (*buflen < ((XDR_QUADLEN(len) << 2) + 4 + 4 + 4)) {
+		err = nfserr_resource;
+		goto out;
+	}
+
+	/* XXX: A call to the translation code should be placed here
+	 * for now send 0  until we have that to indicate the null
+	 * translation */ 
+
+	if ((*buflen -= 4) < 0)
+		return nfserr_resource;
+
+	WRITE32(lfs);	
+	WRITE32(pi);
+	p = xdr_encode_opaque(p, context, len);
+	*buflen -= (XDR_QUADLEN(len) << 2) + 4;
+
+	*pp = p;
+out:
+	security_release_secctx(context, len);
+	return err;
+}
+#else
+static inline __be32
+nfsd4_encode_security_label(struct svc_rqst *rqstp, struct dentry *dentry, __be32 **pp, int *buflen)
+{ return 0; }
+#endif
+
 static __be32 fattr_handle_absent_fs(u32 *bmval0, u32 *bmval1, u32 *rdattr_err)
 {
 	/* As per referral draft:  */
@@ -2438,6 +2538,12 @@  out_acl:
 			get_parent_attributes(exp, &stat);
 		WRITE64(stat.ino);
 	}
+	if (bmval2 & FATTR4_WORD2_SECURITY_LABEL) {
+		status = nfsd4_encode_security_label(rqstp, dentry,
+				&p, &buflen);
+		if (status)
+			goto out;
+	}
 	if (bmval2 & FATTR4_WORD2_SUPPATTR_EXCLCREAT) {
 		WRITE32(3);
 		WRITE32(NFSD_SUPPATTR_EXCLCREAT_WORD0);
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index 07a473f..62dc89b 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -311,7 +311,11 @@  void		nfsd_lockd_shutdown(void);
  | FATTR4_WORD1_TIME_DELTA   | FATTR4_WORD1_TIME_METADATA    \
  | FATTR4_WORD1_TIME_MODIFY     | FATTR4_WORD1_TIME_MODIFY_SET | FATTR4_WORD1_MOUNTED_ON_FILEID)
 
+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+#define NFSD4_SUPPORTED_ATTRS_WORD2 FATTR4_WORD2_SECURITY_LABEL
+#else
 #define NFSD4_SUPPORTED_ATTRS_WORD2 0
+#endif
 
 #define NFSD4_1_SUPPORTED_ATTRS_WORD0 \
 	NFSD4_SUPPORTED_ATTRS_WORD0
@@ -350,7 +354,7 @@  static inline u32 nfsd_suppattrs2(u32 minorversion)
 #define NFSD_WRITEABLE_ATTRS_WORD1 \
 	(FATTR4_WORD1_MODE | FATTR4_WORD1_OWNER | FATTR4_WORD1_OWNER_GROUP \
 	| FATTR4_WORD1_TIME_ACCESS_SET | FATTR4_WORD1_TIME_MODIFY_SET)
-#define NFSD_WRITEABLE_ATTRS_WORD2 0
+#define NFSD_WRITEABLE_ATTRS_WORD2 FATTR4_WORD2_SECURITY_LABEL
 
 #define NFSD_SUPPATTR_EXCLCREAT_WORD0 \
 	NFSD_WRITEABLE_ATTRS_WORD0
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 2b2e239..b2b2e5a 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -28,6 +28,7 @@ 
 #include <asm/uaccess.h>
 #include <linux/exportfs.h>
 #include <linux/writeback.h>
+#include <linux/security.h>
 
 #ifdef CONFIG_NFSD_V3
 #include "xdr3.h"
@@ -621,6 +622,34 @@  int nfsd4_is_junction(struct dentry *dentry)
 		return 0;
 	return 1;
 }
+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
+		struct nfs4_label *label)
+{
+	__be32 error;
+	int host_error;
+	struct dentry *dentry;
+
+	/* XXX: should we have a MAY_SSECCTX? */
+	error = fh_verify(rqstp, fhp, 0 /* S_IFREG */, NFSD_MAY_SATTR);
+	if (error)
+		return error;
+
+	dentry = fhp->fh_dentry;
+
+	mutex_lock(&dentry->d_inode->i_mutex);
+	host_error = security_inode_setsecctx(dentry, label->label, label->len);
+	mutex_unlock(&dentry->d_inode->i_mutex);
+	return nfserrno(host_error);
+}
+#else
+__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
+		struct nfs4_label *label)
+{
+	return -EOPNOTSUPP;
+}
+#endif
+
 #endif /* defined(CONFIG_NFSD_V4) */
 
 #ifdef CONFIG_NFSD_V3
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 5b58941..a9d6f0f 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -56,6 +56,8 @@  int nfsd_mountpoint(struct dentry *, struct svc_export *);
 __be32          nfsd4_set_nfs4_acl(struct svc_rqst *, struct svc_fh *,
                     struct nfs4_acl *);
 int             nfsd4_get_nfs4_acl(struct svc_rqst *, struct dentry *, struct nfs4_acl **);
+__be32          nfsd4_set_nfs4_label(struct svc_rqst *, struct svc_fh *,
+		    struct nfs4_label *);
 #endif /* CONFIG_NFSD_V4 */
 __be32		nfsd_create(struct svc_rqst *, struct svc_fh *,
 				char *name, int len, struct iattr *attrs,
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 546f898..132a671 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -118,6 +118,7 @@  struct nfsd4_create {
 	struct iattr	cr_iattr;           /* request */
 	struct nfsd4_change_info  cr_cinfo; /* response */
 	struct nfs4_acl *cr_acl;
+	struct nfs4_label *cr_label;
 };
 #define cr_linklen	u.link.namelen
 #define cr_linkname	u.link.name
@@ -246,6 +247,7 @@  struct nfsd4_open {
 	struct nfs4_file *op_file;          /* used during processing */
 	struct nfs4_ol_stateid *op_stp;	    /* used during processing */
 	struct nfs4_acl *op_acl;
+	struct nfs4_label *op_label;
 };
 #define op_iattr	iattr
 
@@ -330,6 +332,7 @@  struct nfsd4_setattr {
 	u32		sa_bmval[3];        /* request */
 	struct iattr	sa_iattr;           /* request */
 	struct nfs4_acl *sa_acl;
+	struct nfs4_label *sa_label;
 };
 
 struct nfsd4_setclientid {