> 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 */ > > > +
