| Message ID | 20220131034147.106415-1-andrew@aj.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | image: Control FIT signature verification at runtime | expand |
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
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
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
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
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
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.
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
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.
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
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
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
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
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
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
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);
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(-)