[PATCH 1/3] binman: Add support for externally encrypted blobs

[Simon Glass]

Hi Christian,

On Fri, 30 Jun 2023 at 11:28, Taedcke, Christian
<christian.taedcke-oss at weidmueller.com> wrote:
>
> Hello Simon,
>
> Am 30.06.2023 um 10:57 schrieb Simon Glass:
> > Hi Christian,
> >
> > On Fri, 30 Jun 2023 at 09:20, Taedcke, Christian
> > <christian.taedcke-oss at weidmueller.com> wrote:
> >>
> >> Hello Simon,
> >>
> >> thank you for the initial review. Replies are below.
> >>
> >> Am 29.06.2023 um 21:09 schrieb Simon Glass:
> >>> Hi Christian,
> >>>
> >>> On Tue, 27 Jun 2023 at 08:39, <christian.taedcke-oss at weidmueller.com> wrote:
> >>>>
> >>>> From: Christian Taedcke <christian.taedcke at weidmueller.com>
> >>>>
> >>>> This adds a new etype encrypted that is derived from collection.
> >>>>
> >>>> It creates a new cipher node in the related image similar to the
> >>>> cipher node used by u-boot, see boot/image-cipher.c.
> >>>> Optionally it creates a global /cipher node containing a key and iv
> >>>> using the same nameing convention as used in boot/image-cipher.c.
> >>>>
> >>>> Signed-off-by: Christian Taedcke <christian.taedcke at weidmueller.com>
> >>>> ---
> >>>>
> >>>>    tools/binman/etype/encrypted.py | 98 +++++++++++++++++++++++++++++++++
> >>>>    1 file changed, 98 insertions(+)
> >>>>    create mode 100644 tools/binman/etype/encrypted.py
> >>>>
> >>>> diff --git a/tools/binman/etype/encrypted.py b/tools/binman/etype/encrypted.py
> >>>> new file mode 100644
> >>>> index 0000000000..005ae56acf
> >>>> --- /dev/null
> >>>> +++ b/tools/binman/etype/encrypted.py
> >>>> @@ -0,0 +1,98 @@
> >>>> +# SPDX-License-Identifier: GPL-2.0+
> >>>> +# Copyright 2023 Weidmüller Interface GmbH & Co. KG
> >>>> +# Written by Christian Taedcke <christian.taedcke at weidmueller.com>
> >>>> +#
> >>>> +# Entry-type module for cipher information of encrypted blobs/images
> >>>> +#
> >>>> +
> >>>> +from binman.etype.collection import Entry_collection
> >>>> +from dtoc import fdt_util
> >>>> +from u_boot_pylib import tools
> >>>> +
> >>>> +# This is imported if needed
> >>>> +state = None
> >>>> +
> >>>> +
> >>>> +class Entry_encrypted(Entry_collection):
> >>>> +    """Externally built encrypted binary blob
> >>>> +
> >>>> +    If the file providing this blob is missing, binman can optionally ignore it
> >>>> +    and produce a broken image with a warning.
> >>>> +
> >>>> +    In addition to the inherited 'collection' for Properties / Entry arguments:
> >>>> +        - algo: The encryption algorithm
> >>>
> >>> What possible values are available? Please list them
> >>
> >> Currently the evaluation of the generated cipher nodes is not
> >> implemented in c code in upstream U-Boot. I use aes256-gcm and decrypt
> >> the relevant blobs/images in board-specific code. We plan to also
> >> upstream the c code for decryption later.
> >>
> >> I expect we will support at least aes[128/192/256]-cbc in the future,
> >> since these are already implemented in software in U-Boot and in
> >> addition aes256-gcm via a crypto driver.
> >>
> >> Since decryption is not implemented yet, i didn't want to restrict the
> >> possible algos for now, since board-specific code could implement
> >> decryption for any algorithm here that uses a key and iv (initialization
> >> vector).
> >>
> >> Should i list aes[128/192/256]-cbc and aes256-gcm here or should i state
> >> that the supported algorithms (for now) are board specific?
> >
> > Perhaps the correct answer is to say that nothing is supported yet,
> > but future patches will add certain algorithms TBD?
>
> So something like this would be ok?
>
> - algo: The encryption algorithm. Currently no algorithm is supported
> out-of-the-box. Certain algorithms will be added in future patches.

Yes that seems OK to me.

>
> >
> >>
> >>>
> >>>> +        - iv-name-hint: The name hint for the iv
> >>>
> >>> what is the iv?
> >>
> >> Initialization Vector. Should i use the full name here?
> >
> > Yes, plus explain what it is or where it is documented.
> >
> >>
> >>>
> >>>> +        - key-name-hint: The name hint for the key
> >>>> +        - iv-filename: The name of the file containing the iv
> >>>> +        - key-filename: The name of the file containing the key
> >>>> +
> >>>> +    This entry creates a cipher node in the entries' parent node (i.e. the
> >>>
> >>> entry's
> >>>
> >>>> +    image). Optionally it also creates a cipher node in the root of the device
> >>>> +    tree containg key and iv information.
> >>>
> >>> containing
> >>>
> >>> Overall I think this documentation needs to be expanded.
> >>
> >> Ok. I tried to explain the motivation in the cover letter, see
> >> https://lists.denx.de/pipermail/u-boot/2023-June/521160.html
> >>
> >> Is the cover letter the wrong place for this (should i move the
> >> motivation into the first commit message)?
> >>
> >> I will also try to improve the code documentation here.
> >
> > I mean in the docs for the entry itself (which ends up in
> > entries.rst), since this is what people read.
> >
> > Yes it is good to comment the code as well, but it seems OK to me.
> >
> >>
> >>>
> >>> I wonder why this needs to be an entry type? Could the node be added
> >>> to the description by the user, instead of the entry adding it? It
> >>> feels a little strange to me, but perhaps I just need more info.
> >>
> >> This new entry type basically reads the files containing the
> >> initialization vector and the key and puts it into the device tree. The
> >> initialization vector normally changes whenever the encrypted blob changes.
> >>
> >> Having this as an entry type simplifies the build process of the
> >> resulting image. Otherwise some external script would have to run during
> >> the build process to patch the iv and key into the generated device tree.
> >
> > OK, so the 'cipher' node ends up providing information on how to
> > decode the image. But why put it in the root node? Would it not be
> > better to put it in the node next to the one with the encrypted data?
> > People might encrypt several images separately.
>
> Having several encrypted images with different keys/ivs is supported.
> For this different iv-name-hint and/or different key-name-hint values
> must be used.
>
> I only added this global cipher node because this is done in the same
> way in boot/image-cipher.c. I did not want to introduce a new structure.
> But if it is ok, i could remove the cipher node below root (/cipher) and
> move the key and iv property to the cipher node next to the one with the
> encrypted data.
> This would simplify the structure in the device tree and the code.
> In that case i would remove the iv-name-hint, since it is no longer
> used. But i would keep some kind of key-name-hint to transport the
> information which kind of key should be used, see below for an example.

That seems like a good idea to me.

>
> >>
> >>>
> >>>> +    """
> >>>> +
> >>>> +    def __init__(self, section, etype, node):
> >>>> +        # Put this here to allow entry-docs and help to work without libfdt
> >>>> +        global state
> >>>> +        from binman import state
> >>>> +
> >>>> +        super().__init__(section, etype, node)
> >>>> +        # The property key-filename is not required, because some implementations use keys that
> >>>> +        # are not embedded in the device tree, but e.g. in the device itself
> >>>> +        self.required_props = ['algo', 'key-name-hint', 'iv-filename']
> >>>> +        self._algo = fdt_util.GetString(self._node, 'algo')
> >>>> +        self._iv_name_hint = fdt_util.GetString(self._node, 'iv-name-hint')
> >>>> +        self._key_name_hint = fdt_util.GetString(self._node, 'key-name-hint')
> >>>> +        self._filename_iv = fdt_util.GetString(self._node, 'iv-filename')
> >>>> +        self._filename_key = fdt_util.GetString(self._node, 'key-filename')
> >>>
> >>> Here you should set these variables to None. Move the reading to ReadNode()
> >>>
> >>> Sorry if there are counter-examples in the source code. But this is
> >>> the correct way.
> >>>
> >>>> +
> >>>> +    def ReadNode(self):
> >>>> +        super().ReadNode()
> >>>> +
> >>>> +        iv_filename = tools.get_input_filename(self._filename_iv)
> >>>> +        self._iv = tools.read_file(iv_filename, binary=True)
> >>>
> >>> Please only read the node in this method. Move file reading until
> >>> where it is needed.
> >>>
> >>>> +
> >>>> +        self._key = None
> >>>> +        if self._filename_key:
> >>>> +            key_filename = tools.get_input_filename(self._filename_key)
> >>>> +            self._key = tools.read_file(key_filename, binary=True)
> >>>> +
> >>>> +    def gen_entries(self):
> >>>> +        super().gen_entries()
> >>>> +
> >>>> +        cipher_node = state.AddSubnode(self._node.parent, "cipher")
> >>>> +        cipher_node.AddString("algo", self._algo)
> >>>> +        cipher_node.AddString("key-name-hint", self._key_name_hint)
> >>>> +        if self._iv_name_hint:
> >>>> +            cipher_node.AddString("iv-name-hint", self._iv_name_hint)
> >>>> +        else:
> >>>> +            cipher_node.AddData("iv", self._iv)
> >>>> +
> >>>> +        if self._key or self._iv_name_hint:
> >>>> +            # add cipher node in root
> >>>> +            root_node = self._node.parent.parent.parent
> >>>
> >>> The root node is self.GetImage()._node
> >>>
> >>> But why are you adding something to the root node? This seems quite strange.
> >>
> >> This is shown in the example in the cover letter. The generated device
> >> tree looks like this:
> >>
> >> \ {
> >>          cipher {
> >>                  key-aes256-gcm-keyname {
> >>                          key = <0x...>;
> >>                          iv = <0x...>;
> >>                  };
> >>          };
> >>
> >>          images {
> >>                 ...
> >>                 some-bitstream {
> >>                          ...
> >>                          data = [...]
> >>                          cipher {
> >>                                  algo = "aes256-gcm";
> >>                                  key-name-hint = "keyname";
> >>                          };
> >>                  };
> >> ...
> >>
> >> The cipher node right below the root node (may) contain the actually
> >> used key and iv.
> >> The cipher node below the images node just points to the node inside the
> >> global cipher node. For this the values of the algo, key-name-hint and
> >> optionally the iv-name-hint are used.
> >> The actual key is found in /cipher/key-<algo>-<key-name-hint> or
> >> /cipher/key-<algo>-<key-name-hint>-<iv-name-hint>. This is implemented
> >> this way, because it is also used in boot/image-cipher.c.
> >
> > Oh so how about putting the 'cipher' node in some-bitstream instead,
> > or even just add the encryption info to the 'cipher' node you have
> > shown? Why does it need to be in the root node?
> >
> >>
> >> Side note:
> >> If the hardware/board already contains a key in some secure storage, it
> >> is not necessary to put the key into the device tree. In that case the
> >> property key-name-hint can be used to identify which key should be used
> >> for decryption.
> >
> > OK. In that case, perhaps we should have a property indicating that
> > the key is external, if that isn't obvious.
>
> It is not obvious now if the key is external, since it is just a magic
> string. I think the key source must support these use-cases:
>
> 1. Key is embedded in the device tree. If we do not have a global
> /cipher node anymore, there is no need for a key id. Basically having a
> key property containing the key itself would be enough in that case.
>
> For key embedded in the device tree this would be:
>
> some-bitstream {
>    ...
>    data = [...]
>    cipher {
>      algo = "aes256-gcm";
>      iv = <0x...>;
>      key = <0x...>;
>    };
> };

That looks good

>
> 2. The key is not embedded in the device tree. In that case some kind of
> id is needed to identify which key should be used. This id would be
> specific to the hardware that provides the key. Some kind of string
> would probably be good here.
>
> If the key from some external source is used it could be something like
> this:
>
> some-bitstream {
>    ...
>    data = [...]
>    cipher {
>      algo = "aes256-gcm";
>      iv = <0x...>;
>      key-source = "some-external-key-1";
>    };
> };
>
> Would this be ok (having either a key or key-source property)?

Seems good. I don't know what to use for 'key-source' but perhaps it
could be a phandle reference to the device that holds it? Or just a
string?

>
> >
> >>
> >>>
> >>>> +            name = "cipher"
> >>>> +            cipher_node = root_node.FindNode(name)
> >>>> +            if not cipher_node:
> >>>> +                cipher_node = state.AddSubnode(root_node, name)
> >>>> +            key_node_name = (
> >>>> +                f"key-{self._algo}-{self._key_name_hint}-{self._iv_name_hint}"
> >>>> +                if self._iv_name_hint
> >>>> +                else f"key-{self._algo}-{self._key_name_hint}"
> >>>> +            )
> >>>> +            key_node = cipher_node.FindNode(key_node_name)
> >>>> +            if not key_node:
> >>>
> >>> This behaviour is not clearly documented above.
> >>
> >> Should i document this in the class doc of this entry or in the method
> >> doc of gen_entries()?
> >
> > The class doc has the 'user' documentation, so it should go there. You
> > can copy in the info from your cover letter and also explain the
> > naming of the key node.
> >
> > I might have a few more comments once this is updated.
> >
> >>>
> >>>> +                key_node = state.AddSubnode(cipher_node, key_node_name)
> >>>> +                if self._key:
> >>>> +                    key_node.AddData("key", self._key)
> >>>> +                if self._iv_name_hint:
> >>>> +                    key_node.AddData("iv", self._iv)
> >>>> +
> >>>> +    def ObtainContents(self):
> >>>> +        # ensure that linked content is not added to the device tree again from this entry
> >>>> +        self.SetContents(b'')
> >>>> +        return True
> >>>> +
> >>>> +    def ProcessContents(self):
> >>>> +        # ensure that linked content is not added to the device tree again from this entry
> >>>> +        return self.ProcessContentsUpdate(b'')
> >>>> --
> >>>> 2.34.1
> >>>>
> > Regards,
> > Simon
>
> Regards,
> Christian

Regards,
Simon