Received-SPF: pass (google.com: domain of linux-nfs+bounces-3197-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "Stephen Smalley" <stephen.smalley.work@gmail.com>
Cc: selinux@vger.kernel.org, linux-nfs@vger.kernel.org, chuck.lever@oracle.com,
 jlayton@kernel.org, paul@paul-moore.com, omosnace@redhat.com,
 linux-security-module@vger.kernel.org
Subject: Re: [PATCH v3] nfsd: set security label during create operations
In-reply-to:
 <CAEjxPJ6W7UGvPUMt82+_tB2MPmcmG7JaUjH6HhgjwTqOzQL_xA@mail.gmail.com>
References:
 <>, <CAEjxPJ6W7UGvPUMt82+_tB2MPmcmG7JaUjH6HhgjwTqOzQL_xA@mail.gmail.com>
Date: Wed, 08 May 2024 16:54:25 +1000
Message-id: <171515126555.4857.14866053620991695880@noble.neil.brown.name>

On Tue, 07 May 2024, Stephen Smalley wrote:
> On Mon, May 6, 2024 at 1:46=E2=80=AFAM 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, whi=
ch
> > > would atomically label the new inode at creation time. This would be be=
tter
> > > for SELinux and a similar approach has been used previously
> > > (see security_dentry_create_files_as) but perhaps not usable by other L=
SMs.
> > >
> > > 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=3D$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=3D4.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
>=20
> 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?
>=20

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 =3D (iap->ia_valid & ATTR_SIZE);
 	int		retries;
=20
+	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 |=3D NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
 		ftype =3D 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 &=3D ~(ATTR_UID|ATTR_GID);
=20
-	/*
-	 * Callers expect new file metadata to be committed even
-	 * if the attributes have not changed.
-	 */
-	if (iap->ia_valid)
-		status =3D nfsd_setattr(rqstp, resfhp, attrs, NULL);
-	else
-		status =3D nfserrno(commit_metadata(resfhp));
+	status =3D nfsd_setattr(rqstp, resfhp, attrs, NULL);
=20
 	/*
 	 * Transactional filesystems had a chance to commit changes


Thoughts?

Thanks,
NeilBrown