diff mbox series

[v2] nfsd: set security label during create operations

Message ID 20240502195800.3252-1-stephen.smalley.work@gmail.com (mailing list archive)
State Superseded
Delegated to: Paul Moore
Headers show
Series [v2] nfsd: set security label during create operations | expand

Commit Message

Stephen Smalley May 2, 2024, 7:58 p.m. UTC
When security labeling is enabled, the client can pass a file security
label as part of a create operation for the new file, similar to mode
and other attributes. At present, the security label is received by nfsd
and passed down to nfsd_create_setattr(), but nfsd_setattr() is never
called and therefore the label is never set on the new file. I couldn't
tell if this has always been broken or broke at some point in time. Looking
at nfsd_setattr() I am uncertain as to whether the same issue presents for
file ACLs and therefore requires a similar fix for those. I am not overly
confident that this is the right solution.

An alternative approach would be to introduce a new LSM hook to set the
"create SID" of the current task prior to the actual file creation, which
would atomically label the new inode at creation time. This would be better
for SELinux and a similar approach has been used previously
(see security_dentry_create_files_as) but perhaps not usable by other LSMs.

Reproducer:
1. Install a Linux distro with SELinux - Fedora is easiest
2. git clone https://github.com/SELinuxProject/selinux-testsuite
3. Install the requisite dependencies per selinux-testsuite/README.md
4. Run something like the following script:
MOUNT=$HOME/selinux-testsuite
sudo systemctl start nfs-server
sudo exportfs -o rw,no_root_squash,security_label localhost:$MOUNT
sudo mkdir -p /mnt/selinux-testsuite
sudo mount -t nfs -o vers=4.2 localhost:$MOUNT /mnt/selinux-testsuite
pushd /mnt/selinux-testsuite/
sudo make -C policy load
pushd tests/filesystem
sudo runcon -t test_filesystem_t ./create_file -f trans_test_file \
	-e test_filesystem_filetranscon_t -v
sudo rm -f trans_test_file
popd
sudo make -C policy unload
popd
sudo umount /mnt/selinux-testsuite
sudo exportfs -u localhost:$MOUNT
sudo rmdir /mnt/selinux-testsuite
sudo systemctl stop nfs-server

Expected output:
<eliding noise from commands run prior to or after the test itself>
Process context:
	unconfined_u:unconfined_r:test_filesystem_t:s0-s0:c0.c1023
Created file: trans_test_file
File context: unconfined_u:object_r:test_filesystem_filetranscon_t:s0
File context is correct

Actual output:
<eliding noise from commands run prior to or after the test itself>
Process context:
	unconfined_u:unconfined_r:test_filesystem_t:s0-s0:c0.c1023
Created file: trans_test_file
File context: system_u:object_r:test_file_t:s0
File context error, expected:
	test_filesystem_filetranscon_t
got:
	test_file_t

Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
---
v2 introduces a nfsd_attrs_valid() helper and uses it as suggested by
Jeffrey Layton <jlayton@kernel.org>.

 fs/nfsd/nfsproc.c | 2 +-
 fs/nfsd/vfs.c     | 2 +-
 fs/nfsd/vfs.h     | 8 ++++++++
 3 files changed, 10 insertions(+), 2 deletions(-)

Comments

Chuck Lever May 2, 2024, 8:17 p.m. UTC | #1
On Thu, May 02, 2024 at 03:58:01PM -0400, Stephen Smalley wrote:
> When security labeling is enabled, the client can pass a file security
> label as part of a create operation for the new file, similar to mode
> and other attributes. At present, the security label is received by nfsd
> and passed down to nfsd_create_setattr(), but nfsd_setattr() is never
> called and therefore the label is never set on the new file. I couldn't
> tell if this has always been broken or broke at some point in time.

Might have been introduced on or around commit d6a97d3f589a ("NFSD:
add security label to struct nfsd_attrs"). Neil, can you spare an
eyeball or two for Stephen's patch?


> Looking
> at nfsd_setattr() I am uncertain as to whether the same issue presents for
> file ACLs and therefore requires a similar fix for those. I am not overly
> confident that this is the right solution.
> 
> An alternative approach would be to introduce a new LSM hook to set the
> "create SID" of the current task prior to the actual file creation, which
> would atomically label the new inode at creation time. This would be better
> for SELinux and a similar approach has been used previously
> (see security_dentry_create_files_as) but perhaps not usable by other LSMs.
> 
> Reproducer:
> 1. Install a Linux distro with SELinux - Fedora is easiest
> 2. git clone https://github.com/SELinuxProject/selinux-testsuite
> 3. Install the requisite dependencies per selinux-testsuite/README.md
> 4. Run something like the following script:
> MOUNT=$HOME/selinux-testsuite
> sudo systemctl start nfs-server
> sudo exportfs -o rw,no_root_squash,security_label localhost:$MOUNT
> sudo mkdir -p /mnt/selinux-testsuite
> sudo mount -t nfs -o vers=4.2 localhost:$MOUNT /mnt/selinux-testsuite
> pushd /mnt/selinux-testsuite/
> sudo make -C policy load
> pushd tests/filesystem
> sudo runcon -t test_filesystem_t ./create_file -f trans_test_file \
> 	-e test_filesystem_filetranscon_t -v
> sudo rm -f trans_test_file
> popd
> sudo make -C policy unload
> popd
> sudo umount /mnt/selinux-testsuite
> sudo exportfs -u localhost:$MOUNT
> sudo rmdir /mnt/selinux-testsuite
> sudo systemctl stop nfs-server
> 
> Expected output:
> <eliding noise from commands run prior to or after the test itself>
> Process context:
> 	unconfined_u:unconfined_r:test_filesystem_t:s0-s0:c0.c1023
> Created file: trans_test_file
> File context: unconfined_u:object_r:test_filesystem_filetranscon_t:s0
> File context is correct
> 
> Actual output:
> <eliding noise from commands run prior to or after the test itself>
> Process context:
> 	unconfined_u:unconfined_r:test_filesystem_t:s0-s0:c0.c1023
> Created file: trans_test_file
> File context: system_u:object_r:test_file_t:s0
> File context error, expected:
> 	test_filesystem_filetranscon_t
> got:
> 	test_file_t
> 
> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---
> v2 introduces a nfsd_attrs_valid() helper and uses it as suggested by
> Jeffrey Layton <jlayton@kernel.org>.
> 
>  fs/nfsd/nfsproc.c | 2 +-
>  fs/nfsd/vfs.c     | 2 +-
>  fs/nfsd/vfs.h     | 8 ++++++++
>  3 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index 36370b957b63..3e438159f561 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -389,7 +389,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
>  		 * open(..., O_CREAT|O_TRUNC|O_WRONLY).
>  		 */
>  		attr->ia_valid &= ATTR_SIZE;
> -		if (attr->ia_valid)
> +		if (nfsd_attrs_valid(attr))
>  			resp->status = nfsd_setattr(rqstp, newfhp, &attrs,
>  						    NULL);
>  	}
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 2e41eb4c3cec..29b1f3613800 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1422,7 +1422,7 @@ nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	 * Callers expect new file metadata to be committed even
>  	 * if the attributes have not changed.
>  	 */
> -	if (iap->ia_valid)
> +	if (nfsd_attrs_valid(attrs))
>  		status = nfsd_setattr(rqstp, resfhp, attrs, NULL);
>  	else
>  		status = nfserrno(commit_metadata(resfhp));
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index c60fdb6200fd..57cd70062048 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -60,6 +60,14 @@ static inline void nfsd_attrs_free(struct nfsd_attrs *attrs)
>  	posix_acl_release(attrs->na_dpacl);
>  }
>  
> +static inline bool nfsd_attrs_valid(struct nfsd_attrs *attrs)
> +{
> +	struct iattr *iap = attrs->na_iattr;
> +
> +	return (iap->ia_valid || (attrs->na_seclabel &&
> +		attrs->na_seclabel->len));
> +}
> +
>  __be32		nfserrno (int errno);
>  int		nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
>  		                struct svc_export **expp);
> -- 
> 2.40.1
>
Jeff Layton May 2, 2024, 10:34 p.m. UTC | #2
On Thu, 2024-05-02 at 15:58 -0400, Stephen Smalley wrote:
> When security labeling is enabled, the client can pass a file security
> label as part of a create operation for the new file, similar to mode
> and other attributes. At present, the security label is received by nfsd
> and passed down to nfsd_create_setattr(), but nfsd_setattr() is never
> called and therefore the label is never set on the new file. I couldn't
> tell if this has always been broken or broke at some point in time. Looking
> at nfsd_setattr() I am uncertain as to whether the same issue presents for
> file ACLs and therefore requires a similar fix for those. I am not overly
> confident that this is the right solution.
> 
> An alternative approach would be to introduce a new LSM hook to set the
> "create SID" of the current task prior to the actual file creation, which
> would atomically label the new inode at creation time. This would be better
> for SELinux and a similar approach has been used previously
> (see security_dentry_create_files_as) but perhaps not usable by other LSMs.
> 
> Reproducer:
> 1. Install a Linux distro with SELinux - Fedora is easiest
> 2. git clone https://github.com/SELinuxProject/selinux-testsuite
> 3. Install the requisite dependencies per selinux-testsuite/README.md
> 4. Run something like the following script:
> MOUNT=$HOME/selinux-testsuite
> sudo systemctl start nfs-server
> sudo exportfs -o rw,no_root_squash,security_label localhost:$MOUNT
> sudo mkdir -p /mnt/selinux-testsuite
> sudo mount -t nfs -o vers=4.2 localhost:$MOUNT /mnt/selinux-testsuite
> pushd /mnt/selinux-testsuite/
> sudo make -C policy load
> pushd tests/filesystem
> sudo runcon -t test_filesystem_t ./create_file -f trans_test_file \
> 	-e test_filesystem_filetranscon_t -v
> sudo rm -f trans_test_file
> popd
> sudo make -C policy unload
> popd
> sudo umount /mnt/selinux-testsuite
> sudo exportfs -u localhost:$MOUNT
> sudo rmdir /mnt/selinux-testsuite
> sudo systemctl stop nfs-server
> 
> Expected output:
> <eliding noise from commands run prior to or after the test itself>
> Process context:
> 	unconfined_u:unconfined_r:test_filesystem_t:s0-s0:c0.c1023
> Created file: trans_test_file
> File context: unconfined_u:object_r:test_filesystem_filetranscon_t:s0
> File context is correct
> 
> Actual output:
> <eliding noise from commands run prior to or after the test itself>
> Process context:
> 	unconfined_u:unconfined_r:test_filesystem_t:s0-s0:c0.c1023
> Created file: trans_test_file
> File context: system_u:object_r:test_file_t:s0
> File context error, expected:
> 	test_filesystem_filetranscon_t
> got:
> 	test_file_t
> 
> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---
> v2 introduces a nfsd_attrs_valid() helper and uses it as suggested by
> Jeffrey Layton <jlayton@kernel.org>.
> 
>  fs/nfsd/nfsproc.c | 2 +-
>  fs/nfsd/vfs.c     | 2 +-
>  fs/nfsd/vfs.h     | 8 ++++++++
>  3 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index 36370b957b63..3e438159f561 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -389,7 +389,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
>  		 * open(..., O_CREAT|O_TRUNC|O_WRONLY).
>  		 */
>  		attr->ia_valid &= ATTR_SIZE;
> -		if (attr->ia_valid)
> +		if (nfsd_attrs_valid(attr))
>  			resp->status = nfsd_setattr(rqstp, newfhp, &attrs,
>  						    NULL);
>  	}

This function is for NFSv2, which doesn't support any inode attributes
that aren't represented in ia_valid. We could leave this as-is, but
this is fine too.

> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 2e41eb4c3cec..29b1f3613800 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1422,7 +1422,7 @@ nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	 * Callers expect new file metadata to be committed even
>  	 * if the attributes have not changed.
>  	 */
> -	if (iap->ia_valid)
> +	if (nfsd_attrs_valid(attrs))
>  		status = nfsd_setattr(rqstp, resfhp, attrs, NULL);
>  	else
>  		status = nfserrno(commit_metadata(resfhp));
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index c60fdb6200fd..57cd70062048 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -60,6 +60,14 @@ static inline void nfsd_attrs_free(struct nfsd_attrs *attrs)
>  	posix_acl_release(attrs->na_dpacl);
>  }
>  
> +static inline bool nfsd_attrs_valid(struct nfsd_attrs *attrs)
> +{
> +	struct iattr *iap = attrs->na_iattr;
> +
> +	return (iap->ia_valid || (attrs->na_seclabel &&
> +		attrs->na_seclabel->len));
> +}
> +
>  __be32		nfserrno (int errno);
>  int		nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
>  		                struct svc_export **expp);

Reviewed-by: Jeffrey Layton <jlayton@kernel.org>
kernel test robot May 3, 2024, 7:31 a.m. UTC | #3
Hi Stephen,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.9-rc6 next-20240502]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Stephen-Smalley/nfsd-set-security-label-during-create-operations/20240503-040242
base:   linus/master
patch link:    https://lore.kernel.org/r/20240502195800.3252-1-stephen.smalley.work%40gmail.com
patch subject: [PATCH v2] nfsd: set security label during create operations
config: arm64-randconfig-r123-20240503 (https://download.01.org/0day-ci/archive/20240503/202405031516.kghPPWFt-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 37ae4ad0eef338776c7e2cffb3896153d43dcd90)
reproduce: (https://download.01.org/0day-ci/archive/20240503/202405031516.kghPPWFt-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405031516.kghPPWFt-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from fs/nfsd/nfsproc.c:10:
   In file included from fs/nfsd/cache.h:12:
   In file included from include/linux/sunrpc/svc.h:17:
   In file included from include/linux/sunrpc/xdr.h:17:
   In file included from include/linux/scatterlist.h:8:
   In file included from include/linux/mm.h:2210:
   include/linux/vmstat.h:508:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     508 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     509 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:515:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     515 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     516 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:522:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     522 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:527:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     527 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     528 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:536:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     536 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     537 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
>> fs/nfsd/nfsproc.c:392:24: error: incompatible pointer types passing 'struct iattr *' to parameter of type 'struct nfsd_attrs *' [-Werror,-Wincompatible-pointer-types]
     392 |                 if (nfsd_attrs_valid(attr))
         |                                      ^~~~
   fs/nfsd/vfs.h:63:56: note: passing argument to parameter 'attrs' here
      63 | static inline bool nfsd_attrs_valid(struct nfsd_attrs *attrs)
         |                                                        ^
   5 warnings and 1 error generated.


vim +392 fs/nfsd/nfsproc.c

   240	
   241	/*
   242	 * CREATE processing is complicated. The keyword here is `overloaded.'
   243	 * The parent directory is kept locked between the check for existence
   244	 * and the actual create() call in compliance with VFS protocols.
   245	 * N.B. After this call _both_ argp->fh and resp->fh need an fh_put
   246	 */
   247	static __be32
   248	nfsd_proc_create(struct svc_rqst *rqstp)
   249	{
   250		struct nfsd_createargs *argp = rqstp->rq_argp;
   251		struct nfsd_diropres *resp = rqstp->rq_resp;
   252		svc_fh		*dirfhp = &argp->fh;
   253		svc_fh		*newfhp = &resp->fh;
   254		struct iattr	*attr = &argp->attrs;
   255		struct nfsd_attrs attrs = {
   256			.na_iattr	= attr,
   257		};
   258		struct inode	*inode;
   259		struct dentry	*dchild;
   260		int		type, mode;
   261		int		hosterr;
   262		dev_t		rdev = 0, wanted = new_decode_dev(attr->ia_size);
   263	
   264		dprintk("nfsd: CREATE   %s %.*s\n",
   265			SVCFH_fmt(dirfhp), argp->len, argp->name);
   266	
   267		/* First verify the parent file handle */
   268		resp->status = fh_verify(rqstp, dirfhp, S_IFDIR, NFSD_MAY_EXEC);
   269		if (resp->status != nfs_ok)
   270			goto done; /* must fh_put dirfhp even on error */
   271	
   272		/* Check for NFSD_MAY_WRITE in nfsd_create if necessary */
   273	
   274		resp->status = nfserr_exist;
   275		if (isdotent(argp->name, argp->len))
   276			goto done;
   277		hosterr = fh_want_write(dirfhp);
   278		if (hosterr) {
   279			resp->status = nfserrno(hosterr);
   280			goto done;
   281		}
   282	
   283		inode_lock_nested(dirfhp->fh_dentry->d_inode, I_MUTEX_PARENT);
   284		dchild = lookup_one_len(argp->name, dirfhp->fh_dentry, argp->len);
   285		if (IS_ERR(dchild)) {
   286			resp->status = nfserrno(PTR_ERR(dchild));
   287			goto out_unlock;
   288		}
   289		fh_init(newfhp, NFS_FHSIZE);
   290		resp->status = fh_compose(newfhp, dirfhp->fh_export, dchild, dirfhp);
   291		if (!resp->status && d_really_is_negative(dchild))
   292			resp->status = nfserr_noent;
   293		dput(dchild);
   294		if (resp->status) {
   295			if (resp->status != nfserr_noent)
   296				goto out_unlock;
   297			/*
   298			 * If the new file handle wasn't verified, we can't tell
   299			 * whether the file exists or not. Time to bail ...
   300			 */
   301			resp->status = nfserr_acces;
   302			if (!newfhp->fh_dentry) {
   303				printk(KERN_WARNING 
   304					"nfsd_proc_create: file handle not verified\n");
   305				goto out_unlock;
   306			}
   307		}
   308	
   309		inode = d_inode(newfhp->fh_dentry);
   310	
   311		/* Unfudge the mode bits */
   312		if (attr->ia_valid & ATTR_MODE) {
   313			type = attr->ia_mode & S_IFMT;
   314			mode = attr->ia_mode & ~S_IFMT;
   315			if (!type) {
   316				/* no type, so if target exists, assume same as that,
   317				 * else assume a file */
   318				if (inode) {
   319					type = inode->i_mode & S_IFMT;
   320					switch(type) {
   321					case S_IFCHR:
   322					case S_IFBLK:
   323						/* reserve rdev for later checking */
   324						rdev = inode->i_rdev;
   325						attr->ia_valid |= ATTR_SIZE;
   326	
   327						fallthrough;
   328					case S_IFIFO:
   329						/* this is probably a permission check..
   330						 * at least IRIX implements perm checking on
   331						 *   echo thing > device-special-file-or-pipe
   332						 * by doing a CREATE with type==0
   333						 */
   334						resp->status = nfsd_permission(rqstp,
   335									 newfhp->fh_export,
   336									 newfhp->fh_dentry,
   337									 NFSD_MAY_WRITE|NFSD_MAY_LOCAL_ACCESS);
   338						if (resp->status && resp->status != nfserr_rofs)
   339							goto out_unlock;
   340					}
   341				} else
   342					type = S_IFREG;
   343			}
   344		} else if (inode) {
   345			type = inode->i_mode & S_IFMT;
   346			mode = inode->i_mode & ~S_IFMT;
   347		} else {
   348			type = S_IFREG;
   349			mode = 0;	/* ??? */
   350		}
   351	
   352		attr->ia_valid |= ATTR_MODE;
   353		attr->ia_mode = mode;
   354	
   355		/* Special treatment for non-regular files according to the
   356		 * gospel of sun micro
   357		 */
   358		if (type != S_IFREG) {
   359			if (type != S_IFBLK && type != S_IFCHR) {
   360				rdev = 0;
   361			} else if (type == S_IFCHR && !(attr->ia_valid & ATTR_SIZE)) {
   362				/* If you think you've seen the worst, grok this. */
   363				type = S_IFIFO;
   364			} else {
   365				/* Okay, char or block special */
   366				if (!rdev)
   367					rdev = wanted;
   368			}
   369	
   370			/* we've used the SIZE information, so discard it */
   371			attr->ia_valid &= ~ATTR_SIZE;
   372	
   373			/* Make sure the type and device matches */
   374			resp->status = nfserr_exist;
   375			if (inode && inode_wrong_type(inode, type))
   376				goto out_unlock;
   377		}
   378	
   379		resp->status = nfs_ok;
   380		if (!inode) {
   381			/* File doesn't exist. Create it and set attrs */
   382			resp->status = nfsd_create_locked(rqstp, dirfhp, &attrs, type,
   383							  rdev, newfhp);
   384		} else if (type == S_IFREG) {
   385			dprintk("nfsd:   existing %s, valid=%x, size=%ld\n",
   386				argp->name, attr->ia_valid, (long) attr->ia_size);
   387			/* File already exists. We ignore all attributes except
   388			 * size, so that creat() behaves exactly like
   389			 * open(..., O_CREAT|O_TRUNC|O_WRONLY).
   390			 */
   391			attr->ia_valid &= ATTR_SIZE;
 > 392			if (nfsd_attrs_valid(attr))
   393				resp->status = nfsd_setattr(rqstp, newfhp, &attrs,
   394							    NULL);
   395		}
   396	
   397	out_unlock:
   398		inode_unlock(dirfhp->fh_dentry->d_inode);
   399		fh_drop_write(dirfhp);
   400	done:
   401		fh_put(dirfhp);
   402		if (resp->status != nfs_ok)
   403			goto out;
   404		resp->status = fh_getattr(&resp->fh, &resp->stat);
   405	out:
   406		return rpc_success;
   407	}
   408
Stephen Smalley May 3, 2024, 12:48 p.m. UTC | #4
On Thu, May 2, 2024 at 6:34 PM Jeffrey Layton <jlayton@kernel.org> wrote:
>
> On Thu, 2024-05-02 at 15:58 -0400, Stephen Smalley wrote:
> > When security labeling is enabled, the client can pass a file security
> > label as part of a create operation for the new file, similar to mode
> > and other attributes. At present, the security label is received by nfsd
> > and passed down to nfsd_create_setattr(), but nfsd_setattr() is never
> > called and therefore the label is never set on the new file. I couldn't
> > tell if this has always been broken or broke at some point in time. Looking
> > at nfsd_setattr() I am uncertain as to whether the same issue presents for
> > file ACLs and therefore requires a similar fix for those. I am not overly
> > confident that this is the right solution.
> >
> > An alternative approach would be to introduce a new LSM hook to set the
> > "create SID" of the current task prior to the actual file creation, which
> > would atomically label the new inode at creation time. This would be better
> > for SELinux and a similar approach has been used previously
> > (see security_dentry_create_files_as) but perhaps not usable by other LSMs.
> >
> > Reproducer:
> > 1. Install a Linux distro with SELinux - Fedora is easiest
> > 2. git clone https://github.com/SELinuxProject/selinux-testsuite
> > 3. Install the requisite dependencies per selinux-testsuite/README.md
> > 4. Run something like the following script:
> > MOUNT=$HOME/selinux-testsuite
> > sudo systemctl start nfs-server
> > sudo exportfs -o rw,no_root_squash,security_label localhost:$MOUNT
> > sudo mkdir -p /mnt/selinux-testsuite
> > sudo mount -t nfs -o vers=4.2 localhost:$MOUNT /mnt/selinux-testsuite
> > pushd /mnt/selinux-testsuite/
> > sudo make -C policy load
> > pushd tests/filesystem
> > sudo runcon -t test_filesystem_t ./create_file -f trans_test_file \
> >       -e test_filesystem_filetranscon_t -v
> > sudo rm -f trans_test_file
> > popd
> > sudo make -C policy unload
> > popd
> > sudo umount /mnt/selinux-testsuite
> > sudo exportfs -u localhost:$MOUNT
> > sudo rmdir /mnt/selinux-testsuite
> > sudo systemctl stop nfs-server
> >
> > Expected output:
> > <eliding noise from commands run prior to or after the test itself>
> > Process context:
> >       unconfined_u:unconfined_r:test_filesystem_t:s0-s0:c0.c1023
> > Created file: trans_test_file
> > File context: unconfined_u:object_r:test_filesystem_filetranscon_t:s0
> > File context is correct
> >
> > Actual output:
> > <eliding noise from commands run prior to or after the test itself>
> > Process context:
> >       unconfined_u:unconfined_r:test_filesystem_t:s0-s0:c0.c1023
> > Created file: trans_test_file
> > File context: system_u:object_r:test_file_t:s0
> > File context error, expected:
> >       test_filesystem_filetranscon_t
> > got:
> >       test_file_t
> >
> > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > ---
> > v2 introduces a nfsd_attrs_valid() helper and uses it as suggested by
> > Jeffrey Layton <jlayton@kernel.org>.
> >
> >  fs/nfsd/nfsproc.c | 2 +-
> >  fs/nfsd/vfs.c     | 2 +-
> >  fs/nfsd/vfs.h     | 8 ++++++++
> >  3 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> > index 36370b957b63..3e438159f561 100644
> > --- a/fs/nfsd/nfsproc.c
> > +++ b/fs/nfsd/nfsproc.c
> > @@ -389,7 +389,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
> >                * open(..., O_CREAT|O_TRUNC|O_WRONLY).
> >                */
> >               attr->ia_valid &= ATTR_SIZE;
> > -             if (attr->ia_valid)
> > +             if (nfsd_attrs_valid(attr))
> >                       resp->status = nfsd_setattr(rqstp, newfhp, &attrs,
> >                                                   NULL);
> >       }
>
> This function is for NFSv2, which doesn't support any inode attributes
> that aren't represented in ia_valid. We could leave this as-is, but
> this is fine too.

Sorry, I got over-eager with trying to fix all ia_valid checks. It's
actually wrong so I'll send a 3rd version without it.
diff mbox series

Patch

diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index 36370b957b63..3e438159f561 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -389,7 +389,7 @@  nfsd_proc_create(struct svc_rqst *rqstp)
 		 * open(..., O_CREAT|O_TRUNC|O_WRONLY).
 		 */
 		attr->ia_valid &= ATTR_SIZE;
-		if (attr->ia_valid)
+		if (nfsd_attrs_valid(attr))
 			resp->status = nfsd_setattr(rqstp, newfhp, &attrs,
 						    NULL);
 	}
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 2e41eb4c3cec..29b1f3613800 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1422,7 +1422,7 @@  nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	 * Callers expect new file metadata to be committed even
 	 * if the attributes have not changed.
 	 */
-	if (iap->ia_valid)
+	if (nfsd_attrs_valid(attrs))
 		status = nfsd_setattr(rqstp, resfhp, attrs, NULL);
 	else
 		status = nfserrno(commit_metadata(resfhp));
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index c60fdb6200fd..57cd70062048 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -60,6 +60,14 @@  static inline void nfsd_attrs_free(struct nfsd_attrs *attrs)
 	posix_acl_release(attrs->na_dpacl);
 }
 
+static inline bool nfsd_attrs_valid(struct nfsd_attrs *attrs)
+{
+	struct iattr *iap = attrs->na_iattr;
+
+	return (iap->ia_valid || (attrs->na_seclabel &&
+		attrs->na_seclabel->len));
+}
+
 __be32		nfserrno (int errno);
 int		nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
 		                struct svc_export **expp);