Received-SPF: pass (google.com: domain of linux-nfs+bounces-3139-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <70273db57aa4b6df43ae1f73e6bf3b80abf0c599.camel@kernel.org>
Subject: Re: [PATCH v2] nfsd: set security label during create operations
From: Jeffrey Layton <jlayton@kernel.org>
To: Stephen Smalley <stephen.smalley.work@gmail.com>,
 selinux@vger.kernel.org,  linux-nfs@vger.kernel.org,
 chuck.lever@oracle.com, neilb@suse.de
Cc: paul@paul-moore.com, omosnace@redhat.com, 
	linux-security-module@vger.kernel.org
Date: Thu, 02 May 2024 18:34:46 -0400
In-Reply-To: <20240502195800.3252-1-stephen.smalley.work@gmail.com>
References: <20240502195800.3252-1-stephen.smalley.work@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.0 (3.52.0-1.fc40app1) 
Precedence: bulk
MIME-Version: 1.0

On Thu, 2024-05-02 at 15:58 -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. I couldn't
> tell if this has always been broken or broke at some point in time. Looki=
ng
> at nfsd_setattr() I am uncertain as to whether the same issue presents fo=
r
> file ACLs and therefore requires a similar fix for those. I am not overly
> confident that this is the right solution.
>=20
> 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 bett=
er
> for SELinux and a similar approach has been used previously
> (see security_dentry_create_files_as) but perhaps not usable by other LSM=
s.
>=20
> 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
>=20
> 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
>=20
> 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
>=20
> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---
> v2 introduces a nfsd_attrs_valid() helper and uses it as suggested by
> Jeffrey Layton <jlayton@kernel.org>.
>=20
>  fs/nfsd/nfsproc.c | 2 +-
>  fs/nfsd/vfs.c     | 2 +-
>  fs/nfsd/vfs.h     | 8 ++++++++
>  3 files changed, 10 insertions(+), 2 deletions(-)
>=20
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index 36370b957b63..3e438159f561 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -389,7 +389,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
>  		 * open(..., O_CREAT|O_TRUNC|O_WRONLY).
>  		 */
>  		attr->ia_valid &=3D ATTR_SIZE;
> -		if (attr->ia_valid)
> +		if (nfsd_attrs_valid(attr))
>  			resp->status =3D nfsd_setattr(rqstp, newfhp, &attrs,
>  						    NULL);
>  	}

This function is for NFSv2, which doesn't support any inode attributes
that aren't represented in ia_valid. We could leave this as-is, but
this is fine too.

> 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 =3D nfsd_setattr(rqstp, resfhp, attrs, NULL);
>  	else
>  		status =3D 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);
>  }
> =20
> +static inline bool nfsd_attrs_valid(struct nfsd_attrs *attrs)
> +{
> +	struct iattr *iap =3D 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: Jeffrey Layton <jlayton@kernel.org>