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

[KP Singh]

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?

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

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