| Message ID | 20240503130905.16823-1-stephen.smalley.work@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] nfsd: set security label during create operations | expand |
On Fri, 2024-05-03 at 09:09 -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. 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@gmail.com> > --- > 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. > > 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); Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Fri, May 03, 2024 at 09:09:06AM -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. 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@gmail.com> > --- > 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. > > 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 > Thanks, Stephen! Applied to nfsd-next (for v6.10). Review comments are still welcome.
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@gmail.com> > --- > 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@suse.de> 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 > >
On Mon, May 6, 2024 at 1:46 AM NeilBrown <neilb@suse.de> 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@gmail.com> > > --- > > 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@suse.de> > > 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?
On Tue, 07 May 2024, Stephen Smalley wrote: > On Mon, May 6, 2024 at 1:46 AM NeilBrown <neilb@suse.de> 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@gmail.com> > > > --- > > > 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@suse.de> > > > > 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? > Thanks a good question. I should have asked it myself! No, ACLs aren't covered some other way. They have the same problem. I'm tempted to suggest that we simple drop the guard and call nfsd_setattr() unconditionally. The cost is primarily the we call inode_lock() without needing to do anything. Maybe moving the test inside nfsd_setattr() makes it a bit more obvious what is needed: diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 2e41eb4c3cec..c738e9dfd72f 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -499,6 +499,14 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, bool size_change = (iap->ia_valid & ATTR_SIZE); int retries; + if (!(iap->ia_valid || + (attr->na_seclabel && attr->na_seclabel->len) || + (IS_ENABLED(CONFIG_FS_POSIX_ACL) && attr->na_pacl) || + (IS_ENABLED(CONFIG_FS_POSIX_ACL) && + !attr->na_aclerr && attr->na_dpacl && S_ISDIR(inode->i_mode)))) + /* Don't bother with inode_lock() */ + goto out; + if (iap->ia_valid & ATTR_SIZE) { accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE; ftype = S_IFREG; @@ -1418,14 +1426,7 @@ nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, if (!uid_eq(current_fsuid(), GLOBAL_ROOT_UID)) iap->ia_valid &= ~(ATTR_UID|ATTR_GID); - /* - * Callers expect new file metadata to be committed even - * if the attributes have not changed. - */ - if (iap->ia_valid) - status = nfsd_setattr(rqstp, resfhp, attrs, NULL); - else - status = nfserrno(commit_metadata(resfhp)); + status = nfsd_setattr(rqstp, resfhp, attrs, NULL); /* * Transactional filesystems had a chance to commit changes Thoughts? Thanks, NeilBrown
On Mon, May 6, 2024 at 1:46 AM NeilBrown <neilb@suse.de> 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@gmail.com> > > --- > > 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@suse.de> FWIW, I finally got around to testing Linux v5.14 and it did pass these NFS tests so this was a regression. I haven't been able to bisect yet.
On Wed, May 15, 2024 at 10:52 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Mon, May 6, 2024 at 1:46 AM NeilBrown <neilb@suse.de> 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@gmail.com> > > > --- > > > 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@suse.de> > > FWIW, I finally got around to testing Linux v5.14 and it did pass > these NFS tests so this was a regression. I haven't been able to > bisect yet. Seems to have broken sometime between v5.19 and v6.0. Still bisecting.
On Thu, May 16, 2024 at 1:29 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Wed, May 15, 2024 at 10:52 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > > > On Mon, May 6, 2024 at 1:46 AM NeilBrown <neilb@suse.de> 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@gmail.com> > > > > --- > > > > 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@suse.de> > > > > FWIW, I finally got around to testing Linux v5.14 and it did pass > > these NFS tests so this was a regression. I haven't been able to > > bisect yet. > > Seems to have broken sometime between v5.19 and v6.0. Still bisecting. git bisect ended with: [d6a97d3f589a3a46a16183e03f3774daee251317] NFSD: add security label to struct nfsd_attrs as the breaking commit. Bisecting was a little complicated by other unrelated bugs but I narrowed it to just this particular one.
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);
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@gmail.com> --- 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. fs/nfsd/vfs.c | 2 +- fs/nfsd/vfs.h | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-)