u-boot: signature check for u-boot scripts

[Heiko Schocher]

Hello Simon,

On 13.01.23 00: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?

If you have a fitimage with only one script image ... it is a little
bit overhead to need also a configurations section ... I made this
mistake ...

I have a public key in u-boot.dtb which has 'required = "conf";'
so only bad signed "configurations" are stopped from booting or
in case of script from sourcing ... a bad signed fitimage with
only signed images still gets executed ... also a not signed fitimage
got executed, because I called "source <loadaddr>"  ... so at all
user error or may this is all just theoretical problems, as
in case of secure boot, you should not allow to enter commandline
at all, and of course you need to use protected environment
in such a setup...

I could not add simply another public key
with required = "image";

else on "boot of kernel fitimage", it needs signatures for
all images in images section...

But I am fine with adding a configurations node in the its for
the bootscript.

Sorry for the confusion.

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de