* Re: [PATCH v3] nfsd: set security label during create operations
2024-05-03 13:09 [PATCH v3] nfsd: set security label during create operations Stephen Smalley
@ 2024-05-03 17:31 ` Jeffrey Layton
2024-05-04 15:23 ` Chuck Lever
2024-05-06 5:46 ` NeilBrown
2 siblings, 0 replies; 9+ messages in thread
From: Jeffrey Layton @ 2024-05-03 17:31 UTC (permalink / raw
To: Stephen Smalley, selinux, linux-nfs, chuck.lever, neilb
Cc: paul, omosnace, linux-security-module
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] nfsd: set security label during create operations
2024-05-03 13:09 [PATCH v3] nfsd: set security label during create operations Stephen Smalley
2024-05-03 17:31 ` Jeffrey Layton
@ 2024-05-04 15:23 ` Chuck Lever
2024-05-06 5:46 ` NeilBrown
2 siblings, 0 replies; 9+ messages in thread
From: Chuck Lever @ 2024-05-04 15:23 UTC (permalink / raw
To: Stephen Smalley
Cc: selinux, linux-nfs, jlayton, neilb, paul, omosnace,
linux-security-module
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.
--
Chuck Lever
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] nfsd: set security label during create operations
2024-05-03 13:09 [PATCH v3] nfsd: set security label during create operations Stephen Smalley
2024-05-03 17:31 ` Jeffrey Layton
2024-05-04 15:23 ` Chuck Lever
@ 2024-05-06 5:46 ` NeilBrown
2024-05-06 16:31 ` Stephen Smalley
2024-05-15 14:52 ` Stephen Smalley
2 siblings, 2 replies; 9+ messages in thread
From: NeilBrown @ 2024-05-06 5:46 UTC (permalink / raw
To: Stephen Smalley
Cc: selinux, linux-nfs, chuck.lever, jlayton, paul, omosnace,
linux-security-module, Stephen Smalley
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
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] nfsd: set security label during create operations
2024-05-06 5:46 ` NeilBrown
@ 2024-05-06 16:31 ` Stephen Smalley
2024-05-08 6:54 ` NeilBrown
2024-05-15 14:52 ` Stephen Smalley
1 sibling, 1 reply; 9+ messages in thread
From: Stephen Smalley @ 2024-05-06 16:31 UTC (permalink / raw
To: NeilBrown
Cc: selinux, linux-nfs, chuck.lever, jlayton, paul, omosnace,
linux-security-module
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] nfsd: set security label during create operations
2024-05-06 16:31 ` Stephen Smalley
@ 2024-05-08 6:54 ` NeilBrown
0 siblings, 0 replies; 9+ messages in thread
From: NeilBrown @ 2024-05-08 6:54 UTC (permalink / raw
To: Stephen Smalley
Cc: selinux, linux-nfs, chuck.lever, jlayton, paul, omosnace,
linux-security-module
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] nfsd: set security label during create operations
2024-05-06 5:46 ` NeilBrown
2024-05-06 16:31 ` Stephen Smalley
@ 2024-05-15 14:52 ` Stephen Smalley
2024-05-16 17:29 ` Stephen Smalley
1 sibling, 1 reply; 9+ messages in thread
From: Stephen Smalley @ 2024-05-15 14:52 UTC (permalink / raw
To: NeilBrown
Cc: selinux, linux-nfs, chuck.lever, jlayton, paul, omosnace,
linux-security-module
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] nfsd: set security label during create operations
2024-05-15 14:52 ` Stephen Smalley
@ 2024-05-16 17:29 ` Stephen Smalley
2024-05-22 17:29 ` Stephen Smalley
0 siblings, 1 reply; 9+ messages in thread
From: Stephen Smalley @ 2024-05-16 17:29 UTC (permalink / raw
To: NeilBrown
Cc: selinux, linux-nfs, chuck.lever, jlayton, paul, omosnace,
linux-security-module
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] nfsd: set security label during create operations
2024-05-16 17:29 ` Stephen Smalley
@ 2024-05-22 17:29 ` Stephen Smalley
0 siblings, 0 replies; 9+ messages in thread
From: Stephen Smalley @ 2024-05-22 17:29 UTC (permalink / raw
To: NeilBrown
Cc: selinux, linux-nfs, chuck.lever, jlayton, paul, omosnace,
linux-security-module
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.
^ permalink raw reply [flat|nested] 9+ messages in thread