From a9fd6cba34c05577458523e17c0a638575935ceb Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Tue, 29 Mar 2022 12:53:09 -0700 Subject: [PATCH 1/3] Add move constructor/assignment to BearSSL helpers Fixes #8522 --- libraries/ESP8266WiFi/src/BearSSLHelpers.h | 80 ++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/libraries/ESP8266WiFi/src/BearSSLHelpers.h b/libraries/ESP8266WiFi/src/BearSSLHelpers.h index 397a557522..9903fa4a23 100644 --- a/libraries/ESP8266WiFi/src/BearSSLHelpers.h +++ b/libraries/ESP8266WiFi/src/BearSSLHelpers.h @@ -59,6 +59,21 @@ class PublicKey { // Disable the copy constructor, we're pointer based PublicKey(const PublicKey& that) = delete; + // Allow moves + PublicKey(PublicKey&& that) { + _key = that._key; + that._key = nullptr; + } + + PublicKey& operator=(PublicKey&& that) { + if (this != &that) { + free(_key); + _key = that._key; + that._key = nullptr; + } + return *this; + } + private: brssl::public_key *_key; }; @@ -87,6 +102,21 @@ class PrivateKey { // Disable the copy constructor, we're pointer based PrivateKey(const PrivateKey& that) = delete; + // Allow moves + PrivateKey(PrivateKey&& that) { + _key = that._key; + that._key = nullptr; + } + + PrivateKey& operator=(PrivateKey&& that) { + if (this != &that) { + free(_key); + _key = that._key; + that._key = nullptr; + } + return *this; + } + private: brssl::private_key *_key; }; @@ -123,6 +153,30 @@ class X509List { // Disable the copy constructor, we're pointer based X509List(const X509List& that) = delete; + // Allow moves + X509List(X509List&& that) { + _count = that._count; + _cert = that._cert; + _ta = that._ta; + that._count = 0; + that._cert = nullptr; + that._ta = nullptr; + } + + X509List& operator=(X509List&& that) { + if (this != &that) { + free(_cert); + free(_ta); + _count = that._count; + _cert = that._cert; + _ta = that._ta; + that._count = 0; + that._cert = nullptr; + that._ta = nullptr; + } + return *this; + } + private: size_t _count; br_x509_certificate *_cert; @@ -170,6 +224,32 @@ class ServerSessions { // Returns the number of sessions the cache can hold. uint32_t size() { return _size; } + // Disable the copy constructor, we're pointer based + ServerSessions(const ServerSessions& that) = delete; + + // Allow moves + ServerSessions(ServerSessions&& that) { + _size = that._size; + _store = that._store; + _isDynamic = that._isDynamic; + _cache = that._cache; + that._size = 0; + that._store = nullptr; + } + + ServerSessions& operator=(ServerSessions&& that) { + if (this != &that) { + free(_store); + _size = that._size; + _store = that._store; + _isDynamic = that._isDynamic; + _cache = that._cache; + that._size = 0; + that._store = nullptr; + } + return *this; + } + private: ServerSessions(ServerSession *sessions, uint32_t size, bool isDynamic); From 1291b12b1d8956edf0933bde480b69544b051d32 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Tue, 29 Mar 2022 18:49:31 -0700 Subject: [PATCH 2/3] Also protect the SigningVerifier class --- libraries/ESP8266WiFi/src/BearSSLHelpers.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/libraries/ESP8266WiFi/src/BearSSLHelpers.h b/libraries/ESP8266WiFi/src/BearSSLHelpers.h index 9903fa4a23..c987cac09e 100644 --- a/libraries/ESP8266WiFi/src/BearSSLHelpers.h +++ b/libraries/ESP8266WiFi/src/BearSSLHelpers.h @@ -291,6 +291,24 @@ class SigningVerifier : public UpdaterVerifyClass { SigningVerifier(PublicKey *pubKey) { _pubKey = pubKey; stack_thunk_add_ref(); } ~SigningVerifier() { stack_thunk_del_ref(); } + // Disable the copy constructor, we're pointer based + SigningVerifier(const SigningVerifier& that) = delete; + + // Allow moves + SigningVerifier(SigningVerifier&& that) { + _pubKey = that._pubKey; + that._pubKey = nullptr; + } + + SigningVerifier& operator=(SigningVerifier&& that) { + if (this != &that) { + free(_pubKey); + _pubKey = that._pubKey; + that._pubKey = nullptr; + } + return *this; + } + private: PublicKey *_pubKey; }; From 2d2d22b69d7d2d18de697186f27ad9d70307e3ba Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Thu, 31 Mar 2022 14:34:21 -0700 Subject: [PATCH 3/3] Use smart pointers to simplify code --- libraries/ESP8266WiFi/src/BearSSLHelpers.cpp | 83 ++++------- libraries/ESP8266WiFi/src/BearSSLHelpers.h | 139 ++++--------------- 2 files changed, 53 insertions(+), 169 deletions(-) diff --git a/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp b/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp index 08549cc99a..cfdc6bf77b 100644 --- a/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp +++ b/libraries/ESP8266WiFi/src/BearSSLHelpers.cpp @@ -651,21 +651,17 @@ namespace BearSSL { // ----- Public Key ----- PublicKey::PublicKey() { - _key = nullptr; } PublicKey::PublicKey(const char *pemKey) { - _key = nullptr; parse(pemKey); } PublicKey::PublicKey(const uint8_t *derKey, size_t derLen) { - _key = nullptr; parse(derKey, derLen); } PublicKey::PublicKey(Stream &stream, size_t size) { - _key = nullptr; auto buff = brssl::loadStream(stream, size); if (buff) { parse(buff, size); @@ -674,9 +670,6 @@ PublicKey::PublicKey(Stream &stream, size_t size) { } PublicKey::~PublicKey() { - if (_key) { - brssl::free_public_key(_key); - } } bool PublicKey::parse(const char *pemKey) { @@ -684,11 +677,8 @@ bool PublicKey::parse(const char *pemKey) { } bool PublicKey::parse(const uint8_t *derKey, size_t derLen) { - if (_key) { - brssl::free_public_key(_key); - _key = nullptr; - } - _key = brssl::read_public_key((const char *)derKey, derLen); + std::shared_ptr parsed (brssl::read_public_key((const char *)derKey, derLen), [](brssl::public_key *v){ brssl::free_public_key(v); } ); + _key = parsed; return _key ? true : false; } @@ -723,21 +713,17 @@ const br_ec_public_key *PublicKey::getEC() const { // ----- Private Key ----- PrivateKey::PrivateKey() { - _key = nullptr; } PrivateKey::PrivateKey(const char *pemKey) { - _key = nullptr; parse(pemKey); } PrivateKey::PrivateKey(const uint8_t *derKey, size_t derLen) { - _key = nullptr; parse(derKey, derLen); } PrivateKey::PrivateKey(Stream &stream, size_t size) { - _key = nullptr; auto buff = brssl::loadStream(stream, size); if (buff) { parse(buff, size); @@ -746,9 +732,6 @@ PrivateKey::PrivateKey(Stream &stream, size_t size) { } PrivateKey::~PrivateKey() { - if (_key) { - brssl::free_private_key(_key); - } } bool PrivateKey::parse(const char *pemKey) { @@ -756,11 +739,8 @@ bool PrivateKey::parse(const char *pemKey) { } bool PrivateKey::parse(const uint8_t *derKey, size_t derLen) { - if (_key) { - brssl::free_private_key(_key); - _key = nullptr; - } - _key = brssl::read_private_key((const char *)derKey, derLen); + std::shared_ptr parsed (brssl::read_private_key((const char *)derKey, derLen), [](brssl::private_key *v){ brssl::free_private_key(v); } ); + _key = parsed; return _key ? true : false; } @@ -795,30 +775,18 @@ const br_ec_private_key *PrivateKey::getEC() const { // ----- Certificate Lists ----- X509List::X509List() { - _count = 0; - _cert = nullptr; - _ta = nullptr; } X509List::X509List(const char *pemCert) { - _count = 0; - _cert = nullptr; - _ta = nullptr; append(pemCert); } X509List::X509List(const uint8_t *derCert, size_t derLen) { - _count = 0; - _cert = nullptr; - _ta = nullptr; append(derCert, derLen); } X509List::X509List(Stream &stream, size_t size) { - _count = 0; - _cert = nullptr; - _ta = nullptr; auto buff = brssl::loadStream(stream, size); if (buff) { append(buff, size); @@ -827,11 +795,13 @@ X509List::X509List(Stream &stream, size_t size) { } X509List::~X509List() { - brssl::free_certificates(_cert, _count); // also frees cert + brssl::free_certificates(_cert.get(), _count); // also frees cert for (size_t i = 0; i < _count; i++) { - brssl::free_ta_contents(&_ta[i]); + brssl::free_ta_contents(&_ta.get()[i]); } - free(_ta); + free(_ta.get()); + _cert.release(); + _ta.release(); } bool X509List::append(const char *pemCert) { @@ -846,47 +816,48 @@ bool X509List::append(const uint8_t *derCert, size_t derLen) { } // Add in the certificates - br_x509_certificate *saveCert = _cert; - _cert = (br_x509_certificate*)realloc(_cert, (numCerts + _count) * sizeof(br_x509_certificate)); - if (!_cert) { + auto newCert = (br_x509_certificate*)realloc(_cert.get(), (numCerts + _count) * sizeof(br_x509_certificate)); + if (!newCert) { free(newCerts); - _cert = saveCert; return false; } - memcpy(&_cert[_count], newCerts, numCerts * sizeof(br_x509_certificate)); + memcpy(&newCert[_count], newCerts, numCerts * sizeof(br_x509_certificate)); free(newCerts); + _cert.release(); + _cert.reset(newCert); // Build TAs for each certificate - br_x509_trust_anchor *saveTa = _ta; - _ta = (br_x509_trust_anchor*)realloc(_ta, (numCerts + _count) * sizeof(br_x509_trust_anchor)); - if (!_ta) { - _ta = saveTa; + auto newTa = (br_x509_trust_anchor*)realloc(_ta.get(), (numCerts + _count) * sizeof(br_x509_trust_anchor)); + if (!newTa) { return false; } for (size_t i = 0; i < numCerts; i++) { - br_x509_trust_anchor *newTa = brssl::certificate_to_trust_anchor(&_cert[_count + i]); + br_x509_trust_anchor *aTa = brssl::certificate_to_trust_anchor(&_cert.get()[_count + i]); if (newTa) { - _ta[_count + i ] = *newTa; - free(newTa); + newTa[_count + i ] = *aTa; + free(aTa); } else { return false; // OOM } } + _ta.release(); + _ta.reset(newTa); _count += numCerts; return true; } ServerSessions::~ServerSessions() { - if (_isDynamic && _store != nullptr) - delete _store; + if (!_isDynamic) { + _store.release(); + } } ServerSessions::ServerSessions(ServerSession *sessions, uint32_t size, bool isDynamic) : _size(sessions != nullptr ? size : 0), _store(sessions), _isDynamic(isDynamic) { if (_size > 0) - br_ssl_session_cache_lru_init(&_cache, (uint8_t*)_store, size * sizeof(ServerSession)); + br_ssl_session_cache_lru_init(&_cache, (uint8_t*)_store.get(), size * sizeof(ServerSession)); } const br_ssl_session_cache_class **ServerSessions::getCache() { @@ -961,9 +932,9 @@ extern "C" bool thunk_SigningVerifier_verify(PublicKey *_pubKey, UpdaterHashClas bool SigningVerifier::verify(UpdaterHashClass *hash, const void *signature, uint32_t signatureLen) { if (!_pubKey || !hash || !signature || signatureLen != length()) return false; #if !CORE_MOCK - return thunk_SigningVerifier_verify(_pubKey, hash, signature, signatureLen); + return thunk_SigningVerifier_verify(_pubKey.get(), hash, signature, signatureLen); #else - return SigningVerifier_verify(_pubKey, hash, signature, signatureLen); + return SigningVerifier_verify(_pubKey.get(), hash, signature, signatureLen); #endif } diff --git a/libraries/ESP8266WiFi/src/BearSSLHelpers.h b/libraries/ESP8266WiFi/src/BearSSLHelpers.h index c987cac09e..64fa617519 100644 --- a/libraries/ESP8266WiFi/src/BearSSLHelpers.h +++ b/libraries/ESP8266WiFi/src/BearSSLHelpers.h @@ -26,6 +26,7 @@ #include #include #include +#include // Internal opaque structures, not needed by user applications namespace brssl { @@ -56,26 +57,8 @@ class PublicKey { const br_rsa_public_key *getRSA() const; const br_ec_public_key *getEC() const; - // Disable the copy constructor, we're pointer based - PublicKey(const PublicKey& that) = delete; - - // Allow moves - PublicKey(PublicKey&& that) { - _key = that._key; - that._key = nullptr; - } - - PublicKey& operator=(PublicKey&& that) { - if (this != &that) { - free(_key); - _key = that._key; - that._key = nullptr; - } - return *this; - } - private: - brssl::public_key *_key; + std::shared_ptr _key = nullptr; }; // Holds either a single private RSA or EC key for use when BearSSL wants a secretkey. @@ -99,26 +82,8 @@ class PrivateKey { const br_rsa_private_key *getRSA() const; const br_ec_private_key *getEC() const; - // Disable the copy constructor, we're pointer based - PrivateKey(const PrivateKey& that) = delete; - - // Allow moves - PrivateKey(PrivateKey&& that) { - _key = that._key; - that._key = nullptr; - } - - PrivateKey& operator=(PrivateKey&& that) { - if (this != &that) { - free(_key); - _key = that._key; - that._key = nullptr; - } - return *this; - } - private: - brssl::private_key *_key; + std::shared_ptr _key = nullptr; }; // Holds one or more X.509 certificates and associated trust anchors for @@ -144,43 +109,29 @@ class X509List { return _count; } const br_x509_certificate *getX509Certs() const { - return _cert; + return _cert.get(); } const br_x509_trust_anchor *getTrustAnchors() const { - return _ta; + return _ta.get(); } - // Disable the copy constructor, we're pointer based - X509List(const X509List& that) = delete; - - // Allow moves - X509List(X509List&& that) { + // Enable move with the unique_ptr members + X509List& operator=(X509List&& that) { + if (this != &that) { _count = that._count; - _cert = that._cert; - _ta = that._ta; + _cert.reset(that._cert.get()); + _ta.reset(that._ta.get()); that._count = 0; - that._cert = nullptr; - that._ta = nullptr; - } - - X509List& operator=(X509List&& that) { - if (this != &that) { - free(_cert); - free(_ta); - _count = that._count; - _cert = that._cert; - _ta = that._ta; - that._count = 0; - that._cert = nullptr; - that._ta = nullptr; - } - return *this; + that._cert.release(); + that._ta.release(); + } + return *this; } private: - size_t _count; - br_x509_certificate *_cert; - br_x509_trust_anchor *_ta; + size_t _count = 0; + std::unique_ptr _cert = nullptr; + std::unique_ptr _ta = nullptr; }; // Opaque object which wraps the BearSSL SSL session to make repeated connections @@ -224,32 +175,6 @@ class ServerSessions { // Returns the number of sessions the cache can hold. uint32_t size() { return _size; } - // Disable the copy constructor, we're pointer based - ServerSessions(const ServerSessions& that) = delete; - - // Allow moves - ServerSessions(ServerSessions&& that) { - _size = that._size; - _store = that._store; - _isDynamic = that._isDynamic; - _cache = that._cache; - that._size = 0; - that._store = nullptr; - } - - ServerSessions& operator=(ServerSessions&& that) { - if (this != &that) { - free(_store); - _size = that._size; - _store = that._store; - _isDynamic = that._isDynamic; - _cache = that._cache; - that._size = 0; - that._store = nullptr; - } - return *this; - } - private: ServerSessions(ServerSession *sessions, uint32_t size, bool isDynamic); @@ -257,9 +182,9 @@ class ServerSessions { const br_ssl_session_cache_class **getCache(); // Size of the store in sessions. - uint32_t _size; + uint32_t _size = 0; // Store where the information for the sessions are stored. - ServerSession *_store; + std::unique_ptr _store = nullptr; // Whether the store is dynamically allocated. // If this is true, the store needs to be freed in the destructor. bool _isDynamic; @@ -288,29 +213,17 @@ class SigningVerifier : public UpdaterVerifyClass { virtual bool verify(UpdaterHashClass *hash, const void *signature, uint32_t signatureLen) override; public: - SigningVerifier(PublicKey *pubKey) { _pubKey = pubKey; stack_thunk_add_ref(); } - ~SigningVerifier() { stack_thunk_del_ref(); } - - // Disable the copy constructor, we're pointer based - SigningVerifier(const SigningVerifier& that) = delete; - - // Allow moves - SigningVerifier(SigningVerifier&& that) { - _pubKey = that._pubKey; - that._pubKey = nullptr; + SigningVerifier(PublicKey *pubKey) { + std::shared_ptr newKey (pubKey); + _pubKey = newKey; + stack_thunk_add_ref(); } - - SigningVerifier& operator=(SigningVerifier&& that) { - if (this != &that) { - free(_pubKey); - _pubKey = that._pubKey; - that._pubKey = nullptr; - } - return *this; + ~SigningVerifier() { + stack_thunk_del_ref(); } private: - PublicKey *_pubKey; + std::shared_ptr _pubKey = nullptr; }; // Stack thunked versions of calls