On Mon, May 6, 2024 at 1:46 AM NeilBrown <neilb@xxxxxxx> wrote:
>
> On Fri, 03 May 2024, 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. This bug
> > may have been introduced on or around commit d6a97d3f589a ("NFSD:
> > add security label to struct nfsd_attrs"). 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.
> >
> > 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>
> > ---
> > v3 removes the erroneous and unnecessary change to NFSv2 and updates the
> > description to note the possible origin of the bug. I did not add a
> > Fixes tag however as I have not yet tried confirming that.
>
> I think this bug has always been present - since label support was
> added.
> Commit d6a97d3f589a ("NFSD: add security label to struct nfsd_attrs")
> should have fixed it, but was missing the extra test that you provide.
>
> So
> Fixes: 0c71b7ed5de8 ("nfsd: introduce file_cache_mutex")
> might be appropriate - it fixes the patch, though not a bug introduced
> by the patch.
>
> Thanks for this patch!
> Reviewed-by: NeilBrown <neilb@xxxxxxx>
>
> NeilBrown
Thanks for confirming. Do we need to also check for the ACL case in
nfsd_attrs_valid() or is that covered in some other way?
