[PATCH bpf-next 0/3] bpf: add signature

[PATCH bpf-next 0/3] bpf: add signature

[Matteo Croce]

From: Matteo Croce <mcroce@microsoft.com>

This series add signature verification for BPF files.
The first patch implements the signature validation in the kernel,
the second patch optionally makes the signature mandatory,
the third adds signature generation to bpftool.

This only works with CO-RE programs.

Matteo Croce (3):
  bpf: add signature to eBPF instructions
  bpf: add option to require BPF signature
  bpftool: add signature in skeleton

 crypto/asymmetric_keys/asymmetric_type.c |   1 +
 crypto/asymmetric_keys/pkcs7_verify.c    |   7 +-
 include/linux/verification.h             |   1 +
 include/uapi/linux/bpf.h                 |   2 +
 kernel/bpf/Kconfig                       |  14 ++
 kernel/bpf/syscall.c                     |  51 +++++-
 tools/bpf/bpftool/Makefile               |  14 +-
 tools/bpf/bpftool/gen.c                  |  33 ++++
 tools/bpf/bpftool/main.c                 |  28 +++
 tools/bpf/bpftool/main.h                 |   7 +
 tools/bpf/bpftool/sign.c                 | 218 +++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h           |   2 +
 tools/lib/bpf/skel_internal.h            |   4 +
 13 files changed, 372 insertions(+), 10 deletions(-)
 create mode 100644 tools/bpf/bpftool/sign.c

-- 
2.33.1

[PATCH bpf-next 1/3] bpf: add signature to eBPF instructions

[Matteo Croce]

From: Matteo Croce <mcroce@microsoft.com>

When loading a BPF program, pass a signature which is used to validate
the instructions.
The signature type is the same used to validate the kernel modules.

This happens when loading a program with, respectively, an invalid and
a valid signature:

    # ./core-bad
    [ 8524.417567] Invalid BPF signature for '__loader.prog': -EKEYREJECTED
    failed to open and/or load BPF object
    # ./core-ok

Signed-off-by: Matteo Croce <mcroce@microsoft.com>
---
 crypto/asymmetric_keys/asymmetric_type.c |  1 +
 crypto/asymmetric_keys/pkcs7_verify.c    |  7 +++-
 include/linux/verification.h             |  1 +
 include/uapi/linux/bpf.h                 |  2 +
 kernel/bpf/Kconfig                       |  8 ++++
 kernel/bpf/syscall.c                     | 47 +++++++++++++++++++++---
 6 files changed, 59 insertions(+), 7 deletions(-)

diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
index ad8af3d70ac0..e4f2fee19c5f 100644
--- a/crypto/asymmetric_keys/asymmetric_type.c
+++ b/crypto/asymmetric_keys/asymmetric_type.c
@@ -26,6 +26,7 @@ const char *const key_being_used_for[NR__KEY_BEING_USED_FOR] = {
 	[VERIFYING_KEY_SIGNATURE]		= "key sig",
 	[VERIFYING_KEY_SELF_SIGNATURE]		= "key self sig",
 	[VERIFYING_UNSPECIFIED_SIGNATURE]	= "unspec sig",
+	[VERIFYING_BPF_SIGNATURE]		= "bpf sig",
 };
 EXPORT_SYMBOL_GPL(key_being_used_for);
 
diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index 0b4d07aa8811..ab645f23c021 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -411,12 +411,15 @@ int pkcs7_verify(struct pkcs7_message *pkcs7,
 
 	switch (usage) {
 	case VERIFYING_MODULE_SIGNATURE:
+	case VERIFYING_BPF_SIGNATURE:
 		if (pkcs7->data_type != OID_data) {
-			pr_warn("Invalid module sig (not pkcs7-data)\n");
+			pr_warn("Invalid %s (not pkcs7-data)\n",
+				key_being_used_for[usage]);
 			return -EKEYREJECTED;
 		}
 		if (pkcs7->have_authattrs) {
-			pr_warn("Invalid module sig (has authattrs)\n");
+			pr_warn("Invalid %s (has authattrs)\n",
+				key_being_used_for[usage]);
 			return -EKEYREJECTED;
 		}
 		break;
diff --git a/include/linux/verification.h b/include/linux/verification.h
index a655923335ae..71482644eea0 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -27,6 +27,7 @@ enum key_being_used_for {
 	VERIFYING_KEY_SIGNATURE,
 	VERIFYING_KEY_SELF_SIGNATURE,
 	VERIFYING_UNSPECIFIED_SIGNATURE,
+	VERIFYING_BPF_SIGNATURE,
 	NR__KEY_BEING_USED_FOR
 };
 extern const char *const key_being_used_for[NR__KEY_BEING_USED_FOR];
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index c26871263f1f..bbb4435c7586 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1346,6 +1346,8 @@ union bpf_attr {
 		__aligned_u64	fd_array;	/* array of FDs */
 		__aligned_u64	core_relos;
 		__u32		core_relo_rec_size; /* sizeof(struct bpf_core_relo) */
+		__aligned_u64	signature;	/* instruction's signature */
+		__u32		sig_len;	/* signature size */
 	};
 
 	struct { /* anonymous struct used by BPF_OBJ_* commands */
diff --git a/kernel/bpf/Kconfig b/kernel/bpf/Kconfig
index d24d518ddd63..735979bb8672 100644
--- a/kernel/bpf/Kconfig
+++ b/kernel/bpf/Kconfig
@@ -79,6 +79,14 @@ config BPF_UNPRIV_DEFAULT_OFF
 
 	  If you are unsure how to answer this question, answer Y.
 
+config BPF_SIG
+	bool "BPF signature verification"
+	select SYSTEM_DATA_VERIFICATION
+	depends on BPF_SYSCALL
+	help
+	  Check BPF programs for valid signatures upon load: the signature
+	  is passed via the bpf() syscall together with the instructions.
+
 source "kernel/bpf/preload/Kconfig"
 
 config BPF_LSM
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index b3ada4085f85..5aaa74a72b46 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -32,6 +32,10 @@
 #include <linux/rcupdate_trace.h>
 #include <linux/memcontrol.h>
 
+#ifdef CONFIG_BPF_SIG
+#include <linux/verification.h>
+#endif
+
 #define IS_FD_ARRAY(map) ((map)->map_type == BPF_MAP_TYPE_PERF_EVENT_ARRAY || \
 			  (map)->map_type == BPF_MAP_TYPE_CGROUP_ARRAY || \
 			  (map)->map_type == BPF_MAP_TYPE_ARRAY_OF_MAPS)
@@ -2184,7 +2188,7 @@ static bool is_perfmon_prog_type(enum bpf_prog_type prog_type)
 }
 
 /* last field in 'union bpf_attr' used by this command */
-#define	BPF_PROG_LOAD_LAST_FIELD core_relo_rec_size
+#define	BPF_PROG_LOAD_LAST_FIELD sig_len
 
 static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr)
 {
@@ -2302,6 +2306,43 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr)
 			     bpf_prog_insn_size(prog)) != 0)
 		goto free_prog_sec;
 
+	err = bpf_obj_name_cpy(prog->aux->name, attr->prog_name,
+			       sizeof(attr->prog_name));
+	if (err < 0)
+		goto free_prog_sec;
+
+#ifdef CONFIG_BPF_SIG
+	if (attr->sig_len) {
+		char *signature;
+
+		signature = kmalloc(attr->sig_len, GFP_USER);
+		if (!signature) {
+			err = -ENOMEM;
+			goto free_prog_sec;
+		}
+
+		if (copy_from_user(signature, (char *)attr->signature, attr->sig_len)) {
+			err = -EFAULT;
+			kfree(signature);
+			goto free_prog_sec;
+		}
+
+		err = verify_pkcs7_signature(prog->insns,
+					     prog->len * sizeof(struct bpf_insn),
+					     signature, attr->sig_len,
+					     VERIFY_USE_SECONDARY_KEYRING,
+					     VERIFYING_BPF_SIGNATURE,
+					     NULL, NULL);
+		kfree(signature);
+
+		if (err) {
+			pr_warn("Invalid BPF signature for '%s': %pe\n",
+				prog->aux->name, ERR_PTR(err));
+			goto free_prog_sec;
+		}
+	}
+#endif
+
 	prog->orig_prog = NULL;
 	prog->jited = 0;
 
@@ -2320,10 +2361,6 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr)
 		goto free_prog_sec;
 
 	prog->aux->load_time = ktime_get_boottime_ns();
-	err = bpf_obj_name_cpy(prog->aux->name, attr->prog_name,
-			       sizeof(attr->prog_name));
-	if (err < 0)
-		goto free_prog_sec;
 
 	/* run eBPF verifier */
 	err = bpf_check(&prog, attr, uattr);
-- 
2.33.1

[PATCH bpf-next 2/3] bpf: add option to require BPF signature

[Matteo Croce]

From: Matteo Croce <mcroce@microsoft.com>

Add a compile time option which makes the BPF signature mandatory,
i.e. all programs without signature or with an invalid one are rejected.

CO-RE programs load a program of type BPF_PROG_TYPE_SYSCALL, which then
uses the bpf() syscall to load the final program. This one won't have any
signature, so never enforce signature for programs coming from the kernel.

This happens when loading a program with a missing signature:

    # ip link set lo xdp object xdp.o
    [ 8677.652546] Rejecting BPF '' with no signature

Signed-off-by: Matteo Croce <mcroce@microsoft.com>
---
 kernel/bpf/Kconfig   | 6 ++++++
 kernel/bpf/syscall.c | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/kernel/bpf/Kconfig b/kernel/bpf/Kconfig
index 735979bb8672..fe6e84abe84c 100644
--- a/kernel/bpf/Kconfig
+++ b/kernel/bpf/Kconfig
@@ -87,6 +87,12 @@ config BPF_SIG
 	  Check BPF programs for valid signatures upon load: the signature
 	  is passed via the bpf() syscall together with the instructions.
 
+config BPF_SIG_FORCE
+	bool "Require BPF to be validly signed"
+	depends on BPF_SIG
+	help
+	  Reject unsigned BPF or signed BPF for which we don't have a key.
+
 source "kernel/bpf/preload/Kconfig"
 
 config BPF_LSM
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 5aaa74a72b46..9e36614719fd 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2340,6 +2340,10 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr)
 				prog->aux->name, ERR_PTR(err));
 			goto free_prog_sec;
 		}
+	} else if (IS_ENABLED(CONFIG_BPF_SIG_FORCE) && !uattr.is_kernel) {
+		pr_warn("Rejecting BPF '%s' with no signature\n", prog->aux->name);
+		err = -EKEYREJECTED;
+		goto free_prog_sec;
 	}
 #endif
 
-- 
2.33.1

[PATCH bpf-next 3/3] bpftool: add signature in skeleton

[Matteo Croce]

From: Matteo Croce <mcroce@microsoft.com>

When generating the skeleton, allow to add a signature.
The signature will be passed to the kernel in the newly added field.
As in sign-file, allow specifying "pkcs11:..." as key file, to use the
openssl engine.
Still as in sign-file, read the environment variable KBUILD_SIGN_PIN.

Signed-off-by: Matteo Croce <mcroce@microsoft.com>
---
 tools/bpf/bpftool/Makefile     |  14 ++-
 tools/bpf/bpftool/gen.c        |  33 +++++
 tools/bpf/bpftool/main.c       |  28 +++++
 tools/bpf/bpftool/main.h       |   7 ++
 tools/bpf/bpftool/sign.c       | 218 +++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h |   2 +
 tools/lib/bpf/skel_internal.h  |   4 +
 7 files changed, 303 insertions(+), 3 deletions(-)
 create mode 100644 tools/bpf/bpftool/sign.c

diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index 42eb8eee3d89..d2645c2f4bc9 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -96,9 +96,9 @@ RM ?= rm -f
 
 FEATURE_USER = .bpftool
 FEATURE_TESTS = libbfd disassembler-four-args reallocarray zlib libcap \
-	clang-bpf-co-re
+	clang-bpf-co-re libcrypto
 FEATURE_DISPLAY = libbfd disassembler-four-args zlib libcap \
-	clang-bpf-co-re
+	clang-bpf-co-re libcrypto
 
 check_feat := 1
 NON_CHECK_FEAT_TARGETS := clean uninstall doc doc-clean doc-install doc-uninstall
@@ -131,6 +131,11 @@ CFLAGS += -DUSE_LIBCAP
 LIBS += -lcap
 endif
 
+ifeq ($(feature-libcrypto), 1)
+CFLAGS_CRYPTO := -DUSE_SIGN
+LIBS += -lcrypto
+endif
+
 include $(wildcard $(OUTPUT)*.d)
 
 all: $(OUTPUT)bpftool
@@ -138,6 +143,9 @@ all: $(OUTPUT)bpftool
 BFD_SRCS = jit_disasm.c
 
 SRCS = $(filter-out $(BFD_SRCS),$(wildcard *.c))
+ifneq ($(feature-libcrypto), 1)
+SRCS := $(filter-out sign.c,$(SRCS))
+endif
 
 ifeq ($(feature-libbfd),1)
   LIBS += -lbfd -ldl -lopcodes
@@ -224,7 +232,7 @@ $(BOOTSTRAP_OUTPUT)%.o: %.c $(LIBBPF_BOOTSTRAP_INTERNAL_HDRS) | $(BOOTSTRAP_OUTP
 		-c -MMD $< -o $@
 
 $(OUTPUT)%.o: %.c
-	$(QUIET_CC)$(CC) $(CFLAGS) -c -MMD $< -o $@
+	$(QUIET_CC)$(CC) $(CFLAGS) $(CFLAGS_CRYPTO) -c -MMD $< -o $@
 
 feature-detect-clean:
 	$(call QUIET_CLEAN, feature-detect)
diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index 997a2865e04a..c9f09b222986 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -491,6 +491,10 @@ static int gen_trace(struct bpf_object *obj, const char *obj_name, const char *h
 	struct bpf_map *map;
 	char ident[256];
 	int err = 0;
+#ifdef USE_SIGN
+	char *signature = NULL;
+	int sig_len = 0;
+#endif
 
 	err = bpf_object__gen_loader(obj, &opts);
 	if (err)
@@ -510,6 +514,19 @@ static int gen_trace(struct bpf_object *obj, const char *obj_name, const char *h
 	 * are populated with the loader program.
 	 */
 
+#ifdef USE_SIGN
+	if (sign_bpf) {
+		sig_len = sign(sign_hash, sign_key, sign_cert,
+			       opts.insns, opts.insns_sz,
+			       (unsigned char **)&signature);
+		if (sig_len <= 0) {
+			p_err("failed to sign instructions");
+			err = -EINVAL;
+			goto out;
+		}
+	}
+#endif
+
 	/* finish generating 'struct skel' */
 	codegen("\
 		\n\
@@ -592,6 +609,18 @@ static int gen_trace(struct bpf_object *obj, const char *obj_name, const char *h
 		",
 		opts.insns_sz);
 	print_hex(opts.insns, opts.insns_sz);
+#ifdef USE_SIGN
+	if (sign_bpf) {
+		codegen("\
+			\n\
+			\";						    \n\
+				opts.sig_sz = %d;			    \n\
+				opts.signature = (void *)\"\\		    \n\
+			",
+			sig_len);
+		print_hex(signature, sig_len);
+	}
+#endif
 	codegen("\
 		\n\
 		\";							    \n\
@@ -1090,6 +1119,10 @@ static int do_help(int argc, char **argv)
 		"       %1$s %2$s help\n"
 		"\n"
 		"       " HELP_SPEC_OPTIONS " |\n"
+#ifdef USE_SIGN
+		"                    {-s|--sign} | {-H|--hash} |\n"
+		"                    {-c|--cert} | {-k|--key} |\n"
+#endif
 		"                    {-L|--use-loader} }\n"
 		"",
 		bin_name, "gen");
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 8b71500e7cb2..cea3d07e98e0 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -31,6 +31,10 @@ bool block_mount;
 bool verifier_logs;
 bool relaxed_maps;
 bool use_loader;
+bool sign_bpf;
+const char *sign_hash;
+const char *sign_cert;
+const char *sign_key;
 bool legacy_libbpf;
 struct btf *base_btf;
 struct hashmap *refs_table;
@@ -403,6 +407,12 @@ int main(int argc, char **argv)
 		{ "use-loader",	no_argument,	NULL,	'L' },
 		{ "base-btf",	required_argument, NULL, 'B' },
 		{ "legacy",	no_argument,	NULL,	'l' },
+#ifdef USE_SIGN
+		{ "sign",	no_argument,	NULL,	's' },
+		{ "hash",	required_argument, NULL, 'H' },
+		{ "cert",	required_argument, NULL, 'c' },
+		{ "key",	required_argument, NULL, 'k' },
+#endif
 		{ 0 }
 	};
 	bool version_requested = false;
@@ -416,7 +426,11 @@ int main(int argc, char **argv)
 	bin_name = argv[0];
 
 	opterr = 0;
+#ifdef USE_SIGN
+	while ((opt = getopt_long(argc, argv, "VhpjfLmndB:lsH:c:k:",
+#else
 	while ((opt = getopt_long(argc, argv, "VhpjfLmndB:l",
+#endif
 				  options, NULL)) >= 0) {
 		switch (opt) {
 		case 'V':
@@ -466,6 +480,20 @@ int main(int argc, char **argv)
 		case 'l':
 			legacy_libbpf = true;
 			break;
+#ifdef USE_SIGN
+		case 's':
+			sign_bpf = true;
+			break;
+		case 'H':
+			sign_hash = optarg;
+			break;
+		case 'c':
+			sign_cert = optarg;
+			break;
+		case 'k':
+			sign_key = optarg;
+			break;
+#endif
 		default:
 			p_err("unrecognized option '%s'", argv[optind - 1]);
 			if (json_output)
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 8d76d937a62b..ef82219d3f52 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -91,6 +91,10 @@ extern bool verifier_logs;
 extern bool relaxed_maps;
 extern bool use_loader;
 extern bool legacy_libbpf;
+extern bool sign_bpf;
+extern const char *sign_hash;
+extern const char *sign_cert;
+extern const char *sign_key;
 extern struct btf *base_btf;
 extern struct hashmap *refs_table;
 
@@ -260,4 +264,7 @@ static inline bool hashmap__empty(struct hashmap *map)
 	return map ? hashmap__size(map) == 0 : true;
 }
 
+int sign(const char *hash_algo, const char *key_path, const char *x509_path,
+	 const char *indata, int indatalen, unsigned char **outdata);
+
 #endif
diff --git a/tools/bpf/bpftool/sign.c b/tools/bpf/bpftool/sign.c
new file mode 100644
index 000000000000..ca09dc5f93aa
--- /dev/null
+++ b/tools/bpf/bpftool/sign.c
@@ -0,0 +1,218 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+/* Sign a module file using the given key and certificate.
+ *
+ * Inspired by Linux scripts/sign-file.c
+ * Copyright (C) 2021 Matteo Croce <mcroce@microsoft.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 2.1
+ * of the licence, or (at your option) any later version.
+ */
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdbool.h>
+#include <string.h>
+#include <err.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include <openssl/opensslv.h>
+#include <openssl/bio.h>
+#include <openssl/evp.h>
+#include <openssl/pem.h>
+#include <openssl/err.h>
+#include <openssl/engine.h>
+#include <openssl/cms.h>
+
+#include "main.h"
+
+static const char *key_pass;
+
+static int pem_pw_cb(char *buf, int len, int w, void *v)
+{
+	int pwlen;
+
+	if (!key_pass)
+		return -1;
+
+	pwlen = strlen(key_pass);
+	if (pwlen >= len)
+		return -1;
+
+	strcpy(buf, key_pass);
+
+	/* If it's wrong, don't keep trying it. */
+	key_pass = NULL;
+
+	return pwlen;
+}
+
+static void display_openssl_errors(void)
+{
+	const char *file;
+	char buf[120];
+	int e, line;
+
+	if (!ERR_peek_error())
+		return;
+
+	while ((e = ERR_get_error_line(&file, &line))) {
+		ERR_error_string(e, buf);
+		fprintf(stderr, "- SSL %s: %s:%d\n", buf, file, line);
+	}
+}
+
+static EVP_PKEY *read_private_key(const char *key_path)
+{
+	EVP_PKEY *private_key;
+
+	if (!strncmp(key_path, "pkcs11:", 7)) {
+		ENGINE *e;
+
+		ENGINE_load_builtin_engines();
+		display_openssl_errors();
+		e = ENGINE_by_id("pkcs11");
+		if (!e)
+			return NULL;
+
+		if (!ENGINE_init(e)) {
+			display_openssl_errors();
+			return NULL;
+		}
+		display_openssl_errors();
+
+		if (key_pass)
+			if (!ENGINE_ctrl_cmd_string(e, "PIN", key_pass, 0))
+				return NULL;
+		private_key = ENGINE_load_private_key(e, key_path, NULL, NULL);
+	} else {
+		BIO *b;
+
+		b = BIO_new_file(key_path, "rb");
+		if (!b)
+			return NULL;
+		private_key = PEM_read_bio_PrivateKey(b, NULL, pem_pw_cb, NULL);
+		BIO_free(b);
+	}
+
+	return private_key;
+}
+
+static X509 *read_x509(const char *x509_path)
+{
+	unsigned char buf[2];
+	X509 *x509 = NULL;
+	BIO *b;
+	int n;
+
+	b = BIO_new_file(x509_path, "rb");
+	if (!b) {
+		display_openssl_errors();
+		return NULL;
+	}
+
+	/* Look at the first two bytes of the file to determine the encoding */
+	n = BIO_read(b, buf, 2);
+	if (n != 2) {
+		if (BIO_should_retry(b))
+			fprintf(stderr, "%s: Read wanted retry\n", x509_path);
+		if (n >= 0)
+			fprintf(stderr, "%s: Short read\n", x509_path);
+		display_openssl_errors();
+		goto out_free;
+	}
+
+	if (BIO_reset(b)) {
+		display_openssl_errors();
+		goto out_free;
+	}
+
+	if (buf[0] == 0x30 && buf[1] >= 0x81 && buf[1] <= 0x84)
+		/* Assume raw DER encoded X.509 */
+		x509 = d2i_X509_bio(b, NULL);
+	else
+		/* Assume PEM encoded X.509 */
+		x509 = PEM_read_bio_X509(b, NULL, NULL, NULL);
+
+	if (!x509)
+		display_openssl_errors();
+
+out_free:
+	BIO_free(b);
+
+	return x509;
+}
+
+int sign(const char *hash_algo, const char *key_path, const char *x509_path,
+	 const char *indata, int indatalen, unsigned char **outdata)
+{
+	CMS_ContentInfo *cms = NULL;
+	const EVP_MD *digest_algo;
+	EVP_PKEY *private_key;
+	X509 *x509;
+	BIO *bm;
+
+	OpenSSL_add_all_algorithms();
+	ERR_load_crypto_strings();
+	ERR_clear_error();
+
+	key_pass = getenv("KBUILD_SIGN_PIN");
+
+	/* Open the module file */
+	bm = BIO_new_mem_buf(indata, indatalen);
+	if (!bm) {
+		display_openssl_errors();
+		return -1;
+	}
+
+	/* Read the private key and the X.509 cert the PKCS#7 message
+	 * will point to.
+	 */
+	private_key = read_private_key(key_path);
+	if (!private_key)
+		goto out_free;
+
+	x509 = read_x509(x509_path);
+	if (!x509)
+		goto out_free;
+
+	/* Digest the module data. */
+	OpenSSL_add_all_digests();
+	display_openssl_errors();
+
+	digest_algo = EVP_get_digestbyname(hash_algo);
+	if (!digest_algo) {
+		display_openssl_errors();
+		goto out_free;
+	}
+
+	/* Load the signature message from the digest buffer. */
+	cms = CMS_sign(NULL, NULL, NULL, NULL, CMS_NOCERTS | CMS_PARTIAL |
+		       CMS_BINARY | CMS_DETACHED | CMS_STREAM);
+	if (!cms) {
+		display_openssl_errors();
+		goto out_free;
+	}
+
+	if (!CMS_add1_signer(cms, x509, private_key, digest_algo,
+			     CMS_NOCERTS | CMS_BINARY | CMS_NOSMIMECAP |
+			     CMS_NOATTR)) {
+		display_openssl_errors();
+		goto out_free;
+	}
+
+	if (CMS_final(cms, bm, NULL, CMS_NOCERTS | CMS_BINARY) < 0)
+		display_openssl_errors();
+
+out_free:
+	BIO_free(bm);
+
+	if (!cms)
+		return -1;
+
+	return i2d_CMS_ContentInfo(cms, outdata);
+}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index c26871263f1f..bbb4435c7586 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1346,6 +1346,8 @@ union bpf_attr {
 		__aligned_u64	fd_array;	/* array of FDs */
 		__aligned_u64	core_relos;
 		__u32		core_relo_rec_size; /* sizeof(struct bpf_core_relo) */
+		__aligned_u64	signature;	/* instruction's signature */
+		__u32		sig_len;	/* signature size */
 	};
 
 	struct { /* anonymous struct used by BPF_OBJ_* commands */
diff --git a/tools/lib/bpf/skel_internal.h b/tools/lib/bpf/skel_internal.h
index 0b84d8e6b72a..7c987b79568b 100644
--- a/tools/lib/bpf/skel_internal.h
+++ b/tools/lib/bpf/skel_internal.h
@@ -52,8 +52,10 @@ struct bpf_load_and_run_opts {
 	struct bpf_loader_ctx *ctx;
 	const void *data;
 	const void *insns;
+	const void *signature;
 	__u32 data_sz;
 	__u32 insns_sz;
+	__u32 sig_sz;
 	const char *errstr;
 };
 
@@ -93,6 +95,8 @@ static inline int bpf_load_and_run(struct bpf_load_and_run_opts *opts)
 	attr.prog_type = BPF_PROG_TYPE_SYSCALL;
 	attr.insns = (long) opts->insns;
 	attr.insn_cnt = opts->insns_sz / sizeof(struct bpf_insn);
+	attr.signature = (long) opts->signature;
+	attr.sig_len = opts->sig_sz;
 	attr.license = (long) "Dual BSD/GPL";
 	memcpy(attr.prog_name, "__loader.prog", sizeof("__loader.prog"));
 	attr.fd_array = (long) &map_fd;
-- 
2.33.1

Re: [PATCH bpf-next 0/3] bpf: add signature

[Alexei Starovoitov]

On Fri, Dec 3, 2021 at 11:18 AM Matteo Croce <mcroce@linux.microsoft.com> wrote:
>
> From: Matteo Croce <mcroce@microsoft.com>
>
> This series add signature verification for BPF files.
> The first patch implements the signature validation in the kernel,
> the second patch optionally makes the signature mandatory,
> the third adds signature generation to bpftool.

Matteo,

I think I already mentioned that it's no-go as-is.
We've agreed to go with John's suggestion.

Re: [PATCH bpf-next 0/3] bpf: add signature

[Matteo Croce]

On Fri, Dec 3, 2021 at 8:22 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Dec 3, 2021 at 11:18 AM Matteo Croce <mcroce@linux.microsoft.com> wrote:
> >
> > From: Matteo Croce <mcroce@microsoft.com>
> >
> > This series add signature verification for BPF files.
> > The first patch implements the signature validation in the kernel,
> > the second patch optionally makes the signature mandatory,
> > the third adds signature generation to bpftool.
>
> Matteo,
>
> I think I already mentioned that it's no-go as-is.
> We've agreed to go with John's suggestion.

Hi,

my previous attempt was loading a whole ELF file and parsing it in kernel.
In this series I just validate the instructions against a signature,
as with kernel CO-RE libbpf doesn't need to mangle it.

Which suggestion? I think I missed this one..

-- 
per aspera ad upstream

Re: [PATCH bpf-next 0/3] bpf: add signature

[Alexei Starovoitov]

On Fri, Dec 3, 2021 at 11:36 AM Matteo Croce <mcroce@linux.microsoft.com> wrote:
>
> On Fri, Dec 3, 2021 at 8:22 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Fri, Dec 3, 2021 at 11:18 AM Matteo Croce <mcroce@linux.microsoft.com> wrote:
> > >
> > > From: Matteo Croce <mcroce@microsoft.com>
> > >
> > > This series add signature verification for BPF files.
> > > The first patch implements the signature validation in the kernel,
> > > the second patch optionally makes the signature mandatory,
> > > the third adds signature generation to bpftool.
> >
> > Matteo,
> >
> > I think I already mentioned that it's no-go as-is.
> > We've agreed to go with John's suggestion.
>
> Hi,
>
> my previous attempt was loading a whole ELF file and parsing it in kernel.
> In this series I just validate the instructions against a signature,
> as with kernel CO-RE libbpf doesn't need to mangle it.
>
> Which suggestion? I think I missed this one..

This talk and discussion:
https://linuxplumbersconf.org/event/11/contributions/947/

Re: [PATCH bpf-next 0/3] bpf: add signature

[Luca Boccassi]

[-- Attachment #1: Type: text/plain, Size: 1610 bytes --]

On Fri, 2021-12-03 at 11:37 -0800, Alexei Starovoitov wrote:
> On Fri, Dec 3, 2021 at 11:36 AM Matteo Croce
> <mcroce@linux.microsoft.com> wrote:
> > 
> > On Fri, Dec 3, 2021 at 8:22 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > > 
> > > On Fri, Dec 3, 2021 at 11:18 AM Matteo Croce
> > > <mcroce@linux.microsoft.com> wrote:
> > > > 
> > > > From: Matteo Croce <mcroce@microsoft.com>
> > > > 
> > > > This series add signature verification for BPF files.
> > > > The first patch implements the signature validation in the
> > > > kernel,
> > > > the second patch optionally makes the signature mandatory,
> > > > the third adds signature generation to bpftool.
> > > 
> > > Matteo,
> > > 
> > > I think I already mentioned that it's no-go as-is.
> > > We've agreed to go with John's suggestion.
> > 
> > Hi,
> > 
> > my previous attempt was loading a whole ELF file and parsing it in
> > kernel.
> > In this series I just validate the instructions against a
> > signature,
> > as with kernel CO-RE libbpf doesn't need to mangle it.
> > 
> > Which suggestion? I think I missed this one..
> 
> This talk and discussion:
> https://linuxplumbersconf.org/event/11/contributions/947/

Thanks for the link - but for those of us who don't have ~5 hours to
watch a video recording, would you mind sharing a one line summary,
please? Is there an alternative patch series implementing BPF signing
that you can link us so that we can look at it? Just a link or
googlable reference would be more than enough.

Thank you!

-- 
Kind regards,
Luca Boccassi

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH bpf-next 0/3] bpf: add signature

[Alexei Starovoitov]

On Fri, Dec 3, 2021 at 2:06 PM Luca Boccassi <bluca@debian.org> wrote:
>
> On Fri, 2021-12-03 at 11:37 -0800, Alexei Starovoitov wrote:
> > On Fri, Dec 3, 2021 at 11:36 AM Matteo Croce
> > <mcroce@linux.microsoft.com> wrote:
> > >
> > > On Fri, Dec 3, 2021 at 8:22 PM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > On Fri, Dec 3, 2021 at 11:18 AM Matteo Croce
> > > > <mcroce@linux.microsoft.com> wrote:
> > > > >
> > > > > From: Matteo Croce <mcroce@microsoft.com>
> > > > >
> > > > > This series add signature verification for BPF files.
> > > > > The first patch implements the signature validation in the
> > > > > kernel,
> > > > > the second patch optionally makes the signature mandatory,
> > > > > the third adds signature generation to bpftool.
> > > >
> > > > Matteo,
> > > >
> > > > I think I already mentioned that it's no-go as-is.
> > > > We've agreed to go with John's suggestion.
> > >
> > > Hi,
> > >
> > > my previous attempt was loading a whole ELF file and parsing it in
> > > kernel.
> > > In this series I just validate the instructions against a
> > > signature,
> > > as with kernel CO-RE libbpf doesn't need to mangle it.
> > >
> > > Which suggestion? I think I missed this one..
> >
> > This talk and discussion:
> > https://linuxplumbersconf.org/event/11/contributions/947/
>
> Thanks for the link - but for those of us who don't have ~5 hours to
> watch a video recording, would you mind sharing a one line summary,
> please? Is there an alternative patch series implementing BPF signing
> that you can link us so that we can look at it? Just a link or
> googlable reference would be more than enough.

It's not 5 hours and you have to read slides and watch
John's presentation to follow the conversation.

Re: [PATCH bpf-next 0/3] bpf: add signature

[Matteo Croce]

On Fri, Dec 3, 2021 at 11:20 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Dec 3, 2021 at 2:06 PM Luca Boccassi <bluca@debian.org> wrote:
> >
> > On Fri, 2021-12-03 at 11:37 -0800, Alexei Starovoitov wrote:
> > > On Fri, Dec 3, 2021 at 11:36 AM Matteo Croce
> > > <mcroce@linux.microsoft.com> wrote:
> > > >
> > > > On Fri, Dec 3, 2021 at 8:22 PM Alexei Starovoitov
> > > > <alexei.starovoitov@gmail.com> wrote:
> > > > >
> > > > > On Fri, Dec 3, 2021 at 11:18 AM Matteo Croce
> > > > > <mcroce@linux.microsoft.com> wrote:
> > > > > >
> > > > > > From: Matteo Croce <mcroce@microsoft.com>
> > > > > >
> > > > > > This series add signature verification for BPF files.
> > > > > > The first patch implements the signature validation in the
> > > > > > kernel,
> > > > > > the second patch optionally makes the signature mandatory,
> > > > > > the third adds signature generation to bpftool.
> > > > >
> > > > > Matteo,
> > > > >
> > > > > I think I already mentioned that it's no-go as-is.
> > > > > We've agreed to go with John's suggestion.
> > > >
> > > > Hi,
> > > >
> > > > my previous attempt was loading a whole ELF file and parsing it in
> > > > kernel.
> > > > In this series I just validate the instructions against a
> > > > signature,
> > > > as with kernel CO-RE libbpf doesn't need to mangle it.
> > > >
> > > > Which suggestion? I think I missed this one..
> > >
> > > This talk and discussion:
> > > https://linuxplumbersconf.org/event/11/contributions/947/
> >
> > Thanks for the link - but for those of us who don't have ~5 hours to
> > watch a video recording, would you mind sharing a one line summary,
> > please? Is there an alternative patch series implementing BPF signing
> > that you can link us so that we can look at it? Just a link or
> > googlable reference would be more than enough.
>
> It's not 5 hours and you have to read slides and watch
> John's presentation to follow the conversation.

So, If I have understood correctly, the proposal is to validate the
tools which loads the BPF (e.g. perf, ip) with fs-verity, and only
allow BPF loading from those validated binaries?
That's nice, but I think that this could be complementary to the
instructions signature.
Imagine a validated binary being exploited somehow at runtime, that
could be vector of malicious BPF program load.
Can't we have both available, and use one or other, or even both
together depending on the use case?

Regards,
-- 
per aspera ad upstream

Re: [PATCH bpf-next 0/3] bpf: add signature

[Alexei Starovoitov]

On Fri, Dec 3, 2021 at 4:42 PM Matteo Croce <mcroce@linux.microsoft.com> wrote:
>
> On Fri, Dec 3, 2021 at 11:20 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Fri, Dec 3, 2021 at 2:06 PM Luca Boccassi <bluca@debian.org> wrote:
> > >
> > > On Fri, 2021-12-03 at 11:37 -0800, Alexei Starovoitov wrote:
> > > > On Fri, Dec 3, 2021 at 11:36 AM Matteo Croce
> > > > <mcroce@linux.microsoft.com> wrote:
> > > > >
> > > > > On Fri, Dec 3, 2021 at 8:22 PM Alexei Starovoitov
> > > > > <alexei.starovoitov@gmail.com> wrote:
> > > > > >
> > > > > > On Fri, Dec 3, 2021 at 11:18 AM Matteo Croce
> > > > > > <mcroce@linux.microsoft.com> wrote:
> > > > > > >
> > > > > > > From: Matteo Croce <mcroce@microsoft.com>
> > > > > > >
> > > > > > > This series add signature verification for BPF files.
> > > > > > > The first patch implements the signature validation in the
> > > > > > > kernel,
> > > > > > > the second patch optionally makes the signature mandatory,
> > > > > > > the third adds signature generation to bpftool.
> > > > > >
> > > > > > Matteo,
> > > > > >
> > > > > > I think I already mentioned that it's no-go as-is.
> > > > > > We've agreed to go with John's suggestion.
> > > > >
> > > > > Hi,
> > > > >
> > > > > my previous attempt was loading a whole ELF file and parsing it in
> > > > > kernel.
> > > > > In this series I just validate the instructions against a
> > > > > signature,
> > > > > as with kernel CO-RE libbpf doesn't need to mangle it.
> > > > >
> > > > > Which suggestion? I think I missed this one..
> > > >
> > > > This talk and discussion:
> > > > https://linuxplumbersconf.org/event/11/contributions/947/
> > >
> > > Thanks for the link - but for those of us who don't have ~5 hours to
> > > watch a video recording, would you mind sharing a one line summary,
> > > please? Is there an alternative patch series implementing BPF signing
> > > that you can link us so that we can look at it? Just a link or
> > > googlable reference would be more than enough.
> >
> > It's not 5 hours and you have to read slides and watch
> > John's presentation to follow the conversation.
>
> So, If I have understood correctly, the proposal is to validate the
> tools which loads the BPF (e.g. perf, ip) with fs-verity, and only
> allow BPF loading from those validated binaries?
> That's nice, but I think that this could be complementary to the
> instructions signature.
> Imagine a validated binary being exploited somehow at runtime, that
> could be vector of malicious BPF program load.
> Can't we have both available, and use one or other, or even both
> together depending on the use case?

I'll let John comment.

Re: [PATCH bpf-next 0/3] bpf: add signature

[John Fastabend]

Alexei Starovoitov wrote:
> On Fri, Dec 3, 2021 at 4:42 PM Matteo Croce <mcroce@linux.microsoft.com> wrote:
> >
> > On Fri, Dec 3, 2021 at 11:20 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Fri, Dec 3, 2021 at 2:06 PM Luca Boccassi <bluca@debian.org> wrote:
> > > >
> > > > On Fri, 2021-12-03 at 11:37 -0800, Alexei Starovoitov wrote:
> > > > > On Fri, Dec 3, 2021 at 11:36 AM Matteo Croce
> > > > > <mcroce@linux.microsoft.com> wrote:
> > > > > >
> > > > > > On Fri, Dec 3, 2021 at 8:22 PM Alexei Starovoitov
> > > > > > <alexei.starovoitov@gmail.com> wrote:
> > > > > > >
> > > > > > > On Fri, Dec 3, 2021 at 11:18 AM Matteo Croce
> > > > > > > <mcroce@linux.microsoft.com> wrote:
> > > > > > > >
> > > > > > > > From: Matteo Croce <mcroce@microsoft.com>
> > > > > > > >
> > > > > > > > This series add signature verification for BPF files.
> > > > > > > > The first patch implements the signature validation in the
> > > > > > > > kernel,
> > > > > > > > the second patch optionally makes the signature mandatory,
> > > > > > > > the third adds signature generation to bpftool.
> > > > > > >
> > > > > > > Matteo,
> > > > > > >
> > > > > > > I think I already mentioned that it's no-go as-is.
> > > > > > > We've agreed to go with John's suggestion.
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > my previous attempt was loading a whole ELF file and parsing it in
> > > > > > kernel.
> > > > > > In this series I just validate the instructions against a
> > > > > > signature,
> > > > > > as with kernel CO-RE libbpf doesn't need to mangle it.
> > > > > >
> > > > > > Which suggestion? I think I missed this one..
> > > > >
> > > > > This talk and discussion:
> > > > > https://linuxplumbersconf.org/event/11/contributions/947/
> > > >
> > > > Thanks for the link - but for those of us who don't have ~5 hours to
> > > > watch a video recording, would you mind sharing a one line summary,
> > > > please? Is there an alternative patch series implementing BPF signing
> > > > that you can link us so that we can look at it? Just a link or
> > > > googlable reference would be more than enough.
> > >
> > > It's not 5 hours and you have to read slides and watch
> > > John's presentation to follow the conversation.
> >
> > So, If I have understood correctly, the proposal is to validate the
> > tools which loads the BPF (e.g. perf, ip) with fs-verity, and only
> > allow BPF loading from those validated binaries?
> > That's nice, but I think that this could be complementary to the
> > instructions signature.
> > Imagine a validated binary being exploited somehow at runtime, that
> > could be vector of malicious BPF program load.
> > Can't we have both available, and use one or other, or even both
> > together depending on the use case?
> 
> I'll let John comment.

I'll give the outline of the argument here.

I do not believe signing BPF instructions for real programs provides
much additional security. Given most real programs if the application
or loader is exploited at runtime we have all sorts of trouble. First
simply verifying the program doesn't prevent malicious use of the
program. If its in the network program this means DDOS, data exfiltration,
mitm attacks, many other possibilities. If its enforcement program
most enforcement actions are programmed from this application so system
security is lost already.  If its observability application simply
drops/manipulates observations that it wants. I don't know of any
useful programs that exist in isolation without user space input
and output as a critical component. If its not a privileged user,
well it better not be doing anything critical anyways or disabled
outright for the security focused.

Many critical programs can't be signed by the nature of the program.
Optimizing network app generates optimized code at runtime. Observability
tools JIT the code on the fly, similarly enforcement tools will do the
same. I think the power of being able to optimize JIT the code in
application and give to the kernel is something we will see more and
more of. Saying I'm only going to accept signed programs, for a
distribution or something other than niche use case, is non starter
IMO because it breaks so many real use cases. We should encourage
these optimizing use cases as I see it as critical to performance
and keeping overhead low.

From a purely security standpoint I believe you are better off
defining characteristics an application is allowed to have. For
example allowed to probe kernel memory, make these helpers calls,
have this many instructions, use this much memory, this much cpu,
etc. This lets you sandbox a BPF application (both user space and
kernel side) much nicer than any signing will allow.

If we want to 'sign' programs we should do that from a BPF program
directly where other metadata can be included in the policy. For
example having a hash of the program loaded along with the calls
made and process allows for rich policy decisions. I have other
use cases that need a hash/signature for data blobs, so its on
my todo list but not at the top yet.  But, being able to verify
arbitrary blob of data from BPF feels like a useful operation to me
in general. The fact in your case its a set of eBPF insns and in
my case its some key in a network header shouldn't matter.

The series as is, scanned commit descriptions, is going to break
lots of in-use-today programs if it was ever enabled. And
is not as flexible (can't support bpftrace, etc.) or powerful
(can't consider fine grained policy decisions) as above.

Add a function we can hook after verify (or before up for
debate) and helpers to verify signatures and/or generate
hashes and we get a better more general solution. And it can
also solve your use case even if I believe its not useful and
may break many BPF users running bpftrace, libbpf, etc.

Thanks,
John

Re: [PATCH bpf-next 0/3] bpf: add signature

[Luca Boccassi]

[-- Attachment #1: Type: text/plain, Size: 10232 bytes --]

On Fri, 2021-12-03 at 19:39 -0800, John Fastabend wrote:
> Alexei Starovoitov wrote:
> > On Fri, Dec 3, 2021 at 4:42 PM Matteo Croce
> > <mcroce@linux.microsoft.com> wrote:
> > > 
> > > On Fri, Dec 3, 2021 at 11:20 PM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > > 
> > > > On Fri, Dec 3, 2021 at 2:06 PM Luca Boccassi <bluca@debian.org>
> > > > wrote:
> > > > > 
> > > > > On Fri, 2021-12-03 at 11:37 -0800, Alexei Starovoitov wrote:
> > > > > > On Fri, Dec 3, 2021 at 11:36 AM Matteo Croce
> > > > > > <mcroce@linux.microsoft.com> wrote:
> > > > > > > 
> > > > > > > On Fri, Dec 3, 2021 at 8:22 PM Alexei Starovoitov
> > > > > > > <alexei.starovoitov@gmail.com> wrote:
> > > > > > > > 
> > > > > > > > On Fri, Dec 3, 2021 at 11:18 AM Matteo Croce
> > > > > > > > <mcroce@linux.microsoft.com> wrote:
> > > > > > > > > 
> > > > > > > > > From: Matteo Croce <mcroce@microsoft.com>
> > > > > > > > > 
> > > > > > > > > This series add signature verification for BPF files.
> > > > > > > > > The first patch implements the signature validation
> > > > > > > > > in the
> > > > > > > > > kernel,
> > > > > > > > > the second patch optionally makes the signature
> > > > > > > > > mandatory,
> > > > > > > > > the third adds signature generation to bpftool.
> > > > > > > > 
> > > > > > > > Matteo,
> > > > > > > > 
> > > > > > > > I think I already mentioned that it's no-go as-is.
> > > > > > > > We've agreed to go with John's suggestion.
> > > > > > > 
> > > > > > > Hi,
> > > > > > > 
> > > > > > > my previous attempt was loading a whole ELF file and
> > > > > > > parsing it in
> > > > > > > kernel.
> > > > > > > In this series I just validate the instructions against a
> > > > > > > signature,
> > > > > > > as with kernel CO-RE libbpf doesn't need to mangle it.
> > > > > > > 
> > > > > > > Which suggestion? I think I missed this one..
> > > > > > 
> > > > > > This talk and discussion:
> > > > > > https://linuxplumbersconf.org/event/11/contributions/947/
> > > > > 
> > > > > Thanks for the link - but for those of us who don't have ~5
> > > > > hours to
> > > > > watch a video recording, would you mind sharing a one line
> > > > > summary,
> > > > > please? Is there an alternative patch series implementing BPF
> > > > > signing
> > > > > that you can link us so that we can look at it? Just a link
> > > > > or
> > > > > googlable reference would be more than enough.
> > > > 
> > > > It's not 5 hours and you have to read slides and watch
> > > > John's presentation to follow the conversation.
> > > 
> > > So, If I have understood correctly, the proposal is to validate
> > > the
> > > tools which loads the BPF (e.g. perf, ip) with fs-verity, and
> > > only
> > > allow BPF loading from those validated binaries?
> > > That's nice, but I think that this could be complementary to the
> > > instructions signature.
> > > Imagine a validated binary being exploited somehow at runtime,
> > > that
> > > could be vector of malicious BPF program load.
> > > Can't we have both available, and use one or other, or even both
> > > together depending on the use case?
> > 
> > I'll let John comment.
> 
> I'll give the outline of the argument here.
> 
> I do not believe signing BPF instructions for real programs provides
> much additional security. Given most real programs if the application
> or loader is exploited at runtime we have all sorts of trouble. First
> simply verifying the program doesn't prevent malicious use of the
> program. If its in the network program this means DDOS, data
> exfiltration,
> mitm attacks, many other possibilities. If its enforcement program
> most enforcement actions are programmed from this application so
> system
> security is lost already.  If its observability application simply
> drops/manipulates observations that it wants. I don't know of any
> useful programs that exist in isolation without user space input
> and output as a critical component. If its not a privileged user,
> well it better not be doing anything critical anyways or disabled
> outright for the security focused.
> 
> Many critical programs can't be signed by the nature of the program.
> Optimizing network app generates optimized code at runtime.
> Observability
> tools JIT the code on the fly, similarly enforcement tools will do
> the
> same. I think the power of being able to optimize JIT the code in
> application and give to the kernel is something we will see more and
> more of. Saying I'm only going to accept signed programs, for a
> distribution or something other than niche use case, is non starter
> IMO because it breaks so many real use cases. We should encourage
> these optimizing use cases as I see it as critical to performance
> and keeping overhead low.
> 
> From a purely security standpoint I believe you are better off
> defining characteristics an application is allowed to have. For
> example allowed to probe kernel memory, make these helpers calls,
> have this many instructions, use this much memory, this much cpu,
> etc. This lets you sandbox a BPF application (both user space and
> kernel side) much nicer than any signing will allow.
> 
> If we want to 'sign' programs we should do that from a BPF program
> directly where other metadata can be included in the policy. For
> example having a hash of the program loaded along with the calls
> made and process allows for rich policy decisions. I have other
> use cases that need a hash/signature for data blobs, so its on
> my todo list but not at the top yet.  But, being able to verify
> arbitrary blob of data from BPF feels like a useful operation to me
> in general. The fact in your case its a set of eBPF insns and in
> my case its some key in a network header shouldn't matter.
> 
> The series as is, scanned commit descriptions, is going to break
> lots of in-use-today programs if it was ever enabled. And
> is not as flexible (can't support bpftrace, etc.) or powerful
> (can't consider fine grained policy decisions) as above.
> 
> Add a function we can hook after verify (or before up for
> debate) and helpers to verify signatures and/or generate
> hashes and we get a better more general solution. And it can
> also solve your use case even if I believe its not useful and
> may break many BPF users running bpftrace, libbpf, etc.
> 
> Thanks,
> John

Hello John,

Thank you for the summary, this is much clearer.

First of all, I think there's some misunderstanding: this series does
not enable optional signatures by default, and does not enable
mandatory signatures by default either. So I don't see how it would
break existing use cases as you are saying? Unless I'm missing
something?

There's a kconfig to enable optional signatures - if they are there,
they are verified, if they are not present then nothing different
happens. Unless I am missing something, this should be backward
compatible. This kconfig would likely be enabled in most use cases,
just like optionally signed kernel modules are.

Then there's a kconfig on top of that which makes signatures mandatory.
I would not imagine this to be enabled in may cases, just in custom
builds that have more stringent requirements. It certainly would not be
enabled in generalist distros. Perhaps a more flexible way would be to
introduce a sysctl, like fsverity has with
'fs.verity.require_signatures'? That would be just fine for our use
case. Matteo can we do that instead in the next revision?

Secondly, I understand that for your use case signing programs would
not be the best approach. That's fine, and I'm glad you are working on
an alternative that better fits your model, it will be very interesting
to see how it looks like once implemented. But that model doesn't fit
all cases. In our case at Microsoft, we absolutely want to be able to
pre-define at build time a list of BPF programs that are allowed to be
loaded, and reject anything else. Userspace processes in our case are
mostly old and crufty c++ programs that can most likely be pwned by
looking at them sideways, so they get locked down hard with multiple
redundant layers and so on and so forth. But right now for BPF you only
have a "can load BPF" or "cannot load BPF" knob, and that's it. This is
not good enough: we need to be able to define a list of allowed
payloads, and be able to enforce it, so when (not if) said processes do
get tricked into loading something else, it will fail, despite having
the capability of calling bpf(). Trying to define heuristics is also
not good enough for us - creative malicious actors have a tendency to
come up with ways to chain things that individually are allowed and
benign, but combined in a way that you just couldn't foresee. It would
certainly cover a lot of cases, but not all. A strictly pre-defined
list of what is allowed to run and what is not is what we need for our
case, so that we always know exactly what is going to run and what is
not, and can deal with the consequences accordingly, without nasty
surprises waiting around the corner. Now in my naive view the best way
to achieve this is via signatures and certs, as it's a well-understood
system, with processes already in place to revoke/rotate/etc, and it's
already used for kmods. An alternative would be hard-coding hashes I
guess, but that would be terribly inflexible.

Now in terms of _how_ the signatures are done and validated, I'm sure
there are multiple ways, and if some are better than what this series
implements, then that's not an issue, it can be reworked. But the core
requirement for us is: offline pre-defined list of what is allowed to
run and what is not, with ability for hard enforcement that cannot be
bypassed. Yes, you lose some features like JIT and so on: we don't
care, we don't need those for our use cases. If others have different
needs that's fine, this is all intended to be optional, not mandatory.
There are obviously trade-offs, as always when security is involved,
and each user can decide what's best for them.

Hope this makes sense. Thanks!

-- 
Kind regards,
Luca Boccassi

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH bpf-next 0/3] bpf: add signature

[John Fastabend]

Luca Boccassi wrote:

cutting to just the relevant pieces here.

[...]

> 
> > I'll give the outline of the argument here.
> > 
> > I do not believe signing BPF instructions for real programs provides
> > much additional security. Given most real programs if the application
> > or loader is exploited at runtime we have all sorts of trouble. First
> > simply verifying the program doesn't prevent malicious use of the
> > program. If its in the network program this means DDOS, data
> > exfiltration,
> > mitm attacks, many other possibilities. If its enforcement program
> > most enforcement actions are programmed from this application so
> > system
> > security is lost already.  If its observability application simply
> > drops/manipulates observations that it wants. I don't know of any
> > useful programs that exist in isolation without user space input
> > and output as a critical component. If its not a privileged user,
> > well it better not be doing anything critical anyways or disabled
> > outright for the security focused.
> > 
> > Many critical programs can't be signed by the nature of the program.
> > Optimizing network app generates optimized code at runtime.
> > Observability
> > tools JIT the code on the fly, similarly enforcement tools will do
> > the
> > same. I think the power of being able to optimize JIT the code in
> > application and give to the kernel is something we will see more and
> > more of. Saying I'm only going to accept signed programs, for a
> > distribution or something other than niche use case, is non starter
> > IMO because it breaks so many real use cases. We should encourage
> > these optimizing use cases as I see it as critical to performance
> > and keeping overhead low.
> > 
> > From a purely security standpoint I believe you are better off
> > defining characteristics an application is allowed to have. For
> > example allowed to probe kernel memory, make these helpers calls,
> > have this many instructions, use this much memory, this much cpu,
> > etc. This lets you sandbox a BPF application (both user space and
> > kernel side) much nicer than any signing will allow.
> > 
> > If we want to 'sign' programs we should do that from a BPF program
> > directly where other metadata can be included in the policy. For
> > example having a hash of the program loaded along with the calls
> > made and process allows for rich policy decisions. I have other
> > use cases that need a hash/signature for data blobs, so its on
> > my todo list but not at the top yet.  But, being able to verify
> > arbitrary blob of data from BPF feels like a useful operation to me
> > in general. The fact in your case its a set of eBPF insns and in
> > my case its some key in a network header shouldn't matter.
> > 
> > The series as is, scanned commit descriptions, is going to break
> > lots of in-use-today programs if it was ever enabled. And
> > is not as flexible (can't support bpftrace, etc.) or powerful
> > (can't consider fine grained policy decisions) as above.
> > 
> > Add a function we can hook after verify (or before up for
> > debate) and helpers to verify signatures and/or generate
> > hashes and we get a better more general solution. And it can
> > also solve your use case even if I believe its not useful and
> > may break many BPF users running bpftrace, libbpf, etc.
> > 
> > Thanks,
> > John
> 
> Hello John,
> 
> Thank you for the summary, this is much clearer.
> 
> First of all, I think there's some misunderstanding: this series does
> not enable optional signatures by default, and does not enable
> mandatory signatures by default either. So I don't see how it would
> break existing use cases as you are saying? Unless I'm missing
> something?
> 
> There's a kconfig to enable optional signatures - if they are there,
> they are verified, if they are not present then nothing different
> happens. Unless I am missing something, this should be backward
> compatible. This kconfig would likely be enabled in most use cases,
> just like optionally signed kernel modules are.

Agree, without enforcement things should continue to work.

> 
> Then there's a kconfig on top of that which makes signatures mandatory.
> I would not imagine this to be enabled in may cases, just in custom
> builds that have more stringent requirements. It certainly would not be
> enabled in generalist distros. Perhaps a more flexible way would be to
> introduce a sysctl, like fsverity has with
> 'fs.verity.require_signatures'? That would be just fine for our use
> case. Matteo can we do that instead in the next revision?

We want to manage this from BPF side directly. It looks
like policy decision and we have use cases that are not as
simple as yes/no with global switch. For example, in k8s world
this might be enabled via labels which are user specific per container
policy. e.g. lockdown some containers more strictly than others.

> 
> Secondly, I understand that for your use case signing programs would
> not be the best approach. That's fine, and I'm glad you are working on
> an alternative that better fits your model, it will be very interesting
> to see how it looks like once implemented. But that model doesn't fit
> all cases. In our case at Microsoft, we absolutely want to be able to
> pre-define at build time a list of BPF programs that are allowed to be
> loaded, and reject anything else. Userspace processes in our case are

By building this into BPF you can get the 'reject anything else' policy
and I get the metadata + reject/accept from the same hook. Its
just your program can be very simple.

> mostly old and crufty c++ programs that can most likely be pwned by
> looking at them sideways, so they get locked down hard with multiple
> redundant layers and so on and so forth. But right now for BPF you only
> have a "can load BPF" or "cannot load BPF" knob, and that's it. This is
> not good enough: we need to be able to define a list of allowed
> payloads, and be able to enforce it, so when (not if) said processes do
> get tricked into loading something else, it will fail, despite having

Yikes, this is a bit scary from a sec point of view right? Are those
programs read-only maps or can the C++ program also write into the
maps and control plane. Assuming they do some critical functions then
you really shouldn't be trusting them to not do all sorts of other
horrible things. Anyways not too important to this discussion.

I'll just reiterate (I think you get it though) that simply signing
enforcement doesn't mean now BPF is safe. Further these programs
have very high privileges and can do all sorts of things to the
system. But, sure sig enforcement locks down one avenue of loading
bogus program.

> the capability of calling bpf(). Trying to define heuristics is also
> not good enough for us - creative malicious actors have a tendency to
> come up with ways to chain things that individually are allowed and
> benign, but combined in a way that you just couldn't foresee. It would

Sure, but I would argue some things can be very restrictive and
generally useful. For example, never allow kernel memory read could be
enforced from BPF side directly. Never allow pkt redirect, etc.

> certainly cover a lot of cases, but not all. A strictly pre-defined
> list of what is allowed to run and what is not is what we need for our
> case, so that we always know exactly what is going to run and what is
> not, and can deal with the consequences accordingly, without nasty
> surprises waiting around the corner. Now in my naive view the best way
> to achieve this is via signatures and certs, as it's a well-understood
> system, with processes already in place to revoke/rotate/etc, and it's
> already used for kmods. An alternative would be hard-coding hashes I
> guess, but that would be terribly inflexible.

Another option would be to load your programs at boot time, presumably
with trusted boot enabled and then lock down BPF completely. Then
ensure all your BPF 'programs' are read-only from user<->kernel
interface and this should start looking fairly close to what you
want and all programs are correct by root of trust back to
trusted boot. Would assume you know what programs to load at boot
though. May or may not be a big assumption depending on your env.

> 
> Now in terms of _how_ the signatures are done and validated, I'm sure
> there are multiple ways, and if some are better than what this series
> implements, then that's not an issue, it can be reworked. But the core
> requirement for us is: offline pre-defined list of what is allowed to
> run and what is not, with ability for hard enforcement that cannot be
> bypassed. Yes, you lose some features like JIT and so on: we don't
> care, we don't need those for our use cases. If others have different
> needs that's fine, this is all intended to be optional, not mandatory.
> There are obviously trade-offs, as always when security is involved,
> and each user can decide what's best for them.
> 
> Hope this makes sense. Thanks!

I think I understand your use case. When done as BPF helper you
can get the behavior you want with a one line BPF program
loaded at boot.

int verify_all(struct bpf_prog **prog) {
	return verify_signature(prog->insn,
				prog->len * sizeof(struct bpf_insn),
			        signature, KEYRING, BPF_SIGTYPE);
}

And I can write some more specific things as,

int verify_blobs(void data) {
  int reject = verify_signature(data, data_len, sig, KEYRING, TYPE);
  struct policy_key *key = map_get_key();

  return policy(key, reject);  
}

map_get_key() looks into some datastor with the policy likely using
'current' to dig something up. It doesn't just apply to BPF progs
we can use it on other executables more generally. And I get more
interesting use cases like, allowing 'tc' programs unsigned, but
requiring kernel memory reads to require signatures or any N
other policies that may have value. Or only allowing my dbg user
to run read-only programs, because the dbg maybe shouldn't ever
be writing into packets, etc. Driving least privilege use cases
in fine detail.

By making it a BPF program we side step the debate where the kernel
tries to get the 'right' policy for you, me, everyone now and in
the future. The only way I can see to do this without getting N
policies baked into the kernel and at M different hook points is via
a BPF helper.

Thanks,
John

Re: [PATCH bpf-next 0/3] bpf: add signature

[Arnaldo Carvalho de Melo]

Em Mon, Dec 06, 2021 at 12:40:40PM -0800, John Fastabend escreveu:
> I'll just reiterate (I think you get it though) that simply signing
> enforcement doesn't mean now BPF is safe. Further these programs

I think this was clear from the get go, at most this would help with
fingerpointing :-) I.e. BPF signing is not about making things safer,
its just an attempt to know who messed up.

> have very high privileges and can do all sorts of things to the
> system. But, sure sig enforcement locks down one avenue of loading
> bogus program.
 
> > the capability of calling bpf(). Trying to define heuristics is also
> > not good enough for us - creative malicious actors have a tendency to
> > come up with ways to chain things that individually are allowed and
> > benign, but combined in a way that you just couldn't foresee. It would
 
> Sure, but I would argue some things can be very restrictive and
> generally useful. For example, never allow kernel memory read could be
> enforced from BPF side directly. Never allow pkt redirect, etc.

But this is something unrelated to BPF signing, right? Its something
desirable, I'd say this will be at some point required, i.e. one more
step in having BPF programs to be more like userspace apps, where you
can limit all sorts of things it can do, programmatically, a BPF ulimit,
hey, blimit?
 
- Arnaldo

Re: [PATCH bpf-next 0/3] bpf: add signature

[Luca Boccassi]

[-- Attachment #1: Type: text/plain, Size: 12515 bytes --]

On Mon, 2021-12-06 at 12:40 -0800, John Fastabend wrote:
> Luca Boccassi wrote:
> 
> cutting to just the relevant pieces here.
> 
> [...]
> 
> > 
> > > I'll give the outline of the argument here.
> > > 
> > > I do not believe signing BPF instructions for real programs
> > > provides
> > > much additional security. Given most real programs if the
> > > application
> > > or loader is exploited at runtime we have all sorts of trouble.
> > > First
> > > simply verifying the program doesn't prevent malicious use of the
> > > program. If its in the network program this means DDOS, data
> > > exfiltration,
> > > mitm attacks, many other possibilities. If its enforcement
> > > program
> > > most enforcement actions are programmed from this application so
> > > system
> > > security is lost already.  If its observability application
> > > simply
> > > drops/manipulates observations that it wants. I don't know of any
> > > useful programs that exist in isolation without user space input
> > > and output as a critical component. If its not a privileged user,
> > > well it better not be doing anything critical anyways or disabled
> > > outright for the security focused.
> > > 
> > > Many critical programs can't be signed by the nature of the
> > > program.
> > > Optimizing network app generates optimized code at runtime.
> > > Observability
> > > tools JIT the code on the fly, similarly enforcement tools will
> > > do
> > > the
> > > same. I think the power of being able to optimize JIT the code in
> > > application and give to the kernel is something we will see more
> > > and
> > > more of. Saying I'm only going to accept signed programs, for a
> > > distribution or something other than niche use case, is non
> > > starter
> > > IMO because it breaks so many real use cases. We should encourage
> > > these optimizing use cases as I see it as critical to performance
> > > and keeping overhead low.
> > > 
> > > From a purely security standpoint I believe you are better off
> > > defining characteristics an application is allowed to have. For
> > > example allowed to probe kernel memory, make these helpers calls,
> > > have this many instructions, use this much memory, this much cpu,
> > > etc. This lets you sandbox a BPF application (both user space and
> > > kernel side) much nicer than any signing will allow.
> > > 
> > > If we want to 'sign' programs we should do that from a BPF
> > > program
> > > directly where other metadata can be included in the policy. For
> > > example having a hash of the program loaded along with the calls
> > > made and process allows for rich policy decisions. I have other
> > > use cases that need a hash/signature for data blobs, so its on
> > > my todo list but not at the top yet.  But, being able to verify
> > > arbitrary blob of data from BPF feels like a useful operation to
> > > me
> > > in general. The fact in your case its a set of eBPF insns and in
> > > my case its some key in a network header shouldn't matter.
> > > 
> > > The series as is, scanned commit descriptions, is going to break
> > > lots of in-use-today programs if it was ever enabled. And
> > > is not as flexible (can't support bpftrace, etc.) or powerful
> > > (can't consider fine grained policy decisions) as above.
> > > 
> > > Add a function we can hook after verify (or before up for
> > > debate) and helpers to verify signatures and/or generate
> > > hashes and we get a better more general solution. And it can
> > > also solve your use case even if I believe its not useful and
> > > may break many BPF users running bpftrace, libbpf, etc.
> > > 
> > > Thanks,
> > > John
> > 
> > Hello John,
> > 
> > Thank you for the summary, this is much clearer.
> > 
> > First of all, I think there's some misunderstanding: this series
> > does
> > not enable optional signatures by default, and does not enable
> > mandatory signatures by default either. So I don't see how it would
> > break existing use cases as you are saying? Unless I'm missing
> > something?
> > 
> > There's a kconfig to enable optional signatures - if they are
> > there,
> > they are verified, if they are not present then nothing different
> > happens. Unless I am missing something, this should be backward
> > compatible. This kconfig would likely be enabled in most use cases,
> > just like optionally signed kernel modules are.
> 
> Agree, without enforcement things should continue to work.
> 
> > 
> > Then there's a kconfig on top of that which makes signatures
> > mandatory.
> > I would not imagine this to be enabled in may cases, just in custom
> > builds that have more stringent requirements. It certainly would
> > not be
> > enabled in generalist distros. Perhaps a more flexible way would be
> > to
> > introduce a sysctl, like fsverity has with
> > 'fs.verity.require_signatures'? That would be just fine for our use
> > case. Matteo can we do that instead in the next revision?
> 
> We want to manage this from BPF side directly. It looks
> like policy decision and we have use cases that are not as
> simple as yes/no with global switch. For example, in k8s world
> this might be enabled via labels which are user specific per
> container
> policy. e.g. lockdown some containers more strictly than others.
> 
> > 
> > Secondly, I understand that for your use case signing programs
> > would
> > not be the best approach. That's fine, and I'm glad you are working
> > on
> > an alternative that better fits your model, it will be very
> > interesting
> > to see how it looks like once implemented. But that model doesn't
> > fit
> > all cases. In our case at Microsoft, we absolutely want to be able
> > to
> > pre-define at build time a list of BPF programs that are allowed to
> > be
> > loaded, and reject anything else. Userspace processes in our case
> > are
> 
> By building this into BPF you can get the 'reject anything else'
> policy
> and I get the metadata + reject/accept from the same hook. Its
> just your program can be very simple.
> 
> > mostly old and crufty c++ programs that can most likely be pwned by
> > looking at them sideways, so they get locked down hard with
> > multiple
> > redundant layers and so on and so forth. But right now for BPF you
> > only
> > have a "can load BPF" or "cannot load BPF" knob, and that's it.
> > This is
> > not good enough: we need to be able to define a list of allowed
> > payloads, and be able to enforce it, so when (not if) said
> > processes do
> > get tricked into loading something else, it will fail, despite
> > having
> 
> Yikes, this is a bit scary from a sec point of view right? Are those
> programs read-only maps or can the C++ program also write into the
> maps and control plane. Assuming they do some critical functions then
> you really shouldn't be trusting them to not do all sorts of other
> horrible things. Anyways not too important to this discussion.
> 
> I'll just reiterate (I think you get it though) that simply signing
> enforcement doesn't mean now BPF is safe. Further these programs
> have very high privileges and can do all sorts of things to the
> system. But, sure sig enforcement locks down one avenue of loading
> bogus program.

Oh it's terrifying - but business needs and all that.
But Arnaldo is spot on - it's not strictly about what is more secure,
but more about making it a known quantity. If we can prove what is
allowed to run and what not before any machine has even booted (barring
bugs in sig verification, of course) then the $org_security_team is
satisfied and can sign off on enabling bpf. Otherwise we can keep
dreaming.

> > the capability of calling bpf(). Trying to define heuristics is
> > also
> > not good enough for us - creative malicious actors have a tendency
> > to
> > come up with ways to chain things that individually are allowed and
> > benign, but combined in a way that you just couldn't foresee. It
> > would
> 
> Sure, but I would argue some things can be very restrictive and
> generally useful. For example, never allow kernel memory read could
> be
> enforced from BPF side directly. Never allow pkt redirect, etc.
> 
> > certainly cover a lot of cases, but not all. A strictly pre-defined
> > list of what is allowed to run and what is not is what we need for
> > our
> > case, so that we always know exactly what is going to run and what
> > is
> > not, and can deal with the consequences accordingly, without nasty
> > surprises waiting around the corner. Now in my naive view the best
> > way
> > to achieve this is via signatures and certs, as it's a well-
> > understood
> > system, with processes already in place to revoke/rotate/etc, and
> > it's
> > already used for kmods. An alternative would be hard-coding hashes
> > I
> > guess, but that would be terribly inflexible.
> 
> Another option would be to load your programs at boot time,
> presumably
> with trusted boot enabled and then lock down BPF completely. Then
> ensure all your BPF 'programs' are read-only from user<->kernel
> interface and this should start looking fairly close to what you
> want and all programs are correct by root of trust back to
> trusted boot. Would assume you know what programs to load at boot
> though. May or may not be a big assumption depending on your env.

One of the use cases we have for BPF is on-demand diagnostics, so
loading at boot and blocking afterwards would not work, I think.
Environment is constrained in terms of resources, so don't want to load
anything that is not needed.

> > 
> > Now in terms of _how_ the signatures are done and validated, I'm
> > sure
> > there are multiple ways, and if some are better than what this
> > series
> > implements, then that's not an issue, it can be reworked. But the
> > core
> > requirement for us is: offline pre-defined list of what is allowed
> > to
> > run and what is not, with ability for hard enforcement that cannot
> > be
> > bypassed. Yes, you lose some features like JIT and so on: we don't
> > care, we don't need those for our use cases. If others have
> > different
> > needs that's fine, this is all intended to be optional, not
> > mandatory.
> > There are obviously trade-offs, as always when security is
> > involved,
> > and each user can decide what's best for them.
> > 
> > Hope this makes sense. Thanks!
> 
> I think I understand your use case. When done as BPF helper you
> can get the behavior you want with a one line BPF program
> loaded at boot.
> 
> int verify_all(struct bpf_prog **prog) {
>         return verify_signature(prog->insn,
>                                 prog->len * sizeof(struct bpf_insn),
>                                 signature, KEYRING, BPF_SIGTYPE);
> }
> 
> And I can write some more specific things as,
> 
> int verify_blobs(void data) {
>   int reject = verify_signature(data, data_len, sig, KEYRING, TYPE);
>   struct policy_key *key = map_get_key();
> 
>   return policy(key, reject);  
> }
> 
> map_get_key() looks into some datastor with the policy likely using
> 'current' to dig something up. It doesn't just apply to BPF progs
> we can use it on other executables more generally. And I get more
> interesting use cases like, allowing 'tc' programs unsigned, but
> requiring kernel memory reads to require signatures or any N
> other policies that may have value. Or only allowing my dbg user
> to run read-only programs, because the dbg maybe shouldn't ever
> be writing into packets, etc. Driving least privilege use cases
> in fine detail.
> 
> By making it a BPF program we side step the debate where the kernel
> tries to get the 'right' policy for you, me, everyone now and in
> the future. The only way I can see to do this without getting N
> policies baked into the kernel and at M different hook points is via
> a BPF helper.
> 
> Thanks,
> John

Now this sounds like something that could work - we can prove that this
could be loaded before any writable fs comes up anywhere, so in
principle I think it would be acceptable and free of races. Matteo, we
should talk about this tomorrow.
And this requires some infrastructure work right? Is there a WIP git
tree somewhere that we can test out?

Thank you!

-- 
Kind regards,
Luca Boccassi

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH bpf-next 0/3] bpf: add signature

[Luca Boccassi]

[-- Attachment #1: Type: text/plain, Size: 13374 bytes --]

On Mon, 2021-12-06 at 22:59 +0000, Luca Boccassi wrote:
> On Mon, 2021-12-06 at 12:40 -0800, John Fastabend wrote:
> > Luca Boccassi wrote:
> > 
> > cutting to just the relevant pieces here.
> > 
> > [...]
> > 
> > > 
> > > > I'll give the outline of the argument here.
> > > > 
> > > > I do not believe signing BPF instructions for real programs
> > > > provides
> > > > much additional security. Given most real programs if the
> > > > application
> > > > or loader is exploited at runtime we have all sorts of trouble.
> > > > First
> > > > simply verifying the program doesn't prevent malicious use of the
> > > > program. If its in the network program this means DDOS, data
> > > > exfiltration,
> > > > mitm attacks, many other possibilities. If its enforcement
> > > > program
> > > > most enforcement actions are programmed from this application so
> > > > system
> > > > security is lost already.  If its observability application
> > > > simply
> > > > drops/manipulates observations that it wants. I don't know of any
> > > > useful programs that exist in isolation without user space input
> > > > and output as a critical component. If its not a privileged user,
> > > > well it better not be doing anything critical anyways or disabled
> > > > outright for the security focused.
> > > > 
> > > > Many critical programs can't be signed by the nature of the
> > > > program.
> > > > Optimizing network app generates optimized code at runtime.
> > > > Observability
> > > > tools JIT the code on the fly, similarly enforcement tools will
> > > > do
> > > > the
> > > > same. I think the power of being able to optimize JIT the code in
> > > > application and give to the kernel is something we will see more
> > > > and
> > > > more of. Saying I'm only going to accept signed programs, for a
> > > > distribution or something other than niche use case, is non
> > > > starter
> > > > IMO because it breaks so many real use cases. We should encourage
> > > > these optimizing use cases as I see it as critical to performance
> > > > and keeping overhead low.
> > > > 
> > > > From a purely security standpoint I believe you are better off
> > > > defining characteristics an application is allowed to have. For
> > > > example allowed to probe kernel memory, make these helpers calls,
> > > > have this many instructions, use this much memory, this much cpu,
> > > > etc. This lets you sandbox a BPF application (both user space and
> > > > kernel side) much nicer than any signing will allow.
> > > > 
> > > > If we want to 'sign' programs we should do that from a BPF
> > > > program
> > > > directly where other metadata can be included in the policy. For
> > > > example having a hash of the program loaded along with the calls
> > > > made and process allows for rich policy decisions. I have other
> > > > use cases that need a hash/signature for data blobs, so its on
> > > > my todo list but not at the top yet.  But, being able to verify
> > > > arbitrary blob of data from BPF feels like a useful operation to
> > > > me
> > > > in general. The fact in your case its a set of eBPF insns and in
> > > > my case its some key in a network header shouldn't matter.
> > > > 
> > > > The series as is, scanned commit descriptions, is going to break
> > > > lots of in-use-today programs if it was ever enabled. And
> > > > is not as flexible (can't support bpftrace, etc.) or powerful
> > > > (can't consider fine grained policy decisions) as above.
> > > > 
> > > > Add a function we can hook after verify (or before up for
> > > > debate) and helpers to verify signatures and/or generate
> > > > hashes and we get a better more general solution. And it can
> > > > also solve your use case even if I believe its not useful and
> > > > may break many BPF users running bpftrace, libbpf, etc.
> > > > 
> > > > Thanks,
> > > > John
> > > 
> > > Hello John,
> > > 
> > > Thank you for the summary, this is much clearer.
> > > 
> > > First of all, I think there's some misunderstanding: this series
> > > does
> > > not enable optional signatures by default, and does not enable
> > > mandatory signatures by default either. So I don't see how it would
> > > break existing use cases as you are saying? Unless I'm missing
> > > something?
> > > 
> > > There's a kconfig to enable optional signatures - if they are
> > > there,
> > > they are verified, if they are not present then nothing different
> > > happens. Unless I am missing something, this should be backward
> > > compatible. This kconfig would likely be enabled in most use cases,
> > > just like optionally signed kernel modules are.
> > 
> > Agree, without enforcement things should continue to work.
> > 
> > > 
> > > Then there's a kconfig on top of that which makes signatures
> > > mandatory.
> > > I would not imagine this to be enabled in may cases, just in custom
> > > builds that have more stringent requirements. It certainly would
> > > not be
> > > enabled in generalist distros. Perhaps a more flexible way would be
> > > to
> > > introduce a sysctl, like fsverity has with
> > > 'fs.verity.require_signatures'? That would be just fine for our use
> > > case. Matteo can we do that instead in the next revision?
> > 
> > We want to manage this from BPF side directly. It looks
> > like policy decision and we have use cases that are not as
> > simple as yes/no with global switch. For example, in k8s world
> > this might be enabled via labels which are user specific per
> > container
> > policy. e.g. lockdown some containers more strictly than others.
> > 
> > > 
> > > Secondly, I understand that for your use case signing programs
> > > would
> > > not be the best approach. That's fine, and I'm glad you are working
> > > on
> > > an alternative that better fits your model, it will be very
> > > interesting
> > > to see how it looks like once implemented. But that model doesn't
> > > fit
> > > all cases. In our case at Microsoft, we absolutely want to be able
> > > to
> > > pre-define at build time a list of BPF programs that are allowed to
> > > be
> > > loaded, and reject anything else. Userspace processes in our case
> > > are
> > 
> > By building this into BPF you can get the 'reject anything else'
> > policy
> > and I get the metadata + reject/accept from the same hook. Its
> > just your program can be very simple.
> > 
> > > mostly old and crufty c++ programs that can most likely be pwned by
> > > looking at them sideways, so they get locked down hard with
> > > multiple
> > > redundant layers and so on and so forth. But right now for BPF you
> > > only
> > > have a "can load BPF" or "cannot load BPF" knob, and that's it.
> > > This is
> > > not good enough: we need to be able to define a list of allowed
> > > payloads, and be able to enforce it, so when (not if) said
> > > processes do
> > > get tricked into loading something else, it will fail, despite
> > > having
> > 
> > Yikes, this is a bit scary from a sec point of view right? Are those
> > programs read-only maps or can the C++ program also write into the
> > maps and control plane. Assuming they do some critical functions then
> > you really shouldn't be trusting them to not do all sorts of other
> > horrible things. Anyways not too important to this discussion.
> > 
> > I'll just reiterate (I think you get it though) that simply signing
> > enforcement doesn't mean now BPF is safe. Further these programs
> > have very high privileges and can do all sorts of things to the
> > system. But, sure sig enforcement locks down one avenue of loading
> > bogus program.
> 
> Oh it's terrifying - but business needs and all that.
> But Arnaldo is spot on - it's not strictly about what is more secure,
> but more about making it a known quantity. If we can prove what is
> allowed to run and what not before any machine has even booted (barring
> bugs in sig verification, of course) then the $org_security_team is
> satisfied and can sign off on enabling bpf. Otherwise we can keep
> dreaming.
> 
> > > the capability of calling bpf(). Trying to define heuristics is
> > > also
> > > not good enough for us - creative malicious actors have a tendency
> > > to
> > > come up with ways to chain things that individually are allowed and
> > > benign, but combined in a way that you just couldn't foresee. It
> > > would
> > 
> > Sure, but I would argue some things can be very restrictive and
> > generally useful. For example, never allow kernel memory read could
> > be
> > enforced from BPF side directly. Never allow pkt redirect, etc.
> > 
> > > certainly cover a lot of cases, but not all. A strictly pre-defined
> > > list of what is allowed to run and what is not is what we need for
> > > our
> > > case, so that we always know exactly what is going to run and what
> > > is
> > > not, and can deal with the consequences accordingly, without nasty
> > > surprises waiting around the corner. Now in my naive view the best
> > > way
> > > to achieve this is via signatures and certs, as it's a well-
> > > understood
> > > system, with processes already in place to revoke/rotate/etc, and
> > > it's
> > > already used for kmods. An alternative would be hard-coding hashes
> > > I
> > > guess, but that would be terribly inflexible.
> > 
> > Another option would be to load your programs at boot time,
> > presumably
> > with trusted boot enabled and then lock down BPF completely. Then
> > ensure all your BPF 'programs' are read-only from user<->kernel
> > interface and this should start looking fairly close to what you
> > want and all programs are correct by root of trust back to
> > trusted boot. Would assume you know what programs to load at boot
> > though. May or may not be a big assumption depending on your env.
> 
> One of the use cases we have for BPF is on-demand diagnostics, so
> loading at boot and blocking afterwards would not work, I think.
> Environment is constrained in terms of resources, so don't want to load
> anything that is not needed.
> 
> > > 
> > > Now in terms of _how_ the signatures are done and validated, I'm
> > > sure
> > > there are multiple ways, and if some are better than what this
> > > series
> > > implements, then that's not an issue, it can be reworked. But the
> > > core
> > > requirement for us is: offline pre-defined list of what is allowed
> > > to
> > > run and what is not, with ability for hard enforcement that cannot
> > > be
> > > bypassed. Yes, you lose some features like JIT and so on: we don't
> > > care, we don't need those for our use cases. If others have
> > > different
> > > needs that's fine, this is all intended to be optional, not
> > > mandatory.
> > > There are obviously trade-offs, as always when security is
> > > involved,
> > > and each user can decide what's best for them.
> > > 
> > > Hope this makes sense. Thanks!
> > 
> > I think I understand your use case. When done as BPF helper you
> > can get the behavior you want with a one line BPF program
> > loaded at boot.
> > 
> > int verify_all(struct bpf_prog **prog) {
> >         return verify_signature(prog->insn,
> >                                 prog->len * sizeof(struct bpf_insn),
> >                                 signature, KEYRING, BPF_SIGTYPE);
> > }
> > 
> > And I can write some more specific things as,
> > 
> > int verify_blobs(void data) {
> >   int reject = verify_signature(data, data_len, sig, KEYRING, TYPE);
> >   struct policy_key *key = map_get_key();
> > 
> >   return policy(key, reject);  
> > }
> > 
> > map_get_key() looks into some datastor with the policy likely using
> > 'current' to dig something up. It doesn't just apply to BPF progs
> > we can use it on other executables more generally. And I get more
> > interesting use cases like, allowing 'tc' programs unsigned, but
> > requiring kernel memory reads to require signatures or any N
> > other policies that may have value. Or only allowing my dbg user
> > to run read-only programs, because the dbg maybe shouldn't ever
> > be writing into packets, etc. Driving least privilege use cases
> > in fine detail.
> > 
> > By making it a BPF program we side step the debate where the kernel
> > tries to get the 'right' policy for you, me, everyone now and in
> > the future. The only way I can see to do this without getting N
> > policies baked into the kernel and at M different hook points is via
> > a BPF helper.
> > 
> > Thanks,
> > John
> 
> Now this sounds like something that could work - we can prove that this
> could be loaded before any writable fs comes up anywhere, so in
> principle I think it would be acceptable and free of races. Matteo, we
> should talk about this tomorrow.
> And this requires some infrastructure work right? Is there a WIP git
> tree somewhere that we can test out?
> 
> Thank you!

One question more question: with the signature + kconfig approach,
nothing can disable the signature check. But if the signature checker
is itself a bpf program, is there/can there be anything stopping root
from unloading it?

-- 
Kind regards,
Luca Boccassi

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH bpf-next 0/3] bpf: add signature

[John Fastabend]

[...]

> > > > Hope this makes sense. Thanks!
> > > 
> > > I think I understand your use case. When done as BPF helper you
> > > can get the behavior you want with a one line BPF program
> > > loaded at boot.
> > > 
> > > int verify_all(struct bpf_prog **prog) {
> > >         return verify_signature(prog->insn,
> > >                                 prog->len * sizeof(struct bpf_insn),
> > >                                 signature, KEYRING, BPF_SIGTYPE);
> > > }
> > > 
> > > And I can write some more specific things as,
> > > 
> > > int verify_blobs(void data) {
> > >   int reject = verify_signature(data, data_len, sig, KEYRING, TYPE);
> > >   struct policy_key *key = map_get_key();
> > > 
> > >   return policy(key, reject);  
> > > }
> > > 
> > > map_get_key() looks into some datastor with the policy likely using
> > > 'current' to dig something up. It doesn't just apply to BPF progs
> > > we can use it on other executables more generally. And I get more
> > > interesting use cases like, allowing 'tc' programs unsigned, but
> > > requiring kernel memory reads to require signatures or any N
> > > other policies that may have value. Or only allowing my dbg user
> > > to run read-only programs, because the dbg maybe shouldn't ever
> > > be writing into packets, etc. Driving least privilege use cases
> > > in fine detail.
> > > 
> > > By making it a BPF program we side step the debate where the kernel
> > > tries to get the 'right' policy for you, me, everyone now and in
> > > the future. The only way I can see to do this without getting N
> > > policies baked into the kernel and at M different hook points is via
> > > a BPF helper.
> > > 
> > > Thanks,
> > > John
> > 
> > Now this sounds like something that could work - we can prove that this
> > could be loaded before any writable fs comes up anywhere, so in
> > principle I think it would be acceptable and free of races. Matteo, we
> > should talk about this tomorrow.
> > And this requires some infrastructure work right? Is there a WIP git
> > tree somewhere that we can test out?
> > 
> > Thank you!
> 

I don't have a WIP tree, but I believe it should be fairly easy.
First I would add a wrapper BPF helper for verify_signature() so
we can call it from fentry/freturn context. That can be done on
its own IMO as its a generally useful operation.

Then I would stub a hook point into the BPF load path. The exact
place to put this is going to have some debate I think, but I
would place it immediately after the check_bpf call.

With above two you have enough to do sig verification iiuc.

Early boot loading I would have to check its current status. But I know
folks have been working on it. Maybe its done?

> One question more question: with the signature + kconfig approach,
> nothing can disable the signature check. But if the signature checker
> is itself a bpf program, is there/can there be anything stopping root
> from unloading it?

Interesting. Not that I'm aware of. Currently something with sufficient
privileges could unload the program. Maybe we should have a flag so
early boot programs can signal they shouldn't be unloaded ever. I would
be OK with this and also seems generally useful. I have a case where
I want to always set the socket cookie and we leave it running all the
time. It would be nice if it came up and was pinned at boot.

Maybe slightly better than a flag would be to have a new CAP support
that only early boot has like CAP_BPF_EARLY. From my point of view
this both seems doable with just some smallish changes on BPF side.

Thanks,
John

Re: [PATCH bpf-next 0/3] bpf: add signature

[Luca Boccassi]

[-- Attachment #1: Type: text/plain, Size: 4144 bytes --]

On Wed, 2021-12-08 at 12:17 -0800, John Fastabend wrote:
> [...]
> 
> > > > > Hope this makes sense. Thanks!
> > > > 
> > > > I think I understand your use case. When done as BPF helper you
> > > > can get the behavior you want with a one line BPF program
> > > > loaded at boot.
> > > > 
> > > > int verify_all(struct bpf_prog **prog) {
> > > >         return verify_signature(prog->insn,
> > > >                                 prog->len * sizeof(struct bpf_insn),
> > > >                                 signature, KEYRING, BPF_SIGTYPE);
> > > > }
> > > > 
> > > > And I can write some more specific things as,
> > > > 
> > > > int verify_blobs(void data) {
> > > >   int reject = verify_signature(data, data_len, sig, KEYRING, TYPE);
> > > >   struct policy_key *key = map_get_key();
> > > > 
> > > >   return policy(key, reject);  
> > > > }
> > > > 
> > > > map_get_key() looks into some datastor with the policy likely using
> > > > 'current' to dig something up. It doesn't just apply to BPF progs
> > > > we can use it on other executables more generally. And I get more
> > > > interesting use cases like, allowing 'tc' programs unsigned, but
> > > > requiring kernel memory reads to require signatures or any N
> > > > other policies that may have value. Or only allowing my dbg user
> > > > to run read-only programs, because the dbg maybe shouldn't ever
> > > > be writing into packets, etc. Driving least privilege use cases
> > > > in fine detail.
> > > > 
> > > > By making it a BPF program we side step the debate where the kernel
> > > > tries to get the 'right' policy for you, me, everyone now and in
> > > > the future. The only way I can see to do this without getting N
> > > > policies baked into the kernel and at M different hook points is via
> > > > a BPF helper.
> > > > 
> > > > Thanks,
> > > > John
> > > 
> > > Now this sounds like something that could work - we can prove that this
> > > could be loaded before any writable fs comes up anywhere, so in
> > > principle I think it would be acceptable and free of races. Matteo, we
> > > should talk about this tomorrow.
> > > And this requires some infrastructure work right? Is there a WIP git
> > > tree somewhere that we can test out?
> > > 
> > > Thank you!
> > 
> 
> I don't have a WIP tree, but I believe it should be fairly easy.
> First I would add a wrapper BPF helper for verify_signature() so
> we can call it from fentry/freturn context. That can be done on
> its own IMO as its a generally useful operation.
> 
> Then I would stub a hook point into the BPF load path. The exact
> place to put this is going to have some debate I think, but I
> would place it immediately after the check_bpf call.
> 
> With above two you have enough to do sig verification iiuc.
> 
> Early boot loading I would have to check its current status. But I know
> folks have been working on it. Maybe its done?
> 
> > One question more question: with the signature + kconfig approach,
> > nothing can disable the signature check. But if the signature checker
> > is itself a bpf program, is there/can there be anything stopping root
> > from unloading it?
> 
> Interesting. Not that I'm aware of. Currently something with sufficient
> privileges could unload the program. Maybe we should have a flag so
> early boot programs can signal they shouldn't be unloaded ever. I would
> be OK with this and also seems generally useful. I have a case where
> I want to always set the socket cookie and we leave it running all the
> time. It would be nice if it came up and was pinned at boot.
> 
> Maybe slightly better than a flag would be to have a new CAP support
> that only early boot has like CAP_BPF_EARLY. From my point of view
> this both seems doable with just some smallish changes on BPF side.
> 
> Thanks,
> John

Thanks - again the means of enforcing this are not too important for
our use case, as long as there is something that works reliably and can
be attested.

-- 
Kind regards,
Luca Boccassi

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]