diff mbox series

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

Message ID 20220628122750.1895107-5-roberto.sassu@huawei.com (mailing list archive)
State Handled Elsewhere
Headers show
Series bpf: Add bpf_verify_pkcs7_signature() helper | expand

Commit Message

Roberto Sassu June 28, 2022, 12:27 p.m. UTC 
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@huawei.com>
Reported-by: kernel test robot <lkp@intel.com> (cast warning)
---
 include/uapi/linux/bpf.h       | 24 +++++++++++++
 kernel/bpf/bpf_lsm.c           | 63 ++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h | 24 +++++++++++++
 3 files changed, 111 insertions(+)

Comments

Daniel Borkmann July 6, 2022, 4:03 p.m. UTC | #1 
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@huawei.com>
> Reported-by: kernel test robot <lkp@intel.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 */
> +
KP Singh July 6, 2022, 11:49 p.m. UTC | #2 
On Wed, Jul 6, 2022 at 6:04 PM Daniel Borkmann <daniel@iogearbox.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@huawei.com>
> > Reported-by: kernel test robot <lkp@intel.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?

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.

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/linux/lsm_hooks.h#n1158

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

>
> > +     /* 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 */
> > +
Roberto Sassu July 7, 2022, 11 a.m. UTC | #3 
> From: KP Singh [mailto:kpsingh@kernel.org]
> Sent: Thursday, July 7, 2022 1:49 AM
> On Wed, Jul 6, 2022 at 6:04 PM Daniel Borkmann <daniel@iogearbox.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@huawei.com>
> > > Reported-by: kernel test robot <lkp@intel.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 */
> > > +
Roberto Sassu July 8, 2022, 12:12 p.m. UTC | #4 
> From: Roberto Sassu [mailto:roberto.sassu@huawei.com]
> Sent: Thursday, July 7, 2022 1:01 PM
> > From: KP Singh [mailto:kpsingh@kernel.org]
> > Sent: Thursday, July 7, 2022 1:49 AM
> > On Wed, Jul 6, 2022 at 6:04 PM Daniel Borkmann <daniel@iogearbox.net>
> > wrote:

[...]

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

I still have the impression that a helper for lookup_user_key() is the
preferred solution, despite the access control concern.

David, may ask if this is the correct way to use the key subsystem
API when permission check is deferred? key_permission() is currently
not defined outside the key subsystem.

The first three functions will be exposed by the kernel to eBPF programs
and can be called at any time. bpf_<key_helper> is a generic helper
dealing with a key.

BPF_CALL_2(bpf_lookup_user_key, u32, serial, unsigned long, flags)
{
...
        key_ref = lookup_user_key(serial, flags, KEY_DEFER_PERM_CHECK);
...
        return (unsigned long)key_ref_to_ptr(key_ref);
}

BPF_CALL_X(bpf_<key_helper>, struct key, *key, ...)
{
        ret = key_read_state(key);
...
        ret = key_validate(key);
...
        ret = key_permission(key_ref, <key helper-specific permission>);
...
}

BPF_CALL_1(bpf_key_put, struct key *, key)
{
        key_put(key);
        return 0;
}

An eBPF program would do for example:

SEC("lsm.s/bpf")
int BPF_PROG(bpf, int cmd, union bpf_attr *attr, unsigned int size)
{
        struct key *key;
...
        key = bpf_lookup_user_key(serial, flags);
...
        ret = bpf_key_helper(key, ...);
...
        key_put(key);
}

Thanks

Roberto
diff mbox series

Patch

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)
+ *	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),	\
 	/* */
 
 /* 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;
+
+	/* 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 */
+
 static const struct bpf_func_proto *
 bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
@@ -158,6 +216,11 @@  bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return prog->aux->sleepable ? &bpf_ima_file_hash_proto : NULL;
 	case BPF_FUNC_get_attach_cookie:
 		return bpf_prog_has_trampoline(prog) ? &bpf_get_attach_cookie_proto : NULL;
+#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
+	case BPF_FUNC_verify_pkcs7_signature:
+		return prog->aux->sleepable ?
+		       &bpf_verify_pkcs7_signature_proto : NULL;
+#endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
 	default:
 		return tracing_prog_func_proto(func_id, prog);
 	}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index e81362891596..b4f5ad863281 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/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)
+ *	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),	\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper