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
Subject: Re: [RFC][PATCH] nfsd: set security label during create operations
Date: Thu, 02 May 2024 14:28:44 -0400 [thread overview]
Message-ID: <732fab0f79c5e0f1e521a3a5dfa652f80d83eac5.camel@kernel.org> (raw)
In-Reply-To: <20240502175818.21890-1-stephen.smalley.work@gmail.com>
On Thu, 2024-05-02 at 13: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.
>
Nice catch. I think you're correct on file ACLs too.
We're probably saved in many cases by the fact that clients usually
send ACLs and seclabels alongside other attributes during a create.
Obviously, that's not _always_ the case though.
> 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>
> ---
> fs/nfsd/vfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 2e41eb4c3cec..9b777ea7ef26 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 (iap->ia_valid || (attrs->na_seclabel && attrs->na_seclabel->len))
> status = nfsd_setattr(rqstp, resfhp, attrs, NULL);
> else
> status = nfserrno(commit_metadata(resfhp));
This looks like the right approach to me, but can we instead add a
nfsd_attrs_valid() helper function that checks ia_valid and does the
test above?
Then we can add similar tests for ACLs to it later, once we do a bit
more investigation.
Thanks,
--
Jeffrey Layton <jlayton@kernel.org>
prev parent reply other threads:[~2024-05-02 18:28 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-02 17:58 [RFC][PATCH] nfsd: set security label during create operations Stephen Smalley
2024-05-02 18:28 ` Jeffrey Layton [this message]
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=732fab0f79c5e0f1e521a3a5dfa652f80d83eac5.camel@kernel.org \
--to=jlayton@kernel.org \
--cc=chuck.lever@oracle.com \
--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 \
--cc=stephen.smalley.work@gmail.com \
/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).