Re: [PATCH 11/12] bpftool: Add support for signing BPF programs

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Tue, 2025-06-10 at 10:50 +0200, KP Singh wrote:
> On Sun, Jun 8, 2025 at 4:03 PM James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > 
> > [+keyrings]
> > On Sat, 2025-06-07 at 01:29 +0200, KP Singh wrote:
> > [...]
> > > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> > > index f010295350be..e1dbbca91e34 100644
> > > --- a/tools/bpf/bpftool/prog.c
> > > +++ b/tools/bpf/bpftool/prog.c
> > > @@ -23,6 +23,7 @@
> > >  #include <linux/err.h>
> > >  #include <linux/perf_event.h>
> > >  #include <linux/sizes.h>
> > > +#include <linux/keyctl.h>
> > > 
> > >  #include <bpf/bpf.h>
> > >  #include <bpf/btf.h>
> > > @@ -1875,6 +1876,8 @@ static int try_loader(struct
> > > gen_loader_opts
> > > *gen)
> > >  {
> > >       struct bpf_load_and_run_opts opts = {};
> > >       struct bpf_loader_ctx *ctx;
> > > +     char sig_buf[MAX_SIG_SIZE];
> > > +     __u8 prog_sha[SHA256_DIGEST_LENGTH];
> > >       int ctx_sz = sizeof(*ctx) + 64 * max(sizeof(struct
> > > bpf_map_desc),
> > >                                            sizeof(struct
> > > bpf_prog_desc));
> > >       int log_buf_sz = (1u << 24) - 1;
> > > @@ -1898,6 +1901,24 @@ static int try_loader(struct
> > > gen_loader_opts
> > > *gen)
> > >       opts.insns = gen->insns;
> > >       opts.insns_sz = gen->insns_sz;
> > >       fds_before = count_open_fds();
> > > +
> > > +     if (sign_progs) {
> > > +             opts.excl_prog_hash = prog_sha;
> > > +             opts.excl_prog_hash_sz = sizeof(prog_sha);
> > > +             opts.signature = sig_buf;
> > > +             opts.signature_sz = MAX_SIG_SIZE;
> > > +             opts.keyring_id = KEY_SPEC_SESSION_KEYRING;
> > > +
> > 
> > This looks wrong on a couple of levels.  Firstly, if you want
> > system level integrity you can't search the session keyring because
> > any process can join (subject to keyring permissions) and the
> > owner, who is presumably the one inserting the bpf program, can add
> > any key they like.
> > 
> 
> Wanting system level integrity is a security policy question, so this
> is something that needs to be implemented at the security layer, the
> LSM can deny the keys / keyring IDs they don't trust.  Session
> keyrings are for sure useful for delegated signing of BPF programs
> when dynamically generated.

The problem is you're hard coding it at light skeleton creation time. 
Plus there doesn't seem to be any ability to use the system keyrings
anyway as the kernel code only looks up the user keyrings.  Since
actual key ids are volatile handles which change from boot to boot (so
can't be stored in anything durable) this can only be used for keyring
specifiers, so it would also make sense to check this is actually a
specifier (system keyring specifiers are positive and user specifiers
negative, so it's easy to check for the range).

> > The other problem with this scheme is that the keyring_id itself
> > has no checked integrity, which means that even if a script was
> > marked as
> 
> If an attacker can modify a binary that has permissions to load BPF
> programs and update the keyring ID then we have other issues.

It's a classic supply chain attack (someone modifies the light skeleton
between the creator and the consumer), even Google is claiming SLSA
guarantees, so you can't just wave it away as "other issues".

>  So, this does not work in independence, signed BPF programs do not
> really make sense without trusted execution).

The other patch set provided this ability using signed hash chains, so
absolutely there are signed bpf programmes that can work absent a
trusted user execution environment.  It may not be what you want for
your use case (which is why the other patch set allowed for both), but
there are lots of integrity use cases out there wanting precisely this.

> > system keyring only anyone can binary edit the user space program
> > to change it to their preferred keyring and it will still work.  If
> > you want variable keyrings, they should surely be part of the
> > validated policy.
> 
> The policy is what I expect to be implemented in the LSM layer. A
> variable keyring ID is a critical part of the UAPI to create
> different "rings of trust" e.g. LSM can enforce that network programs
> can be loaded with a derived key, and have a different keyring for
> unprivileged BPF programs.

You can't really have it both ways: either the keyring is part of the
LSM supplied policy in which case it doesn't make much sense to have it
in the durable attributes (and the LSM would have to set it before the
signature is verified) or it's part of the durable attribute embedded
security information and should be integrity protected.

I suppose we could compromise and say it should not be part of the
light skeleton durable attributes but should be set (or supplied by
policy) at BPF_PROG_LOAD time.

I should also note that when other systems use derived keys in
different keyrings, they usually have a specific named trusted keyring
(like _ima and .ima) which has policy enforced rules for adding keys.

Regards,

James


> This patch implements the signing support, not the security policy
> for it.
> 
> - KP