| Message ID | 20240502175818.21890-1-stephen.smalley.work@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] nfsd: set security label during create operations | expand |
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@gmail.com> > --- > 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,
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));
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> --- fs/nfsd/vfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)