Date:   Thu, 7 Jul 2022 11:00:58 +0000
From:   Roberto Sassu <roberto.sassu@...wei.com>
To:     KP Singh <kpsingh@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>
CC:     "ast@...nel.org" <ast@...nel.org>,
        "andrii@...nel.org" <andrii@...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-security-module@...r.kernel.org" 
        <linux-security-module@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v6 4/5] bpf: Add bpf_verify_pkcs7_signature() helper

> From: KP Singh [mailto:kpsingh@...nel.org]
> Sent: Thursday, July 7, 2022 1:49 AM
> On Wed, Jul 6, 2022 at 6:04 PM Daniel Borkmann <daniel@...earbox.net>
> wrote:
> >
> > On 6/28/22 2:27 PM, Roberto Sassu wrote:
> > > Add the bpf_verify_pkcs7_signature() helper, to give eBPF security modules
> > > the ability to check the validity of a signature against supplied data, by
> > > using user-provided or system-provided keys as trust anchor.
> > >
> > > The new helper makes it possible to enforce mandatory policies, as eBPF
> > > programs might be allowed to make security decisions only based on data
> > > sources the system administrator approves.
> > >
> > > The caller should provide both the data to be verified and the signature as
> > > eBPF dynamic pointers (to minimize the number of parameters).
> > >
> > > The caller should also provide a trusted keyring serial, together with key
> > > lookup-specific flags, to determine which keys can be used for signature
> > > verification. Alternatively, the caller could specify zero as serial value
> > > (not valid, serials must be positive), and provide instead a special
> > > keyring ID.
> > >
> > > Key lookup flags are defined in include/linux/key.h and can be: 1, to
> > > request that special keyrings be created if referred to directly; 2 to
> > > permit partially constructed keys to be found.
> > >
> > > Special IDs are defined in include/linux/verification.h and can be: 0 for
> > > the primary keyring (immutable keyring of system keys); 1 for both the
> > > primary and secondary keyring (where keys can be added only if they are
> > > vouched for by existing keys in those keyrings); 2 for the platform keyring
> > > (primarily used by the integrity subsystem to verify a kexec'ed kerned
> > > image and, possibly, the initramfs signature).
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@...wei.com>
> > > Reported-by: kernel test robot <lkp@...el.com> (cast warning)
> >
> > nit: Given this a new feature not a fix to existing code, there is no need to
> >       add the above reported-by from kbuild bot.

Ok.

> > > ---
> > >   include/uapi/linux/bpf.h       | 24 +++++++++++++
> > >   kernel/bpf/bpf_lsm.c           | 63 ++++++++++++++++++++++++++++++++++
> > >   tools/include/uapi/linux/bpf.h | 24 +++++++++++++
> > >   3 files changed, 111 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index e81362891596..b4f5ad863281 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -5325,6 +5325,29 @@ union bpf_attr {
> > >    *          **-EACCES** if the SYN cookie is not valid.
> > >    *
> > >    *          **-EPROTONOSUPPORT** if CONFIG_IPV6 is not builtin.
> > > + *
> > > + * long bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr, struct
> bpf_dynptr *sig_ptr, u32 trusted_keyring_serial, unsigned long lookup_flags,
> unsigned long trusted_keyring_id)
> >
> > nit: for the args instead of ulong, just do u64

Ok.

> > > + *   Description
> > > + *           Verify the PKCS#7 signature *sig_ptr* against the supplied
> > > + *           *data_ptr* with keys in a keyring with serial
> > > + *           *trusted_keyring_serial*, searched with *lookup_flags*, if the
> > > + *           parameter value is positive, or alternatively in a keyring with
> > > + *           special ID *trusted_keyring_id* if *trusted_keyring_serial* is
> > > + *           zero.
> > > + *
> > > + *           *lookup_flags* are defined in include/linux/key.h and can be: 1,
> > > + *           to request that special keyrings be created if referred to
> > > + *           directly; 2 to permit partially constructed keys to be found.
> > > + *
> > > + *           Special IDs are defined in include/linux/verification.h and can
> > > + *           be: 0 for the primary keyring (immutable keyring of system
> > > + *           keys); 1 for both the primary and secondary keyring (where keys
> > > + *           can be added only if they are vouched for by existing keys in
> > > + *           those keyrings); 2 for the platform keyring (primarily used by
> > > + *           the integrity subsystem to verify a kexec'ed kerned image and,
> > > + *           possibly, the initramfs signature).
> > > + *   Return
> > > + *           0 on success, a negative value on error.
> > >    */
> > >   #define __BPF_FUNC_MAPPER(FN)               \
> > >       FN(unspec),                     \
> > > @@ -5535,6 +5558,7 @@ union bpf_attr {
> > >       FN(tcp_raw_gen_syncookie_ipv6), \
> > >       FN(tcp_raw_check_syncookie_ipv4),       \
> > >       FN(tcp_raw_check_syncookie_ipv6),       \
> > > +     FN(verify_pkcs7_signature),     \
> >
> > (Needs rebase)
> >
> > >       /* */
> > >
> > >   /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > > diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > > index c1351df9f7ee..401bda01ad84 100644
> > > --- a/kernel/bpf/bpf_lsm.c
> > > +++ b/kernel/bpf/bpf_lsm.c
> > > @@ -16,6 +16,8 @@
> > >   #include <linux/bpf_local_storage.h>
> > >   #include <linux/btf_ids.h>
> > >   #include <linux/ima.h>
> > > +#include <linux/verification.h>
> > > +#include <linux/key.h>
> > >
> > >   /* For every LSM hook that allows attachment of BPF programs, declare a
> nop
> > >    * function where a BPF program can be attached.
> > > @@ -132,6 +134,62 @@ static const struct bpf_func_proto
> bpf_get_attach_cookie_proto = {
> > >       .arg1_type      = ARG_PTR_TO_CTX,
> > >   };
> > >
> > > +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> > > +BPF_CALL_5(bpf_verify_pkcs7_signature, struct bpf_dynptr_kern *,
> data_ptr,
> > > +        struct bpf_dynptr_kern *, sig_ptr, u32, trusted_keyring_serial,
> > > +        unsigned long, lookup_flags, unsigned long, trusted_keyring_id)
> > > +{
> > > +     key_ref_t trusted_keyring_ref;
> > > +     struct key *trusted_keyring;
> > > +     int ret;
> > > +
> > > +     /* Keep in sync with defs in include/linux/key.h. */
> > > +     if (lookup_flags > KEY_LOOKUP_PARTIAL)
> > > +             return -EINVAL;
> >
> > iiuc, the KEY_LOOKUP_* is a mask, so you could also combine the two, e.g.
> > KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL. I haven't seen you
> mentioning anything
> > specific on why it is not allowed. What's the rationale, if it's intentional
> > if should probably be documented?

It was a mistake. Will fix it.

> I think this was a part of the digilim threat model (only allow
> limited lookup operations),
> but this seems to be conflating the policy into the implementation of
> the helper.

Uhm, yes, but these flags should not affect that requirement.

As long as I can select the primary or secondary keyring reliably
with the pre-determined IDs, that should be fine.

> Roberto, can this not be implemented in digilim as a BPF LSM check
> that attaches to the key_permission LSM hook?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/li
> nux/lsm_hooks.h#n1158

The pre-determined IDs handled by verify_pkcs7_signature() are
more effective in selecting the proper key.

> > At minimum I also think the helper description needs to be improved for
> people
> > to understand enough w/o reading through the kernel source, e.g. wrt
> lookup_flags
> > since I haven't seen it in your selftests either ... when does a user need to
> > use the given flags.
> >
> > nit: when both trusted_keyring_serial and trusted_keyring_id are passed to the
> > helper, then this should be rejected as invalid argument? (Kind of feels a bit
> > like we're cramming two things in one helper.. KP, thoughts? :))
> 
> EINVAL when both are passed seems reasonable. The signature (pun?) of the
> does seem to get bloated, but I am not sure if it's worth adding two
> helpers here.

Ok for EINVAL. Should I change the trusted_keyring_id type to signed,
and use -1 when it should not be specified?

Thanks

Roberto

> > > +     /* Keep in sync with defs in include/linux/verification.h. */
> > > +     if (trusted_keyring_id > (unsigned
> long)VERIFY_USE_PLATFORM_KEYRING)
> > > +             return -EINVAL;
> > > +
> > > +     if (trusted_keyring_serial) {
> > > +             trusted_keyring_ref = lookup_user_key(trusted_keyring_serial,
> > > +                                                   lookup_flags,
> > > +                                                   KEY_NEED_SEARCH);
> > > +             if (IS_ERR(trusted_keyring_ref))
> > > +                     return PTR_ERR(trusted_keyring_ref);
> > > +
> > > +             trusted_keyring = key_ref_to_ptr(trusted_keyring_ref);
> > > +             goto verify;
> > > +     }
> > > +
> > > +     trusted_keyring = (struct key *)trusted_keyring_id;
> > > +verify:
> > > +     ret = verify_pkcs7_signature(data_ptr->data,
> > > +                                  bpf_dynptr_get_size(data_ptr),
> > > +                                  sig_ptr->data,
> > > +                                  bpf_dynptr_get_size(sig_ptr),
> > > +                                  trusted_keyring,
> > > +                                  VERIFYING_UNSPECIFIED_SIGNATURE, NULL,
> > > +                                  NULL);
> > > +     if (trusted_keyring_serial)
> > > +             key_put(trusted_keyring);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static const struct bpf_func_proto bpf_verify_pkcs7_signature_proto = {
> > > +     .func           = bpf_verify_pkcs7_signature,
> > > +     .gpl_only       = false,
> > > +     .ret_type       = RET_INTEGER,
> > > +     .arg1_type      = ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_LOCAL,
> > > +     .arg2_type      = ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_LOCAL,
> > > +     .arg3_type      = ARG_ANYTHING,
> > > +     .arg4_type      = ARG_ANYTHING,
> > > +     .arg5_type      = ARG_ANYTHING,
> > > +     .allowed        = bpf_ima_inode_hash_allowed,
> > > +};
> > > +#endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
> > > +

Powered by blists - more mailing lists