[U-Boot] [PATCH] common, image-sig: [BUG?] if no valid signature node found, do not boot signed FIT image

[Simon Glass]

Hi Heiko,

On 8 June 2017 at 22:52, Heiko Schocher <hs at denx.de> wrote:
> Hello Simon,
>
> Am 09.06.2017 um 05:05 schrieb Simon Glass:
>>
>> Hi Heiko,
>>
>> On 8 June 2017 at 03:52, Heiko Schocher <hs at denx.de> wrote:
>>>
>>> fit_image_verify_required_sigs() must return != 0, on error.
>>>
>>> When fit_image_verify_required_sigs() does not find a signature
>>> node, it returns 0, which leads in booting a signed FIT image.
>>>
>>> Fix this!
>>>
>>> Signed-off-by: Heiko Schocher <hs at denx.de>
>>> ---
>>>
>>> Found on an imx28 based board, with key dtb appended to u-boot.bin.
>>>
>>> Booting signed FIT image without an valid key dtb appended to u-boot.bin
>>> shows:
>
> [...]
>>>
>>>   common/image-sig.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/common/image-sig.c b/common/image-sig.c
>>> index 455f2b9..646fb08 100644
>>> --- a/common/image-sig.c
>>> +++ b/common/image-sig.c
>>> @@ -265,7 +265,7 @@ int fit_image_verify_required_sigs(const void *fit,
>>> int image_noffset,
>>>          if (sig_node < 0) {
>>>                  debug("%s: No signature node found: %s\n", __func__,
>>>                        fdt_strerror(sig_node));
>>> -               return 0;
>>> +               return 1;
>>
>>
>> Thanks for finding/fixing this! I suggest returning -EPERM.
>
>
> Ok, changed.
>
>> Also note that using image-based security is somewhat insecure since
>> people can mix and match them. Configuration signing is preferred if
>> you can do it.
>
>
> I do this, here my configurations node from the its file:
>
>         configurations {
>                 default = "conf at 1";
>                 conf at 1 {
>                         description = "board config 1";
>                         kernel = "kernel at 1";
>                         fdt = "fdt at 1";
>                         ramdisk = "ramdisk at 1";
>                         signature at 1 {
>                                 algo = "sha256,rsa4096";
>                                 key-name-hint = "dev";
>                         };
>                 };
>         };
>
>> As Tom said, can you add a test please?
>
>
> Hmm... tried with current U-Boot, the steps described in
>
> test/image/test-fit.py
>
> # make O=sandbox sandbox_config
> # make O=sandbox
> # ./test/image/test-fit.py -u sandbox/u-boot
>
> and get:
>
> pollux:u-boot hs [master] $ ./test/image/test-fit.py -u sandbox/u-boot
> FIT Tests
> =========
[...]

> Traceback (most recent call last):
>   File "./test/image/test-fit.py", line 481, in <module>
>     run_tests()
>   File "./test/image/test-fit.py", line 470, in run_tests
>     run_fit_test(mkimage, options.u_boot)
>   File "./test/image/test-fit.py", line 388, in run_fit_test
>     fail('Kernel not loaded', stdout)
>   File "./test/image/test-fit.py", line 306, in fail
>     raise ValueError("Test '%s' failed: %s" % (test_name, msg))
> ValueError: Test 'Kernel load' failed: Kernel not loaded
> pollux:u-boot hs [master] $
>
> Can you verify this?
>

Yes I see that too. I bisected it and sent a patch.

Regards,
Simon

> Thanks!
>
> bye,
> Heiko
>
>>
>>>          }
>>>
>>>          fdt_for_each_subnode(noffset, sig_blob, sig_node) {
>>> --
>>> 2.7.4
>>>
>>
>> Regards,
>> Simon
>>
>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany