diff mbox

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

Message ID 1496915565-29790-1-git-send-email-hs@denx.de
State RFC
Delegated to: Tom Rini
Headers show

Commit Message

Heiko Schocher June 8, 2017, 9:52 a.m. UTC
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@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:

Using FEC1 device
TFTP from server 192.168.1.1; our IP address is 192.168.20.103
Filename '/tftpboot/xxx/20170509/signed.fit'.
Load address: 0x42000000
Loading: #################################################################
[...]
         ###########
         3.3 MiB/s
done
Bytes transferred = 12560801 (bfa9a1 hex)
   Using 'conf@1' configuration
   Verifying Hash Integrity ... OK
   Trying 'kernel@1' kernel subimage
     Description:  Linux kernel
     Created:      2017-06-08   9:10:14 UTC
     Type:         Kernel Image
     Compression:  uncompressed
     Data Start:   0x420000c0
     Data Size:    4078928 Bytes = 3.9 MiB
     Architecture: ARM
     OS:           Linux
     Load Address: 0x40008000
     Entry Point:  0x40008000
     Hash algo:    sha256
     Hash value:   6d1dce3e08133ac4d34c0e07ce266f5cffc6f5a2713619c9ff76ca4b04df4a5b
     Sign algo:    sha256,rsa4096:dev
     Sign value:   xxx
     Timestamp:    2017-06-08   9:10:15 UTC
   Verifying Hash Integrity ... sha256+ sha256,rsa4096:dev- OK
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

   Using 'conf@1' configuration
   Trying 'ramdisk@1' ramdisk subimage
     Description:  miro ramdisk
     Created:      2017-06-08   9:10:14 UTC
     Type:         RAMDisk Image
     Compression:  gzip compressed
     Data Start:   0x423e92b0
     Data Size:    8457506 Bytes = 8.1 MiB
     Architecture: ARM
     OS:           Linux
     Load Address: 0x00000000
     Entry Point:  0x00000000
     Hash algo:    sha256
     Hash value:   da60884efa4373e7003940a56271c326f159ff74356ded28d8ebe108af807cda
     Sign algo:    sha256,rsa4096:dev
     Sign value:   xxx
     Timestamp:    2017-06-08   9:10:15 UTC
   Verifying Hash Integrity ... sha256+ sha256,rsa4096:dev- OK
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   Using 'conf@1' configuration
   Trying 'fdt@1' fdt subimage
     Description:  miro device tree
     Created:      2017-06-08   9:10:14 UTC
     Type:         Flat Device Tree
     Compression:  uncompressed
     Data Start:   0x423e41b4
     Data Size:    19852 Bytes = 19.4 KiB
     Architecture: ARM
     Hash algo:    sha256
     Hash value:   9b39c3ab6227bb8f0bcebc0bb64439248a6670dfe873bb1c6536764e0dc1623c
     Sign algo:    sha256,rsa4096:dev
     Sign value:   xxx
     Timestamp:    2017-06-08   9:10:15 UTC
   Verifying Hash Integrity ... sha256+ sha256,rsa4096:dev- OK
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   Booting using the fdt blob at 0x423e41b4
   Loading Kernel Image ... OK
   Loading Ramdisk to 47303000, end 47b13d22 ... OK
   Loading Device Tree to 472fb000, end 47302d8b ... OK

Starting kernel ...

[    0.000000] Booting Linux on physical CPU 0x0
[    0.000000] Linux version 4.11.0 (hs@hercules.denx.de) (gcc version 6.2.0 (GCC) ) #1 Tue May 16 07:31:30 CEST 2017
[...]

And linux boots ... which is bad, as we have no valid key appended
to u-boot.bin ...

With this patch it shows:

   Using 'conf@1' configuration
   Verifying Hash Integrity ... OK
   Trying 'kernel@1' kernel subimage
     Description:  Linux kernel
     Created:      2017-06-08   9:34:28 UTC
     Type:         Kernel Image
     Compression:  uncompressed
     Data Start:   0x420000c0
     Data Size:    4078928 Bytes = 3.9 MiB
     Architecture: ARM
     OS:           Linux
     Load Address: 0x40008000
     Entry Point:  0x40008000
     Hash algo:    sha256
     Hash value:   6d1dce3e08133ac4d34c0e07ce266f5cffc6f5a2713619c9ff76ca4b04df4a5b
     Sign algo:    sha256,rsa4096:dev
     Sign value:   xxx
     Timestamp:    2017-06-08   9:34:29 UTC
   Verifying Hash Integrity ... error!
Unable to verify required signature for '' hash node in 'kernel@1' image node
Bad Data Hash
ERROR: can't get kernel image!
=>


 common/image-sig.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Tom Rini June 9, 2017, 1:06 a.m. UTC | #1
On Thu, Jun 08, 2017 at 11:52:45AM +0200, Heiko Schocher 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@denx.de>

Good catch.  Did you confirm that the sandbox tests still work?  And can
you construct a new test for this case?  Thanks!
Simon Glass June 9, 2017, 3:05 a.m. UTC | #2
Hi Heiko,

On 8 June 2017 at 03:52, Heiko Schocher <hs@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@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:
>
> Using FEC1 device
> TFTP from server 192.168.1.1; our IP address is 192.168.20.103
> Filename '/tftpboot/xxx/20170509/signed.fit'.
> Load address: 0x42000000
> Loading: #################################################################
> [...]
>          ###########
>          3.3 MiB/s
> done
> Bytes transferred = 12560801 (bfa9a1 hex)
>    Using 'conf@1' configuration
>    Verifying Hash Integrity ... OK
>    Trying 'kernel@1' kernel subimage
>      Description:  Linux kernel
>      Created:      2017-06-08   9:10:14 UTC
>      Type:         Kernel Image
>      Compression:  uncompressed
>      Data Start:   0x420000c0
>      Data Size:    4078928 Bytes = 3.9 MiB
>      Architecture: ARM
>      OS:           Linux
>      Load Address: 0x40008000
>      Entry Point:  0x40008000
>      Hash algo:    sha256
>      Hash value:   6d1dce3e08133ac4d34c0e07ce266f5cffc6f5a2713619c9ff76ca4b04df4a5b
>      Sign algo:    sha256,rsa4096:dev
>      Sign value:   xxx
>      Timestamp:    2017-06-08   9:10:15 UTC
>    Verifying Hash Integrity ... sha256+ sha256,rsa4096:dev- OK
>                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>    Using 'conf@1' configuration
>    Trying 'ramdisk@1' ramdisk subimage
>      Description:  miro ramdisk
>      Created:      2017-06-08   9:10:14 UTC
>      Type:         RAMDisk Image
>      Compression:  gzip compressed
>      Data Start:   0x423e92b0
>      Data Size:    8457506 Bytes = 8.1 MiB
>      Architecture: ARM
>      OS:           Linux
>      Load Address: 0x00000000
>      Entry Point:  0x00000000
>      Hash algo:    sha256
>      Hash value:   da60884efa4373e7003940a56271c326f159ff74356ded28d8ebe108af807cda
>      Sign algo:    sha256,rsa4096:dev
>      Sign value:   xxx
>      Timestamp:    2017-06-08   9:10:15 UTC
>    Verifying Hash Integrity ... sha256+ sha256,rsa4096:dev- OK
>                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>    Using 'conf@1' configuration
>    Trying 'fdt@1' fdt subimage
>      Description:  miro device tree
>      Created:      2017-06-08   9:10:14 UTC
>      Type:         Flat Device Tree
>      Compression:  uncompressed
>      Data Start:   0x423e41b4
>      Data Size:    19852 Bytes = 19.4 KiB
>      Architecture: ARM
>      Hash algo:    sha256
>      Hash value:   9b39c3ab6227bb8f0bcebc0bb64439248a6670dfe873bb1c6536764e0dc1623c
>      Sign algo:    sha256,rsa4096:dev
>      Sign value:   xxx
>      Timestamp:    2017-06-08   9:10:15 UTC
>    Verifying Hash Integrity ... sha256+ sha256,rsa4096:dev- OK
>                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>    Booting using the fdt blob at 0x423e41b4
>    Loading Kernel Image ... OK
>    Loading Ramdisk to 47303000, end 47b13d22 ... OK
>    Loading Device Tree to 472fb000, end 47302d8b ... OK
>
> Starting kernel ...
>
> [    0.000000] Booting Linux on physical CPU 0x0
> [    0.000000] Linux version 4.11.0 (hs@hercules.denx.de) (gcc version 6.2.0 (GCC) ) #1 Tue May 16 07:31:30 CEST 2017
> [...]
>
> And linux boots ... which is bad, as we have no valid key appended
> to u-boot.bin ...
>
> With this patch it shows:
>
>    Using 'conf@1' configuration
>    Verifying Hash Integrity ... OK
>    Trying 'kernel@1' kernel subimage
>      Description:  Linux kernel
>      Created:      2017-06-08   9:34:28 UTC
>      Type:         Kernel Image
>      Compression:  uncompressed
>      Data Start:   0x420000c0
>      Data Size:    4078928 Bytes = 3.9 MiB
>      Architecture: ARM
>      OS:           Linux
>      Load Address: 0x40008000
>      Entry Point:  0x40008000
>      Hash algo:    sha256
>      Hash value:   6d1dce3e08133ac4d34c0e07ce266f5cffc6f5a2713619c9ff76ca4b04df4a5b
>      Sign algo:    sha256,rsa4096:dev
>      Sign value:   xxx
>      Timestamp:    2017-06-08   9:34:29 UTC
>    Verifying Hash Integrity ... error!
> Unable to verify required signature for '' hash node in 'kernel@1' image node
> Bad Data Hash
> ERROR: can't get kernel image!
> =>
>
>
>  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.

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.

As Tom said, can you add a test please?

>         }
>
>         fdt_for_each_subnode(noffset, sig_blob, sig_node) {
> --
> 2.7.4
>

Regards,
Simon
diff mbox

Patch

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;
 	}
 
 	fdt_for_each_subnode(noffset, sig_blob, sig_node) {