Signed boot

[ghost]

This draft PR continues the work by @dinomani to add signature to the px4 binary. This implements the signature checking to the bootloader. This is compatible with signature creation and the table of contents in PR: https://github.com/PX4/PX4-Autopilot/pull/16201

This implementation has the following properties:

• The signature checking is added to the target only when "CRYPTO_HAL" environment variable is defined. Otherwise a standard bootloader is built. For example: $ CRYPTO_HAL=monocypher make px4fmuv5_bl Creates a bootloader with cypto functionality defined in crypto_hal/monocypher directory.
• Adding other crypto functionality/algorithms (e.g. using HW crypto on some platforms) can be added just by adding a new "crypto_hal" implementation, and setting that to the environment variable when building

This PR also increases the maximum size of the bootloader in px4fmu_v5 linker script, since the example monocypher doesn't fit into the default 16kB area.

[LorenzMeier]

@davids5 Let's take this up on the next PX4 Dev call.

[jlaitine]

So I made the comments using another github account, which I later found out useless and deleted. That's why the comments show me as "ghost". The commits themselves seem to map properly to my main github account (based on email address).

[jlaitine]

Made another small fix for README.txt. The BOOT flag is actually used by bootloader to select the bootable image & vectors.

[jlaitine]

Rest of the style check issues fixed. Can't fix the monocypher submodule though, as it is just cloned as is from upstream repo

[jlaitine]

I will still add the flag to set vectors separately, which was requested by @davids5

[jlaitine]

@davids5 Can you please check if the topmost commit now implements your wish for separate vectors block properly? If it is ok, I will also update the commit on the px4 side to update the flags accordingly in the header file

[davids5]

@davids5 Can you please check if the topmost commit now implements your wish for separate vectors block properly? If it is ok, I will also update the commit on the px4 side to update the flags accordingly in the header file

From a reservation standpoint: Defining the the data - were good. Thnak you for adding that.

The missing piece can come later as follows: Assuming the TOC of type vector block has passed verification. There is then a TBD call to decrypt(start, end, target), That decrypts the block from the TOC and stores it at the target. The verified target address is what is then used for the vec_base. For testing the decrypt(start, end, target) can just be a memcpy but we have to modify the app start up code to be awear of the vector table and not clobber it.

[jlaitine]

@davids5 Can you please check if the topmost commit now implements your wish for separate vectors block properly? If it is ok, I will also update the commit on the px4 side to update the flags accordingly in the header file

From a reservation standpoint: Defining the the data - were good. Thnak you for adding that.

The missing piece can come later as follows: Assuming the TOC of type vector block has passed verification. There is then a TBD call to decrypt(start, end, target), That decrypts the block from the TOC and stores it at the target. The verified target address is what is then used for the vec_base. For testing the decrypt(start, end, target) can just be a memcpy but we have to modify the app start up code to be awear of the vector table and not clobber it.

Like this?

For example in linker script instead of doing:

SRAM1 (rwx) : ORIGIN = 0x20020000, LENGTH = 368K

You can just do SRAM1 (rwx) : ORIGIN = 0x20020200, LENGTH = 368K- 512

And then put the vectors table decryption target address to 0x20020000

Then the application startup code wouldn't even know that the vectors are there, since it is completely outside it's linker map.

But, did you still want the decryption functionality (or just memcpy) in this PR, or some later one?

The bl can also work in a way that if there is a target address defined, but no decryption flag, it will only copy With decryption flag it would decrypt to target address.

So if there is target address defined, the final data will end up in there and that address will be used.

[davids5]

@davids5 Can you please check if the topmost commit now implements your wish for separate vectors block properly? If it is ok, I will also update the commit on the px4 side to update the flags accordingly in the header file

From a reservation standpoint: Defining the the data - were good. Thnak you for adding that. The missing piece can come later as follows: Assuming the TOC of type vector block has passed verification. There is then a TBD call to decrypt(start, end, target), That decrypts the block from the TOC and stores it at the target. The verified target address is what is then used for the vec_base. For testing the decrypt(start, end, target) can just be a memcpy but we have to modify the app start up code to be awear of the vector table and not clobber it.

Like this?

For example in linker script instead of doing:

SRAM1 (rwx) : ORIGIN = 0x20020000, LENGTH = 368K

You can just do SRAM1 (rwx) : ORIGIN = 0x20020200, LENGTH = 368K- 512

And then put the vectors table decryption target address to 0x20020000

Yes exactly.

Then the application startup code wouldn't even know that the vectors are there, since it is completely outside it's linker map.

But, did you still want the decryption functionality (or just memcpy) in this PR, or some later one?

I will leave that up to you and your schedule.

If you can add the dycrypt now that would be great. Then the testing can be for the complete feature set and we have a more accurate size requirement witch will prevent future issues.

The bl can also work in a way that if there is a target address defined, but no decryption flag, it will only copy With decryption flag it would decrypt to target address.

So if there is target address defined, the final data will end up in there and that address will be used.

[jlaitine]

Adding decrypt would be simple. However, decryption functionality is not very interesting on STM, since there is no secure way to provision/store private keys. That's the reason why I only implemented signature checking for now, but reserved the decryption flag for the future.

If you are happy with storing private keys from files in the same way as public keys are stored, I can add that for sure. But maintaining the chain of trust in any production would be painful (the bootloader binary itself would need to be treated the same way as private keys..)

I see that in the future the key storage in the bootloader will turn into chip specific layer, basically doing key management using some security hw.

[davids5]

Adding decrypt would be simple. However, decryption functionality is not very interesting on STM, since there is no secure way to provision/store private keys. That's the reason why I only implemented signature checking for now, but reserved the decryption flag for the future.

If you are happy with storing private keys from files in the same way as public keys are stored, I can add that for sure. But maintaining the chain of trust in any production would be painful (the bootloader binary itself would need to be treated the same way as private keys..)

I see that in the future the key storage in the bootloader will turn into chip specific layer, basically doing key management using some security hw.

You make a great point and we have had to do that before with the BL bin.

Long term I am planning on using it is the SE050 (Secure element) to do the decryption in a secure session, but that is going to more work and code.

I am assuming the pk used Epk(vectors) will be a different key. So maybe the thing to do is to put the hook in with that pk in the clear and we can, test it and decide to use it or not. It will give us a good sizing metric?

If you agree, then have at it, If not we can merge and do all this later.

[jlaitine]

You make a great point and we have had to do that before with the BL bin.

Long term I am planning on using it is the SE050 (Secure element) to do the decryption in a secure session, but that is going to more work and code.

I am assuming the pk used Epk(vectors) will be a different key. So maybe the thing to do is to put the hook in with that pk in the clear and we can, test it and decide to use it or not. It will give us a good sizing metric?

If you agree, then have at it, If not we can merge and do all this later.

It is not many lines to code, so I will just add support for decryption and embedding private keys, the same way as public keys are done now. It can be then used if someone so wishes, and at least the interfaces are there.

[dinomani]

Hi, David, i would like to stay within the initial goal of that PR, and do only the authentication of the app. Adding the possibility to encrypt the vector table with a private key should be done in an other PR. in my opinion. The use of this feature is very limited for the STM32 controllers.
This adds more complexity to the whole process of generating binaries, the requirement for this are not a 100% clear. I think this increases the complexity of this PR, makes it harder for people to understand and use.

[jlaitine]

Ok, taking into account all requests here, I just added a flag for copying data from flash to ram, and also the interface for decryption as per @davids5 's request. Since we don't know users for the implementation with "monocypher" library, I left out the actual implementation from the monocypher hal layer.

Adding that for anyone who needs it would be simple; just adding a private key somewhere and calling monocypher crypto_unlock(..), which will then implement RFC 8439 w. xchacha20. This would add some 3KB of used flash to the bootloader, so it is doable for anyone who actually wants to do that.

Maybe this is as far as this PR should go?

[Igor-Misic]

Merged with #187