One of the desirable features in security is the ability to restrict import of data to a given system based on data authenticity. If data import can be restricted, it would be possible to enforce a system-wide policy based on the signing keys the system owner trusts.
This feature is widely used in the kernel. For example, if the restriction is enabled, kernel modules can be plugged in only if they are signed with a key whose public part is in the primary or secondary keyring.
For eBPF, it can be useful as well. For example, it might be useful to authenticate data an eBPF program makes security decisions on.
After a discussion in the eBPF mailing list, it was decided that the stated goal should be accomplished by introducing a new helper: bpf_verify_pkcs7_signature(), dedicated to verify PKCS#7 signatures. More helpers will be introduced later, as necessary.
The job of bpf_verify_pkcs7_signature() is simply to call the corresponding signature verification function verify_pkcs7_signature(). Data and signature can be provided to the new helper with two dynamic pointers, to reduce the number of parameters.
The keyring can be provided from its serial number, with the new helper bpf_lookup_user_key(). Since it acquires a reference of the found key, the corresponding release helper bpf_key_put() has been introduced to decrement the reference count. The eBPF verifier has been enhanced to ensure that the key reference count is always decreased, when incremented, or otherwise it refuses to load the program. This ability is being verified with the lookup_user_key_norelease test.
While the new helpers provide great flexibility, they seem to be suboptimal in terms of security guarantees. If the goal is to do signature verification with system-provided keys (e.g. from the built-in keyring), the eBPF program would have to rely on the user space counterpart to search the correct keyring and to pass its serial. If only the eBPF program is signed and verified, there is not certainty that this operation is done correctly by unverified code.
Instead, since verify_pkcs7_signature() understands a pre-determined set of struct key pointer values, which translates into the corresponding system keyrings, the keyring ID parameter has been added as well to the eBPF helper. It is considered only if the passed struct key pointer is NULL. That would guaranteed, assuming that the keyring ID is hardcoded, that signature verification is always done with the desired keys.
The introduced helpers can be called only from sleepable programs, because of memory allocation (with key flag KEY_LOOKUP_CREATE) and crypto operations. For example, the lsm.s/bpf attach point is suitable, fexit/array_map_update_elem is not.
A test was added to check the ability of bpf_verify_pkcs7_signature() to verify PKCS#7 signatures from the session keyring, a newly-created keyring, and from the secondary keyring (taking an existing kernel module for the verification). The test does not fail if a suitable kernel module is not found (needs support from the CI).
The patch set is organized as follows.
Patch 1 exports bpf_dynptr_get_size(), to obtain the real size of data carried by a dynamic pointer. Patch 2 introduces the bpf_lookup_user_key() and bpf_key_put() helpers. Patch 3 introduces the bpf_verify_pkcs7_signature() helper. Patches 4 and 5 respectively add the test for the first and the last helper.
Changelog
v4: - Remove bpf_request_key_by_id(), don't return an invalid pointer that other helpers can use - Pass the keyring ID (without ULONG_MAX, suggested by Alexei) to bpf_verify_pkcs7_signature() - Introduce bpf_lookup_user_key() and bpf_key_put() helpers (suggested by Alexei) - Add lookup_key_norelease test, to ensure that the verifier blocks eBPF programs which don't decrement the key reference count - Parse raw PKCS#7 signature instead of module-style signature in the verify_pkcs7_signature test (suggested by Alexei) - Parse kernel module in user space and pass raw PKCS#7 signature to the eBPF program for signature verification
v3: - Rename bpf_verify_signature() back to bpf_verify_pkcs7_signature() to avoid managing different parameters for each signature verification function in one helper (suggested by Daniel) - Use dynamic pointers and export bpf_dynptr_get_size() (suggested by Alexei) - Introduce bpf_request_key_by_id() to give more flexibility to the caller of bpf_verify_pkcs7_signature() to retrieve the appropriate keyring (suggested by Alexei) - Fix test by reordering the gcc command line, always compile sign-file - Improve helper support check mechanism in the test
v2: - Rename bpf_verify_pkcs7_signature() to a more generic bpf_verify_signature() and pass the signature type (suggested by KP) - Move the helper and prototype declaration under #ifdef so that user space can probe for support for the helper (suggested by Daniel) - Describe better the keyring types (suggested by Daniel) - Include linux/bpf.h instead of vmlinux.h to avoid implicit or redeclaration - Make the test selfcontained (suggested by Alexei)
v1: - Don't define new map flag but introduce simple wrapper of verify_pkcs7_signature() (suggested by Alexei and KP)
Roberto Sassu (5): bpf: Export bpf_dynptr_get_size() bpf: Add bpf_lookup_user_key() and bpf_key_put() helpers bpf: Add bpf_verify_pkcs7_signature() helper selftests/bpf: Add test for unreleased key references selftests/bpf: Add test for bpf_verify_pkcs7_signature() helper
include/linux/bpf.h | 1 + include/uapi/linux/bpf.h | 33 ++ kernel/bpf/bpf_lsm.c | 85 +++++ kernel/bpf/helpers.c | 2 +- kernel/bpf/verifier.c | 6 +- scripts/bpf_doc.py | 2 + tools/include/uapi/linux/bpf.h | 33 ++ tools/testing/selftests/bpf/Makefile | 14 +- tools/testing/selftests/bpf/config | 2 + .../prog_tests/lookup_user_key_norelease.c | 52 +++ .../bpf/prog_tests/verify_pkcs7_sig.c | 341 ++++++++++++++++++ .../progs/test_lookup_user_key_norelease.c | 24 ++ .../bpf/progs/test_verify_pkcs7_sig.c | 90 +++++ .../testing/selftests/bpf/verify_sig_setup.sh | 104 ++++++ 14 files changed, 783 insertions(+), 6 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/lookup_user_key_norelease.c create mode 100644 tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c create mode 100644 tools/testing/selftests/bpf/progs/test_lookup_user_key_norelease.c create mode 100644 tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c create mode 100755 tools/testing/selftests/bpf/verify_sig_setup.sh
Export bpf_dynptr_get_size(), so that kernel code dealing with eBPF dynamic pointers can obtain the real size of data carried by this data structure.
Reviewed-by: Joanne Koong joannelkoong@gmail.com Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- include/linux/bpf.h | 1 + kernel/bpf/helpers.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index d05e1495a06e..3ec0f167e9d7 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -2521,5 +2521,6 @@ void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data, enum bpf_dynptr_type type, u32 offset, u32 size); void bpf_dynptr_set_null(struct bpf_dynptr_kern *ptr); int bpf_dynptr_check_size(u32 size); +u32 bpf_dynptr_get_size(struct bpf_dynptr_kern *ptr);
#endif /* _LINUX_BPF_H */ diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index a1c84d256f83..3f5ff8dbd3cb 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -1430,7 +1430,7 @@ static void bpf_dynptr_set_type(struct bpf_dynptr_kern *ptr, enum bpf_dynptr_typ ptr->size |= type << DYNPTR_TYPE_SHIFT; }
-static u32 bpf_dynptr_get_size(struct bpf_dynptr_kern *ptr) +u32 bpf_dynptr_get_size(struct bpf_dynptr_kern *ptr) { return ptr->size & DYNPTR_SIZE_MASK; }
Add the bpf_lookup_user_key() and bpf_key_put() helpers, to respectively search a key with a given serial, and release the reference count of the found key.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- include/uapi/linux/bpf.h | 16 ++++++++++++ kernel/bpf/bpf_lsm.c | 46 ++++++++++++++++++++++++++++++++++ kernel/bpf/verifier.c | 6 +++-- scripts/bpf_doc.py | 2 ++ tools/include/uapi/linux/bpf.h | 16 ++++++++++++ 5 files changed, 84 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index e81362891596..7bbcf2cd105d 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -5325,6 +5325,20 @@ union bpf_attr { * **-EACCES** if the SYN cookie is not valid. * * **-EPROTONOSUPPORT** if CONFIG_IPV6 is not builtin. + * + * struct key *bpf_lookup_user_key(u32 serial, unsigned long flags) + * Description + * Search a key with a given *serial* and the provided *flags*, and + * increment the reference count of the key. + * Return + * A key pointer if the key is found, a NULL pointer otherwise. + * + * void bpf_key_put(struct key *key) + * Description + * Decrement the reference count of the key obtained with the + * bpf_lookup_user_key() helper. + * Return + * 0 */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -5535,6 +5549,8 @@ union bpf_attr { FN(tcp_raw_gen_syncookie_ipv6), \ FN(tcp_raw_check_syncookie_ipv4), \ FN(tcp_raw_check_syncookie_ipv6), \ + FN(lookup_user_key), \ + FN(key_put), \ /* */
/* 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..bbbf9640f391 100644 --- a/kernel/bpf/bpf_lsm.c +++ b/kernel/bpf/bpf_lsm.c @@ -132,6 +132,46 @@ static const struct bpf_func_proto bpf_get_attach_cookie_proto = { .arg1_type = ARG_PTR_TO_CTX, };
+#ifdef CONFIG_KEYS +BTF_ID_LIST_SINGLE(btf_key_ids, struct, key) + +BPF_CALL_2(bpf_lookup_user_key, u32, serial, unsigned long, flags) +{ + key_ref_t key_ref; + + key_ref = lookup_user_key(serial, flags, KEY_NEED_SEARCH); + if (IS_ERR(key_ref)) + return (unsigned long)NULL; + + return (unsigned long)key_ref_to_ptr(key_ref); +} + +static const struct bpf_func_proto bpf_lookup_user_key_proto = { + .func = bpf_lookup_user_key, + .gpl_only = false, + .ret_type = RET_PTR_TO_BTF_ID_OR_NULL, + .ret_btf_id = &btf_key_ids[0], + .arg1_type = ARG_ANYTHING, + .arg2_type = ARG_ANYTHING, + .allowed = bpf_ima_inode_hash_allowed, +}; + +BPF_CALL_1(bpf_key_put, struct key *, key) +{ + key_put(key); + return 0; +} + +static const struct bpf_func_proto bpf_key_put_proto = { + .func = bpf_key_put, + .gpl_only = false, + .ret_type = RET_VOID, + .arg1_type = ARG_PTR_TO_BTF_ID | OBJ_RELEASE, + .arg1_btf_id = &btf_key_ids[0], + .allowed = bpf_ima_inode_hash_allowed, +}; +#endif /* CONFIG_KEYS */ + static const struct bpf_func_proto * bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) { @@ -158,6 +198,12 @@ 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_KEYS + case BPF_FUNC_lookup_user_key: + return &bpf_lookup_user_key_proto; + case BPF_FUNC_key_put: + return &bpf_key_put_proto; +#endif /* CONFIG_KEYS */ default: return tracing_prog_func_proto(func_id, prog); } diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index bf72dc511df6..12f06ca649a4 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -483,7 +483,8 @@ static bool may_be_acquire_function(enum bpf_func_id func_id) func_id == BPF_FUNC_sk_lookup_udp || func_id == BPF_FUNC_skc_lookup_tcp || func_id == BPF_FUNC_map_lookup_elem || - func_id == BPF_FUNC_ringbuf_reserve; + func_id == BPF_FUNC_ringbuf_reserve || + func_id == BPF_FUNC_lookup_user_key; }
static bool is_acquire_function(enum bpf_func_id func_id, @@ -495,7 +496,8 @@ static bool is_acquire_function(enum bpf_func_id func_id, func_id == BPF_FUNC_sk_lookup_udp || func_id == BPF_FUNC_skc_lookup_tcp || func_id == BPF_FUNC_ringbuf_reserve || - func_id == BPF_FUNC_kptr_xchg) + func_id == BPF_FUNC_kptr_xchg || + func_id == BPF_FUNC_lookup_user_key) return true;
if (func_id == BPF_FUNC_map_lookup_elem && diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py index a0ec321469bd..3d5a7ad6f493 100755 --- a/scripts/bpf_doc.py +++ b/scripts/bpf_doc.py @@ -637,6 +637,7 @@ class PrinterHelpers(Printer): 'struct bpf_dynptr', 'struct iphdr', 'struct ipv6hdr', + 'struct key', ] known_types = { '...', @@ -690,6 +691,7 @@ class PrinterHelpers(Printer): 'struct bpf_dynptr', 'struct iphdr', 'struct ipv6hdr', + 'struct key', } mapped_types = { 'u8': '__u8', diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index e81362891596..7bbcf2cd105d 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -5325,6 +5325,20 @@ union bpf_attr { * **-EACCES** if the SYN cookie is not valid. * * **-EPROTONOSUPPORT** if CONFIG_IPV6 is not builtin. + * + * struct key *bpf_lookup_user_key(u32 serial, unsigned long flags) + * Description + * Search a key with a given *serial* and the provided *flags*, and + * increment the reference count of the key. + * Return + * A key pointer if the key is found, a NULL pointer otherwise. + * + * void bpf_key_put(struct key *key) + * Description + * Decrement the reference count of the key obtained with the + * bpf_lookup_user_key() helper. + * Return + * 0 */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -5535,6 +5549,8 @@ union bpf_attr { FN(tcp_raw_gen_syncookie_ipv6), \ FN(tcp_raw_check_syncookie_ipv4), \ FN(tcp_raw_check_syncookie_ipv6), \ + FN(lookup_user_key), \ + FN(key_put), \ /* */
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
On Tue, Jun 21, 2022 at 06:37:54PM +0200, Roberto Sassu wrote:
Add the bpf_lookup_user_key() and bpf_key_put() helpers, to respectively search a key with a given serial, and release the reference count of the found key.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
include/uapi/linux/bpf.h | 16 ++++++++++++ kernel/bpf/bpf_lsm.c | 46 ++++++++++++++++++++++++++++++++++ kernel/bpf/verifier.c | 6 +++-- scripts/bpf_doc.py | 2 ++ tools/include/uapi/linux/bpf.h | 16 ++++++++++++ 5 files changed, 84 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index e81362891596..7bbcf2cd105d 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -5325,6 +5325,20 @@ union bpf_attr {
**-EACCES** if the SYN cookie is not valid.**-EPROTONOSUPPORT** if CONFIG_IPV6 is not builtin.
- struct key *bpf_lookup_user_key(u32 serial, unsigned long flags)
- Description
Search a key with a given *serial* and the provided *flags*, and
increment the reference count of the key.
Why passing 'flags' is ok to do? Please think through every line of the patch.
From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] Sent: Wednesday, June 22, 2022 12:33 AM On Tue, Jun 21, 2022 at 06:37:54PM +0200, Roberto Sassu wrote:
Add the bpf_lookup_user_key() and bpf_key_put() helpers, to respectively search a key with a given serial, and release the reference count of the found key.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
include/uapi/linux/bpf.h | 16 ++++++++++++ kernel/bpf/bpf_lsm.c | 46 ++++++++++++++++++++++++++++++++++ kernel/bpf/verifier.c | 6 +++-- scripts/bpf_doc.py | 2 ++ tools/include/uapi/linux/bpf.h | 16 ++++++++++++ 5 files changed, 84 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index e81362891596..7bbcf2cd105d 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -5325,6 +5325,20 @@ union bpf_attr {
**-EACCES** if the SYN cookie is not valid.**-EPROTONOSUPPORT** if CONFIG_IPV6 is not builtin.
- struct key *bpf_lookup_user_key(u32 serial, unsigned long flags)
- Description
Search a key with a given *serial* and the provided *flags*, and
increment the reference count of the key.Why passing 'flags' is ok to do? Please think through every line of the patch.
To be honest, I thought about it. Probably yes, I should do some sanitization, like I did for the keyring ID. When I checked lookup_user_key(), I saw that flags are checked individually, so an arbitrary value passed to the helper should not cause harm. Will do sanitization, if you prefer. It is just that we have to keep the eBPF code in sync with key flag definition (unless we have a 'last' flag).
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Yang Xi, Li He
From: Roberto Sassu [mailto:roberto.sassu@huawei.com] Sent: Wednesday, June 22, 2022 9:12 AM
From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] Sent: Wednesday, June 22, 2022 12:33 AM On Tue, Jun 21, 2022 at 06:37:54PM +0200, Roberto Sassu wrote:
Add the bpf_lookup_user_key() and bpf_key_put() helpers, to respectively search a key with a given serial, and release the reference count of the found key.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
include/uapi/linux/bpf.h | 16 ++++++++++++ kernel/bpf/bpf_lsm.c | 46 ++++++++++++++++++++++++++++++++++ kernel/bpf/verifier.c | 6 +++-- scripts/bpf_doc.py | 2 ++ tools/include/uapi/linux/bpf.h | 16 ++++++++++++ 5 files changed, 84 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index e81362891596..7bbcf2cd105d 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -5325,6 +5325,20 @@ union bpf_attr {
**-EACCES** if the SYN cookie is not valid.**-EPROTONOSUPPORT** if CONFIG_IPV6 is not builtin.
- struct key *bpf_lookup_user_key(u32 serial, unsigned long flags)
- Description
Search a key with a given *serial* and the provided *flags*, and
increment the reference count of the key.Why passing 'flags' is ok to do? Please think through every line of the patch.
To be honest, I thought about it. Probably yes, I should do some sanitization, like I did for the keyring ID. When I checked lookup_user_key(), I saw that flags are checked individually, so an arbitrary value passed to the helper should not cause harm. Will do sanitization, if you prefer. It is just that we have to keep the eBPF code in sync with key flag definition (unless we have a 'last' flag).
I'm not sure that having a helper for lookup_user_key() alone is correct. By having separate helpers for lookup and usage of the key, nothing would prevent an eBPF program to ask for a permission to pass the access control check, and then use the key for something completely different from what it requested.
Looking at how lookup_user_key() is used in security/keys/keyctl.c, it seems clear that it should be used together with the operation that needs to be performed. Only in this way, the key permission would make sense.
What do you think (also David)?
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Yang Xi, Li He
On Thu, Jun 23, 2022 at 5:36 AM Roberto Sassu roberto.sassu@huawei.com wrote:
From: Roberto Sassu [mailto:roberto.sassu@huawei.com] Sent: Wednesday, June 22, 2022 9:12 AM
From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] Sent: Wednesday, June 22, 2022 12:33 AM On Tue, Jun 21, 2022 at 06:37:54PM +0200, Roberto Sassu wrote:
Add the bpf_lookup_user_key() and bpf_key_put() helpers, to respectively search a key with a given serial, and release the reference count of the found key.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
include/uapi/linux/bpf.h | 16 ++++++++++++ kernel/bpf/bpf_lsm.c | 46 ++++++++++++++++++++++++++++++++++ kernel/bpf/verifier.c | 6 +++-- scripts/bpf_doc.py | 2 ++ tools/include/uapi/linux/bpf.h | 16 ++++++++++++ 5 files changed, 84 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index e81362891596..7bbcf2cd105d 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -5325,6 +5325,20 @@ union bpf_attr {
**-EACCES** if the SYN cookie is not valid.**-EPROTONOSUPPORT** if CONFIG_IPV6 is not builtin.
- struct key *bpf_lookup_user_key(u32 serial, unsigned long flags)
Description
Search a key with a given *serial* and the provided *flags*, and
increment the reference count of the key.Why passing 'flags' is ok to do? Please think through every line of the patch.
To be honest, I thought about it. Probably yes, I should do some sanitization, like I did for the keyring ID. When I checked lookup_user_key(), I saw that flags are checked individually, so an arbitrary value passed to the helper should not cause harm. Will do sanitization, if you prefer. It is just that we have to keep the eBPF code in sync with key flag definition (unless we have a 'last' flag).
I'm not sure that having a helper for lookup_user_key() alone is correct. By having separate helpers for lookup and usage of the key, nothing would prevent an eBPF program to ask for a permission to pass the access control check, and then use the key for something completely different from what it requested.
Looking at how lookup_user_key() is used in security/keys/keyctl.c, it seems clear that it should be used together with the operation that needs to be performed. Only in this way, the key permission would make sense.
lookup is roughly equivalent to open when all permission checks are done. And using the key is read/write.
What do you think (also David)?
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Yang Xi, Li He
Please use a different email server and get rid of this.
From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] Sent: Thursday, June 23, 2022 10:54 PM On Thu, Jun 23, 2022 at 5:36 AM Roberto Sassu roberto.sassu@huawei.com wrote:
From: Roberto Sassu [mailto:roberto.sassu@huawei.com] Sent: Wednesday, June 22, 2022 9:12 AM
From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] Sent: Wednesday, June 22, 2022 12:33 AM On Tue, Jun 21, 2022 at 06:37:54PM +0200, Roberto Sassu wrote:
Add the bpf_lookup_user_key() and bpf_key_put() helpers, to respectively search a key with a given serial, and release the reference count of the found key.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
include/uapi/linux/bpf.h | 16 ++++++++++++ kernel/bpf/bpf_lsm.c | 46
++++++++++++++++++++++++++++++++++
kernel/bpf/verifier.c | 6 +++-- scripts/bpf_doc.py | 2 ++ tools/include/uapi/linux/bpf.h | 16 ++++++++++++ 5 files changed, 84 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index e81362891596..7bbcf2cd105d 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -5325,6 +5325,20 @@ union bpf_attr {
**-EACCES** if the SYN cookie is not valid.**-EPROTONOSUPPORT** if CONFIG_IPV6 is not builtin.
- struct key *bpf_lookup_user_key(u32 serial, unsigned long flags)
Description
Search a key with a given *serial* and the provided *flags*,and
increment the reference count of the key.Why passing 'flags' is ok to do? Please think through every line of the patch.
To be honest, I thought about it. Probably yes, I should do some sanitization, like I did for the keyring ID. When I checked lookup_user_key(), I saw that flags are checked individually, so an arbitrary value passed to the helper should not cause harm. Will do sanitization, if you prefer. It is just that we have to keep the eBPF code in sync with key flag definition (unless we have a 'last' flag).
I'm not sure that having a helper for lookup_user_key() alone is correct. By having separate helpers for lookup and usage of the key, nothing would prevent an eBPF program to ask for a permission to pass the access control check, and then use the key for something completely different from what it requested.
Looking at how lookup_user_key() is used in security/keys/keyctl.c, it seems clear that it should be used together with the operation that needs to be performed. Only in this way, the key permission would make sense.
lookup is roughly equivalent to open when all permission checks are done. And using the key is read/write.
For bpf_verify_pkcs7_signature(), we need the search permission on the keyring containing the key used for signature verification.
Thanks
Roberto
(I was not told otherwise, I use my corporate email to send work to outside, so I have to keep the notice)
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Yang Xi, Li He
On Fri, Jun 24, 2022 at 8:32 AM Roberto Sassu roberto.sassu@huawei.com wrote:
From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] Sent: Thursday, June 23, 2022 10:54 PM On Thu, Jun 23, 2022 at 5:36 AM Roberto Sassu roberto.sassu@huawei.com wrote:
From: Roberto Sassu [mailto:roberto.sassu@huawei.com] Sent: Wednesday, June 22, 2022 9:12 AM
From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] Sent: Wednesday, June 22, 2022 12:33 AM On Tue, Jun 21, 2022 at 06:37:54PM +0200, Roberto Sassu wrote:
Add the bpf_lookup_user_key() and bpf_key_put() helpers, to respectively search a key with a given serial, and release the reference count of the found key.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
include/uapi/linux/bpf.h | 16 ++++++++++++ kernel/bpf/bpf_lsm.c | 46
++++++++++++++++++++++++++++++++++
kernel/bpf/verifier.c | 6 +++-- scripts/bpf_doc.py | 2 ++ tools/include/uapi/linux/bpf.h | 16 ++++++++++++ 5 files changed, 84 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index e81362891596..7bbcf2cd105d 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -5325,6 +5325,20 @@ union bpf_attr {
**-EACCES** if the SYN cookie is not valid.**-EPROTONOSUPPORT** if CONFIG_IPV6 is not builtin.
- struct key *bpf_lookup_user_key(u32 serial, unsigned long flags)
Description
Search a key with a given *serial* and the provided *flags*,and
increment the reference count of the key.Why passing 'flags' is ok to do? Please think through every line of the patch.
To be honest, I thought about it. Probably yes, I should do some sanitization, like I did for the keyring ID. When I checked lookup_user_key(), I saw that flags are checked individually, so an arbitrary value passed to the helper should not cause harm. Will do sanitization, if you prefer. It is just that we have to keep the eBPF code in sync with key flag definition (unless we have a 'last' flag).
I'm not sure that having a helper for lookup_user_key() alone is correct. By having separate helpers for lookup and usage of the key, nothing would prevent an eBPF program to ask for a permission to pass the access control check, and then use the key for something completely different from what it requested.
Looking at how lookup_user_key() is used in security/keys/keyctl.c, it seems clear that it should be used together with the operation that needs to be performed. Only in this way, the key permission would make sense.
lookup is roughly equivalent to open when all permission checks are done. And using the key is read/write.
For bpf_verify_pkcs7_signature(), we need the search permission on the keyring containing the key used for signature verification.
you mean lookup_user_key(serial, flags, KEY_NEED_SEARCH) ?
right. and ? what's your point?
From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] Sent: Friday, June 24, 2022 6:50 PM On Fri, Jun 24, 2022 at 8:32 AM Roberto Sassu roberto.sassu@huawei.com wrote:
From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] Sent: Thursday, June 23, 2022 10:54 PM On Thu, Jun 23, 2022 at 5:36 AM Roberto Sassu
wrote:
From: Roberto Sassu [mailto:roberto.sassu@huawei.com] Sent: Wednesday, June 22, 2022 9:12 AM
From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] Sent: Wednesday, June 22, 2022 12:33 AM On Tue, Jun 21, 2022 at 06:37:54PM +0200, Roberto Sassu wrote: > Add the bpf_lookup_user_key() and bpf_key_put() helpers, to
respectively
> search a key with a given serial, and release the reference count of
the
> found key. > > Signed-off-by: Roberto Sassu roberto.sassu@huawei.com > --- > include/uapi/linux/bpf.h | 16 ++++++++++++ > kernel/bpf/bpf_lsm.c | 46
++++++++++++++++++++++++++++++++++
> kernel/bpf/verifier.c | 6 +++-- > scripts/bpf_doc.py | 2 ++ > tools/include/uapi/linux/bpf.h | 16 ++++++++++++ > 5 files changed, 84 insertions(+), 2 deletions(-) > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index e81362891596..7bbcf2cd105d 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -5325,6 +5325,20 @@ union bpf_attr { > * **-EACCES** if the SYN cookie is not valid. > * > * **-EPROTONOSUPPORT** if CONFIG_IPV6 is not builtin. > + * > + * struct key *bpf_lookup_user_key(u32 serial, unsigned long flags) > + * Description > + * Search a key with a given *serial* and the provided
*flags*,
and
> + * increment the reference count of the key.
Why passing 'flags' is ok to do? Please think through every line of the patch.
To be honest, I thought about it. Probably yes, I should do some sanitization, like I did for the keyring ID. When I checked lookup_user_key(), I saw that flags are checked individually, so an arbitrary value passed to the helper should not cause harm. Will do sanitization, if you prefer. It is just that we have to keep the eBPF code in sync with key flag definition (unless we have a 'last' flag).
I'm not sure that having a helper for lookup_user_key() alone is correct. By having separate helpers for lookup and usage of the key, nothing would prevent an eBPF program to ask for a permission to pass the access control check, and then use the key for something completely different from what it requested.
Looking at how lookup_user_key() is used in security/keys/keyctl.c, it seems clear that it should be used together with the operation that needs to be performed. Only in this way, the key permission would make sense.
lookup is roughly equivalent to open when all permission checks are done. And using the key is read/write.
For bpf_verify_pkcs7_signature(), we need the search permission on the keyring containing the key used for signature verification.
you mean lookup_user_key(serial, flags, KEY_NEED_SEARCH) ?
right. and ? what's your point?
It is hardcoded. Does not necessarily reflect the operation that will be performed on the key.
On the other hand, if I add the permission as parameter to bpf_lookup_user_key(), an eBPF program can pass an arbitrary value, and then do something completely different with the key.
Roberto
From: Roberto Sassu Sent: Friday, June 24, 2022 5:33 PM
From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] Sent: Thursday, June 23, 2022 10:54 PM On Thu, Jun 23, 2022 at 5:36 AM Roberto Sassu roberto.sassu@huawei.com wrote:
From: Roberto Sassu [mailto:roberto.sassu@huawei.com] Sent: Wednesday, June 22, 2022 9:12 AM
From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] Sent: Wednesday, June 22, 2022 12:33 AM On Tue, Jun 21, 2022 at 06:37:54PM +0200, Roberto Sassu wrote:
Add the bpf_lookup_user_key() and bpf_key_put() helpers, to
respectively
search a key with a given serial, and release the reference count of the found key.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
include/uapi/linux/bpf.h | 16 ++++++++++++ kernel/bpf/bpf_lsm.c | 46
++++++++++++++++++++++++++++++++++
kernel/bpf/verifier.c | 6 +++-- scripts/bpf_doc.py | 2 ++ tools/include/uapi/linux/bpf.h | 16 ++++++++++++ 5 files changed, 84 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index e81362891596..7bbcf2cd105d 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -5325,6 +5325,20 @@ union bpf_attr {
**-EACCES** if the SYN cookie is not valid.**-EPROTONOSUPPORT** if CONFIG_IPV6 is not builtin.
- struct key *bpf_lookup_user_key(u32 serial, unsigned long flags)
Description
Search a key with a given *serial* and the provided *flags*,and
increment the reference count of the key.Why passing 'flags' is ok to do? Please think through every line of the patch.
To be honest, I thought about it. Probably yes, I should do some sanitization, like I did for the keyring ID. When I checked lookup_user_key(), I saw that flags are checked individually, so an arbitrary value passed to the helper should not cause harm. Will do sanitization, if you prefer. It is just that we have to keep the eBPF code in sync with key flag definition (unless we have a 'last' flag).
I'm not sure that having a helper for lookup_user_key() alone is correct. By having separate helpers for lookup and usage of the key, nothing would prevent an eBPF program to ask for a permission to pass the access control check, and then use the key for something completely different from what it requested.
Looking at how lookup_user_key() is used in security/keys/keyctl.c, it seems clear that it should be used together with the operation that needs to be performed. Only in this way, the key permission would make sense.
lookup is roughly equivalent to open when all permission checks are done. And using the key is read/write.
For bpf_verify_pkcs7_signature(), we need the search permission on the keyring containing the key used for signature verification.
Thanks
Roberto
(I was not told otherwise, I use my corporate email to send work to outside, so I have to keep the notice)
I can remove it. Just to let you know.
Roberto
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 keyring pointer obtained with bpf_lookup_user_key() or, alternatively, a keyring ID with values defined in verification.h. While the first choice gives users more flexibility, the second offers better security guarantees, as the keyring selection will not depend on possibly untrusted user space but on the kernel itself.
Defined keyring IDs are: 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).
Note: since the keyring ID assignment is understood only by verify_pkcs7_signature(), it must be passed directly to the corresponding helper, rather than to a separate new helper returning a struct key pointer with the keyring ID as a pointer value. If such pointer is passed to any other helper which does not check its validity, an illegal memory access could occur.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com Reported-by: kernel test robot lkp@intel.com (cast warning) --- include/uapi/linux/bpf.h | 17 +++++++++++++++ kernel/bpf/bpf_lsm.c | 39 ++++++++++++++++++++++++++++++++++ tools/include/uapi/linux/bpf.h | 17 +++++++++++++++ 3 files changed, 73 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 7bbcf2cd105d..524bed4d7170 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -5339,6 +5339,22 @@ union bpf_attr { * bpf_lookup_user_key() helper. * Return * 0 + * + * long bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr, struct bpf_dynptr *sig_ptr, struct key *trusted_keys, unsigned long keyring_id) + * Description + * Verify the PKCS#7 signature *sig* against the supplied *data* + * with keys in *trusted_keys* or in a keyring with ID + * *keyring_id*. + * + * *keyring_id* can have the following values defined in + * verification.h: 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), \ @@ -5551,6 +5567,7 @@ union bpf_attr { FN(tcp_raw_check_syncookie_ipv6), \ FN(lookup_user_key), \ FN(key_put), \ + 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 bbbf9640f391..1fd94958e88f 100644 --- a/kernel/bpf/bpf_lsm.c +++ b/kernel/bpf/bpf_lsm.c @@ -16,6 +16,7 @@ #include <linux/bpf_local_storage.h> #include <linux/btf_ids.h> #include <linux/ima.h> +#include <linux/verification.h>
/* For every LSM hook that allows attachment of BPF programs, declare a nop * function where a BPF program can be attached. @@ -170,6 +171,39 @@ static const struct bpf_func_proto bpf_key_put_proto = { .arg1_btf_id = &btf_key_ids[0], .allowed = bpf_ima_inode_hash_allowed, }; + +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION +BPF_CALL_4(bpf_verify_pkcs7_signature, struct bpf_dynptr_kern *, data_ptr, + struct bpf_dynptr_kern *, sig_ptr, struct key *, trusted_keys, + unsigned long, keyring_id) +{ + struct key *_trusted_keys = trusted_keys; + + if (!_trusted_keys && + keyring_id <= (unsigned long)VERIFY_USE_PLATFORM_KEYRING) + _trusted_keys = (struct key *)keyring_id; + + return verify_pkcs7_signature(data_ptr->data, + bpf_dynptr_get_size(data_ptr), + sig_ptr->data, + bpf_dynptr_get_size(sig_ptr), + _trusted_keys, + VERIFYING_UNSPECIFIED_SIGNATURE, NULL, + NULL); +} + +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_PTR_TO_BTF_ID_OR_NULL, + .arg3_btf_id = &btf_key_ids[0], + .arg4_type = ARG_ANYTHING, + .allowed = bpf_ima_inode_hash_allowed, +}; +#endif /* CONFIG_SYSTEM_DATA_VERIFICATION */ #endif /* CONFIG_KEYS */
static const struct bpf_func_proto * @@ -203,6 +237,11 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_lookup_user_key_proto; case BPF_FUNC_key_put: return &bpf_key_put_proto; +#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 */ #endif /* CONFIG_KEYS */ 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 7bbcf2cd105d..524bed4d7170 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -5339,6 +5339,22 @@ union bpf_attr { * bpf_lookup_user_key() helper. * Return * 0 + * + * long bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr, struct bpf_dynptr *sig_ptr, struct key *trusted_keys, unsigned long keyring_id) + * Description + * Verify the PKCS#7 signature *sig* against the supplied *data* + * with keys in *trusted_keys* or in a keyring with ID + * *keyring_id*. + * + * *keyring_id* can have the following values defined in + * verification.h: 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), \ @@ -5551,6 +5567,7 @@ union bpf_attr { FN(tcp_raw_check_syncookie_ipv6), \ FN(lookup_user_key), \ FN(key_put), \ + FN(verify_pkcs7_signature), \ /* */
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
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 keyring pointer obtained with bpf_lookup_user_key() or, alternatively, a keyring ID with values defined in verification.h. While the first choice gives users more flexibility, the second offers better security guarantees, as the keyring selection will not depend on possibly untrusted user space but on the kernel itself.
Defined keyring IDs are: 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).
Note: since the keyring ID assignment is understood only by verify_pkcs7_signature(), it must be passed directly to the corresponding helper, rather than to a separate new helper returning a struct key pointer with the keyring ID as a pointer value. If such pointer is passed to any other helper which does not check its validity, an illegal memory access could occur.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com Reported-by: kernel test robot lkp@intel.com (cast warning)
include/uapi/linux/bpf.h | 17 +++++++++++++++ kernel/bpf/bpf_lsm.c | 39 ++++++++++++++++++++++++++++++++++ tools/include/uapi/linux/bpf.h | 17 +++++++++++++++ 3 files changed, 73 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 7bbcf2cd105d..524bed4d7170 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -5339,6 +5339,22 @@ union bpf_attr {
bpf_lookup_user_key() helper.- Return
0
- long bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr, struct bpf_dynptr *sig_ptr, struct key *trusted_keys, unsigned long keyring_id)
- Description
Verify the PKCS#7 signature *sig* against the supplied *data*
with keys in *trusted_keys* or in a keyring with ID
*keyring_id*.
Would be nice to give precedence here so that its obvious order between trusted_keys and keyring_id.
*keyring_id* can have the following values defined in
verification.h: 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.
From: John Fastabend [mailto:john.fastabend@gmail.com] Sent: Wednesday, June 22, 2022 12:28 AM 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 keyring pointer obtained with bpf_lookup_user_key() or, alternatively, a keyring ID with values defined in verification.h. While the first choice gives users more flexibility, the second offers better security guarantees, as the keyring selection will not depend on possibly untrusted user space but on the kernel itself.
Defined keyring IDs are: 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).
Note: since the keyring ID assignment is understood only by verify_pkcs7_signature(), it must be passed directly to the corresponding helper, rather than to a separate new helper returning a struct key pointer with the keyring ID as a pointer value. If such pointer is passed to any other helper which does not check its validity, an illegal memory access could occur.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com Reported-by: kernel test robot lkp@intel.com (cast warning)
include/uapi/linux/bpf.h | 17 +++++++++++++++ kernel/bpf/bpf_lsm.c | 39 ++++++++++++++++++++++++++++++++++ tools/include/uapi/linux/bpf.h | 17 +++++++++++++++ 3 files changed, 73 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 7bbcf2cd105d..524bed4d7170 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -5339,6 +5339,22 @@ union bpf_attr {
bpf_lookup_user_key() helper.- Return
0
- long bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr, struct
bpf_dynptr *sig_ptr, struct key *trusted_keys, unsigned long keyring_id)
- Description
Verify the PKCS#7 signature *sig* against the supplied *data*
with keys in *trusted_keys* or in a keyring with ID
*keyring_id*.Would be nice to give precedence here so that its obvious order between trusted_keys and keyring_id.
Did you mean to add at the end of the sentence:
or in a keyring with ID *keyring_id*, if *trusted_keys* is NULL.
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Yang Xi, Li He
*keyring_id* can have the following values defined in
verification.h: 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.
Roberto Sassu wrote:
From: John Fastabend [mailto:john.fastabend@gmail.com] Sent: Wednesday, June 22, 2022 12:28 AM 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 keyring pointer obtained with bpf_lookup_user_key() or, alternatively, a keyring ID with values defined in verification.h. While the first choice gives users more flexibility, the second offers better security guarantees, as the keyring selection will not depend on possibly untrusted user space but on the kernel itself.
Defined keyring IDs are: 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).
Note: since the keyring ID assignment is understood only by verify_pkcs7_signature(), it must be passed directly to the corresponding helper, rather than to a separate new helper returning a struct key pointer with the keyring ID as a pointer value. If such pointer is passed to any other helper which does not check its validity, an illegal memory access could occur.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com Reported-by: kernel test robot lkp@intel.com (cast warning)
include/uapi/linux/bpf.h | 17 +++++++++++++++ kernel/bpf/bpf_lsm.c | 39 ++++++++++++++++++++++++++++++++++ tools/include/uapi/linux/bpf.h | 17 +++++++++++++++ 3 files changed, 73 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 7bbcf2cd105d..524bed4d7170 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -5339,6 +5339,22 @@ union bpf_attr {
bpf_lookup_user_key() helper.- Return
0
- long bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr, struct
bpf_dynptr *sig_ptr, struct key *trusted_keys, unsigned long keyring_id)
- Description
Verify the PKCS#7 signature *sig* against the supplied *data*
with keys in *trusted_keys* or in a keyring with ID
*keyring_id*.Would be nice to give precedence here so that its obvious order between trusted_keys and keyring_id.
Did you mean to add at the end of the sentence:
or in a keyring with ID *keyring_id*, if *trusted_keys* is NULL.
Yes something like this.
Ensure that the verifier detects the attempt of acquiring a reference of a key through the helper bpf_lookup_user_key(), without releasing that reference with bpf_key_put(), and refuses to load the program.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- .../prog_tests/lookup_user_key_norelease.c | 52 +++++++++++++++++++ .../progs/test_lookup_user_key_norelease.c | 24 +++++++++ 2 files changed, 76 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/lookup_user_key_norelease.c create mode 100644 tools/testing/selftests/bpf/progs/test_lookup_user_key_norelease.c
diff --git a/tools/testing/selftests/bpf/prog_tests/lookup_user_key_norelease.c b/tools/testing/selftests/bpf/prog_tests/lookup_user_key_norelease.c new file mode 100644 index 000000000000..6753c4f591e3 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/lookup_user_key_norelease.c @@ -0,0 +1,52 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH + * + * Author: Roberto Sassu roberto.sassu@huawei.com + */ + +#include <test_progs.h> + +#include "test_lookup_user_key_norelease.skel.h" + +#define LOG_BUF_SIZE 16384 + +void test_lookup_user_key_norelease(void) +{ + char *buf = NULL, *result; + struct test_lookup_user_key_norelease *skel = NULL; + int ret; + + LIBBPF_OPTS(bpf_object_open_opts, opts); + + buf = malloc(LOG_BUF_SIZE); + if (!ASSERT_OK_PTR(buf, "malloc")) + goto close_prog; + + opts.kernel_log_buf = buf; + opts.kernel_log_size = LOG_BUF_SIZE; + opts.kernel_log_level = 1; + + skel = test_lookup_user_key_norelease__open_opts(&opts); + if (!ASSERT_OK_PTR(skel, "test_lookup_user_key_norelease__open_opts")) + goto close_prog; + + ret = test_lookup_user_key_norelease__load(skel); + if (!ASSERT_LT(ret, 0, "test_lookup_user_key_norelease__load\n")) + goto close_prog; + + if (strstr(buf, "unknown func bpf_lookup_user_key")) { + printf("%s:SKIP:bpf_lookup_user_key() helper not supported\n", + __func__); + test__skip(); + goto close_prog; + } + + result = strstr(buf, "Unreleased reference"); + ASSERT_OK_PTR(result, "Error message not found"); + +close_prog: + free(buf); + test_lookup_user_key_norelease__destroy(skel); +} diff --git a/tools/testing/selftests/bpf/progs/test_lookup_user_key_norelease.c b/tools/testing/selftests/bpf/progs/test_lookup_user_key_norelease.c new file mode 100644 index 000000000000..cfe474c77886 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_lookup_user_key_norelease.c @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH + * + * Author: Roberto Sassu roberto.sassu@huawei.com + */ + +#include <errno.h> +#include <stdlib.h> +#include <limits.h> +#include <linux/bpf.h> +#include <linux/keyctl.h> +#include <bpf/bpf_helpers.h> +#include <bpf/bpf_tracing.h> + +char _license[] SEC("license") = "GPL"; + +SEC("lsm.s/bpf") +int BPF_PROG(bpf, int cmd, union bpf_attr *attr, unsigned int size) +{ + bpf_lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0); + return 0; +}
Roberto Sassu wrote:
Ensure that the verifier detects the attempt of acquiring a reference of a key through the helper bpf_lookup_user_key(), without releasing that reference with bpf_key_put(), and refuses to load the program.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
Any reason not to add these to ./verifier/ref_tracking.c tests? Seems it might be easier to follow there and test both good/bad cases.
From: John Fastabend [mailto:john.fastabend@gmail.com] Sent: Wednesday, June 22, 2022 12:36 AM Roberto Sassu wrote:
Ensure that the verifier detects the attempt of acquiring a reference of a key through the helper bpf_lookup_user_key(), without releasing that reference with bpf_key_put(), and refuses to load the program.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
Any reason not to add these to ./verifier/ref_tracking.c tests? Seems it might be easier to follow there and test both good/bad cases.
Oh, I didn't know about it. Will move the test.
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Yang Xi, Li He
Ensure that signature verification is performed successfully from an eBPF program, with the new bpf_verify_pkcs7_signature() helper.
Generate a testing signature key and compile sign-file from scripts/, so that the test is selfcontained. Also, try to verify an existing kernel module.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- tools/testing/selftests/bpf/Makefile | 14 +- tools/testing/selftests/bpf/config | 2 + .../bpf/prog_tests/verify_pkcs7_sig.c | 341 ++++++++++++++++++ .../bpf/progs/test_verify_pkcs7_sig.c | 90 +++++ .../testing/selftests/bpf/verify_sig_setup.sh | 104 ++++++ 5 files changed, 548 insertions(+), 3 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c create mode 100644 tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c create mode 100755 tools/testing/selftests/bpf/verify_sig_setup.sh
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index cb8e552e1418..bacd97808ad5 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -14,6 +14,7 @@ BPFTOOLDIR := $(TOOLSDIR)/bpf/bpftool APIDIR := $(TOOLSINCDIR)/uapi GENDIR := $(abspath ../../../../include/generated) GENHDR := $(GENDIR)/autoconf.h +HOSTPKG_CONFIG := pkg-config
ifneq ($(wildcard $(GENHDR)),) GENFLAGS := -DHAVE_GENHDR @@ -75,7 +76,7 @@ TEST_PROGS := test_kmod.sh \ test_xsk.sh
TEST_PROGS_EXTENDED := with_addr.sh \ - with_tunnels.sh ima_setup.sh \ + with_tunnels.sh ima_setup.sh verify_sig_setup.sh \ test_xdp_vlan.sh test_bpftool.py
# Compile but not part of 'make run_tests' @@ -84,7 +85,7 @@ TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \ test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \ xdpxceiver xdp_redirect_multi xdp_synproxy
-TEST_CUSTOM_PROGS = $(OUTPUT)/urandom_read +TEST_CUSTOM_PROGS = $(OUTPUT)/urandom_read $(OUTPUT)/sign-file
# Emit succinct information message describing current building step # $1 - generic step name (e.g., CC, LINK, etc); @@ -189,6 +190,12 @@ $(OUTPUT)/urandom_read: urandom_read.c urandom_read_aux.c $(OUTPUT)/liburandom_r -fuse-ld=$(LLD) -Wl,-znoseparate-code \ -Wl,-rpath=. -Wl,--build-id=sha1 -o $@
+$(OUTPUT)/sign-file: ../../../../scripts/sign-file.c + $(call msg,SIGN-FILE,,$@) + $(Q)$(CC) $(shell $(HOSTPKG_CONFIG)--cflags libcrypto 2> /dev/null) \ + $< -o $@ \ + $(shell $(HOSTPKG_CONFIG) --libs libcrypto 2> /dev/null || echo -lcrypto) + $(OUTPUT)/bpf_testmod.ko: $(VMLINUX_BTF) $(wildcard bpf_testmod/Makefile bpf_testmod/*.[ch]) $(call msg,MOD,,$@) $(Q)$(RM) bpf_testmod/bpf_testmod.ko # force re-compilation @@ -512,7 +519,8 @@ TRUNNER_EXTRA_SOURCES := test_progs.c cgroup_helpers.c trace_helpers.c \ TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko \ $(OUTPUT)/liburandom_read.so \ $(OUTPUT)/xdp_synproxy \ - ima_setup.sh \ + $(OUTPUT)/sign-file \ + ima_setup.sh verify_sig_setup.sh \ $(wildcard progs/btf_dump_test_case_*.c) TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS) -DENABLE_ATOMICS_TESTS diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config index c05904d631ec..76b65acd897e 100644 --- a/tools/testing/selftests/bpf/config +++ b/tools/testing/selftests/bpf/config @@ -63,3 +63,5 @@ CONFIG_NETFILTER_XT_MATCH_STATE=y CONFIG_IP_NF_FILTER=y CONFIG_IP_NF_TARGET_SYNPROXY=y CONFIG_IP_NF_RAW=y +CONFIG_MODULE_SIG=y +CONFIG_KEYS=y diff --git a/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c new file mode 100644 index 000000000000..bdf5b48e7837 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c @@ -0,0 +1,341 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH + * + * Author: Roberto Sassu roberto.sassu@huawei.com + */ + +#include <stdio.h> +#include <errno.h> +#include <stdlib.h> +#include <unistd.h> +#include <endian.h> +#include <limits.h> +#include <sys/stat.h> +#include <sys/wait.h> +#include <sys/mman.h> +#include <linux/keyctl.h> +#include <test_progs.h> + +#include "test_verify_pkcs7_sig.skel.h" + +#define MAX_DATA_SIZE (1024 * 1024) +#define MAX_SIG_SIZE 1024 +#define LOG_BUF_SIZE 16384 + +/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */ +#define MODULE_SIG_STRING "~Module signature appended~\n" + +/* + * Module signature information block. + * + * The constituents of the signature section are, in order: + * + * - Signer's name + * - Key identifier + * - Signature data + * - Information block + */ +struct module_signature { + u8 algo; /* Public-key crypto algorithm [0] */ + u8 hash; /* Digest algorithm [0] */ + u8 id_type; /* Key identifier type [PKEY_ID_PKCS7] */ + u8 signer_len; /* Length of signer's name [0] */ + u8 key_id_len; /* Length of key identifier [0] */ + u8 __pad[3]; + __be32 sig_len; /* Length of signature data */ +}; + +struct data { + u8 data[MAX_DATA_SIZE]; + u32 data_len; + u8 sig[MAX_SIG_SIZE]; + u32 sig_len; +}; + +static int _run_setup_process(const char *setup_dir, const char *cmd) +{ + int child_pid, child_status; + + child_pid = fork(); + if (child_pid == 0) { + execlp("./verify_sig_setup.sh", "./verify_sig_setup.sh", cmd, + setup_dir, NULL); + exit(errno); + + } else if (child_pid > 0) { + waitpid(child_pid, &child_status, 0); + return WEXITSTATUS(child_status); + } + + return -EINVAL; +} + +static int populate_data_item_str(const char *tmp_dir, struct data *data_item) +{ + struct stat st; + char data_template[] = "/tmp/dataXXXXXX"; + char path[PATH_MAX]; + int ret, fd, child_status, child_pid; + + data_item->data_len = 4; + memcpy(data_item->data, "test", data_item->data_len); + + fd = mkstemp(data_template); + if (fd == -1) + return -errno; + + ret = write(fd, data_item->data, data_item->data_len); + + close(fd); + + if (ret != data_item->data_len) { + ret = -EIO; + goto out; + } + + child_pid = fork(); + + if (child_pid == -1) { + ret = -errno; + goto out; + } + + if (child_pid == 0) { + snprintf(path, sizeof(path), "%s/signing_key.pem", tmp_dir); + + return execlp("./sign-file", "./sign-file", "-d", "sha256", + path, path, data_template, NULL); + } + + waitpid(child_pid, &child_status, 0); + + ret = WEXITSTATUS(child_status); + if (ret) + goto out; + + snprintf(path, sizeof(path), "%s.p7s", data_template); + + ret = stat(path, &st); + if (ret == -1) { + ret = -errno; + goto out; + } + + if (st.st_size > sizeof(data_item->sig)) { + ret = -EINVAL; + goto out_sig; + } + + data_item->sig_len = st.st_size; + + fd = open(path, O_RDONLY); + if (fd == -1) { + ret = -errno; + goto out_sig; + } + + ret = read(fd, data_item->sig, data_item->sig_len); + + close(fd); + + if (ret != data_item->sig_len) { + ret = -EIO; + goto out_sig; + } + + ret = 0; +out_sig: + unlink(path); +out: + unlink(data_template); + return ret; +} + +static int populate_data_item_mod(struct data *data_item) +{ + char mod_path[PATH_MAX], *mod_path_ptr; + struct stat st; + void *mod; + FILE *fp; + struct module_signature ms; + int ret, fd, modlen, marker_len, sig_len; + + data_item->data_len = 0; + + if (stat("/lib/modules", &st) == -1) + return 0; + + /* Requires CONFIG_TCP_CONG_BIC=m. */ + fp = popen("find /lib/modules/$(uname -r) -name tcp_bic.ko", "r"); + if (!fp) + return 0; + + mod_path_ptr = fgets(mod_path, sizeof(mod_path), fp); + pclose(fp); + + if (!mod_path_ptr) + return 0; + + mod_path_ptr = strchr(mod_path, '\n'); + if (!mod_path_ptr) + return 0; + + *mod_path_ptr = '\0'; + + if (stat(mod_path, &st) == -1) + return 0; + + modlen = st.st_size; + marker_len = sizeof(MODULE_SIG_STRING) - 1; + + fd = open(mod_path, O_RDONLY); + if (fd == -1) + return -errno; + + mod = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0); + + close(fd); + + if (mod == MAP_FAILED) + return -errno; + + if (strncmp(mod + modlen - marker_len, MODULE_SIG_STRING, marker_len)) { + ret = -EINVAL; + goto out; + } + + modlen -= marker_len; + + memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms)); + + sig_len = __be32_to_cpu(ms.sig_len); + modlen -= sig_len + sizeof(ms); + + if (modlen > sizeof(data_item->data)) { + ret = -E2BIG; + goto out; + } + + memcpy(data_item->data, mod, modlen); + data_item->data_len = modlen; + + if (sig_len > sizeof(data_item->sig)) { + ret = -E2BIG; + goto out; + } + + memcpy(data_item->sig, mod + modlen, sig_len); + data_item->sig_len = sig_len; + ret = 0; +out: + munmap(mod, st.st_size); + return ret; +} + +void test_verify_pkcs7_sig(void) +{ + char tmp_dir_template[] = "/tmp/verify_sigXXXXXX"; + char *tmp_dir; + char *buf = NULL; + struct test_verify_pkcs7_sig *skel = NULL; + struct bpf_map *map; + struct data data; + int ret, zero = 0; + + LIBBPF_OPTS(bpf_object_open_opts, opts); + + /* Trigger creation of session keyring. */ + syscall(__NR_request_key, "keyring", "_uid.0", NULL, + KEY_SPEC_SESSION_KEYRING); + + tmp_dir = mkdtemp(tmp_dir_template); + if (!ASSERT_OK_PTR(tmp_dir, "mkdtemp")) + return; + + ret = _run_setup_process(tmp_dir, "setup"); + if (!ASSERT_OK(ret, "_run_setup_process")) + goto close_prog; + + buf = malloc(LOG_BUF_SIZE); + if (!ASSERT_OK_PTR(buf, "malloc")) + goto close_prog; + + opts.kernel_log_buf = buf; + opts.kernel_log_size = LOG_BUF_SIZE; + opts.kernel_log_level = 1; + + skel = test_verify_pkcs7_sig__open_opts(&opts); + if (!ASSERT_OK_PTR(skel, "test_verify_pkcs7_sig__open_opts")) + goto close_prog; + + ret = test_verify_pkcs7_sig__load(skel); + + if (ret < 0 && strstr(buf, "unknown func bpf_verify_pkcs7_signature")) { + printf( + "%s:SKIP:bpf_verify_pkcs7_signature() helper not supported\n", + __func__); + test__skip(); + goto close_prog; + } + + if (!ASSERT_OK(ret, "test_verify_pkcs7_sig__load\n")) + goto close_prog; + + ret = test_verify_pkcs7_sig__attach(skel); + if (!ASSERT_OK(ret, "test_verify_pkcs7_sig__attach\n")) + goto close_prog; + + map = bpf_object__find_map_by_name(skel->obj, "data_input"); + if (!ASSERT_OK_PTR(map, "data_input not found")) + goto close_prog; + + ret = populate_data_item_str(tmp_dir, &data); + if (!ASSERT_OK(ret, "populate_data_item_str\n")) + goto close_prog; + + skel->bss->monitored_pid = getpid(); + skel->bss->keyring_serial = KEY_SPEC_SESSION_KEYRING; + skel->bss->keyring_id = -1; + + ret = bpf_map_update_elem(bpf_map__fd(map), &zero, &data, BPF_ANY); + if (!ASSERT_OK(ret, "bpf_map_update_elem\n")) + goto close_prog; + + /* Search the verification key in the testing keyring. */ + skel->bss->keyring_serial = syscall(__NR_request_key, "keyring", + "ebpf_testing_keyring", NULL, + KEY_SPEC_SESSION_KEYRING); + + ret = bpf_map_update_elem(bpf_map__fd(map), &zero, &data, BPF_ANY); + if (!ASSERT_OK(ret, "bpf_map_update_elem\n")) + goto close_prog; + + /* Corrupt data (signature verification should fail). */ + data.data[0] = 'a'; + ret = bpf_map_update_elem(bpf_map__fd(map), &zero, &data, BPF_ANY); + if (!ASSERT_LT(ret, 0, "bpf_map_update_elem data_input\n")) + goto close_prog; + + ret = populate_data_item_mod(&data); + if (!ASSERT_OK(ret, "populate_data_item_mod\n")) + goto close_prog; + + if (data.data_len) { + skel->bss->keyring_id = 1; + ret = bpf_map_update_elem(bpf_map__fd(map), &zero, &data, + BPF_ANY); + ASSERT_OK(ret, "bpf_map_update_elem\n"); + } + +close_prog: + _run_setup_process(tmp_dir, "cleanup"); + free(buf); + + if (!skel) + return; + + skel->bss->monitored_pid = 0; + test_verify_pkcs7_sig__destroy(skel); +} diff --git a/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c new file mode 100644 index 000000000000..5d99fcbcb26d --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c @@ -0,0 +1,90 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH + * + * Author: Roberto Sassu roberto.sassu@huawei.com + */ + +#include <errno.h> +#include <stdlib.h> +#include <limits.h> +#include <linux/bpf.h> +#include <bpf/bpf_helpers.h> +#include <bpf/bpf_tracing.h> + +#define MAX_DATA_SIZE (1024 * 1024) +#define MAX_SIG_SIZE 1024 + +typedef __u8 u8; +typedef __u16 u16; +typedef __u32 u32; +typedef __u64 u64; + +u32 monitored_pid; +u32 keyring_serial; +int keyring_id; + +struct data { + u8 data[MAX_DATA_SIZE]; + u32 data_len; + u8 sig[MAX_SIG_SIZE]; + u32 sig_len; +}; + +struct { + __uint(type, BPF_MAP_TYPE_ARRAY); + __uint(max_entries, 1); + __type(key, __u32); + __type(value, struct data); +} data_input SEC(".maps"); + +char _license[] SEC("license") = "GPL"; + +SEC("lsm.s/bpf") +int BPF_PROG(bpf, int cmd, union bpf_attr *attr, unsigned int size) +{ + struct bpf_dynptr data_ptr, sig_ptr; + struct data *data_val; + struct key *trusted_keys = NULL; + u32 sig_len; + u32 pid; + u64 value; + int ret, zero = 0, lookup = 0; + + pid = bpf_get_current_pid_tgid() >> 32; + if (pid != monitored_pid) + return 0; + + data_val = bpf_map_lookup_elem(&data_input, &zero); + if (!data_val) + return 0; + + bpf_probe_read(&value, sizeof(value), &attr->value); + + bpf_copy_from_user(data_val, sizeof(struct data), + (void *)(unsigned long)value); + + if (data_val->data_len > sizeof(data_val->data)) + return -EINVAL; + + bpf_dynptr_from_mem(data_val->data, data_val->data_len, 0, &data_ptr); + + if (data_val->sig_len > sizeof(data_val->sig)) + return -EINVAL; + + bpf_dynptr_from_mem(data_val->sig, data_val->sig_len, 0, &sig_ptr); + + if (keyring_id == -1) { + trusted_keys = bpf_lookup_user_key(keyring_serial, 0); + lookup = 1; + } + + ret = bpf_verify_pkcs7_signature(&data_ptr, &sig_ptr, trusted_keys, + keyring_id); + + if (lookup && trusted_keys) + bpf_key_put(trusted_keys); + + return ret; +} diff --git a/tools/testing/selftests/bpf/verify_sig_setup.sh b/tools/testing/selftests/bpf/verify_sig_setup.sh new file mode 100755 index 000000000000..ba08922b4a27 --- /dev/null +++ b/tools/testing/selftests/bpf/verify_sig_setup.sh @@ -0,0 +1,104 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 + +set -e +set -u +set -o pipefail + +VERBOSE="${SELFTESTS_VERBOSE:=0}" +LOG_FILE="$(mktemp /tmp/verify_sig_setup.log.XXXXXX)" + +x509_genkey_content="\ +[ req ] +default_bits = 2048 +distinguished_name = req_distinguished_name +prompt = no +string_mask = utf8only +x509_extensions = myexts + +[ req_distinguished_name ] +CN = eBPF Signature Verification Testing Key + +[ myexts ] +basicConstraints=critical,CA:FALSE +keyUsage=digitalSignature +subjectKeyIdentifier=hash +authorityKeyIdentifier=keyid +" + +usage() +{ + echo "Usage: $0 <setup|cleanup <existing_tmp_dir>" + exit 1 +} + +setup() +{ + local tmp_dir="$1" + + echo "${x509_genkey_content}" > ${tmp_dir}/x509.genkey + + openssl req -new -nodes -utf8 -sha256 -days 36500 \ + -batch -x509 -config ${tmp_dir}/x509.genkey \ + -outform PEM -out ${tmp_dir}/signing_key.pem \ + -keyout ${tmp_dir}/signing_key.pem 2>&1 + + openssl x509 -in ${tmp_dir}/signing_key.pem -out \ + ${tmp_dir}/signing_key.der -outform der + + key_id=$(cat ${tmp_dir}/signing_key.der | keyctl padd asymmetric ebpf_testing_key @s) + + keyring_id=$(keyctl newring ebpf_testing_keyring @s) + keyctl link $key_id $keyring_id +} + +cleanup() { + local tmp_dir="$1" + + keyctl unlink $(keyctl search @s asymmetric ebpf_testing_key) @s + keyctl unlink $(keyctl search @s keyring ebpf_testing_keyring) @s + rm -rf ${tmp_dir} +} + +catch() +{ + local exit_code="$1" + local log_file="$2" + + if [[ "${exit_code}" -ne 0 ]]; then + cat "${log_file}" >&3 + fi + + rm -f "${log_file}" + exit ${exit_code} +} + +main() +{ + [[ $# -ne 2 ]] && usage + + local action="$1" + local tmp_dir="$2" + + [[ ! -d "${tmp_dir}" ]] && echo "Directory ${tmp_dir} doesn't exist" && exit 1 + + if [[ "${action}" == "setup" ]]; then + setup "${tmp_dir}" + elif [[ "${action}" == "cleanup" ]]; then + cleanup "${tmp_dir}" + else + echo "Unknown action: ${action}" + exit 1 + fi +} + +trap 'catch "$?" "${LOG_FILE}"' EXIT + +if [[ "${VERBOSE}" -eq 0 ]]; then + # Save the stderr to 3 so that we can output back to + # it incase of an error. + exec 3>&2 1>"${LOG_FILE}" 2>&1 +fi + +main "$@" +rm -f "${LOG_FILE}"
On Tue, Jun 21, 2022 at 06:37:57PM +0200, Roberto Sassu wrote:
- if (child_pid == 0) {
snprintf(path, sizeof(path), "%s/signing_key.pem", tmp_dir);return execlp("./sign-file", "./sign-file", "-d", "sha256",path, path, data_template, NULL);
Did you miss my earlier reply requesting not to do this module_signature append and use signature directly?
From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] Sent: Wednesday, June 22, 2022 12:32 AM On Tue, Jun 21, 2022 at 06:37:57PM +0200, Roberto Sassu wrote:
- if (child_pid == 0) {
snprintf(path, sizeof(path), "%s/signing_key.pem", tmp_dir);return execlp("./sign-file", "./sign-file", "-d", "sha256",path, path, data_template, NULL);Did you miss my earlier reply requesting not to do this module_signature append and use signature directly?
I didn't miss. sign-file is producing the raw PKCS#7 signature here (-d).
I'm doing something slightly different, to test the keyring ID part. I'm retrieving an existing kernel module (actually this does not work in the CI), parsing it to extract the raw signature, and passing it to the eBPF program for verification.
Since the kernel module is signed with a key in the built-in keyring, passing 1 or 0 as ID should work.
Roberto
(sorry, I have to keep the email signature by German law)
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Yang Xi, Li He
On Wed, Jun 22, 2022 at 12:06 AM Roberto Sassu roberto.sassu@huawei.com wrote:
From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] Sent: Wednesday, June 22, 2022 12:32 AM On Tue, Jun 21, 2022 at 06:37:57PM +0200, Roberto Sassu wrote:
- if (child_pid == 0) {
snprintf(path, sizeof(path), "%s/signing_key.pem", tmp_dir);return execlp("./sign-file", "./sign-file", "-d", "sha256",path, path, data_template, NULL);Did you miss my earlier reply requesting not to do this module_signature append and use signature directly?
I didn't miss. sign-file is producing the raw PKCS#7 signature here (-d).
I'm doing something slightly different, to test the keyring ID part. I'm retrieving an existing kernel module (actually this does not work in the CI), parsing it to extract the raw signature, and passing it to the eBPF program for verification.
We don't have signed modules in CI. When you make changes like this you have to explain that in the commit log.
Since the kernel module is signed with a key in the built-in keyring, passing 1 or 0 as ID should work.
Roberto
(sorry, I have to keep the email signature by German law)
I don't believe that's the case since plenty of people work from Germany and regularly contribute patches without such banners.
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Yang Xi, Li He
linux-kselftest-mirror@lists.linaro.org
-
Alexei Starovoitov -
John Fastabend -
Roberto Sassu