linux-kernel - RE: [PATCH v5 2/5] bpf: Add bpf_lookup_user_key() and bpf_key

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] [day] [month] [year] [list]
Message-ID: <a01f79b1a3874ab796b4d3d270e53618@huawei.com>
Date:   Fri, 24 Jun 2022 17:38:59 +0000
From:   Roberto Sassu <roberto.sassu@...wei.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
CC:     "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "andrii@...nel.org" <andrii@...nel.org>,
        "kpsingh@...nel.org" <kpsingh@...nel.org>,
        "john.fastabend@...il.com" <john.fastabend@...il.com>,
        "songliubraving@...com" <songliubraving@...com>,
        "kafai@...com" <kafai@...com>, "yhs@...com" <yhs@...com>,
        "dhowells@...hat.com" <dhowells@...hat.com>,
        "keyrings@...r.kernel.org" <keyrings@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v5 2/5] bpf: Add bpf_lookup_user_key() and bpf_key_put()
 helpers

> From: Alexei Starovoitov [mailto:alexei.starovoitov@...il.com]
> Sent: Friday, June 24, 2022 6:50 PM
> On Fri, Jun 24, 2022 at 8:32 AM Roberto Sassu <roberto.sassu@...wei.com>
> wrote:
> >
> > > From: Alexei Starovoitov [mailto:alexei.starovoitov@...il.com]
> > > Sent: Thursday, June 23, 2022 10:54 PM
> > > On Thu, Jun 23, 2022 at 5:36 AM Roberto Sassu
> <roberto.sassu@...wei.com>
> > > wrote:
> > > >
> > > > > From: Roberto Sassu [mailto:roberto.sassu@...wei.com]
> > > > > Sent: Wednesday, June 22, 2022 9:12 AM
> > > > > > From: Alexei Starovoitov [mailto:alexei.starovoitov@...il.com]
> > > > > > Sent: Wednesday, June 22, 2022 12:33 AM
> > > > > > On Tue, Jun 21, 2022 at 06:37:54PM +0200, Roberto Sassu wrote:
> > > > > > > Add the bpf_lookup_user_key() and bpf_key_put() helpers, to
> respectively
> > > > > > > search a key with a given serial, and release the reference count of
> the
> > > > > > > found key.
> > > > > > >
> > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@...wei.com>
> > > > > > > ---
> > > > > > >  include/uapi/linux/bpf.h       | 16 ++++++++++++
> > > > > > >  kernel/bpf/bpf_lsm.c           | 46
> > > ++++++++++++++++++++++++++++++++++
> > > > > > >  kernel/bpf/verifier.c          |  6 +++--
> > > > > > >  scripts/bpf_doc.py             |  2 ++
> > > > > > >  tools/include/uapi/linux/bpf.h | 16 ++++++++++++
> > > > > > >  5 files changed, 84 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > > > > > index e81362891596..7bbcf2cd105d 100644
> > > > > > > --- a/include/uapi/linux/bpf.h
> > > > > > > +++ b/include/uapi/linux/bpf.h
> > > > > > > @@ -5325,6 +5325,20 @@ union bpf_attr {
> > > > > > >   *               **-EACCES** if the SYN cookie is not valid.
> > > > > > >   *
> > > > > > >   *               **-EPROTONOSUPPORT** if CONFIG_IPV6 is not builtin.
> > > > > > > + *
> > > > > > > + * struct key *bpf_lookup_user_key(u32 serial, unsigned long flags)
> > > > > > > + *       Description
> > > > > > > + *               Search a key with a given *serial* and the provided
> *flags*,
> > > and
> > > > > > > + *               increment the reference count of the key.
> > > > > >
> > > > > > Why passing 'flags' is ok to do?
> > > > > > Please think through every line of the patch.
> > > > >
> > > > > To be honest, I thought about it. Probably yes, I should do some
> > > > > sanitization, like I did for the keyring ID. When I checked
> > > > > lookup_user_key(), I saw that flags are checked individually, so
> > > > > an arbitrary value passed to the helper should not cause harm.
> > > > > Will do sanitization, if you prefer. It is just that we have to keep
> > > > > the eBPF code in sync with key flag definition (unless we have
> > > > > a 'last' flag).
> > > >
> > > > I'm not sure that having a helper for lookup_user_key() alone is
> > > > correct. By having separate helpers for lookup and usage of the
> > > > key, nothing would prevent an eBPF program to ask for a
> > > > permission to pass the access control check, and then use the
> > > > key for something completely different from what it requested.
> > > >
> > > > Looking at how lookup_user_key() is used in security/keys/keyctl.c,
> > > > it seems clear that it should be used together with the operation
> > > > that needs to be performed. Only in this way, the key permission
> > > > would make sense.
> > >
> > > lookup is roughly equivalent to open when all permission checks are done.
> > > And using the key is read/write.
> >
> > For bpf_verify_pkcs7_signature(), we need the search permission
> > on the keyring containing the key used for signature verification.
> 
> you mean lookup_user_key(serial, flags, KEY_NEED_SEARCH) ?
> 
> right. and ? what's your point?

It is hardcoded. Does not necessarily reflect the operation
that will be performed on the key.

On the other hand, if I add the permission as parameter to
bpf_lookup_user_key(), an eBPF program can pass an arbitrary
value, and then do something completely different with the key.

Roberto