spl: allow board_spl_fit_post_load() to fail

spl: allow board_spl_fit_post_load() to fail

[Patrick Wildt]

On i.MX platforms board_spl_fit_post_load() can check the loaded
SPL image for authenticity using its HAB engine.  U-Boot's SPL
mechanism allows booting images from other sources as well, but
in the current setup the SPL would just hang if it encounters an
image that does not pass scrutiny.  Allowing the function to return
an error, allows the SPL to try booting from another source as a
fallback instead of ending up as a brick.

Signed-off-by: Patrick Wildt <patrick@blueri.se>

diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
index fd3fa04600..b8f6fcb4df 100644
--- a/arch/arm/mach-imx/spl.c
+++ b/arch/arm/mach-imx/spl.c
@@ -311,7 +311,7 @@ ulong board_spl_fit_size_align(ulong size)
 	return size;
 }
 
-void board_spl_fit_post_load(ulong load_addr, size_t length)
+int board_spl_fit_post_load(ulong load_addr, size_t length)
 {
 	u32 offset = length - CONFIG_CSF_SIZE;
 
@@ -319,8 +319,10 @@ void board_spl_fit_post_load(ulong load_addr, size_t length)
 				       offset + IVT_SIZE + CSF_PAD_SIZE,
 				       offset)) {
 		puts("spl: ERROR:  image authentication unsuccessful\n");
-		hang();
+		return -1;
 	}
+
+	return 0;
 }
 #endif
 
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index c51e4beb1c..21c873c5fb 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -24,8 +24,9 @@ DECLARE_GLOBAL_DATA_PTR;
 #define CONFIG_SYS_BOOTM_LEN	(64 << 20)
 #endif
 
-__weak void board_spl_fit_post_load(ulong load_addr, size_t length)
+__weak int board_spl_fit_post_load(ulong load_addr, size_t length)
 {
+	return 0;
 }
 
 __weak ulong board_spl_fit_size_align(ulong size)
@@ -678,7 +679,9 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	spl_image->flags |= SPL_FIT_FOUND;
 
 #ifdef CONFIG_IMX_HAB
-	board_spl_fit_post_load((ulong)fit, size);
+	ret = board_spl_fit_post_load((ulong)fit, size);
+	if (ret)
+		return ret;
 #endif
 
 	return 0;
diff --git a/include/spl.h b/include/spl.h
index 6bf9fd8beb..93d5a5a1f3 100644
--- a/include/spl.h
+++ b/include/spl.h
@@ -560,7 +560,7 @@ int board_return_to_bootrom(struct spl_image_info *spl_image,
  * board_spl_fit_post_load - allow process images after loading finished
  *
  */
-void board_spl_fit_post_load(ulong load_addr, size_t length);
+int board_spl_fit_post_load(ulong load_addr, size_t length);
 
 /**
  * board_spl_fit_size_align - specific size align before processing payload

spl: allow board_spl_fit_post_load() to fail

[Heinrich Schuchardt]

On 5/9/20 6:13 PM, Patrick Wildt wrote:
> On i.MX platforms board_spl_fit_post_load() can check the loaded
> SPL image for authenticity using its HAB engine.  U-Boot's SPL
> mechanism allows booting images from other sources as well, but
> in the current setup the SPL would just hang if it encounters an
> image that does not pass scrutiny.  Allowing the function to return
> an error, allows the SPL to try booting from another source as a
> fallback instead of ending up as a brick.
>
> Signed-off-by: Patrick Wildt <patrick@blueri.se>

Could an intruder abuse this by destroying a signed image and providing
an unsigned image on a source under his control?

Best regards

Heinrich

spl: allow board_spl_fit_post_load() to fail

[Patrick Wildt]

On Sat, May 09, 2020 at 06:38:33PM +0200, Heinrich Schuchardt wrote:
> On 5/9/20 6:13 PM, Patrick Wildt wrote:
> > On i.MX platforms board_spl_fit_post_load() can check the loaded
> > SPL image for authenticity using its HAB engine.  U-Boot's SPL
> > mechanism allows booting images from other sources as well, but
> > in the current setup the SPL would just hang if it encounters an
> > image that does not pass scrutiny.  Allowing the function to return
> > an error, allows the SPL to try booting from another source as a
> > fallback instead of ending up as a brick.
> >
> > Signed-off-by: Patrick Wildt <patrick@blueri.se>
> 
> Could an intruder abuse this by destroying a signed image and providing
> an unsigned image on a source under his control?
> 
> Best regards
> 
> Heinrich

Sure, let's think about it here.  Maybe you have some more thoughts to
add.

First of all, the SPL goes through all the boot devices, and if there's
none to find with an image, it will hang.  It will hang like it does
before the diff.  The only difference is that it tries additional
sources before hanging.  Thus the attacker, unless he can exploit it
in his first trial, or is able to force a reset, must have some access
to reset the machine to have it boot and try  again.  This seems like he
must have some kind of local or remote phyiscal access.

Let's assume the image is on the network or on another remote medium.
Then I guess the attacker will just try to attack that medium, and the
alternate boot sources won't make a difference.

I guess that means we should focus on local sources.  I think we can
also ignore a removable SD card, since he can just put in another one
and try again.

So maybe let's think about SPI flash and eMMC, soldered on, not directly
accessible.  If he has physical access, I guess he could open up the box
and desolder a few pins, or add some voltage here and there to try and
disrupt the bootup.  But, then maybe it's easier to just desolder the
whole SPI/eMMC and add his own.

But what if he doesn't have that access?  If he's remote?  Ok, he will
probably have to exploit the daemon (webserver or whatever) to gain some
code execution.  Then he'll try and become root, so he can access the
disks.  Then I figure he'll try and overwrite or remove the image.
With this, on the next reboot it will (hopefully) fail to boot, unless
he already has an exploit, then my patch won't make a difference.

I figure the real issue could be that when the attacker has physical
access, manages to remove/replace the image with a fallback to load from
a device like an SD card, that it's now easier for him to try and find
an exploit.

Am I missing something?

Best regards,
Patrick

spl: allow board_spl_fit_post_load() to fail

[Heinrich Schuchardt]

On 5/9/20 7:45 PM, Patrick Wildt wrote:
> On Sat, May 09, 2020 at 06:38:33PM +0200, Heinrich Schuchardt wrote:
>> On 5/9/20 6:13 PM, Patrick Wildt wrote:
>>> On i.MX platforms board_spl_fit_post_load() can check the loaded
>>> SPL image for authenticity using its HAB engine.  U-Boot's SPL
>>> mechanism allows booting images from other sources as well, but
>>> in the current setup the SPL would just hang if it encounters an
>>> image that does not pass scrutiny.  Allowing the function to return
>>> an error, allows the SPL to try booting from another source as a
>>> fallback instead of ending up as a brick.
>>>
>>> Signed-off-by: Patrick Wildt <patrick@blueri.se>
>>
>> Could an intruder abuse this by destroying a signed image and providing
>> an unsigned image on a source under his control?
>>
>> Best regards
>>
>> Heinrich
>
> Sure, let's think about it here.  Maybe you have some more thoughts to
> add.
>
> First of all, the SPL goes through all the boot devices, and if there's
> none to find with an image, it will hang.  It will hang like it does
> before the diff.  The only difference is that it tries additional
> sources before hanging.  Thus the attacker, unless he can exploit it
> in his first trial, or is able to force a reset, must have some access
> to reset the machine to have it boot and try  again.  This seems like he
> must have some kind of local or remote phyiscal access.
>
> Let's assume the image is on the network or on another remote medium.
> Then I guess the attacker will just try to attack that medium, and the
> alternate boot sources won't make a difference.
>
> I guess that means we should focus on local sources.  I think we can
> also ignore a removable SD card, since he can just put in another one
> and try again.
>
> So maybe let's think about SPI flash and eMMC, soldered on, not directly
> accessible.  If he has physical access, I guess he could open up the box
> and desolder a few pins, or add some voltage here and there to try and
> disrupt the bootup.  But, then maybe it's easier to just desolder the
> whole SPI/eMMC and add his own.
>
> But what if he doesn't have that access?  If he's remote?  Ok, he will
> probably have to exploit the daemon (webserver or whatever) to gain some
> code execution.  Then he'll try and become root, so he can access the
> disks.  Then I figure he'll try and overwrite or remove the image.
> With this, on the next reboot it will (hopefully) fail to boot, unless
> he already has an exploit, then my patch won't make a difference.
>
> I figure the real issue could be that when the attacker has physical
> access, manages to remove/replace the image with a fallback to load from
> a device like an SD card, that it's now easier for him to try and find
> an exploit.

This last scenario is what I was thinking of.

So if HAB is used it may be ok to search all devices for signed images
but non-signed images should be rejected. Is that given with your patch?

Best regards

Heinrich

>
> Am I missing something?
>
> Best regards,
> Patrick
>

spl: allow board_spl_fit_post_load() to fail

[Patrick Wildt]

On Sat, May 09, 2020 at 08:09:39PM +0200, Heinrich Schuchardt wrote:
> On 5/9/20 7:45 PM, Patrick Wildt wrote:
> > On Sat, May 09, 2020 at 06:38:33PM +0200, Heinrich Schuchardt wrote:
> >> On 5/9/20 6:13 PM, Patrick Wildt wrote:
> >>> On i.MX platforms board_spl_fit_post_load() can check the loaded
> >>> SPL image for authenticity using its HAB engine.  U-Boot's SPL
> >>> mechanism allows booting images from other sources as well, but
> >>> in the current setup the SPL would just hang if it encounters an
> >>> image that does not pass scrutiny.  Allowing the function to return
> >>> an error, allows the SPL to try booting from another source as a
> >>> fallback instead of ending up as a brick.
> >>>
> >>> Signed-off-by: Patrick Wildt <patrick@blueri.se>
> >>
> >> Could an intruder abuse this by destroying a signed image and providing
> >> an unsigned image on a source under his control?
> >>
> >> Best regards
> >>
> >> Heinrich
> >
> > Sure, let's think about it here.  Maybe you have some more thoughts to
> > add.
> >
> > First of all, the SPL goes through all the boot devices, and if there's
> > none to find with an image, it will hang.  It will hang like it does
> > before the diff.  The only difference is that it tries additional
> > sources before hanging.  Thus the attacker, unless he can exploit it
> > in his first trial, or is able to force a reset, must have some access
> > to reset the machine to have it boot and try  again.  This seems like he
> > must have some kind of local or remote phyiscal access.
> >
> > Let's assume the image is on the network or on another remote medium.
> > Then I guess the attacker will just try to attack that medium, and the
> > alternate boot sources won't make a difference.
> >
> > I guess that means we should focus on local sources.  I think we can
> > also ignore a removable SD card, since he can just put in another one
> > and try again.
> >
> > So maybe let's think about SPI flash and eMMC, soldered on, not directly
> > accessible.  If he has physical access, I guess he could open up the box
> > and desolder a few pins, or add some voltage here and there to try and
> > disrupt the bootup.  But, then maybe it's easier to just desolder the
> > whole SPI/eMMC and add his own.
> >
> > But what if he doesn't have that access?  If he's remote?  Ok, he will
> > probably have to exploit the daemon (webserver or whatever) to gain some
> > code execution.  Then he'll try and become root, so he can access the
> > disks.  Then I figure he'll try and overwrite or remove the image.
> > With this, on the next reboot it will (hopefully) fail to boot, unless
> > he already has an exploit, then my patch won't make a difference.
> >
> > I figure the real issue could be that when the attacker has physical
> > access, manages to remove/replace the image with a fallback to load from
> > a device like an SD card, that it's now easier for him to try and find
> > an exploit.
> 
> This last scenario is what I was thinking of.
> 
> So if HAB is used it may be ok to search all devices for signed images
> but non-signed images should be rejected. Is that given with your patch?
> 
> Best regards
> 
> Heinrich

I think you have a very good point there.  No, with that diff I think
there's an issue, but I have added a second diff here.  I'll explain.

i.MX's jump_to_image_no_args() has a check:

        if (spl_image->flags & SPL_FIT_FOUND) {
                image_entry();
        } else {
                [...]
                if (!imx_hab_authenticate_image(spl_image->load_addr,

That means that it will jump right away if the flags say SPL_FIT_FOUND.
Unfortunately my diff sets that flag before returning an error, and
that means if the fallback tried to boot a non-FIT, it will skip the
HAB check and jump right away.  I have also not (yet) seen code that
resets the flag.

I figure we should just set the flag after doing the post load.  Since
it's a "fit_post_load"-function, we can assume that that function does
not depend on the SPL_FIT_FOUND flag to do its work.  I mean, it's only
called because it parses a FIT in the first place.

Thanks for making me think about this again!

Best regards,
Patrick

diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
index fd3fa04600..b8f6fcb4df 100644
--- a/arch/arm/mach-imx/spl.c
+++ b/arch/arm/mach-imx/spl.c
@@ -311,7 +311,7 @@ ulong board_spl_fit_size_align(ulong size)
 	return size;
 }
 
-void board_spl_fit_post_load(ulong load_addr, size_t length)
+int board_spl_fit_post_load(ulong load_addr, size_t length)
 {
 	u32 offset = length - CONFIG_CSF_SIZE;
 
@@ -319,8 +319,10 @@ void board_spl_fit_post_load(ulong load_addr, size_t length)
 				       offset + IVT_SIZE + CSF_PAD_SIZE,
 				       offset)) {
 		puts("spl: ERROR:  image authentication unsuccessful\n");
-		hang();
+		return -1;
 	}
+
+	return 0;
 }
 #endif
 
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index c51e4beb1c..7e9aff9240 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -24,8 +24,9 @@ DECLARE_GLOBAL_DATA_PTR;
 #define CONFIG_SYS_BOOTM_LEN	(64 << 20)
 #endif
 
-__weak void board_spl_fit_post_load(ulong load_addr, size_t length)
+__weak int board_spl_fit_post_load(ulong load_addr, size_t length)
 {
+	return 0;
 }
 
 __weak ulong board_spl_fit_size_align(ulong size)
@@ -675,11 +676,12 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	if (spl_image->entry_point == FDT_ERROR || spl_image->entry_point == 0)
 		spl_image->entry_point = spl_image->load_addr;
 
-	spl_image->flags |= SPL_FIT_FOUND;
-
 #ifdef CONFIG_IMX_HAB
-	board_spl_fit_post_load((ulong)fit, size);
+	ret = board_spl_fit_post_load((ulong)fit, size);
+	if (ret)
+		return ret;
 #endif
 
+	spl_image->flags |= SPL_FIT_FOUND;
 	return 0;
 }
diff --git a/include/spl.h b/include/spl.h
index 6bf9fd8beb..93d5a5a1f3 100644
--- a/include/spl.h
+++ b/include/spl.h
@@ -560,7 +560,7 @@ int board_return_to_bootrom(struct spl_image_info *spl_image,
  * board_spl_fit_post_load - allow process images after loading finished
  *
  */
-void board_spl_fit_post_load(ulong load_addr, size_t length);
+int board_spl_fit_post_load(ulong load_addr, size_t length);
 
 /**
  * board_spl_fit_size_align - specific size align before processing payload