From: Stephen Smalley <stephen.smalley.work@gmail.com>
To: Jeffrey Layton <jlayton@kernel.org>
Cc: selinux@vger.kernel.org, linux-nfs@vger.kernel.org,
chuck.lever@oracle.com, neilb@suse.de, paul@paul-moore.com,
omosnace@redhat.com, linux-security-module@vger.kernel.org
Subject: Re: [PATCH v2] nfsd: set security label during create operations
Date: Fri, 3 May 2024 08:48:42 -0400 [thread overview]
Message-ID: <CAEjxPJ6O-Zo=7xJ9j2=mOLEO0dWPYx9AHVRtPqaUPYx9rbsVpA@mail.gmail.com> (raw)
In-Reply-To: <70273db57aa4b6df43ae1f73e6bf3b80abf0c599.camel@kernel.org>
On Thu, May 2, 2024 at 6:34 PM Jeffrey Layton <jlayton@kernel.org> wrote:
>
> 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. 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. I am not overly
> > confident that this is the right solution.
> >
> > 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>
> > ---
> > v2 introduces a nfsd_attrs_valid() helper and uses it as suggested by
> > Jeffrey Layton <jlayton@kernel.org>.
> >
> > fs/nfsd/nfsproc.c | 2 +-
> > fs/nfsd/vfs.c | 2 +-
> > fs/nfsd/vfs.h | 8 ++++++++
> > 3 files changed, 10 insertions(+), 2 deletions(-)
> >
> > 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 &= ATTR_SIZE;
> > - if (attr->ia_valid)
> > + if (nfsd_attrs_valid(attr))
> > resp->status = 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.
Sorry, I got over-eager with trying to fix all ia_valid checks. It's
actually wrong so I'll send a 3rd version without it.
next prev parent reply other threads:[~2024-05-03 12:48 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-02 19:58 [PATCH v2] nfsd: set security label during create operations Stephen Smalley
2024-05-02 20:17 ` Chuck Lever
2024-05-02 22:34 ` Jeffrey Layton
2024-05-03 12:48 ` Stephen Smalley [this message]
2024-05-03 7:31 ` kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAEjxPJ6O-Zo=7xJ9j2=mOLEO0dWPYx9AHVRtPqaUPYx9rbsVpA@mail.gmail.com' \
--to=stephen.smalley.work@gmail.com \
--cc=chuck.lever@oracle.com \
--cc=jlayton@kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=neilb@suse.de \
--cc=omosnace@redhat.com \
--cc=paul@paul-moore.com \
--cc=selinux@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).