[PATCH] image: Control FIT signature verification at runtime

[PATCH] image: Control FIT signature verification at runtime

[Andrew Jeffery]

Some platform designs include support for disabling secure-boot via a
jumper on the board. Sometimes this control can be separate from the
mechanism enabling the root-of-trust for the platform. Add support for
this latter scenario by allowing boards to implement
board_fit_image_require_verfied(), which is then invoked in the usual
FIT verification paths.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
Hi,

This patch is extracted from and motivated by a series adding run-time
control of FIT signature verification to u-boot in OpenBMC:

https://lore.kernel.org/openbmc/20220131012538.73021-1-andrew@aj.id.au/

Unfortunately the OpenBMC u-boot tree is quite a way behind on tracking
upstream and contains a bunch of out-of-tree work as well. As such I'm
looking to upstream the couple of changes that make sense against
master.

Please take a look!

Andrew

 boot/Kconfig     |  8 ++++++++
 boot/image-fit.c | 21 +++++++++++++++++----
 include/image.h  |  9 +++++++++
 3 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/boot/Kconfig b/boot/Kconfig
index c8d5906cd304..ec413151fd5a 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -78,6 +78,14 @@ config FIT_SIGNATURE
 	  format support in this case, enable it using
 	  CONFIG_LEGACY_IMAGE_FORMAT.
 
+if FIT_SIGNATURE
+config FIT_RUNTIME_SIGNATURE
+	bool "Control verification of FIT uImages at runtime"
+	help
+	  This option allows board support to disable verification of
+	  signatures at runtime, for example through the state of a GPIO.
+endif # FIT_SIGNATURE
+
 config FIT_SIGNATURE_MAX_SIZE
 	hex "Max size of signed FIT structures"
 	depends on FIT_SIGNATURE
diff --git a/boot/image-fit.c b/boot/image-fit.c
index f01cafe4e277..919dbfa4ee1d 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -1308,6 +1308,14 @@ static int fit_image_check_hash(const void *fit, int noffset, const void *data,
 	return 0;
 }
 
+#ifndef __weak
+#define __weak
+#endif
+__weak int board_fit_image_require_verified(void)
+{
+	return 1;
+}
+
 int fit_image_verify_with_data(const void *fit, int image_noffset,
 			       const void *key_blob, const void *data,
 			       size_t size)
@@ -1319,6 +1327,7 @@ int fit_image_verify_with_data(const void *fit, int image_noffset,
 
 	/* Verify all required signatures */
 	if (FIT_IMAGE_ENABLE_VERIFY &&
+	    fit_image_require_verified() &&
 	    fit_image_verify_required_sigs(fit, image_noffset, data, size,
 					   key_blob, &verify_all)) {
 		err_msg = "Unable to verify required signature";
@@ -1340,9 +1349,11 @@ int fit_image_verify_with_data(const void *fit, int image_noffset,
 						 &err_msg))
 				goto error;
 			puts("+ ");
-		} else if (FIT_IMAGE_ENABLE_VERIFY && verify_all &&
-				!strncmp(name, FIT_SIG_NODENAME,
-					strlen(FIT_SIG_NODENAME))) {
+		} else if (FIT_IMAGE_ENABLE_VERIFY &&
+			   fit_image_require_verified() &&
+			   verify_all &&
+			   !strncmp(name, FIT_SIG_NODENAME,
+				    strlen(FIT_SIG_NODENAME))) {
 			ret = fit_image_check_sig(fit, noffset, data, size,
 						  gd_fdt_blob(), -1, &err_msg);
 
@@ -2061,7 +2072,9 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
 		if (image_type == IH_TYPE_KERNEL)
 			images->fit_uname_cfg = fit_base_uname_config;
 
-		if (FIT_IMAGE_ENABLE_VERIFY && images->verify) {
+		if (FIT_IMAGE_ENABLE_VERIFY &&
+		    fit_image_require_verified() &&
+		    images->verify) {
 			puts("   Verifying Hash Integrity ... ");
 			if (fit_config_verify(fit, cfg_noffset)) {
 				puts("Bad Data Hash\n");
diff --git a/include/image.h b/include/image.h
index 97e5f2eb24d6..98900c2e839b 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1173,6 +1173,15 @@ int calculate_hash(const void *data, int data_len, const char *algo,
 # define FIT_IMAGE_ENABLE_VERIFY	CONFIG_IS_ENABLED(FIT_SIGNATURE)
 #endif
 
+/*
+ * Further, allow run-time control of verification, e.g. via a jumper
+ */
+#if defined(CONFIG_FIT_RUNTIME_SIGNATURE)
+# define fit_image_require_verified()	board_fit_image_require_verified()
+#else
+# define fit_image_require_verified()	FIT_IMAGE_ENABLE_VERIFY
+#endif
+
 #ifdef USE_HOSTCC
 void *image_get_host_blob(void);
 void image_set_host_blob(void *host_blob);
-- 
2.32.0

RE: [PATCH] image: Control FIT signature verification at runtime

[ChiaWei Wang]

Hi Andrew,

I am curious about the usage scenario.
Is the runtime control required for production release?
As this control acts like a backdoor to bypass the chain-of-trust.
If it is for debugging/development purposes, should we encourage the use of unsigned images under RD environments?
Beyond this, I have no concern as the patch provides more flexibility.

> From: Andrew Jeffery <andrew@aj.id.au>
> Sent: Monday, January 31, 2022 11:42 AM
> 
> Some platform designs include support for disabling secure-boot via a jumper
> on the board. Sometimes this control can be separate from the mechanism
> enabling the root-of-trust for the platform. Add support for this latter scenario
> by allowing boards to implement board_fit_image_require_verfied(), which is
> then invoked in the usual FIT verification paths.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> Hi,
> 
> This patch is extracted from and motivated by a series adding run-time control
> of FIT signature verification to u-boot in OpenBMC:
> 
> https://lore.kernel.org/openbmc/20220131012538.73021-1-andrew@aj.id.au/
> 
> Unfortunately the OpenBMC u-boot tree is quite a way behind on tracking
> upstream and contains a bunch of out-of-tree work as well. As such I'm looking
> to upstream the couple of changes that make sense against master.
> 
> Please take a look!
> 
> Andrew
> 
>  boot/Kconfig     |  8 ++++++++
>  boot/image-fit.c | 21 +++++++++++++++++----  include/image.h  |  9
> +++++++++
>  3 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/boot/Kconfig b/boot/Kconfig index c8d5906cd304..ec413151fd5a
> 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -78,6 +78,14 @@ config FIT_SIGNATURE
>  	  format support in this case, enable it using
>  	  CONFIG_LEGACY_IMAGE_FORMAT.
> 
> +if FIT_SIGNATURE
> +config FIT_RUNTIME_SIGNATURE
> +	bool "Control verification of FIT uImages at runtime"
> +	help
> +	  This option allows board support to disable verification of
> +	  signatures at runtime, for example through the state of a GPIO.
> +endif # FIT_SIGNATURE
> +

Using "depends on" might be preferred for Kconfig dependency.

Regards,
Chiawei

Re: [PATCH] image: Control FIT signature verification at runtime

[Andrew Jeffery]

On Mon, 7 Feb 2022, at 11:37, ChiaWei Wang wrote:
> Hi Andrew,
>
> I am curious about the usage scenario.
> Is the runtime control required for production release?

Yes.

> As this control acts like a backdoor to bypass the chain-of-trust.

Right, just as strap pin controlling the SB ROM in the 2600 allows bypass.

It's just another one of these affecting a different boot stage.

> If it is for debugging/development purposes, should we encourage the 
> use of unsigned images under RD environments?
> Beyond this, I have no concern as the patch provides more flexibility.
>
>> From: Andrew Jeffery <andrew@aj.id.au>
>> Sent: Monday, January 31, 2022 11:42 AM
>> 
>> Some platform designs include support for disabling secure-boot via a jumper
>> on the board. Sometimes this control can be separate from the mechanism
>> enabling the root-of-trust for the platform. Add support for this latter scenario
>> by allowing boards to implement board_fit_image_require_verfied(), which is
>> then invoked in the usual FIT verification paths.
>> 
>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>> ---
>> Hi,
>> 
>> This patch is extracted from and motivated by a series adding run-time control
>> of FIT signature verification to u-boot in OpenBMC:
>> 
>> https://lore.kernel.org/openbmc/20220131012538.73021-1-andrew@aj.id.au/
>> 
>> Unfortunately the OpenBMC u-boot tree is quite a way behind on tracking
>> upstream and contains a bunch of out-of-tree work as well. As such I'm looking
>> to upstream the couple of changes that make sense against master.
>> 
>> Please take a look!
>> 
>> Andrew
>> 
>>  boot/Kconfig     |  8 ++++++++
>>  boot/image-fit.c | 21 +++++++++++++++++----  include/image.h  |  9
>> +++++++++
>>  3 files changed, 34 insertions(+), 4 deletions(-)
>> 
>> diff --git a/boot/Kconfig b/boot/Kconfig index c8d5906cd304..ec413151fd5a
>> 100644
>> --- a/boot/Kconfig
>> +++ b/boot/Kconfig
>> @@ -78,6 +78,14 @@ config FIT_SIGNATURE
>>  	  format support in this case, enable it using
>>  	  CONFIG_LEGACY_IMAGE_FORMAT.
>> 
>> +if FIT_SIGNATURE
>> +config FIT_RUNTIME_SIGNATURE
>> +	bool "Control verification of FIT uImages at runtime"
>> +	help
>> +	  This option allows board support to disable verification of
>> +	  signatures at runtime, for example through the state of a GPIO.
>> +endif # FIT_SIGNATURE
>> +
>
> Using "depends on" might be preferred for Kconfig dependency.

Yes, that's probably better.

Thanks for taking a look.

Andrew

Re: [PATCH] image: Control FIT signature verification at runtime

[Alex G.]

On 1/30/22 21:41, Andrew Jeffery wrote:
> Some platform designs include support for disabling secure-boot via a
> jumper on the board. Sometimes this control can be separate from the
> mechanism enabling the root-of-trust for the platform. Add support for
> this latter scenario by allowing boards to implement
> board_fit_image_require_verfied(), which is then invoked in the usual
> FIT verification paths.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> Hi,
> 
> This patch is extracted from and motivated by a series adding run-time
> control of FIT signature verification to u-boot in OpenBMC:
> 
> https://lore.kernel.org/openbmc/20220131012538.73021-1-andrew@aj.id.au/
> 
> Unfortunately the OpenBMC u-boot tree is quite a way behind on tracking
> upstream and contains a bunch of out-of-tree work as well. As such I'm
> looking to upstream the couple of changes that make sense against
> master.

I don't understand the practical use of a mechanism to disable security 
if secure boot was enabled in the first place. Sure it can be 
technically done, but why is this not a bad idea?

> Please take a look!
> 
> Andrew
> 
>   boot/Kconfig     |  8 ++++++++
>   boot/image-fit.c | 21 +++++++++++++++++----
>   include/image.h  |  9 +++++++++
>   3 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/boot/Kconfig b/boot/Kconfig
> index c8d5906cd304..ec413151fd5a 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -78,6 +78,14 @@ config FIT_SIGNATURE
>   	  format support in this case, enable it using
>   	  CONFIG_LEGACY_IMAGE_FORMAT.
>   
> +if FIT_SIGNATURE
> +config FIT_RUNTIME_SIGNATURE
> +	bool "Control verification of FIT uImages at runtime"
> +	help
> +	  This option allows board support to disable verification of
> +	  signatures at runtime, for example through the state of a GPIO.

The commit message does a much nicer job explaining this option, even 
though it is this kconfig help text that almost all users will ever see.

> +endif # FIT_SIGNATURE
> +
>   config FIT_SIGNATURE_MAX_SIZE
>   	hex "Max size of signed FIT structures"
>   	depends on FIT_SIGNATURE
> diff --git a/boot/image-fit.c b/boot/image-fit.c
> index f01cafe4e277..919dbfa4ee1d 100644
> --- a/boot/image-fit.c
> +++ b/boot/image-fit.c
> @@ -1308,6 +1308,14 @@ static int fit_image_check_hash(const void *fit, int noffset, const void *data,
>   	return 0;
>   }
>   
> +#ifndef __weak
> +#define __weak
> +#endif

What?

> +__weak int board_fit_image_require_verified(void)
> +{
> +	return 1;
> +}
> +

I caution against having any platform security related functionality as 
a weak function. Did I get the right function, or something else with 
the same name was compiled that returns 0 and silently disables security 
in my platform?

I think a weak function in this context is a source of confusion and 
future bugs. You could instead require the symbol to be defined if and 
only if the appropriate kconfig is selected. Symbol not defined -> 
compiler error. Symbol defined twice -> compiler error. Solves the 
issues in the preceding paragraph.

[snip]

> diff --git a/include/image.h b/include/image.h
> index 97e5f2eb24d6..98900c2e839b 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -1173,6 +1173,15 @@ int calculate_hash(const void *data, int data_len, const char *algo,
>   # define FIT_IMAGE_ENABLE_VERIFY	CONFIG_IS_ENABLED(FIT_SIGNATURE)
>   #endif
>   
> +/*
> + * Further, allow run-time control of verification, e.g. via a jumper
> + */
> +#if defined(CONFIG_FIT_RUNTIME_SIGNATURE)
> +# define fit_image_require_verified()	board_fit_image_require_verified()
> +#else
> +# define fit_image_require_verified()	FIT_IMAGE_ENABLE_VERIFY
> +#endif
> +

image.h is also used for host code. When only changing target code 
behavior, there should be a very good reason to modify common code. I'm 
not convinced that threshold has been met.

My second concern here is subjective: I don't like functions defined as 
macros, further depending on config selections. It makes many code 
parsers and IDEs poop their pantaloons. It makes u-boot harder to work 
with as a result. I suggest finding a way to turn this into a static inline.

Alex

Re: [PATCH] image: Control FIT signature verification at runtime

[Dhananjay Phadke]

On 2/8/2022 1:55 PM, Andrew Jeffery wrote:
> Right, just as strap pin controlling the SB ROM in the 2600 allows bypass.
> 
> It's just another one of these affecting a different boot stage.

Why would someone leave such external exploit open in production?
Fusing OTPCFG0[6]=1 would ignore external strap and OTPCFG0[1]=1 would 
enable secure boot with no way to bypass.

Regards,
Dhananjay

Re: [PATCH] image: Control FIT signature verification at runtime

[Andrew Jeffery]

Hi Alex, thanks for taking a look at the patch.

On Sun, 13 Feb 2022, at 05:25, Alex G. wrote:
> On 1/30/22 21:41, Andrew Jeffery wrote:
>> Some platform designs include support for disabling secure-boot via a
>> jumper on the board. Sometimes this control can be separate from the
>> mechanism enabling the root-of-trust for the platform. Add support for
>> this latter scenario by allowing boards to implement
>> board_fit_image_require_verfied(), which is then invoked in the usual
>> FIT verification paths.
>> 
>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>> ---
>> Hi,
>> 
>> This patch is extracted from and motivated by a series adding run-time
>> control of FIT signature verification to u-boot in OpenBMC:
>> 
>> https://lore.kernel.org/openbmc/20220131012538.73021-1-andrew@aj.id.au/
>> 
>> Unfortunately the OpenBMC u-boot tree is quite a way behind on tracking
>> upstream and contains a bunch of out-of-tree work as well. As such I'm
>> looking to upstream the couple of changes that make sense against
>> master.
>
> I don't understand the practical use of a mechanism to disable security 
> if secure boot was enabled in the first place. Sure it can be 
> technically done, but why is this not a bad idea?

Right, I think this question is an indication that I could write a more
informative commit message, so if we converge on something acceptable
I'll update it. Let me provide some more context:

As mentioned above this is motivated by use with BMCs, specifically on
the ASPEED AST2600, in some specific platform contexts.

Platforms can be designed with two styles of firmware management in
mind:

1. The typical approach - No owner control: Manufacturer supplied
firmware with secure-boot always enabled

2. The atypical approach - Full owner control: Owner-controlled firmware
with secure-boot optionally enabled

For quite a few contributing to OpenBMC, the manufacturer and the owner
are the same, and so the typical approach is a good match. It probably
is the use case Dhananjay was considering[1]. It also caters to the
traditionally closed-source firmware ecosystem where manufacturer
control is retained.

[1] https://lore.kernel.org/openbmc/016ae207-2ecb-1817-d10e-12819c8c40ef@linux.microsoft.com/

The second approach requires open-source firmware stacks combined with
platforms designed to enable owner control. There are some ecosystems
that allow this (e.g. OpenPOWER). On the host side for those systems
it's possible to secure-boot them using firmware and kernels signed
entirely and only by user-controlled keys. We're looking to enable this
for the BMC side too, as much as we can.

For completeness (i.e. stating this to make the argument self-contained,
not implying that you're unaware of it), for secure-boot to be
meaningful at a given point in the boot process we need all previous
elements of the boot process to have been verified. That is, it's not
enough to verify u-boot if the u-boot SPL is not verified.

Where such systems use the AST2600, limitations of the AST2600
secure-boot design come into play:

3. All secure-boot configuration is held in OTP memory integrated into
the SoC

4. The OTP memory must be write-protected to prevent attackers
installing arbitrary keys without physical presence

5. The OTP is write-protected by configuration held in the OTP.

The consequence with respect to 2. is that the system manufacturer
either must:

6. Program and write-protect the OTP during production, or

7. Ship the system with the OTP in a vulnerable state.

We figure the latter isn't desirable which means dealing with
limitations of the former.

If the OTP is programmed (with the required public keys) and
write-protected by the manufacturer, then when configured as a
secure-boot root-of-trust, the AST2600 must only boot an SPL image
signed by the manufacturer. From here there are two options for owner
control: 

8. The manufacturer signs arbitrary SPLs upon request. This requires
trust from both parties and potentially a lot of auditing focus from the
manufacturer.

9. The manufacturer publishes the source for the signed u-boot SPL
binary in a fashion that allows reproducible builds for verification by
inspection. Firmware signed by owner-controlled keys can only be applied
beyond this boot stage.

Despite the compromise, the latter approach seems to be the most
reasonable in the circumstances.

Again for completeness, broadly, security can be divided into two
categories:

10. Software security
11. Physical security

Controlling secure-boot in a way that requires physical presence rules
out the ability to influence the boot process via (remote) software
attacks. This is beneficial. The approach at the platform level does not
yet solve for physical security, however given this is motivated by use
on BMCs, physical security concerns could be viewed as taken care of in
the sense that the systems are likely installed in a datacenter or some
other controlled environment.

With that in mind:

To escape the manufacturer's key-chain for owner-controlled signatures
the concept is the manufacturer-signed SPL (or u-boot payload) will load
keys from an external, write-protected EEPROM. These keys are used to
verify the next element of the boot process, providing user control.

To configure owner-controlled keys the EEPROM write-protect must be
disabled. This may, for example, be done via a physical jumper. If left
with write-protection disabled the matching public key for the signature
on the payload can arbitrarily be installed into the EEPROM which makes
secure-boot verification moot. The patch avoids the run-around in this
last behaviour by providing a platform hook to read the state of what is
effectively the EEPROM write-protect pin.

>
>> Please take a look!
>> 
>> Andrew
>> 
>>   boot/Kconfig     |  8 ++++++++
>>   boot/image-fit.c | 21 +++++++++++++++++----
>>   include/image.h  |  9 +++++++++
>>   3 files changed, 34 insertions(+), 4 deletions(-)
>> 
>> diff --git a/boot/Kconfig b/boot/Kconfig
>> index c8d5906cd304..ec413151fd5a 100644
>> --- a/boot/Kconfig
>> +++ b/boot/Kconfig
>> @@ -78,6 +78,14 @@ config FIT_SIGNATURE
>>   	  format support in this case, enable it using
>>   	  CONFIG_LEGACY_IMAGE_FORMAT.
>>   
>> +if FIT_SIGNATURE
>> +config FIT_RUNTIME_SIGNATURE
>> +	bool "Control verification of FIT uImages at runtime"
>> +	help
>> +	  This option allows board support to disable verification of
>> +	  signatures at runtime, for example through the state of a GPIO.
>
> The commit message does a much nicer job explaining this option, even 
> though it is this kconfig help text that almost all users will ever see.

I'll fix this once we finish up on the discussion above.

>
>> +endif # FIT_SIGNATURE
>> +
>>   config FIT_SIGNATURE_MAX_SIZE
>>   	hex "Max size of signed FIT structures"
>>   	depends on FIT_SIGNATURE
>> diff --git a/boot/image-fit.c b/boot/image-fit.c
>> index f01cafe4e277..919dbfa4ee1d 100644
>> --- a/boot/image-fit.c
>> +++ b/boot/image-fit.c
>> @@ -1308,6 +1308,14 @@ static int fit_image_check_hash(const void *fit, int noffset, const void *data,
>>   	return 0;
>>   }
>>   
>> +#ifndef __weak
>> +#define __weak
>> +#endif
>
> What?

boot/image-fit.c is built by the tools as well as into the u-boot{,-spl}
images, but __weak is only defined when __KERNEL__ is defined, which is
not how the tools are compiled.

This is a bit of hack but if I resolve your concern about using weak
symbols below then this goes away.

>
>> +__weak int board_fit_image_require_verified(void)
>> +{
>> +	return 1;
>> +}
>> +
>
> I caution against having any platform security related functionality as 
> a weak function. Did I get the right function, or something else with 
> the same name was compiled that returns 0 and silently disables security 
> in my platform?
>
> I think a weak function in this context is a source of confusion and 
> future bugs. You could instead require the symbol to be defined if and 
> only if the appropriate kconfig is selected. Symbol not defined -> 
> compiler error. Symbol defined twice -> compiler error. Solves the 
> issues in the preceding paragraph.

Okay.

>
> [snip]
>
>> diff --git a/include/image.h b/include/image.h
>> index 97e5f2eb24d6..98900c2e839b 100644
>> --- a/include/image.h
>> +++ b/include/image.h
>> @@ -1173,6 +1173,15 @@ int calculate_hash(const void *data, int data_len, const char *algo,
>>   # define FIT_IMAGE_ENABLE_VERIFY	CONFIG_IS_ENABLED(FIT_SIGNATURE)
>>   #endif
>>   
>> +/*
>> + * Further, allow run-time control of verification, e.g. via a jumper
>> + */
>> +#if defined(CONFIG_FIT_RUNTIME_SIGNATURE)
>> +# define fit_image_require_verified()	board_fit_image_require_verified()
>> +#else
>> +# define fit_image_require_verified()	FIT_IMAGE_ENABLE_VERIFY
>> +#endif
>> +
>
> image.h is also used for host code. When only changing target code 
> behavior, there should be a very good reason to modify common code. I'm 
> not convinced that threshold has been met.

I guess I could move it into boot/image-fit.c as that's the only user,
but given its role that too doesn't really address your concern :)

Joel had related comments on the patch on the OpenBMC list[2]

[2] https://lore.kernel.org/openbmc/CACPK8XcY=afrQ9bE2A3q1tC8Hhxmx3oVM7k_C_fVoYGbLJ4OUg@mail.gmail.com/

How do you propose I add target-specific behaviour? Or is the patch just
a non-starter? As mentioned above, moving the behaviour to
boot/image-fit.c doesn't address this concern.

>
> My second concern here is subjective: I don't like functions defined as 
> macros, further depending on config selections. It makes many code 
> parsers and IDEs poop their pantaloons. It makes u-boot harder to work 
> with as a result. I suggest finding a way to turn this into a static inline.

That's fine, I'm not wedded to the approach so happy to clean it up.

I was trying to make sure the change had no impact on code generation
with CONFIG_FIT_RUNTIME_SIGNATURE=n without relying too much on the
optimiser to clean things up. I'll reassess what I've done to see if we
can get a decent outcome both ways.

Andrew

Re: [PATCH] image: Control FIT signature verification at runtime

[Dhananjay Phadke]

On 2/13/2022 5:13 PM, Andrew Jeffery wrote:
> Right, I think this question is an indication that I could write a more
> informative commit message, so if we converge on something acceptable
> I'll update it. Let me provide some more context:
> 
> As mentioned above this is motivated by use with BMCs, specifically on
> the ASPEED AST2600, in some specific platform contexts.
> 
> Platforms can be designed with two styles of firmware management in
> mind:
> 
> 1. The typical approach - No owner control: Manufacturer supplied
> firmware with secure-boot always enabled
> 
> 2. The atypical approach - Full owner control: Owner-controlled firmware
> with secure-boot optionally enabled
> 
> For quite a few contributing to OpenBMC, the manufacturer and the owner
> are the same, and so the typical approach is a good match. It probably
> is the use case Dhananjay was considering[1]. It also caters to the
> traditionally closed-source firmware ecosystem where manufacturer
> control is retained.
> 
> [1]https://lore.kernel.org/openbmc/016ae207-2ecb-1817-d10e-12819c8c40ef@linux.microsoft.com/
> 
> The second approach requires open-source firmware stacks combined with
> platforms designed to enable owner control. There are some ecosystems
> that allow this (e.g. OpenPOWER). On the host side for those systems
> it's possible to secure-boot them using firmware and kernels signed
> entirely and only by user-controlled keys. We're looking to enable this
> for the BMC side too, as much as we can.
> 
> For completeness (i.e. stating this to make the argument self-contained,
> not implying that you're unaware of it), for secure-boot to be
> meaningful at a given point in the boot process we need all previous
> elements of the boot process to have been verified. That is, it's not
> enough to verify u-boot if the u-boot SPL is not verified.
> 
> Where such systems use the AST2600, limitations of the AST2600
> secure-boot design come into play:
> 
> 3. All secure-boot configuration is held in OTP memory integrated into
> the SoC
> 
> 4. The OTP memory must be write-protected to prevent attackers
> installing arbitrary keys without physical presence
> 
> 5. The OTP is write-protected by configuration held in the OTP.
> 
> The consequence with respect to 2. is that the system manufacturer
> either must:
> 
> 6. Program and write-protect the OTP during production, or
> 
> 7. Ship the system with the OTP in a vulnerable state.
> 
> We figure the latter isn't desirable which means dealing with
> limitations of the former.
> 
> If the OTP is programmed (with the required public keys) and
> write-protected by the manufacturer, then when configured as a
> secure-boot root-of-trust, the AST2600 must only boot an SPL image
> signed by the manufacturer. From here there are two options for owner
> control:
> 
> 8. The manufacturer signs arbitrary SPLs upon request. This requires
> trust from both parties and potentially a lot of auditing focus from the
> manufacturer.
> 
> 9. The manufacturer publishes the source for the signed u-boot SPL
> binary in a fashion that allows reproducible builds for verification by
> inspection. Firmware signed by owner-controlled keys can only be applied
> beyond this boot stage.
> 
> Despite the compromise, the latter approach seems to be the most
> reasonable in the circumstances.
> 
> Again for completeness, broadly, security can be divided into two
> categories:
> 
> 10. Software security
> 11. Physical security
> 
> Controlling secure-boot in a way that requires physical presence rules
> out the ability to influence the boot process via (remote) software
> attacks. This is beneficial. The approach at the platform level does not
> yet solve for physical security, however given this is motivated by use
> on BMCs, physical security concerns could be viewed as taken care of in
> the sense that the systems are likely installed in a datacenter or some
> other controlled environment.

We can decouple HW RoT and runtime control on enforcing secure boot
(requiring one or keys) on FIT image. Conflating two raises lot of
questions.

There's not much case for disabling HW RoT, which implies the bootloader
(U-Boot or more) has to be trusted after board is manufactured
(OTPstraps, especially OTPCFG0[6], are programmed).

There's indeed a case for disabling secure boot on OS FIT image -
If bootloader is trusted, it's possible to remotely push the policy to
disable runtime FIT image secure boot. Such policy push must use secure 
transport (someway authenticated) and storage (simplest U-Boot env).
This is helpful in cases like booting diagnostic images if board has to
be RMA'ed and diagnostic images won't be signed by production keys.

There's a key-requirement policy already implemented [1].

[1] 
https://lore.kernel.org/u-boot/cover.1597643014.git.thiruan@linux.microsoft.com/

Board code can patch "required-policy" = none at runtime based 
appropriate logic.

Regards,
Dhananjay

> 
> With that in mind:
> 
> To escape the manufacturer's key-chain for owner-controlled signatures
> the concept is the manufacturer-signed SPL (or u-boot payload) will load
> keys from an external, write-protected EEPROM. These keys are used to
> verify the next element of the boot process, providing user control.
> 
> To configure owner-controlled keys the EEPROM write-protect must be
> disabled. This may, for example, be done via a physical jumper. If left
> with write-protection disabled the matching public key for the signature
> on the payload can arbitrarily be installed into the EEPROM which makes
> secure-boot verification moot. The patch avoids the run-around in this
> last behaviour by providing a platform hook to read the state of what is
> effectively the EEPROM write-protect pin.

Re: [PATCH] image: Control FIT signature verification at runtime

[Andrew Jeffery]

On Tue, 15 Feb 2022, at 05:44, Dhananjay Phadke wrote:
> On 2/13/2022 5:13 PM, Andrew Jeffery wrote:
>> Right, I think this question is an indication that I could write a more
>> informative commit message, so if we converge on something acceptable
>> I'll update it. Let me provide some more context:
>> 
>> As mentioned above this is motivated by use with BMCs, specifically on
>> the ASPEED AST2600, in some specific platform contexts.
>> 
>> Platforms can be designed with two styles of firmware management in
>> mind:
>> 
>> 1. The typical approach - No owner control: Manufacturer supplied
>> firmware with secure-boot always enabled
>> 
>> 2. The atypical approach - Full owner control: Owner-controlled firmware
>> with secure-boot optionally enabled
>> 
>> For quite a few contributing to OpenBMC, the manufacturer and the owner
>> are the same, and so the typical approach is a good match. It probably
>> is the use case Dhananjay was considering[1]. It also caters to the
>> traditionally closed-source firmware ecosystem where manufacturer
>> control is retained.
>> 
>> [1]https://lore.kernel.org/openbmc/016ae207-2ecb-1817-d10e-12819c8c40ef@linux.microsoft.com/
>> 
>> The second approach requires open-source firmware stacks combined with
>> platforms designed to enable owner control. There are some ecosystems
>> that allow this (e.g. OpenPOWER). On the host side for those systems
>> it's possible to secure-boot them using firmware and kernels signed
>> entirely and only by user-controlled keys. We're looking to enable this
>> for the BMC side too, as much as we can.
>> 
>> For completeness (i.e. stating this to make the argument self-contained,
>> not implying that you're unaware of it), for secure-boot to be
>> meaningful at a given point in the boot process we need all previous
>> elements of the boot process to have been verified. That is, it's not
>> enough to verify u-boot if the u-boot SPL is not verified.
>> 
>> Where such systems use the AST2600, limitations of the AST2600
>> secure-boot design come into play:
>> 
>> 3. All secure-boot configuration is held in OTP memory integrated into
>> the SoC
>> 
>> 4. The OTP memory must be write-protected to prevent attackers
>> installing arbitrary keys without physical presence
>> 
>> 5. The OTP is write-protected by configuration held in the OTP.
>> 
>> The consequence with respect to 2. is that the system manufacturer
>> either must:
>> 
>> 6. Program and write-protect the OTP during production, or
>> 
>> 7. Ship the system with the OTP in a vulnerable state.
>> 
>> We figure the latter isn't desirable which means dealing with
>> limitations of the former.
>> 
>> If the OTP is programmed (with the required public keys) and
>> write-protected by the manufacturer, then when configured as a
>> secure-boot root-of-trust, the AST2600 must only boot an SPL image
>> signed by the manufacturer. From here there are two options for owner
>> control:
>> 
>> 8. The manufacturer signs arbitrary SPLs upon request. This requires
>> trust from both parties and potentially a lot of auditing focus from the
>> manufacturer.
>> 
>> 9. The manufacturer publishes the source for the signed u-boot SPL
>> binary in a fashion that allows reproducible builds for verification by
>> inspection. Firmware signed by owner-controlled keys can only be applied
>> beyond this boot stage.
>> 
>> Despite the compromise, the latter approach seems to be the most
>> reasonable in the circumstances.
>> 
>> Again for completeness, broadly, security can be divided into two
>> categories:
>> 
>> 10. Software security
>> 11. Physical security
>> 
>> Controlling secure-boot in a way that requires physical presence rules
>> out the ability to influence the boot process via (remote) software
>> attacks. This is beneficial. The approach at the platform level does not
>> yet solve for physical security, however given this is motivated by use
>> on BMCs, physical security concerns could be viewed as taken care of in
>> the sense that the systems are likely installed in a datacenter or some
>> other controlled environment.
>
> We can decouple HW RoT and runtime control on enforcing secure boot
> (requiring one or keys) on FIT image. Conflating two raises lot of
> questions.

Right. They are decoupled. What I'm proposing in the patch only affects 
FIT verification.

Cheers,

Andrew

Re: [PATCH] image: Control FIT signature verification at runtime

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 3241 bytes --]

On Mon, Feb 14, 2022 at 11:14:53AM -0800, Dhananjay Phadke wrote:
> On 2/13/2022 5:13 PM, Andrew Jeffery wrote:
> 
> We can decouple HW RoT and runtime control on enforcing secure boot
> (requiring one or keys) on FIT image. Conflating two raises lot of
> questions.

I won't claim to be a security expert but I don't understand this statement.
What are the "lots of questions" that are raised?

> There's not much case for disabling HW RoT, which implies the bootloader
> (U-Boot or more) has to be trusted after board is manufactured
> (OTPstraps, especially OTPCFG0[6], are programmed).
> 
> There's indeed a case for disabling secure boot on OS FIT image -

Why wouldn't you want to replace the bootloader just as easily as you can
replace the kernel / OS itself?  I don't understand why this is more special
than any other software.  Bootloaders are replaced on "real" systems all the
time.  There are multiple efforts to be able to replace BIOS/UEFI with a free
implementation as well.

I would consider the "HW RoT" to be the software in ROMs and not anything
which can be replaced, like u-boot.  Why are you extending it to include u-boot?

> If bootloader is trusted, it's possible to remotely push the policy to
> disable runtime FIT image secure boot. Such policy push must use secure 
> transport (someway authenticated) and storage (simplest U-Boot env).
> This is helpful in cases like booting diagnostic images if board has to
> be RMA'ed and diagnostic images won't be signed by production keys.

For second-hand / recycled hardware, I'm not sure the bootloader _is_ trusted.
It is also possible that I punt on some aspects of supply-chain security and
simply replace it all when it arrives in my hands.  ie. If I can securely
replace all the bits, I really don't care if it was tampered with in transit.

> There's a key-requirement policy already implemented [1].
> 
> [1] 
> https://lore.kernel.org/u-boot/cover.1597643014.git.thiruan@linux.microsoft.com/
> 
> Board code can patch "required-policy" = none at runtime based 
> appropriate logic.
> 
> Regards,
> Dhananjay
> 
> > 
> > With that in mind:
> > 
> > To escape the manufacturer's key-chain for owner-controlled signatures
> > the concept is the manufacturer-signed SPL (or u-boot payload) will load
> > keys from an external, write-protected EEPROM. These keys are used to
> > verify the next element of the boot process, providing user control.
> > 
> > To configure owner-controlled keys the EEPROM write-protect must be
> > disabled. This may, for example, be done via a physical jumper. If left
> > with write-protection disabled the matching public key for the signature
> > on the payload can arbitrarily be installed into the EEPROM which makes
> > secure-boot verification moot. The patch avoids the run-around in this
> > last behaviour by providing a platform hook to read the state of what is
> > effectively the EEPROM write-protect pin.

Isn't this jumper proposal just like the TCG Physical Presence requirements?
This is a software implementation and requires a particular hardware design for
it to be done right, but it seems to be along the same lines.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] image: Control FIT signature verification at runtime

[Andrew Jeffery]

On Tue, 15 Feb 2022, at 09:43, Patrick Williams wrote:
> On Mon, Feb 14, 2022 at 11:14:53AM -0800, Dhananjay Phadke wrote:
>> On 2/13/2022 5:13 PM, Andrew Jeffery wrote:
>> 
>> We can decouple HW RoT and runtime control on enforcing secure boot
>> (requiring one or keys) on FIT image. Conflating two raises lot of
>> questions.
>
> I won't claim to be a security expert but I don't understand this statement.
> What are the "lots of questions" that are raised?

I was trying to avoid derailing the review with this, but here we are.

I have the same question as Patrick. What are your concerns here?

>> > 
>> > With that in mind:
>> > 
>> > To escape the manufacturer's key-chain for owner-controlled signatures
>> > the concept is the manufacturer-signed SPL (or u-boot payload) will load
>> > keys from an external, write-protected EEPROM. These keys are used to
>> > verify the next element of the boot process, providing user control.
>> > 
>> > To configure owner-controlled keys the EEPROM write-protect must be
>> > disabled. This may, for example, be done via a physical jumper. If left
>> > with write-protection disabled the matching public key for the signature
>> > on the payload can arbitrarily be installed into the EEPROM which makes
>> > secure-boot verification moot. The patch avoids the run-around in this
>> > last behaviour by providing a platform hook to read the state of what is
>> > effectively the EEPROM write-protect pin.
>
> Isn't this jumper proposal just like the TCG Physical Presence requirements?
> This is a software implementation and requires a particular hardware design for
> it to be done right, but it seems to be along the same lines.

Possibly. I'll defer to Chris on that.

Andrew

Re: [PATCH] image: Control FIT signature verification at runtime

[Dhananjay Phadke]

On 2/14/2022 3:13 PM, Patrick Williams wrote:
> On Mon, Feb 14, 2022 at 11:14:53AM -0800, Dhananjay Phadke wrote:
>> On 2/13/2022 5:13 PM, Andrew Jeffery wrote:
>>
>> We can decouple HW RoT and runtime control on enforcing secure boot
>> (requiring one or keys) on FIT image. Conflating two raises lot of
>> questions.
> 
> I won't claim to be a security expert but I don't understand this statement.
> What are the "lots of questions" that are raised?
> 
>> There's not much case for disabling HW RoT, which implies the bootloader
>> (U-Boot or more) has to be trusted after board is manufactured
>> (OTPstraps, especially OTPCFG0[6], are programmed).
>>
>> There's indeed a case for disabling secure boot on OS FIT image -
> 
> Why wouldn't you want to replace the bootloader just as easily as you can
> replace the kernel / OS itself?  I don't understand why this is more special
> than any other software.  Bootloaders are replaced on "real" systems all the
> time.  There are multiple efforts to be able to replace BIOS/UEFI with a free
> implementation as well.
> 
> I would consider the "HW RoT" to be the software in ROMs and not anything
> which can be replaced, like u-boot.  Why are you extending it to include u-boot?

Agree that ROM maybe just immutable code + some logic, but almost surely
lacks high level stack e.g. network stack to communicate securely and
change boot behavior (remote unlock).

Bootloader (U-Boot in Aspeed case) happens to be the first component
with rich stack to enable such workflows without / before physical
intervention.

It also means, for defense against physical attacks (e.g. replace boot
SPI), first mutable component (bootloader) must be trusted by immutable
component earlier in boot chain. Further secure boot chain may have its
own control knobs from compile or runtime.


> 
>> If bootloader is trusted, it's possible to remotely push the policy to
>> disable runtime FIT image secure boot. Such policy push must use secure
>> transport (someway authenticated) and storage (simplest U-Boot env).
>> This is helpful in cases like booting diagnostic images if board has to
>> be RMA'ed and diagnostic images won't be signed by production keys.
> 
> For second-hand / recycled hardware, I'm not sure the bootloader _is_ trusted.
> It is also possible that I punt on some aspects of supply-chain security and
> simply replace it all when it arrives in my hands.  ie. If I can securely
> replace all the bits, I really don't care if it was tampered with in transit.
> 
>> There's a key-requirement policy already implemented [1].
>>
>> [1]
>> https://lore.kernel.org/u-boot/cover.1597643014.git.thiruan@linux.microsoft.com/
>>
>> Board code can patch "required-policy" = none at runtime based
>> appropriate logic.
>>

[...]

> 
> Isn't this jumper proposal just like the TCG Physical Presence requirements?
> This is a software implementation and requires a particular hardware design for
> it to be done right, but it seems to be along the same lines.

I'm supporting idea of having control on FIT verification, just pointed
that it maybe done by board code by just patching U-Boot control FDT,
either the "required-policy" property at /signature or "required"
property in individual key nodes.


Regards,
Dhananjay

Re: [PATCH] image: Control FIT signature verification at runtime

[Andrew Jeffery]

On Tue, 15 Feb 2022, at 13:42, Dhananjay Phadke wrote:
> On 2/14/2022 3:13 PM, Patrick Williams wrote:
>> On Mon, Feb 14, 2022 at 11:14:53AM -0800, Dhananjay Phadke wrote:
>>> There's a key-requirement policy already implemented [1].
>>>
>>> [1]
>>> https://lore.kernel.org/u-boot/cover.1597643014.git.thiruan@linux.microsoft.com/
>>>
>>> Board code can patch "required-policy" = none at runtime based
>>> appropriate logic.
>>>
>
> [...]
>
>> 
>> Isn't this jumper proposal just like the TCG Physical Presence requirements?
>> This is a software implementation and requires a particular hardware design for
>> it to be done right, but it seems to be along the same lines.
>
> I'm supporting idea of having control on FIT verification, just pointed
> that it maybe done by board code by just patching U-Boot control FDT,
> either the "required-policy" property at /signature or "required"
> property in individual key nodes.

This might separate the logic out in a way that's acceptable to Alex.

Let me poke at it.

Thanks,

Andrew

Re: [PATCH] image: Control FIT signature verification at runtime

[Andrew Jeffery]

On Tue, 15 Feb 2022, at 13:55, Andrew Jeffery wrote:
> On Tue, 15 Feb 2022, at 13:42, Dhananjay Phadke wrote:
>> On 2/14/2022 3:13 PM, Patrick Williams wrote:
>>> On Mon, Feb 14, 2022 at 11:14:53AM -0800, Dhananjay Phadke wrote:
>>>> There's a key-requirement policy already implemented [1].
>>>>
>>>> [1]
>>>> https://lore.kernel.org/u-boot/cover.1597643014.git.thiruan@linux.microsoft.com/
>>>>
>>>> Board code can patch "required-policy" = none at runtime based
>>>> appropriate logic.
>>>>
>>
>> [...]
>>
>>> 
>>> Isn't this jumper proposal just like the TCG Physical Presence requirements?
>>> This is a software implementation and requires a particular hardware design for
>>> it to be done right, but it seems to be along the same lines.
>>
>> I'm supporting idea of having control on FIT verification, just pointed
>> that it maybe done by board code by just patching U-Boot control FDT,
>> either the "required-policy" property at /signature or "required"
>> property in individual key nodes.
>
> This might separate the logic out in a way that's acceptable to Alex.
>
> Let me poke at it.

I've thought about this some more and adding support for
`required-mode = "none";` or similar seems like a massive footgun given
that (as I understand it) the FIT image as a whole isn't verified. Only
supporting "all" or "any" seems okay because some verification must
succeed in the context of the keys available in the current stage.

After some internal discussion this effort has been set aside so I'm not
going to pursue it further for the moment. I don't think it's easy to
proceed anyway without feedback from Alex.

Andrew

Re: [PATCH] image: Control FIT signature verification at runtime

[Alex G.]

On 2/27/22 19:29, Andrew Jeffery wrote:
> 
> 
> On Tue, 15 Feb 2022, at 13:55, Andrew Jeffery wrote:
>> On Tue, 15 Feb 2022, at 13:42, Dhananjay Phadke wrote:
>>> On 2/14/2022 3:13 PM, Patrick Williams wrote:
>>>> On Mon, Feb 14, 2022 at 11:14:53AM -0800, Dhananjay Phadke wrote:
>>>>> There's a key-requirement policy already implemented [1].
>>>>>
>>>>> [1]
>>>>> https://lore.kernel.org/u-boot/cover.1597643014.git.thiruan@linux.microsoft.com/
>>>>>
>>>>> Board code can patch "required-policy" = none at runtime based
>>>>> appropriate logic.
>>>>>
>>>
>>> [...]
>>>
>>>>
>>>> Isn't this jumper proposal just like the TCG Physical Presence requirements?
>>>> This is a software implementation and requires a particular hardware design for
>>>> it to be done right, but it seems to be along the same lines.
>>>
>>> I'm supporting idea of having control on FIT verification, just pointed
>>> that it maybe done by board code by just patching U-Boot control FDT,
>>> either the "required-policy" property at /signature or "required"
>>> property in individual key nodes.
>>
>> This might separate the logic out in a way that's acceptable to Alex.
>>
>> Let me poke at it.
> 
> I've thought about this some more and adding support for
> `required-mode = "none";` or similar seems like a massive footgun given
> that (as I understand it) the FIT image as a whole isn't verified. Only
> supporting "all" or "any" seems okay because some verification must
> succeed in the context of the keys available in the current stage.
> 
> After some internal discussion this effort has been set aside so I'm not
> going to pursue it further for the moment. I don't think it's easy to
> proceed anyway without feedback from Alex.

Don't let my thoughts stop you. I don't think there is a perfect way to 
address this situation, and we don't have to. Code can be changed later.

As a general preference, I would like to see a single decision point on 
whether to verify/skip. It can be changing `required-mode = "none", or 
any other similar solution. Keep in mind that the FIT is the image 
you're trying to authenticate. It is completely different from 
"required-mode", which is part of u-boot's or SPL's embedded dtb.

Alex

Re: [PATCH] image: Control FIT signature verification at runtime

[Andrew Jeffery]

On Tue, 1 Mar 2022, at 08:42, Alex G. wrote:
> On 2/27/22 19:29, Andrew Jeffery wrote:
>> 
>> 
>> On Tue, 15 Feb 2022, at 13:55, Andrew Jeffery wrote:
>>> On Tue, 15 Feb 2022, at 13:42, Dhananjay Phadke wrote:
>>>> On 2/14/2022 3:13 PM, Patrick Williams wrote:
>>>>> On Mon, Feb 14, 2022 at 11:14:53AM -0800, Dhananjay Phadke wrote:
>>>>>> There's a key-requirement policy already implemented [1].
>>>>>>
>>>>>> [1]
>>>>>> https://lore.kernel.org/u-boot/cover.1597643014.git.thiruan@linux.microsoft.com/
>>>>>>
>>>>>> Board code can patch "required-policy" = none at runtime based
>>>>>> appropriate logic.
>>>>>>
>>>>
>>>> [...]
>>>>
>>>>>
>>>>> Isn't this jumper proposal just like the TCG Physical Presence requirements?
>>>>> This is a software implementation and requires a particular hardware design for
>>>>> it to be done right, but it seems to be along the same lines.
>>>>
>>>> I'm supporting idea of having control on FIT verification, just pointed
>>>> that it maybe done by board code by just patching U-Boot control FDT,
>>>> either the "required-policy" property at /signature or "required"
>>>> property in individual key nodes.
>>>
>>> This might separate the logic out in a way that's acceptable to Alex.
>>>
>>> Let me poke at it.
>> 
>> I've thought about this some more and adding support for
>> `required-mode = "none";` or similar seems like a massive footgun given
>> that (as I understand it) the FIT image as a whole isn't verified. Only
>> supporting "all" or "any" seems okay because some verification must
>> succeed in the context of the keys available in the current stage.
>> 
>> After some internal discussion this effort has been set aside so I'm not
>> going to pursue it further for the moment. I don't think it's easy to
>> proceed anyway without feedback from Alex.
>
> Don't let my thoughts stop you. I don't think there is a perfect way to 
> address this situation, and we don't have to. Code can be changed later.
>
> As a general preference, I would like to see a single decision point on 
> whether to verify/skip. It can be changing `required-mode = "none", or 
> any other similar solution. Keep in mind that the FIT is the image 
> you're trying to authenticate. It is completely different from 
> "required-mode", which is part of u-boot's or SPL's embedded dtb.

Ah yes, I wasn't thinking of it that way, so maybe it could work. I'll 
consider it all again, but we also determined that we could get away 
without this functionality for now anyway.

Andrew