lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAADnVQKD5e9vKsSo1TPeBm5hr6j4GzQeHqRURoBJyB++VOwHCw@mail.gmail.com>
Date:   Wed, 26 Oct 2022 10:14:43 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Roberto Sassu <roberto.sassu@...weicloud.com>
Cc:     Casey Schaufler <casey@...aufler-ca.com>,
        KP Singh <kpsingh@...nel.org>,
        Florent Revest <revest@...omium.org>,
        Brendan Jackman <jackmanb@...omium.org>,
        Paul Moore <paul@...l-moore.com>,
        James Morris <jmorris@...ei.org>,
        "Serge E . Hallyn" <serge@...lyn.com>, bpf <bpf@...r.kernel.org>,
        LSM List <linux-security-module@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        nicolas.bouchinet@...p-os.org, Mimi Zohar <zohar@...ux.ibm.com>,
        linux-integrity <linux-integrity@...r.kernel.org>
Subject: Re: [RFC][PATCH] bpf: Check xattr name/value pair from bpf_lsm_inode_init_security()

On Wed, Oct 26, 2022 at 1:42 AM Roberto Sassu
<roberto.sassu@...weicloud.com> wrote:
>
> On 10/26/2022 8:37 AM, Alexei Starovoitov wrote:
> > On Tue, Oct 25, 2022 at 7:58 AM Casey Schaufler <casey@...aufler-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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ