u-boot: signature check for u-boot scripts

[Sean Anderson]

On 1/12/23 18:43, Simon Glass wrote:
> Hi,
> 
> On Thu, 12 Jan 2023 at 09:03, Sean Anderson <sean.anderson at seco.com> wrote:
>>
>> On 1/11/23 01:13, Heiko Schocher wrote:
>> > Hello Sean,
>> >
>> > Thanks for your answer!
>> >
>> > On 10.01.23 17:27, Sean Anderson wrote:
>> >> On 1/10/23 08:18, Heiko Schocher wrote:
>> >>> Hello Simon,
>> > [...]
>> >>> While writting this email ... in [3] the line
>> >>>
>> >>>  require = "conf"
>> >>>
>> >>> poped into my eyes .... and in fit_image_verify_required_sigs() there is check:
>> >>>
>> >>>                 if (!required || strcmp(required, "image"))
>> >>>                         continue;
>> >>>
>> >>> and yes! changing in [3]
>> >>>
>> >>> -required = "conf";
>> >>> +required = "image";
>> >>>
>> >>> makes sourcing the signed script working (error in case of no
>> >>> signature or wrong signature)! ... but booting the signed fitimage
>> >>> now breaks ... so it seems, I cannot use configuration signing with
>> >>> images signing ?
>> >>>
>> >>> I tried to add two key nodes in signature node of u-boot dtb ... one with
>> >>> require = "conf" and one with require = "image" ... but no luck...
>> >>>
>> >>> Also adding a configurations section to scripts its file did not helped
>> >>> (which will not prevent the problem sourcing a not signed script)
>> >>
>> >> As you discovered, you must either have required = "image", in which case
>> >>
>> >>     source :
>> >>
>> >> will be secure. Otherwise, you must use
>> >>
>> >>     source \#
>> >>
>> >> Any other way is not secure.
>> >
>> > My "hack" checks a configuration signature in fitimage with script in it...
>> > so also "secure" ...
>> >
>> > BTW: why we need a env variable to enable checking in cmd/source.c?
>> >      I would say, if verify fit images is enabled we always should check
>> >      signature ... but this is another question...
>>
>> I think it's to allow disabling things for debugging. If the variable
>> does not exist, it defaults to verifying.
>>
>> > So I tried your suggestion:
>> >
>> > => tftp 100000 script.bin.signed;setenv verify 1;source \#100000
>> > Speed: 1000, full duplex
>> > Using ethernet at 24000 device
>> > TFTP from server 192.168.3.1; our IP address is 192.168.3.40
>> > Filename 'script.bin.signed'.
>> > Load address: 0x100000
>> > Loading: #
>> >          233.4 KiB/s
>> > done
>> > Bytes transferred = 1679 (68f hex)
>> > ## Executing script at 00000000
>> > Wrong image format for "source" command
>> > =>
>> >
>> > same for
>> >
>> > => source \#100000:script-1
>> > ## Executing script at 00000000
>> > Wrong image format for "source" command
>> > =>
>> >
>> > Which is the error message from the switch in image_source_script()
>> > from cmd/source.c ...
>>
>> Right.
>>
>> What kind of image is your script? Do you have CONFIG_FIT (and *only*
>> CONFIG_FIT) enabled?
>>
>> > (check if fitimage "is okay"):
>> > => source 100000
>> > ## Executing script at 00100000
>> > sha256+ sha256,rsa2048:dev+ Hallo from script
>> > => source 100000:script-1
>> > ## Executing script at 00100000
>> > sha256+ sha256,rsa2048:dev+ Hallo from script
>> > => source 100000:script-2
>> > ## Executing script at 00100000
>> > Can't find 'script-2' FIT subimage
>> > =>
>> >
>> > and changing hash in fitimages signature leads to:
>> > => mw 1001c0 0 1
>> > => source 100000:script-1
>> > ## Executing script at 00100000
>> > sha256+ sha256,rsa2048:dev- Hallo from script
>> > =>
>> >
>> > As I described ... problem "hash is detected, but script is executed",
>> > as public key in u-boots dtb has required = "conf"; (as it is used also
>> > for fitimage boot, where we use conf signing)
>> >
>> > May you have an example (u-boot.dtb, its and complete working command
>> > for a signed fitimage script)?
>>
>> $ cat << EOF >> dm-verity.its
>> /dts-v1/;
>>
>> / {
>>     description = "dm-verity boot parameters";
>>     #address-cells = <1>;
>>
>>     images {
>>         dm-verity {
>>             data = /incbin/("dm-verity.scr");
>>             type = "script";
>>             arch = "arm64";
>>             compression = "none";
>>             hash-1 {
>>                 algo = "sha256";
>>             };
>>         };
>>     };
>>
>>     configurations {
>>         default = "conf";
>>         conf {
>>             description = "Load dm-verity boot parameters";
>>             script = "dm-verity";
>>             signature {
>>                 algo = "sha256,rsa2048";
>>                 key-name-hint = "u-boot";
>>                 sign-images = "script";
>>             };
>>         };
>>     };
>> };
>> EOF
>> $ uboot-mkimage -EB 0x40 -f dm-verity.its dm-verity.itb
>> $ uboot-mkimage -EB 0x40 -r -F -k /path/to/keys dm-verity.itb
>>
>> > The main problem is (I think) that we check for fitimages which are
>> > used for booting kernels, a "signed configuration" and in fitimage for scripts
>> > only "image" signatures ... and a combination of both is not possible
>> > (except I also sign the image nodes in kernel fitimage too ... which
>> > than leads in checking configuration signature and image signature on
>> > boot... but may a way to go (and disabling hash check) ?
>>
>> Yes, which is why you need to have # in the source command to force only
>> using configurations. IMO we should also check image-only FITs, but
>> Simon disagrees.
> 
> I'm not sure what I disagree with, exactly. Signing images leaves one
> open to a mix-and-match attack, so I believe it is better to put the
> script into the config so it is signed along with the kernel, etc. But
> I am fine with us supporting signed images containing scripts...in
> fact I thought we already did?

I suppose we do, but we just call fit_image_verify which works only if
you have `required = "images"`. But this is problematic for boot images,
so you need `required = "images", "configs"` and end up signing both
(not the worst I suppose).

--Sean