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

Adds feature to decrypt uploaded image bin files. Used esp-idf to encrypt a bin file. #5807

Merged
merged 20 commits into from
Feb 7, 2024

Conversation

theeprawn
Copy link
Contributor

This adds decryption buffer before writing images to Flash in Update.cpp. Uses esp-idf 'espsecure.py encrypt_flash_data' with AES 256bit key(32bytes) address offset for tweaking/salting the key when flash_crypt_conf is non-zero.

Also added example sketch, with ota_key, unencrypted image file and same file encrypted using same setting as in example file.

I been using this method for over a year for production code for upgrading files either from webserver using HTTPClient or as in example using webserver on ESP32 to upload image files via a webbrowser. This way the production code is always encrypted. The ESP32 flash encryption is enabled as well, using different AES key set in efuses, the Update writes to encrypted partition.
So flow would be as OTA encrypted image is uploaded, Update decrypts it back to uncrypted data using ota_key, ota_address and flash_crypt_conf setting, then Update encrypts the unencrpted data to flash memory it using the efuse key and settings.

Check beginning of _writeBuffer() this is where image is decrypted then pass it on as before.

add function for setting up decryption
setupCrypt(const uint8_t *cryptKey, size_t cryptAddress, uint8_t cryptConfig, int cryptMode)

and some helper functions
setCryptKey(const uint8_t *cryptKey)
setCryptAddress(const size_t cryptAddress)
setCryptConfig(const uint8_t cryptConfig)
setCryptMode(const int cryptMode)

Hope that all makes sense, I not so great at explaining things or how to contribute

@CLAassistant
Copy link

CLAassistant commented Oct 26, 2021

CLA assistant check
All committers have signed the CLA.

#define ENCRYPTED_TWEAK_BLOCK_SIZE 32
#define ENCRYPTED_KEY_SIZE 32

#define U_AES_DECRYPT_NONE 0
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to separate the option for app and spiffs and use something like:

#define U_AES_DECRYPT_NONE       0
#define U_AES_DECRYPT_AUTO       1
#define U_AES_DECRYPT_ON         2

This will make the example and option selection much easier for the users

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sound good with me, I will simplify user options. Probably just go with those three as you listed and then user can decide image tyoe(app, spiffs, ..) with command U_FLASH, U_SPIFFS when calling Upate.begin() function. I make changes & retest. Note auto only works with app image atm, using magic byte check, I will check but dont think spiffs using a special byte at begining of image.
One question, I see only latest Arduino test are successfull, version 14 or earlier fail because #include <hwcrypto/aes.h> not found. I not got earlier version to test with to see if there a simple solution.

@me-no-dev me-no-dev added the Status: Test needed Issue needs testing label Nov 9, 2021
@@ -5,6 +5,7 @@
#include <MD5Builder.h>
#include <functional>
#include "esp_partition.h"
#include <hwcrypto/aes.h>
Copy link
Member

Choose a reason for hiding this comment

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

change to "aes/esp_aes.h"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated files with changes and hopefully made more understandable comments and easier to follow tweak code. I tested with encrypted image files. The image file was encrypted using espsecure, tested with crypt config value 0x0 to 0xf and a range of address values from 0x00000000 throught to 0x00800000. All were succesfully loaded and ran on esp-wroom32 module.
I noticed encrypted addresses above 0x00fffff0 aren't decrypt correctly, it seems espsecure.py tweak code doesnt limit lower 24bit address bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @me-no-dev I've updated Ardunio-esp32 v2.0.1 it was on v1.0.6 before, hence why I was using <hwcrypto/aes.h>. I see the additional boards manage url to json has changed. So now updated to test with 2.0.1, the "aes/esp_aes.h" removed the compiler error. Would it be possible to run workflows to check all is good.

@torntrousers
Copy link
Contributor

Hi @theeprawn , I'm really interested in this, especially the updating from webserver using HTTPClient that you mentioned but don't have an example of here. Do you have the code for an HTTClient update example that you could add to this PR so I could play around with it?

@theeprawn
Copy link
Contributor Author

Hi @torntrousers, sorry things abit busy here atm. I try write a example for firmware updating, but this PR should work on current HTTPclients that use 'update.h', just setup Crypt key and other settings as necessary, before image starts transfering, by calling 'Update.setupCrypt(OTA_KEY, ... );'

@theeprawn
Copy link
Contributor Author

Hi @torntrousers. Eventually found some time to write an example for using HTTPClient, included simplified server PHP code that can be used to by sketches to update firmware with newer versions. Also included direct download.
Note I removed '_verifyHeader' function from Update which only checks magic byte, but before buffer is decrypted so fails. '_verifyHeader' function wasnt needed because _writeBuffer also checks magic byte a little later after buffer been decrypted.

@stale
Copy link

stale bot commented Apr 16, 2022

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the wontfix label Apr 16, 2022
@VojtechBartoska VojtechBartoska added Status: Review needed Issue or PR is awaiting review and removed Status: Test needed Issue needs testing labels Jan 30, 2024
@VojtechBartoska
Copy link
Contributor

Hello @theeprawn, can you please sign CLA?

@VojtechBartoska VojtechBartoska added this to the 3.0.0-RC1 milestone Jan 30, 2024
@theeprawn
Copy link
Contributor Author

Hi @VojtechBartoska. Hopefully signed CLA now, sorry I still a bit of newie to how use github for posting code.

@VojtechBartoska
Copy link
Contributor

thanks @theeprawn, CLA is fine. Feel free to review @lucasssvaz & @P-R-O-C-H-Y. Thanks

Copy link
Contributor

github-actions bot commented Feb 5, 2024

Warnings
⚠️

Some issues found for the commit messages in this PR:

  • the commit message "Add files via upload":
    • summary looks empty
    • type/action looks empty
  • the commit message "Add files via upload":
    • summary looks empty
    • type/action looks empty
  • the commit message "Add files via upload":
    • summary looks empty
    • type/action looks empty
  • the commit message "Add files via upload":
    • summary looks empty
    • type/action looks empty
  • the commit message "Fix CI":
    • summary looks empty
    • type/action looks empty
  • the commit message "Fix comments and formatting":
    • summary looks empty
    • type/action looks empty
  • the commit message "Fix format":
    • summary looks empty
    • type/action looks empty
  • the commit message "Format example":
    • summary looks empty
    • type/action looks empty
  • the commit message "Remove binaries and QoL improvements":
    • summary looks empty
    • type/action looks empty
  • the commit message "Skip H2":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update Update.h":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update Update.h":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update Updater.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update Updater.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "Use new":
    • summary looks empty
    • type/action looks empty

Please fix these commit messages - here are some basic tips:

  • follow Conventional Commits style
  • correct format of commit message should be: <type/action>(<scope/component>): <summary>, for example fix(esp32): Fixed startup timeout issue
  • allowed types are: change,ci,docs,feat,fix,refactor,remove,revert,test
  • sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter
  • avoid Jira references in commit messages (unavailable/irrelevant for our customers)

TIP: Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).

⚠️ Please consider squashing your 20 commits (simplifying branch history).

👋 Hello theeprawn, we appreciate your contribution to this project!


Click to see more instructions ...


This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...


We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against e135775

@me-no-dev me-no-dev merged commit aceea3e into espressif:master Feb 7, 2024
39 checks passed
@Jason2866
Copy link
Collaborator

Jason2866 commented Feb 9, 2024

@theeprawn @lucasssvaz @me-no-dev
This PR brakes compile with the esp32-c2.

/home/runner/.platformio/packages/toolchain-riscv32-esp/bin/../lib/gcc/riscv32-esp-elf/12.2.0/../../../../riscv32-esp-elf/bin/ld: .pio/build/tasmota32c2/lib4b6/libUpdate.a(Updater.cpp.o): in function `UpdateClass::_decryptBuffer()':
/home/runner/.platformio/packages/framework-arduinoespressif32/libraries/Update/src/Updater.cpp:316: undefined reference to `esp_aes_init'
/home/runner/.platformio/packages/toolchain-riscv32-esp/bin/../lib/gcc/riscv32-esp-elf/12.2.0/../../../../riscv32-esp-elf/bin/ld: /home/runner/.platformio/packages/framework-arduinoespressif32/libraries/Update/src/Updater.cpp:325: undefined reference to `esp_aes_crypt_ecb'
/home/runner/.platformio/packages/toolchain-riscv32-esp/bin/../lib/gcc/riscv32-esp-elf/12.2.0/../../../../riscv32-esp-elf/bin/ld: /home/runner/.platformio/packages/framework-arduinoespressif32/libraries/Update/src/Updater.cpp:321: undefined reference to `esp_aes_setkey'

CI run where it fails https://github.com/Jason2866/platform-espressif32/actions/runs/7842916607/job/21402162987

Not all devices have hardware AES support. See in esp_config.h in the esp32c2 include path

/* The following units have ESP32 hardware support,
   uncommenting each _ALT macro will use the
   hardware-accelerated implementation. */
#ifdef CONFIG_MBEDTLS_HARDWARE_AES
#define MBEDTLS_AES_ALT
#else
#undef MBEDTLS_AES_ALT
#endif

Jason2866 added a commit to tasmota/arduino-esp32 that referenced this pull request Feb 9, 2024
* Update Updater.cpp
* Update Update.h
@lucasssvaz
Copy link
Collaborator

@Jason2866 Thanks for the heads up, I'll open a PR to fix it

Jason2866 added a commit to tasmota/arduino-esp32 that referenced this pull request Feb 9, 2024
Jason2866 added a commit to tasmota/arduino-esp32 that referenced this pull request Feb 9, 2024
Jason2866 added a commit to tasmota/arduino-esp32 that referenced this pull request Feb 9, 2024
* rm WPA2 Enterprise AP connect support (#324)
* Revert crypt update PR espressif#5807 (#326)
Jason2866 added a commit to tasmota/arduino-esp32 that referenced this pull request Feb 9, 2024
* rm WPA2 Enterprise AP connect support (#324)
* Revert crypt update PR espressif#5807 (#326)
Jason2866 added a commit to tasmota/arduino-esp32 that referenced this pull request Feb 22, 2024
* rm WPA2 Enterprise AP connect support (#324)
* Revert crypt update PR espressif#5807 (#326)
Jason2866 added a commit to tasmota/arduino-esp32 that referenced this pull request Feb 27, 2024
* rm WPA2 Enterprise AP connect support
* rm WiFiClientSecure
* Revert crypt update PR espressif#5807
* add ETH_PHY_JL1101
Jason2866 added a commit to tasmota/arduino-esp32 that referenced this pull request Feb 27, 2024
* rm components from idf_component.yml
* rm WPA2 Enterprise AP connect support
* rm WiFiClientSecure
* Revert crypt update PR espressif#5807
* add ETH_PHY_JL1101
Jason2866 added a commit to tasmota/arduino-esp32 that referenced this pull request Mar 5, 2024
* rm components from idf_component.yml
* rm WPA2 Enterprise AP connect support
* rm WiFiClientSecure
* Revert crypt update PR espressif#5807
* add ETH_PHY_JL1101
Jason2866 added a commit to tasmota/arduino-esp32 that referenced this pull request Mar 5, 2024
* rm components from idf_component.yml
* rm WPA2 Enterprise AP connect support
* rm WiFiClientSecure
* Revert crypt update PR espressif#5807
* add ETH_PHY_JL1101
@butaikis
Copy link

@theeprawn Hi, can you explain why here

if( mbedtls_aes_crypt_ecb( &ctx, MBEDTLS_AES_ENCRYPT, _cryptBuffer, _cryptBuffer ) ){ //use MBEDTLS_AES_ENCRYPT to decrypt flash code

why is MBEDTLS_AES_ENCRYPT used? There is a decryption process going on... (MBEDTLS_AES_DECRYPT)

@theeprawn
Copy link
Contributor Author

@butaikis Hi, FYI, copied from Flash Encryption Algorithm section. Hope that helps
https://docs.espressif.com/projects/esp-idf/en/stable/esp32/security/flash-encryption.html#manual-encryption

AES algorithm is used inverted in flash encryption, so the flash encryption "encrypt" operation is AES decrypt and the "decrypt" operation is AES encrypt. This is for performance reasons and does not alter the effeciency of the algorithm.

@butaikis
Copy link

@butaikis Hi, FYI, copied from Flash Encryption Algorithm section. Hope that helps https://docs.espressif.com/projects/esp-idf/en/stable/esp32/security/flash-encryption.html#manual-encryption

AES algorithm is used inverted in flash encryption, so the flash encryption "encrypt" operation is AES decrypt and the "decrypt" operation is AES encrypt. This is for performance reasons and does not alter the effeciency of the algorithm.

@theeprawn get it, thanks!

Jason2866 added a commit to tasmota/arduino-esp32 that referenced this pull request Mar 20, 2024
* rm components from idf_component.yml
* rm WPA2 Enterprise AP connect support
* rm WiFiClientSecure
* Revert crypt update PR espressif#5807
* add ETH_PHY_JL1101
* add LittleFS to partitions
* remove `chip-debug-report`
* check and include "chip-debug-report.h" when exists
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Review needed Issue or PR is awaiting review
Projects
Development

Successfully merging this pull request may close these issues.

8 participants