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

Add support for creating self-decrypting binaries, and use 4-way AES key shares instead of just the AES key #207

Open
wants to merge 73 commits into
base: develop
Choose a base branch
from

Conversation

will-v-pi
Copy link
Contributor

@will-v-pi will-v-pi commented Feb 24, 2025

This adds support for creating self-decrypting binaries, which can be created using

picotool encrypt --sign --embed hello_world.elf hello_world.enc.elf my_aesfile.bin my_iv_salt.bin my_sigfile.pem my_otp.json

This introduces a breaking change to picotool encrypt to take a 4-way AES key share, rather than just taking an AES key, as this makes it simpler to mask the power signature when decrypting the binary. The only difference from a user perspective is that they now need to use a 1024 bit binary file instead of the 256 bit file used before. The AES key is derived from the 4-way share as follows:

aes_key.words[i] = aes_key_share.words[i*4]
                 ^ aes_key_share.words[i*4 + 1]
                 ^ aes_key_share.words[i*4 + 2]
                 ^ aes_key_share.words[i*4 + 3];

This also introduces a breaking change that picotool encrypt now requires an IV salt binary to be passed to it as the 4th file, so the signing_key is now the 5th file and the OTP JSON is now the 6th file.

This PR requires raspberrypi/pico-sdk#2233 so should be merged after that

@kilograham
Copy link
Contributor

kilograham commented Mar 22, 2025

  1. I had forgotten? that encrypt was already an available option. Was that used with the pre-existing pico-example? How does this change affect that example (is the encrypt command now backwards compatible? (edit: i guess i should read the PR description!)
  2. I worry about rollback versions (and indeed other versions) - we should probably copy that across to the decrypting header
  3. I believe the dealing with TBYB and friends IS handled (due to pico-sdk PR) just wanted to double check with you - i still need to delve into this PR more deeply

@will-v-pi
Copy link
Contributor Author

  1. I had forgotten? that encrypt was already an available option. Was that used with the pre-existing pico-example? How does this change affect that example (is the encrypt command now backwards compatible? (edit: i guess i should read the PR description!)
  2. I worry about rollback versions (and indeed other versions) - we should probably copy that across to the decrypting header
  3. I believe the dealing with TBYB and friends IS handled (due to pico-sdk PR) just wanted to double check with you - i still need to delve into this PR more deeply
  1. I've modified the example to use the new command Encrypted improvements pico-examples#619 - but yes, the SDK CMake function and the picotool command are not backwards compatible.
  2. They should have been copied across, along with TBYB - lines 5124-5139 in main.cpp - but this wasn't working as the block choice code would choose the first block to copy data from. I've changed this so it will now choose to copy the last block if it has an IMAGE_DEF, otherwise choose the first block, as the last block will be the better choice.
  3. For self-decrypting binaries, TBYB and everything else is handled entirely by the bootrom - the decryption stage shouldn't time out the watchdog, so the user binary can call explicit_buy as usual. The decryption stage just gets loaded into SRAM along with the encrypted binary by the bootrom, and decrypts it in place, so doesn't need any handling for TBYB etc. For encrypted binaries with a separate decrypting bootloader, the handling is improved by the rom_pick_ab_update_partition PR.

@kilograham
Copy link
Contributor

A few other random thoughts

  1. I think we should support passing a 256 bit AES key and generating 4 shares (I dont think this tells you much if the AES key is secure random even if picotool does less well -though it should be using dev/random already, right via c++ lib if i reacll)
  2. I think we should support passing the AES key and the IV salt as a hex on the command line too. could probably live with just treating anything starting with 0x as hex not filename, and check it is the right length.
  3. Can we lose the -t on the AES key and salt unless there are useful different file types

@kilograham
Copy link
Contributor

I wonder if we should explicitly zero unused RAM in the LOAD_MAP of the self-encrypting binary (probably)

will-v-pi and others added 20 commits April 1, 2025 14:31
If the last block has an image_def, then it is a better choice of block than the first block, so the items for the new block should be copied from it instead
enc_bootloader requires SDK 2.1.2

rename binary_data to embedded_data

move mbedtls TODO into header
expand comment on configuring mbedtls
2.0.0->2.1.2 message
reword mbedtls todo
These would mess up signing/encryption of an already signed/encrypted binary
Remove they -t argument from all key files (bin & pem) and JSON files, as they should always have the correct extension so it's not necessary
Implement suggested changes, and add note about IV salt
Specify file types where useful for untyped files (json, pem, bin)
Expand IV salt description
Abstract filename_to_hex_array into separate function
@will-v-pi
Copy link
Contributor Author

Rebased onto the latest develop branch

Comment on lines +2721 to +2722
bool filename_to_hex_array(uint8_t idx, uint8_t *array, size_t size) {
auto filename = settings.filenames[idx];
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO this function would feel more self-contained if it was directly passed the string, instead of being passed an index into an array? 😕

if (!filename.empty() && filename.find("0x") == 0) {
// Hex string instead of file
if (filename.size() != size*2 + 2) {
fail(ERROR_ARGS, "Hex string must be %d characters long (the supplied string is %d characters)", size*2, filename.size() - 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, maybe this function should also be passed some kind of identifier to be used in the error-messages? 🤔 Because I guess if you passed both the AES key and also the salt by hex-value, then with this current message you wouldn't know which one of them was wrong!

Remove uneccessary extra setup, which isn't actually required and throws deprecation warnings in CMake 4.0.0
…ach OTP page

Disabled for now behind `#if FIB_WORKAROUND`, until the bootloader change to handle this is implemented
@will-v-pi
Copy link
Contributor Author

Quick note about TBYB functionality - TLDR is that after being bought, the encrypted binary will need to call rom_explicit_buy(NULL, 0) on every boot to cancel the watchdog, but other than that everything works as expected - I think this should be fine

For TBYB to work, it must be set on both the outer decrypting bootloader and the inner encrypted binary. This is because if it is only set on the outer bootloader, then the chain_image call with clear the TBYB pending flag, so the inner binary won't call explicit_buy correctly.

With it set on both the outer bootloader and the inner binary, on first boot both the bootloader and inner binary will boot with TBYB, and then the inner binary can call explicit_buy as normal to clear the flag on the outer bootloader (and write OTP, do erase, etc). It is not possible to clear the flag on the inner binary, as it is encrypted, so you can't just modify one bit.

On subsequent boots, the outer bootloader will boot without TBYB, but the inner binary will have TBYB set by the chain_image call - therefore the inner binary will need to call explicit_buy every boot, to disable the watchdog. Because this explicit_buy will never do anything more than disable the watchdog, you can pass (NULL, 0) to it instead of a real buffer, as it does not need a buffer if no flash operations are being performed.

The following would be an example of the TBYB process for an encrypted binary:

boot_info_t boot_info = {};
    int ret = rom_get_boot_info(&boot_info);
    if (ret) {
        if (boot_info.tbyb_and_update_info & BOOT_TBYB_AND_UPDATE_FLAG_BUY_PENDING) {
            uint32_t flash_update_base = boot_info.reboot_params[0];
            if (flash_update_base) {
                // Do self-check etc
            }
            uint32_t buf_size = flash_update_base ? 4096 : 0;
            uint8_t* buffer = flash_update_base ? malloc(buf_size) : NULL;
            int ret = rom_explicit_buy(buffer, buf_size);
            assert(ret == 0);
            if (buffer) free(buffer);
        }
    }

as opposed to a normal binary which wouldn't need to check the flash_update_base because BOOT_TBYB_AND_UPDATE_FLAG_BUY_PENDING will only be set on first boot

These only cause issues when encrypting, as the old block needs to be included in the new load_map

When signing, the old load_map can be used again without issue
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.

"ERROR: Found overlapping memory" on encrypted binary
3 participants