Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signed boot #179

Closed
wants to merge 10 commits into from
Closed

Signed boot #179

wants to merge 10 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 16, 2020

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: PX4/PX4-Autopilot#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.

@ghost ghost mentioned this pull request Nov 16, 2020
@ghost ghost marked this pull request as draft November 16, 2020 14:16
@@ -342,7 +342,7 @@
# define INTERFACE_USART 1
# define USBDEVICESTRING "PX4 BL FMU v5.x"
# define USBPRODUCTID 0x0032
# define BOOT_DELAY_ADDRESS 0x000001a0
# define BOOT_DELAY_ADDRESS 0x00000200
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we can move this not still need it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is actually a bugfix, I was not moving it, but just correcting the address where the boot delay signature is for real. I think that currently the bootloader is not finding the boot delay signature at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jlaitine is right. F7 has a slightly bigger vector table than F4
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This same bug actually exists for some other targets as well, like stm32h7, the boot delay signature location has just been copy pasted from F4, even though the vectors size is different.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other then on the Solo, It has never been used. We should see if we can derive it from the HW lib's vector definitions and avoid this class of error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened another issue for this topic #184 so we can focus on secure boot here. Let's keep this bug fix here as it is.

@ghost ghost marked this pull request as ready for review November 25, 2020 16:29
@LorenzMeier
Copy link
Member

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

@LorenzMeier LorenzMeier added this to Low priority in Devcall via automation Jan 1, 2021
@jlaitine
Copy link
Contributor

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).

Copy link
Member

@davids5 davids5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline

bl.c Outdated
@@ -543,6 +589,8 @@ bootloader(unsigned timeout)
/* (re)start the timer system */
arch_systic_init();

//crypto_test_bench();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be #ifdefed ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is leftovers from Dino's original patches, I have no need for it. I think it is ok to just remove it

@@ -0,0 +1,5 @@
//Public key to verify signed binaries
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the key need to be passed to the build and this removed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure; maybe a macro pointing to the key file, and including in public_key.h? I'd still prefer having the keys in files, and not embedding the key itself into any macro/variable.


static const uint8_t public_keys[][32] = {
{
#include "keys/key0.pub"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto to above

v1.12 Release Coordination automation moved this from ToDo to In Progress Jan 13, 2021
Makefile Outdated
@@ -42,11 +42,18 @@ export FLAGS = -std=gnu99 \
-lnosys \
-Wl,-gc-sections \
-Wl,-g \
-Werror
-Werror \
-Xlinker -Map=$(BUILD_DIR_ROOT)/$@/output.map
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jlaitine please also see this:
tiiuae#1
If we are going to add the ".map" file for the bootloader, it is nicer if had a uniform name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed it. I cherry picked you fix on top in this same PR, is this ok?

This adds a check of a digital signature. The signature is build with
an asymmetric encryption. There is a Sha512 calculated over the
application and then it is encrypted with the owners private key.
The application is only run, when the signature matches,
else the bootloader is not exited.
@jlaitine jlaitine force-pushed the signed_boot branch 3 times, most recently from c3ec991 to 2271c6d Compare January 14, 2021 12:30
@jlaitine
Copy link
Contributor

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

@jlaitine jlaitine force-pushed the signed_boot branch 2 times, most recently from b851330 to 7f426f9 Compare January 14, 2021 18:59
@jlaitine
Copy link
Contributor

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

@mrpollo mrpollo added this to In progress in Security SIG via automation Jan 14, 2021
@mrpollo mrpollo removed this from In progress in Security SIG Jan 14, 2021
@jlaitine
Copy link
Contributor

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

@jlaitine jlaitine force-pushed the signed_boot branch 2 times, most recently from 0cf5afe to 6faffb7 Compare January 15, 2021 07:47
@jlaitine
Copy link
Contributor

@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

@jlaitine jlaitine force-pushed the signed_boot branch 2 times, most recently from a742f8b to 2f93df9 Compare January 15, 2021 08:02
@dinomani
Copy link
Contributor

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
Copy link
Contributor

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?

@jlaitine jlaitine force-pushed the signed_boot branch 2 times, most recently from 7aecba6 to d1495f3 Compare January 18, 2021 10:31
Copy link
Member

@davids5 davids5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - @dinomani Any review feed back?

v1.12 Release Coordination automation moved this from In Progress to Reviewed Jan 18, 2021
@Igor-Misic Igor-Misic added this to Review in progress in High-Priority Queue Jan 20, 2021
Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks generally very clean, good work

{
const uint32_t toc_start_u32 = APP_LOAD_ADDRESS + BOOT_DELAY_ADDRESS + 8;
const image_toc_start_t *toc_start = (const image_toc_start_t *)toc_start_u32;
const image_toc_entry_t *entry = (image_toc_entry_t *)(toc_start_u32 + sizeof(image_toc_start_t));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const image_toc_entry_t *entry = (image_toc_entry_t *)(toc_start_u32 + sizeof(image_toc_start_t));
const image_toc_entry_t *entry = (const image_toc_entry_t *)(toc_start_u32 + sizeof(image_toc_start_t));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, I'll update it to make it look cleaner

/* If start and end markers found and are in limits and the
* first app (containing the TOC) is within flashable area,
* return the first entry and the number of entries */
if (i <= MAX_TOC_ENTRIES &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly necessary, but you could add:

Suggested change
if (i <= MAX_TOC_ENTRIES &&
if (i <= MAX_TOC_ENTRIES && i > 0 &&

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will add this

}

app_signature_ptr = (volatile uint8_t *)toc_entries[sig_idx].start;
len = (size_t)toc_entries[idx].end - (size_t)toc_entries[idx].start;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check that this does not exceed the flash size?
What happens if len is 0?

Copy link
Contributor

@jlaitine jlaitine Jan 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that runtime checks are not needed any more here;
The TOC is a constant table, and the contents have been verified to be as the originator/vendor intended since the TOC is within the first app, and the signature for that is always checked.

I doesn't harm to have have the checks here, except for a few words of flash being used. I can add the checks if you feel like it, or I can also just comment this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TOC is a constant table, and the contents have been verified to be as the originator/vendor intended since the TOC is within the first app, and the signature for that is always checked.

As verify_app is also used to validate the first entry, the TOC cannot be trusted here yet. Or do I miss something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had put the check for the first app start/end boundaries into the code that verifies that the TOC itself is valid in bool find_toc()

Thanks for looking it through, please let me know if you think that there is something wrong with this approach!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the reason for not doing it for every entry in the TOC is, that I didn't want to limit the entries to be within the internal flash; there could be other memories, e.g. one TOC entry can initiate a copy/decrypt to SRAM and the later one check it from there....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I've seen those but they are not enough. Specifically, the first entry must be checked for:

  • sig_idx bound check
  • len could be 0 (if you confirm this fails the crypto_ed25519_check check, it's enough to add a comment) or even wrap around as it's unsigned and it's possible to pass end < start (which passes the check in find_toc). An additional check for end > start fixes both.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are valid points, even though I don't see how it could pass the signature check if the sig_idx or len fields would be forged, this is the type of thing which should be fixed. Especially if someone uses the common TOC code with some other crypto algorithms.

Thanks again, I will add end>start and sig_idx bounds checks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the find_toc function;

While checking the sig_idx, I thing the following should also be taken into account:

  • The values from board_info structure are not trustworthy, they come along the app
  • If checking the sig_idx, also the start and end addresses of the signature should be checked the same way as for the app itself.
    Hopefully I got it right this time...

return false;
}

app_signature_ptr = (volatile uint8_t *)toc_entries[sig_idx].start;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a bound check for sig_idx

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment above applies to this one too, TOC should be trusted at this point.

@mrpollo mrpollo mentioned this pull request Jan 21, 2021
@jlaitine jlaitine force-pushed the signed_boot branch 2 times, most recently from 6fe77d7 to 8e60d27 Compare January 22, 2021 12:48
High-Priority Queue automation moved this from Review in progress to Reviewer approved Jan 22, 2021
Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks solid now, thanks


/* The signature idx for the first app must be within the TOC, and
* and the signature must be within the flash area, not overlapping
* the app. Also emsure that end > start.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* the app. Also emsure that end > start.
* the app. Also ensure that end > start.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still fixed this one comment

jlaitine and others added 9 commits January 22, 2021 20:05
- Add parsing of image table-of-contents
- Add "crypto_hal" for easy porting for other security framworks
- Add a simple crypto_hal reference using monocypher
- Add a test key corresponding to the one in px4

Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
To fit in the crypto functions when needed

Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
It is at 0x200 on stm32f7, not at 0x1a0

Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
Change hard-coded key file names to use environment variables instead

In the compilation environment, there has to be the following environment
variables defined, in order to compile signature checking with monocypher:

CRYPTO_HAL=monocypher
PUBLIC_KEY0=<key0 filename>

Up to 4 public keys can be added to bootloader by defining PUBLIC_KEY0...3

Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
Readme file describing the use and structures for signed boot

Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
TOC_FLAG1_VTORS can be used to set vector table
separately from application

TOC_FLAG1_RDCT can be used to mark that the block is an
R&D certificate

Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
@Igor-Misic
Copy link
Member

Igor-Misic commented Jan 28, 2021

Merged with #187

@Igor-Misic Igor-Misic closed this Jan 28, 2021
High-Priority Queue automation moved this from Reviewer approved to Done Jan 28, 2021
v1.12 Release Coordination automation moved this from Reviewed to Done Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants