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

[Taedcke, Christian]

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?

> 
>> +        - iv-name-hint: The name hint for the iv
> 
> what is the iv?

Initialization Vector. Should i use the full name here?

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

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

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.

> 
>> +            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()?
> 
>> +                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