Received-SPF: pass (google.com: domain of linux-nfs+bounces-3339-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240503130905.16823-1-stephen.smalley.work@gmail.com>
 <171497439414.9775.6998904788791406674@noble.neil.brown.name>
 <CAEjxPJ6DTNY3p9MmdV0K1A7No7joczGTeOe26Q4wr6yujk9zKA@mail.gmail.com> <CAEjxPJ6y=WmGqRkj+Qrj9x5+-u74=DEt0JCWmCpRu4EZufpmkg@mail.gmail.com>
In-Reply-To: <CAEjxPJ6y=WmGqRkj+Qrj9x5+-u74=DEt0JCWmCpRu4EZufpmkg@mail.gmail.com>
From: Stephen Smalley <stephen.smalley.work@gmail.com>
Date: Wed, 22 May 2024 13:29:28 -0400
Message-ID: <CAEjxPJ7Wh3jnH7fjYorT3DCw_iK48NzfG9t6c3to66L_WgVt0A@mail.gmail.com>
Subject: Re: [PATCH v3] nfsd: set security label during create operations
To: NeilBrown <neilb@suse.de>
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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, May 16, 2024 at 1:29=E2=80=AFPM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Wed, May 15, 2024 at 10:52=E2=80=AFAM Stephen Smalley
> <stephen.smalley.work@gmail.com> 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 secur=
ity
> > > > label as part of a create operation for the new file, similar to mo=
de
> > > > and other attributes. At present, the security label is received by=
 nfsd
> > > > and passed down to nfsd_create_setattr(), but nfsd_setattr() is nev=
er
> > > > called and therefore the label is never set on the new file. This b=
ug
> > > > 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 b=
e better
> > > > for SELinux and a similar approach has been used previously
> > > > (see security_dentry_create_files_as) but perhaps not usable by oth=
er 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=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-tests=
uite
> > > > 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 update=
s 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 introduce=
d
> > > 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.