On Thu, 2024-05-02 at 13: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. > Nice catch. I think you're correct on file ACLs too. We're probably saved in many cases by the fact that clients usually send ACLs and seclabels alongside other attributes during a create. Obviously, that's not _always_ the case though. > 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> > --- > fs/nfsd/vfs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 2e41eb4c3cec..9b777ea7ef26 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 (iap->ia_valid || (attrs->na_seclabel && attrs->na_seclabel->len)) > status = nfsd_setattr(rqstp, resfhp, attrs, NULL); > else > status = nfserrno(commit_metadata(resfhp)); This looks like the right approach to me, but can we instead add a nfsd_attrs_valid() helper function that checks ia_valid and does the test above? Then we can add similar tests for ACLs to it later, once we do a bit more investigation. Thanks, -- Jeffrey Layton <jlayton@xxxxxxxxxx>
