From: Casey Schaufler <casey@schaufler-ca.com>
To: Roberto Sassu <roberto.sassu@huaweicloud.com>,
KP Singh <kpsingh@kernel.org>,
Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Florent Revest <revest@chromium.org>,
Brendan Jackman <jackmanb@chromium.org>,
Paul Moore <paul@paul-moore.com>,
James Morris <jmorris@namei.org>,
"Serge E . Hallyn" <serge@hallyn.com>, bpf <bpf@vger.kernel.org>,
LSM List <linux-security-module@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
nicolas.bouchinet@clip-os.org, Mimi Zohar <zohar@linux.ibm.com>,
linux-integrity <linux-integrity@vger.kernel.org>,
casey@schaufler-ca.com
Subject: Re: [RFC][PATCH] bpf: Check xattr name/value pair from bpf_lsm_inode_init_security()
Date: Fri, 28 Oct 2022 08:01:11 -0700 [thread overview]
Message-ID: <3752db45-c89e-dfa7-0bd0-246421daf8cc@schaufler-ca.com> (raw)
In-Reply-To: <edf0ec89c61fbee68fd537981982e14b1674393d.camel@huaweicloud.com>
On 10/28/2022 1:48 AM, Roberto Sassu wrote:
> On Thu, 2022-10-27 at 12:39 +0200, KP Singh wrote:
>> On Wed, Oct 26, 2022 at 7:14 PM Alexei Starovoitov
>> <alexei.starovoitov@gmail.com> wrote:
>>> On Wed, Oct 26, 2022 at 1:42 AM Roberto Sassu
>>> <roberto.sassu@huaweicloud.com> wrote:
>>>> On 10/26/2022 8:37 AM, Alexei Starovoitov wrote:
>>>>> On Tue, Oct 25, 2022 at 7:58 AM Casey Schaufler <
>>>>> casey@schaufler-ca.com> wrote:
>>>>>> On 10/25/2022 12:43 AM, Roberto Sassu wrote:
>>>>>>> On Mon, 2022-10-24 at 19:13 -0700, Alexei Starovoitov
>>>>>>> wrote:
>>>>>>>> I'm looking at security_inode_init_security() and it is
>>>>>>>> indeed messy.
>>>>>>>> Per file system initxattrs callback that processes
>>>>>>>> kmalloc-ed
>>>>>>>> strings.
>>>>>>>> Yikes.
>>>>>>>>
>>>>>>>> In the short term we should denylist inode_init_security
>>>>>>>> hook to
>>>>>>>> disallow attaching bpf-lsm there. set/getxattr should be
>>>>>>>> done
>>>>>>>> through kfuncs instead of such kmalloc-a-string hack.
>>>>>>> Inode_init_security is an example. It could be that the
>>>>>>> other hooks are
>>>>>>> affected too. What happens if they get arbitrary positive
>>>>>>> values too?
>>>>>> TL;DR - Things will go cattywampus.
>>>>>>
>>>>>> The LSM infrastructure is an interface that has "grown
>>>>>> organically",
>>>>>> and isn't necessarily consistent in what it requires of the
>>>>>> security
>>>>>> module implementations. There are cases where it assumes that
>>>>>> the
>>>>>> security module hooks are well behaved, as you've discovered.
>>>>>> I have
>>>>>> no small amount of fear that someone is going to provide an
>>>>>> eBPF
>>>>>> program for security_secid_to_secctx(). There has been an
>>>>>> assumption,
>>>>>> oft stated, that all security modules are going to be
>>>>>> reviewed as
>>>>>> part of the upstream process. The review process ought to
>>>>>> catch hooks
>>>>>> that return unacceptable values. Alas, we've lost that with
>>>>>> BPF.
>>>>>>
>>>>>> It would take a(nother) major overhaul of the LSM
>>>>>> infrastructure to
>>>>>> make it safe against hooks that are not well behaved. From
>>>>>> what I have
>>>>>> seen so far it wouldn't be easy/convenient/performant to do
>>>>>> it in the
>>>>>> BPF security module either. I personally think that BPF needs
>>>>>> to
>>>>>> ensure that the eBPF implementations don't return
>>>>>> inappropriate values,
>>>>>> but I understand why that is problematic.
>>>>> That's an accurate statement. Thank you.
>>>>>
>>>>> Going back to the original question...
>>>>> We fix bugs when we discover them.
>>>>> Regardless of the subsystem they belong to.
>>>>> No finger pointing.
>>>> I'm concerned about the following situation:
>>>>
>>>> struct <something> *function()
>>>> {
>>>>
>>>> ret = security_*();
>>>> if (ret)
>>>> return ERR_PTR(ret);
>>>>
>>>> }
>>>>
>>>> int caller()
>>>> {
>>>> ptr = function()
>>>> if (IS_ERR(ptr)
>>>> goto out;
>>>>
>>>> <use of invalid pointer>
>>>> }
>>>>
>>>> I quickly found an occurrence of this:
>>>>
>>>> static int lookup_one_common()
>>>> {
>>>>
>>>> [...]
>>>>
>>>> return inode_permission();
>>>> }
>>>>
>>>> struct dentry *try_lookup_one_len()
>>>> {
>>>>
>>>> [...]
>>>>
>>>> err = lookup_one_common(&init_user_ns, name, base, len,
>>>> &this);
>>>> if (err)
>>>> return ERR_PTR(err);
>>>>
>>>>
>>>> Unfortunately, attaching to inode_permission causes the kernel
>>>> to crash immediately (it does not happen with negative return
>>>> values).
>>>>
>>>> So, I think the fix should be broader, and not limited to the
>>>> inode_init_security hook. Will try to see how it can be fixed.
>>> I see. Let's restrict bpf-lsm return values to IS_ERR_VALUE.
>>> Trivial verifier change.
>> Thanks, yes this indeed is an issue. We need to do a few things:
>>
>> 1. Restrict some hooks that we know the BPF LSM will never need.
>> 2. A verifier function that checks return values of LSM
>> hooks.
>> For most LSK hooks IS_ERR_VALUE is fine, however, there are some
>> hooks
>> like *xattr hooks that use a return value of 1 to indicate a
>> capability check is required which might need special handling.
> I looked at security.c:
>
> /*
> * SELinux and Smack integrate the cap call,
> * so assume that all LSMs supplying this call do so.
> */
>
> Other than checking the return value, probably we should also wrap
> bpf_lsm_inode_{set,remove}xattr() to do the capability check, right?
Long term I hope to fix the way capabilities are dealt with in
this hook, but for now your suggestion seems reasonable.
>
> Roberto
>
prev parent reply other threads:[~2022-10-28 15:01 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-21 16:46 [RFC][PATCH] bpf: Check xattr name/value pair from bpf_lsm_inode_init_security() Roberto Sassu
2022-10-23 23:36 ` Alexei Starovoitov
2022-10-24 9:25 ` Roberto Sassu
2022-10-24 15:28 ` Roberto Sassu
2022-10-25 2:13 ` Alexei Starovoitov
2022-10-25 7:43 ` Roberto Sassu
2022-10-25 14:57 ` Casey Schaufler
2022-10-26 6:37 ` Alexei Starovoitov
2022-10-26 8:42 ` Roberto Sassu
2022-10-26 17:14 ` Alexei Starovoitov
2022-10-27 10:39 ` KP Singh
2022-10-27 15:52 ` Casey Schaufler
2022-10-28 8:48 ` Roberto Sassu
2022-10-28 15:01 ` Casey Schaufler [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=3752db45-c89e-dfa7-0bd0-246421daf8cc@schaufler-ca.com \
--to=casey@schaufler-ca.com \
--cc=alexei.starovoitov@gmail.com \
--cc=bpf@vger.kernel.org \
--cc=jackmanb@chromium.org \
--cc=jmorris@namei.org \
--cc=kpsingh@kernel.org \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=nicolas.bouchinet@clip-os.org \
--cc=paul@paul-moore.com \
--cc=revest@chromium.org \
--cc=roberto.sassu@huaweicloud.com \
--cc=serge@hallyn.com \
--cc=zohar@linux.ibm.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 NNTP newsgroup(s).