RE: [PATCH v6 4/5] bpf: Add bpf_verify_pkcs7_signature() helper

From: Roberto Sassu
Date: Thu Jul 07 2022 - 07:01:09 EST

Next message: Robert Foss: "Re: [PATCH RESEND 0/2] drm: bridge: adv7511: two fixes for CEC"
Previous message: Baolin Wang: "Re: [PATCH 1/3] hwspinlock: qcom: Add support for mmio usage to sfpb-mutex"
In reply to: KP Singh: "Re: [PATCH v6 4/5] bpf: Add bpf_verify_pkcs7_signature() helper"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> From: KP Singh [mailto:kpsingh@xxxxxxxxxx]
> Sent: Thursday, July 7, 2022 1:49 AM
> On Wed, Jul 6, 2022 at 6:04 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> 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@xxxxxxxxxx>
> > > Reported-by: kernel test robot <lkp@xxxxxxxxx> (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 */
> > > +

Next message: Robert Foss: "Re: [PATCH RESEND 0/2] drm: bridge: adv7511: two fixes for CEC"
Previous message: Baolin Wang: "Re: [PATCH 1/3] hwspinlock: qcom: Add support for mmio usage to sfpb-mutex"
In reply to: KP Singh: "Re: [PATCH v6 4/5] bpf: Add bpf_verify_pkcs7_signature() helper"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]