On Thu, May 2, 2024 at 6:34 PM Jeffrey Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxx> > > --- > > v2 introduces a nfsd_attrs_valid() helper and uses it as suggested by > > Jeffrey Layton <jlayton@xxxxxxxxxx>. > > > > 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.
