From a0c7a85649a3c48c05e70f3014769af23d01ec3b Mon Sep 17 00:00:00 2001 From: Max Prokhorov Date: Tue, 13 Sep 2022 15:57:42 +0300 Subject: [PATCH] Fix Updater non-zero _verify->length() once again (#8545) Amends #8507 I took the liberty to also do some refactoring; specifically, fixing signed vs. unsigned mismatch in len, using pointer object vs. the original manual malloc & free, try to have named constants for certain addresses and lengths, plus localize printing of u8 arrays. The suggested test to have a 'dummy' verifier works just fine. (...how it actually works and gets the hash to compare with is a whole other question, though) Another issue noticed while testing, in the underlying bearssl api there's an actual limit for hash length. https://github.com/earlephilhower/bearssl-esp8266/blob/6105635531027f5b298aa656d44be2289b2d434f/inc/bearssl_rsa.h#L257 --- cores/esp8266/Updater.cpp | 87 ++++++++++++-------- libraries/ESP8266WiFi/src/BearSSLHelpers.cpp | 10 ++- tools/signing.py | 5 +- 3 files changed, 63 insertions(+), 39 deletions(-) diff --git a/cores/esp8266/Updater.cpp b/cores/esp8266/Updater.cpp index c194c1d23..858b93d24 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 = sizeof(uint32_t); 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; - if (expectedSigLen > 0) { - ESP.flashRead(_startAddress + _size - sizeof(uint32_t), &sigLen, sizeof(uint32_t)); - } #ifdef DEBUG_UPDATER - DEBUG_UPDATER.printf_P(PSTR("[Updater] sigLen: %d\n"), sigLen); + DEBUG_UPDATER.printf_P(PSTR("[Updater] expected sigLen: %lu\n"), expectedSigLen); #endif + if (expectedSigLen > 0) { + ESP.flashRead(sigLenAddr, &sigLen, SigSize); +#ifdef DEBUG_UPDATER + 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 } diff --git a/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp b/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp index 08549cc99..dcf04562e 100644 --- a/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp +++ b/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp @@ -937,10 +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[hash->len()]; + // 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(), sizeof(vrf), _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 { diff --git a/tools/signing.py b/tools/signing.py index 40405288c..9274bb537 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("