Date:   Wed, 6 Jul 2022 18:03:49 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Roberto Sassu <roberto.sassu@...wei.com>, ast@...nel.org,
        andrii@...nel.org, kpsingh@...nel.org, john.fastabend@...il.com,
        songliubraving@...com, kafai@...com, yhs@...com,
        dhowells@...hat.com
Cc:     keyrings@...r.kernel.org, bpf@...r.kernel.org,
        netdev@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 4/5] bpf: Add bpf_verify_pkcs7_signature() helper

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.

> ---
>   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

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

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? :))

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