From a396196ca37eabc7f9940a2848fdf2b621f16a60 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Tue, 19 Apr 2022 22:54:08 +0300 Subject: [PATCH 1/5] Correctly offset when sigLen is present --- cores/esp8266/Updater.cpp | 85 ++++++++++++++++++++++++--------------- 1 file changed, 52 insertions(+), 33 deletions(-) diff --git a/cores/esp8266/Updater.cpp b/cores/esp8266/Updater.cpp index c194c1d23f..53b0781ccb 100644 --- a/cores/esp8266/Updater.cpp +++ b/cores/esp8266/Updater.cpp @@ -1,9 +1,12 @@ +#include #include "Updater.h" #include "eboot_command.h" #include #include #include "StackThunk.h" +#include + //#define DEBUG_UPDATER Serial #include @@ -224,71 +227,87 @@ bool UpdaterClass::end(bool evenIfRemaining){ } if (_verify) { + // If expectedSigLen is non-zero, we expect the last four bytes of the buffer to + // contain a matching length field, preceded by the bytes of the signature itself. + // But if expectedSigLen is zero, we expect neither a signature nor a length field; + static constexpr uint32_t sigSize = 4; const uint32_t expectedSigLen = _verify->length(); - // If expectedSigLen is non-zero, we expect the last four bytes of the buffer to - // contain a matching length field, preceded by the bytes of the signature itself. - // But if expectedSigLen is zero, we expect neither a signature nor a length field; + const uint32_t sigLenAddr = _startAddress + _size - sigSize; uint32_t sigLen = 0; +#ifdef DEBUG_UPDATER + DEBUG_UPDATER.printf_P(PSTR("[Updater] expected sigLen: %lu\n"), expectedSigLen); +#endif if (expectedSigLen > 0) { - ESP.flashRead(_startAddress + _size - sizeof(uint32_t), &sigLen, sizeof(uint32_t)); - } + ESP.flashRead(sigLenAddr, &sigLen, sigSize); #ifdef DEBUG_UPDATER - DEBUG_UPDATER.printf_P(PSTR("[Updater] sigLen: %d\n"), sigLen); + DEBUG_UPDATER.printf_P(PSTR("[Updater] sigLen from flash: %lu\n"), sigLen); #endif + } + if (sigLen != expectedSigLen) { _setError(UPDATE_ERROR_SIGN); _reset(); return false; } - int binSize = _size; + auto binSize = _size; if (expectedSigLen > 0) { - _size -= (sigLen + sizeof(uint32_t) /* The siglen word */); - } - _hash->begin(); + if (binSize < (sigLen + sigSize)) { + _setError(UPDATE_ERROR_SIGN); + _reset(); + return false; + } + binSize -= (sigLen + sigSize); #ifdef DEBUG_UPDATER - DEBUG_UPDATER.printf_P(PSTR("[Updater] Adjusted binsize: %d\n"), binSize); + DEBUG_UPDATER.printf_P(PSTR("[Updater] Adjusted size (without the signature and sigLen): %lu\n"), binSize); #endif - // Calculate the MD5 and hash using proper size - uint8_t buff[128] __attribute__((aligned(4))); - for(int i = 0; i < binSize; i += sizeof(buff)) { - ESP.flashRead(_startAddress + i, (uint32_t *)buff, sizeof(buff)); - size_t read = std::min((int)sizeof(buff), binSize - i); - _hash->add(buff, read); + } + + // Calculate hash of the payload, 128 bytes at a time + alignas(alignof(uint32_t)) uint8_t buff[128]; + + _hash->begin(); + for (uint32_t offset = 0; offset < binSize; offset += sizeof(buff)) { + auto len = std::min(sizeof(buff), binSize - offset); + ESP.flashRead(_startAddress + offset, reinterpret_cast(&buff[0]), len); + _hash->add(buff, len); } _hash->end(); + #ifdef DEBUG_UPDATER - unsigned char *ret = (unsigned char *)_hash->hash(); - DEBUG_UPDATER.printf_P(PSTR("[Updater] Computed Hash:")); - for (int i=0; i<_hash->len(); i++) DEBUG_UPDATER.printf(" %02x", ret[i]); - DEBUG_UPDATER.printf("\n"); + auto debugByteArray = [](const char *name, const unsigned char *hash, int len) { + DEBUG_UPDATER.printf_P("[Updater] %s:", name); + for (int i = 0; i < len; ++i) { + DEBUG_UPDATER.printf(" %02x", hash[i]); + } + DEBUG_UPDATER.printf("\n"); + }; + debugByteArray(PSTR("Computed Hash"), + reinterpret_cast(_hash->hash()), + _hash->len()); #endif - uint8_t *sig = nullptr; // Safe to free if we don't actually malloc + std::unique_ptr sig; if (expectedSigLen > 0) { - sig = (uint8_t*)malloc(sigLen); + const uint32_t sigAddr = _startAddress + binSize; + sig.reset(new (std::nothrow) uint8_t[sigLen]); if (!sig) { _setError(UPDATE_ERROR_SIGN); _reset(); return false; } - ESP.flashRead(_startAddress + binSize, sig, sigLen); + ESP.flashRead(sigAddr, sig.get(), sigLen); #ifdef DEBUG_UPDATER - DEBUG_UPDATER.printf_P(PSTR("[Updater] Received Signature:")); - for (size_t i=0; iverify(_hash, (void *)sig, sigLen)) { - free(sig); + if (!_verify->verify(_hash, sig.get(), sigLen)) { _setError(UPDATE_ERROR_SIGN); _reset(); return false; } - free(sig); + _size = binSize; // Adjust size to remove signature, not part of bin payload #ifdef DEBUG_UPDATER @@ -301,7 +320,7 @@ bool UpdaterClass::end(bool evenIfRemaining){ return false; } #ifdef DEBUG_UPDATER - else DEBUG_UPDATER.printf_P(PSTR("MD5 Success: %s\n"), _target_md5.c_str()); + else DEBUG_UPDATER.printf_P(PSTR("[Updater] MD5 Success: %s\n"), _target_md5.c_str()); #endif } From 40373f81c32b53c675bc7847dc1f619446375483 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Tue, 19 Apr 2022 22:54:36 +0300 Subject: [PATCH 2/5] Allocate fixed size array for the RSA verifier ref. https://github.com/earlephilhower/bearssl-esp8266/blob/6105635531027f5b298aa656d44be2289b2d434f/inc/bearssl_rsa.h#L257 (and should've probably changed the type to size_t) --- libraries/ESP8266WiFi/src/BearSSLHelpers.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp b/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp index 08549cc99a..8cb6dd40d0 100644 --- a/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp +++ b/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp @@ -938,9 +938,12 @@ uint32_t SigningVerifier::length() extern "C" bool SigningVerifier_verify(PublicKey *_pubKey, UpdaterHashClass *hash, const void *signature, uint32_t signatureLen) { if (_pubKey->isRSA()) { bool ret; - unsigned char vrf[hash->len()]; + unsigned char vrf[64]; + if (hash->len() > 64) { + return false; + } br_rsa_pkcs1_vrfy vrfy = br_rsa_pkcs1_vrfy_get_default(); - ret = vrfy((const unsigned char *)signature, signatureLen, hash->oid(), sizeof(vrf), _pubKey->getRSA(), vrf); + ret = vrfy((const unsigned char *)signature, signatureLen, hash->oid(), hash->len(), _pubKey->getRSA(), vrf); if (!ret || memcmp(vrf, hash->hash(), sizeof(vrf)) ) { return false; } else { From f6ac4dfeae5dea6b4ee56b0e64c7830778ae5d9b Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Thu, 19 May 2022 07:38:59 +0300 Subject: [PATCH 3/5] dup commit msg in the comment --- libraries/ESP8266WiFi/src/BearSSLHelpers.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp b/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp index 8cb6dd40d0..dcf04562e0 100644 --- a/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp +++ b/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp @@ -937,13 +937,14 @@ uint32_t SigningVerifier::length() // directly inside the class function for ease of use. extern "C" bool SigningVerifier_verify(PublicKey *_pubKey, UpdaterHashClass *hash, const void *signature, uint32_t signatureLen) { if (_pubKey->isRSA()) { - bool ret; - unsigned char vrf[64]; - if (hash->len() > 64) { + // see https://github.com/earlephilhower/bearssl-esp8266/blob/6105635531027f5b298aa656d44be2289b2d434f/inc/bearssl_rsa.h#L257 + static constexpr int HashLengthMax = 64; + unsigned char vrf[HashLengthMax]; + if (hash->len() > HashLengthMax) { return false; } br_rsa_pkcs1_vrfy vrfy = br_rsa_pkcs1_vrfy_get_default(); - ret = vrfy((const unsigned char *)signature, signatureLen, hash->oid(), hash->len(), _pubKey->getRSA(), vrf); + bool ret = vrfy((const unsigned char *)signature, signatureLen, hash->oid(), hash->len(), _pubKey->getRSA(), vrf); if (!ret || memcmp(vrf, hash->hash(), sizeof(vrf)) ) { return false; } else { From 822b9fbaebeac06e44c7190c44feea44e36e9a79 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Wed, 1 Jun 2022 11:23:06 +0300 Subject: [PATCH 4/5] make use of language size helpers --- cores/esp8266/Updater.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cores/esp8266/Updater.cpp b/cores/esp8266/Updater.cpp index 53b0781ccb..858b93d249 100644 --- a/cores/esp8266/Updater.cpp +++ b/cores/esp8266/Updater.cpp @@ -230,16 +230,16 @@ bool UpdaterClass::end(bool evenIfRemaining){ // If expectedSigLen is non-zero, we expect the last four bytes of the buffer to // contain a matching length field, preceded by the bytes of the signature itself. // But if expectedSigLen is zero, we expect neither a signature nor a length field; - static constexpr uint32_t sigSize = 4; + static constexpr uint32_t SigSize = sizeof(uint32_t); const uint32_t expectedSigLen = _verify->length(); - const uint32_t sigLenAddr = _startAddress + _size - sigSize; + const uint32_t sigLenAddr = _startAddress + _size - SigSize; uint32_t sigLen = 0; #ifdef DEBUG_UPDATER DEBUG_UPDATER.printf_P(PSTR("[Updater] expected sigLen: %lu\n"), expectedSigLen); #endif if (expectedSigLen > 0) { - ESP.flashRead(sigLenAddr, &sigLen, sigSize); + ESP.flashRead(sigLenAddr, &sigLen, SigSize); #ifdef DEBUG_UPDATER DEBUG_UPDATER.printf_P(PSTR("[Updater] sigLen from flash: %lu\n"), sigLen); #endif @@ -253,12 +253,12 @@ bool UpdaterClass::end(bool evenIfRemaining){ auto binSize = _size; if (expectedSigLen > 0) { - if (binSize < (sigLen + sigSize)) { + if (binSize < (sigLen + SigSize)) { _setError(UPDATE_ERROR_SIGN); _reset(); return false; } - binSize -= (sigLen + sigSize); + binSize -= (sigLen + SigSize); #ifdef DEBUG_UPDATER DEBUG_UPDATER.printf_P(PSTR("[Updater] Adjusted size (without the signature and sigLen): %lu\n"), binSize); #endif From 3d59f0d4f9a5b8f376ca584b8808a114ef9872cc Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Wed, 1 Jun 2022 11:23:42 +0300 Subject: [PATCH 5/5] also make use of language byte packing --- tools/signing.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tools/signing.py b/tools/signing.py index 40405288c9..9274bb537a 100755 --- a/tools/signing.py +++ b/tools/signing.py @@ -4,6 +4,7 @@ import argparse import hashlib import os +import struct import subprocess import sys @@ -30,7 +31,7 @@ def sign_and_write(data, priv_key, out_file): with open(out_file, "wb") as out: out.write(data) out.write(signout) - out.write(b'\x00\x01\x00\x00') + out.write(struct.pack("