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
>
> fs/nfsd/vfs.c | 2 +-
> fs/nfsd/vfs.h | 8 ++++++++
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> 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
>
>
