RE: [PATCH 1/3] bpf: Add BPF_F_VERIFY_ELEM to require signature verification on map values

[Roberto Sassu]

> From: KP Singh [mailto:kpsingh@xxxxxxxxxx]
> Sent: Friday, June 3, 2022 5:18 PM
> On Fri, Jun 3, 2022 at 3:11 PM Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> wrote:
> >
> > > From: KP Singh [mailto:kpsingh@xxxxxxxxxx]
> > > Sent: Friday, June 3, 2022 2:08 PM
> > > On Wed, May 25, 2022 at 3:21 PM Roberto Sassu
> <roberto.sassu@xxxxxxxxxx>
> > > wrote:
> > > >
> > > > In some cases, it is desirable to ensure that a map contains data from
> > > > authenticated sources, for example if map data are used for making
> security
> > > > decisions.
> > >
> > > I am guessing this comes from the discussion we had about digilim.
> > > I remember we discussed a BPF helper that could verify signatures.
> > > Why would that approach not work?
> >
> > The main reason is that signature verification can be done also
> > for non-sleepable hooks. For example, one is fexit/array_map_update_elem.
> 
> For your use-case, why is it not possible to hook the LSM hook "bpf"
> i.e security_bpf and then check if there is a MAP_UPDATE_ELEM operation?

It would require the following: a new helper to compare the user space
fd with the address of the map in the eBPF program; copy data from
user space to kernel space (verify_pkcs7_signatureI() expects kernel
memory). That copy would happen twice.

> > Currently the helper in patch 2 just returns the size of verified data.
> > With an additional parameter, it could also be used as a helper for
> > signature verification by any eBPF programs.
> >
> 
> Your bpf_map_verify_value_sig hard codes the type of signature
> (bpf_map_verify_value_sig as verify_pkcs7_signature)
> its implementation. This is not extensible.

It is hardcoded now, but it wouldn't if there are more verification
functions. For example, if 'id_type' of module_signature is set
to PKEY_ID_PGP, bpf_map_verify_value_sig() would call
verify_pgp_signature() (assuming that support for PGP keys and
signatures is added to the kernel).

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Yang Xi, Li He

> What we discussed was an extensible helper that can be used for
> different signature types.
> 
> > To be honest, I like more the idea of a map flag, as it is more
> > clear that signature verification is being done. Otherwise,
> > we would need to infer it from the eBPF program code.
> >
> > Thanks
> >
> > Roberto
> >
> > HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> > Managing Director: Li Peng, Yang Xi, Li He
> >
> > > > Such restriction is achieved by verifying the signature of map values, at
> > > > the time those values are added to the map with the bpf() system call
> (more
> > > > specifically, when the commands passed to bpf() are
> BPF_MAP_UPDATE_ELEM
> > > or
> > > > BPF_MAP_UPDATE_BATCH). Mmappable maps are not allowed in this
> case.
> > > >
> > > > Signature verification is initially done with keys in the primary and
> > > > secondary kernel keyrings, similarly to kernel modules. This allows system
> > > > owners to enforce a system-wide policy based on the keys they trust.
> > > > Support for additional keyrings could be added later, based on use case
> > > > needs.
> > > >
> > > > Signature verification is done only for those maps for which the new map
> > > > flag BPF_F_VERIFY_ELEM is set. When the flag is set, the kernel expects
> map
> > > > values to be in the following format:
> > > >
> > > > +-------------------------------+---------------+-----+-----------------+
> > > > | verified data+sig size (be32) | verified data | sig | unverified data |
> > > > +-------------------------------+---------------+-----+-----------------+
> > > >
> > > > where sig is a module-style appended signature as generated by the
> > > > sign-file tool. The verified data+sig size (in big endian) must be
> > > > explicitly provided (it is not generated by sign-file), as it cannot be
> > > > determined in other ways (currently, the map value size is fixed). It can
> > > > be obtained from the size of the file created by sign-file.
> > > >
> > > > Introduce the new map flag BPF_F_VERIFY_ELEM, and additionally call the
> > > new
> > > > function bpf_map_verify_value_sig() from bpf_map_update_value() if the
> flag
> > > > is set. bpf_map_verify_value_sig(), declared as global for a new helper, is
> > > > basically equivalent to mod_verify_sig(). It additionally does the marker
> > > > check, that for kernel modules is done in module_sig_check(), and the
> > > > parsing of the verified data+sig size.
> > > >
> > > > Currently, enable the usage of the flag only for the array map. Support for
> > > > more map types can be added later.
> > > >
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > > > ---
> 
> [...]
> 
> > > > +                                            NULL, NULL);
> > > > +               if (ret < 0)
> > > > +                       return ret;
> > > > +       }
> > > > +
> > > > +       return modlen;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(bpf_map_verify_value_sig);
> > > >
> > > >  #define BPF_MAP_UPDATE_ELEM_LAST_FIELD flags
> > > >
> > > > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > > > index f4009dbdf62d..a8e7803d2593 100644
> > > > --- a/tools/include/uapi/linux/bpf.h
> > > > +++ b/tools/include/uapi/linux/bpf.h
> > > > @@ -1226,6 +1226,9 @@ enum {
> > > >
> > > >  /* Create a map that is suitable to be an inner map with dynamic max
> entries
> > > */
> > > >         BPF_F_INNER_MAP         = (1U << 12),
> > > > +
> > > > +/* Verify map value (fmt: ver data+sig size(be32), ver data, sig, unver
> data) */
> > > > +       BPF_F_VERIFY_ELEM       = (1U << 13)
> > > >  };
> > > >
> > > >  /* Flags for BPF_PROG_QUERY. */
> > > > --
> > > > 2.25.1
> > > >