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

Bootloader downgrade protection #238

Merged
merged 28 commits into from
Oct 8, 2019

Conversation

szszszsz
Copy link
Contributor

@szszszsz szszszsz commented Aug 6, 2019

This PR adds downgrade protection for the bootloader. Work in progress. Comments welcomed.

Basic idea is to add another page after bootloader for its own data, which would consist entirely at the moment of the last used application firmware version.
The application firmware will contain the version in a known place, which bootloader will compare against before validating of the uploaded application, and will allow or deny, depending on, whenever the firmware is newer or equal the version of the last used firmware.

@nickray
Copy link
Member

nickray commented Aug 6, 2019

This is a great idea, thanks for tackling it! FYI, @hughsie supplied integration with fwupd (not completely functional yet) in #221, once both are done it would be great to allow fwupd to run this check pre-flashing.

I haven't looked at the code yet, but please make sure this is implemented in such a way that the same firmware updates work for "old" and "new" bootloader, we don't want to deal with more than one firmware for all of Solo/Tap/Somu.

About the flash layout will need to sync also with https://github.com/solokeys/openpgp so it doesn't clash.

@szszszsz
Copy link
Contributor Author

szszszsz commented Aug 6, 2019

You are welcome!

fwupd to run this check pre-flashing

This is aimed rather to be additional layer for the case, where adversary tries to update device using a custom updater.

make sure this is implemented in such a way that the same firmware updates work for "old" and "new" bootloader

Will check - perhaps the bootloader data page will have to be moved before the user data space starts, which is not hard by itself, but perhaps it will block some user space extension options in the future. Have not seen the OpenPGP project yet.

@szszszsz
Copy link
Contributor Author

szszszsz commented Aug 7, 2019

It builds, but I am not sure what to do with the reported Codacy warning. This behavior is intended. Are there any force/ignore flags for the checker?

Edit: about the APPLICATION_START_PAGE constant, and the flash layout, the start page is used only for the bootloader, and the main application does not use it. Concluding, the application should not care, where it is placed. The user data page is not changed.

  • To be tested in action with the same firmware, but different (old and new) bootloaders.
  • Add same modifications to the _extra linker files.

@szszszsz
Copy link
Contributor Author

szszszsz commented Aug 7, 2019

Logic modifications are finished, some cleanup might be applied (surely Git commits; also remove excess debug messages or comments). Will test tomorrow. I encourage to review and/or test.

To decide, should version flag be moved to some other place. Currently it starts 8 bytes before the auth_word AFAIR.

@nickray
Copy link
Member

nickray commented Aug 7, 2019

It builds, but I am not sure what to do with the reported Codacy warning. This behavior is intended. Are there any force/ignore flags for the checker?

We haven't configured Codacy so far, just running with defaults. Very open to suggestions for improvement. Do you have an account at Codacy? I tried to add you (so you could poke around) with your Nitrokey and private Gmail, it stated "email is invalid".

@szszszsz
Copy link
Contributor Author

szszszsz commented Aug 8, 2019

It seems it passed the tests second time without changes (tests' results not reproducible within a compilation unit?), so no need anymore for now (but thank you!). I do not have account there yet, but I plan to familiarize with it later - looks interesting.

@conorpp
Copy link
Member

conorpp commented Aug 11, 2019

This is great!

Changing the start page of the application won't work with previous devices. Since old bootloaders will jump to the old start location, and will be off by 1 page. I think it'd be better to put the bootloader data page at the end of the flash region by the application data.

@szszszsz
Copy link
Contributor Author

szszszsz commented Aug 13, 2019

OK! Indeed this would be easier, will do.
Yes, if we would follow my original solution we would need to prepare separate linking scripts for the firmware with the old and new bootloaders (with proper segment start).

Edit: clarification

@szszszsz szszszsz force-pushed the bootloader-downgrade-protection branch from 7545101 to 3ce7682 Compare August 24, 2019 08:18
@szszszsz szszszsz force-pushed the bootloader-downgrade-protection branch from 3ce7682 to a053bbc Compare August 24, 2019 08:26
@szszszsz
Copy link
Contributor Author

szszszsz commented Aug 24, 2019

Improved solution by:

  • moving the application version to the end of the firmware code, concatenated;
  • moving the bootloader data (the last used application firmware version) to the APPLICATION_END_PAGE+8;

Bootloader now checks version of the application by reading last written 4 bytes. App version does not use static address now. New solution is transparent for the old bootloaders. Version check is done now only in the verifying bootloader.

The +8 offset comes from avoiding overwriting by the 0x41 mark added during the all.hex merge with the solo-python tool.

Please review.

Edit: additionally a memory layout struct is added, to use it instead of the macro definitions.
https://github.com/solokeys/solo/blob/a053bbc669a09655f2d8f26c93f510feebd6eee8/targets/stm32l432/src/memory_layout.h#L52-L66

@szszszsz szszszsz changed the title [WIP] Bootloader downgrade protection Bootloader downgrade protection Aug 24, 2019
@conorpp
Copy link
Member

conorpp commented Aug 24, 2019

Awesome!

I like the use of version being the last data written / last_written_app_address = (uint8_t *)ptr + len - 8 + 4;.

It all looks good to me and building & programming the application works fine on old bootloader. I currently don't have spare devices to convert to secure to fully test -- @nickray do you have some?

@conorpp conorpp requested a review from nickray September 2, 2019 13:51
@conorpp conorpp assigned conorpp and unassigned conorpp Sep 2, 2019
@conorpp conorpp self-requested a review September 2, 2019 13:52
szszszsz added a commit to Nitrokey/pynitrokey that referenced this pull request Sep 7, 2019
To correctly calculate the hash of the application image. page -19 contains variable bootloader data now.

Related: solokeys/solo1#238
szszszsz added a commit to Nitrokey/pynitrokey that referenced this pull request Sep 7, 2019
Correct application image size for hash and signature calculation.

Related:
 solokeys/solo1#238
 Nitrokey/nitrokey-fido2-firmware#3
szszszsz added a commit to Nitrokey/pynitrokey that referenced this pull request Sep 7, 2019
To correctly calculate the hash of the application image. page -19 contains variable bootloader data now.

Related: solokeys/solo1#238
@szszszsz
Copy link
Contributor Author

szszszsz commented Sep 7, 2019

Here are the required changes in the solo-python to make a valid signature for the new bootloader: solokeys/solo1-cli#36

@conorpp conorpp merged commit 9158453 into solokeys:master Oct 8, 2019
@conorpp
Copy link
Member

conorpp commented Nov 7, 2019

@all-contributors please add @Nitrokey for code, tests, ideas

@allcontributors
Copy link
Contributor

@conorpp

I've put up a pull request to add @Nitrokey! 🎉

@szszszsz szszszsz deleted the bootloader-downgrade-protection branch January 22, 2020 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants