From a1ee21e31857775a86b0ffc3ca5111c4f48fde6f Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Fri, 1 Feb 2019 10:24:32 -0800 Subject: [PATCH] Fix memory related issues w/BearSSL server/client Because the constructors of the BSSL client and server add a reference count to the stack_thunk, if there is no copy constructor defined then the stack thunk reference count can get out of sync causing the stack thunk memory to be freed while still in use. That could cause random crashes or hangs. Add a very basic copy constructor to the WiFiClientSecure and WiFiServerSecure objects, using the default operator= to duplicate simple types and shared_ptr classes. The _cipher_list element (used only w/custom ciphers) could be freed while still in use if copies of the WiFiClientSecure object were made. Use a shared_ptr which will only free when the last reference is deleted. The axTLS compatibility mode calls allocate and store elements needed for SSL connections (unlike normal BearSSL calls). These elements could be freed mistakenly while still in use if copies of the WiFiClientSecure were made by the app. Convert to a separately managed shared_ptr to ensure they live as long as any referencing objects before deletion. Same done for the axTLS compatability for WiFiServerSecure. --- .../src/WiFiClientSecureBearSSL.cpp | 62 +++++++++---------- .../ESP8266WiFi/src/WiFiClientSecureBearSSL.h | 14 +++-- .../src/WiFiServerSecureBearSSL.cpp | 40 ++++++------ .../ESP8266WiFi/src/WiFiServerSecureBearSSL.h | 7 ++- 4 files changed, 63 insertions(+), 60 deletions(-) diff --git a/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp b/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp index a4fae44696..ac13adde49 100644 --- a/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp +++ b/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp @@ -79,9 +79,8 @@ void WiFiClientSecure::_clear() { _recvapp_buf = nullptr; _recvapp_len = 0; _oom_err = false; - _deleteChainKeyTA = false; _session = nullptr; - _cipher_list = NULL; + _cipher_list = nullptr; _cipher_cnt = 0; } @@ -92,6 +91,9 @@ void WiFiClientSecure::_clearAuthenticationSettings() { _knownkey = nullptr; _sk = nullptr; _ta = nullptr; + _axtls_ta = nullptr; + _axtls_chain = nullptr; + _axtls_sk = nullptr; } @@ -102,20 +104,23 @@ WiFiClientSecure::WiFiClientSecure() : WiFiClient() { stack_thunk_add_ref(); } +WiFiClientSecure::WiFiClientSecure(const WiFiClientSecure &rhs) : WiFiClient(rhs) { + *this = rhs; + stack_thunk_add_ref(); +} + WiFiClientSecure::~WiFiClientSecure() { if (_client) { _client->unref(); _client = nullptr; } - free(_cipher_list); + _cipher_list = nullptr; // std::shared will free if last reference _freeSSL(); - // Serial.printf("Max stack usage: %d bytes\n", br_thunk_get_max_usage()); stack_thunk_del_ref(); - if (_deleteChainKeyTA) { - delete _ta; - delete _chain; - delete _sk; - } + // Clean up any dangling axtls compat structures, if needed + _axtls_ta = nullptr; + _axtls_chain = nullptr; + _axtls_sk = nullptr; } WiFiClientSecure::WiFiClientSecure(ClientContext* client, @@ -808,12 +813,12 @@ extern "C" { // Set custom list of ciphers bool WiFiClientSecure::setCiphers(const uint16_t *cipherAry, int cipherCount) { - free(_cipher_list); - _cipher_list = (uint16_t *)malloc(cipherCount * sizeof(uint16_t)); - if (!_cipher_list) { + _cipher_list = nullptr; + _cipher_list = std::shared_ptr(new uint16_t[cipherCount], std::default_delete()); + if (!_cipher_list.get()) { return false; } - memcpy_P(_cipher_list, cipherAry, cipherCount * sizeof(uint16_t)); + memcpy_P(_cipher_list.get(), cipherAry, cipherCount * sizeof(uint16_t)); _cipher_cnt = cipherCount; return true; } @@ -895,10 +900,10 @@ bool WiFiClientSecure::_connectSSL(const char* hostName) { } // If no cipher list yet set, use defaults - if (_cipher_list == NULL) { + if (_cipher_list.get() == nullptr) { br_ssl_client_base_init(_sc.get(), suites_P, sizeof(suites_P) / sizeof(suites_P[0])); } else { - br_ssl_client_base_init(_sc.get(), _cipher_list, _cipher_cnt); + br_ssl_client_base_init(_sc.get(), _cipher_list.get(), _cipher_cnt); } // Only failure possible in the installation is OOM if (!_installClientX509Validator()) { @@ -1300,32 +1305,23 @@ bool WiFiClientSecure::probeMaxFragmentLength(IPAddress ip, uint16_t port, uint1 // AXTLS compatibility interfaces bool WiFiClientSecure::setCACert(const uint8_t* pk, size_t size) { - if (_ta && _deleteChainKeyTA) { - delete _ta; - _ta = nullptr; - } - _ta = new X509List(pk, size); - _deleteChainKeyTA = true; + _axtls_ta = nullptr; + _axtls_ta = std::shared_ptr(new X509List(pk, size)); + _ta = _axtls_ta.get(); return _ta ? true : false; } bool WiFiClientSecure::setCertificate(const uint8_t* pk, size_t size) { - if (_chain && _deleteChainKeyTA) { - delete _chain; - _chain = nullptr; - } - _chain = new X509List(pk, size); - _deleteChainKeyTA = true; + _axtls_chain = nullptr; + _axtls_chain = std::shared_ptr(new X509List(pk, size)); + _chain = _axtls_chain.get(); return _chain ? true : false; } bool WiFiClientSecure::setPrivateKey(const uint8_t* pk, size_t size) { - if (_sk && _deleteChainKeyTA) { - delete _sk; - _sk = nullptr; - } - _sk = new PrivateKey(pk, size); - _deleteChainKeyTA = true; + _axtls_sk = nullptr; + _axtls_sk = std::shared_ptr(new PrivateKey(pk, size)); + _sk = _axtls_sk.get(); return _sk ? true : false; } diff --git a/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h b/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h index 460ba1cc52..0ac9003c78 100644 --- a/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h +++ b/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h @@ -34,6 +34,7 @@ namespace BearSSL { class WiFiClientSecure : public WiFiClient { public: WiFiClientSecure(); + WiFiClientSecure(const WiFiClientSecure &rhs); ~WiFiClientSecure() override; int connect(CONST IPAddress& ip, uint16_t port) override; @@ -206,6 +207,14 @@ class WiFiClientSecure : public WiFiClient { bool _handshake_done; bool _oom_err; + // AXTLS compatibility shim elements: + // AXTLS managed memory for certs and keys, while BearSSL assumes + // the app manages these. Use this local storage for holding the + // BearSSL created objects in a shared form. + std::shared_ptr _axtls_ta; + std::shared_ptr _axtls_chain; + std::shared_ptr _axtls_sk; + // Optional storage space pointer for session parameters // Will be used on connect and updated on close Session *_session; @@ -218,7 +227,7 @@ class WiFiClientSecure : public WiFiClient { unsigned _knownkey_usages; // Custom cipher list pointer or NULL if default - uint16_t *_cipher_list; + std::shared_ptr _cipher_list; uint8_t _cipher_cnt; unsigned char *_recvapp_buf; @@ -255,9 +264,6 @@ class WiFiClientSecure : public WiFiClient { bool _installServerX509Validator(const X509List *client_CA_ta); // Setup X509 client cert validation, if supplied uint8_t *_streamLoad(Stream& stream, size_t size); - - // AXTLS compatible mode needs to delete the stored certs and keys on destruction - bool _deleteChainKeyTA; }; }; diff --git a/libraries/ESP8266WiFi/src/WiFiServerSecureBearSSL.cpp b/libraries/ESP8266WiFi/src/WiFiServerSecureBearSSL.cpp index d39237104c..087c90b4e8 100644 --- a/libraries/ESP8266WiFi/src/WiFiServerSecureBearSSL.cpp +++ b/libraries/ESP8266WiFi/src/WiFiServerSecureBearSSL.cpp @@ -49,13 +49,15 @@ WiFiServerSecure::WiFiServerSecure(uint16_t port) : WiFiServer(port) { stack_thunk_add_ref(); } -// Destructor only checks if we need to delete compatibilty cert/key +WiFiServerSecure::WiFiServerSecure(const WiFiServerSecure &rhs) : WiFiServer(rhs) { + *this = rhs; + stack_thunk_add_ref(); +} + WiFiServerSecure::~WiFiServerSecure() { stack_thunk_del_ref(); - if (_deleteChainAndKey) { - delete _chain; - delete _sk; - } + _axtls_chain = nullptr; + _axtls_sk = nullptr; } // Specify a RSA-signed certificate and key for the server. Only copies the pointer, the @@ -76,8 +78,6 @@ void WiFiServerSecure::setECCert(const X509List *chain, unsigned cert_issuer_key // Return a client if there's an available connection waiting. If one is returned, // then any validation (i.e. client cert checking) will have succeeded. WiFiClientSecure WiFiServerSecure::available(uint8_t* status) { - WiFiClientSecure client; - (void) status; // Unused if (_unclaimed) { if (_sk && _sk->isRSA()) { @@ -85,35 +85,31 @@ WiFiClientSecure WiFiServerSecure::available(uint8_t* status) { _unclaimed = _unclaimed->next(); result.setNoDelay(_noDelay); DEBUGV("WS:av\r\n"); - client = result; + return result; } else if (_sk && _sk->isEC()) { WiFiClientSecure result(_unclaimed, _chain, _cert_issuer_key_type, _sk, _iobuf_in_size, _iobuf_out_size, _client_CA_ta); _unclaimed = _unclaimed->next(); result.setNoDelay(_noDelay); DEBUGV("WS:av\r\n"); - client = result; + return result; } else { // No key was defined, so we can't actually accept and attempt accept() and SSL handshake. DEBUGV("WS:nokey\r\n"); } - } else { - optimistic_yield(1000); } - return client; + + // Something weird, return a no-op object + optimistic_yield(1000); + return WiFiClientSecure(); } void WiFiServerSecure::setServerKeyAndCert(const uint8_t *key, int keyLen, const uint8_t *cert, int certLen) { - X509List *chain = new X509List(cert, certLen); - PrivateKey *sk = new PrivateKey(key, keyLen); - if (!chain || !key) { - // OOM, fail gracefully - delete chain; - delete sk; - return; - } - _deleteChainAndKey = true; - setRSACert(chain, sk); + _axtls_chain = nullptr; + _axtls_sk = nullptr; + _axtls_chain = std::shared_ptr(new X509List(cert, certLen)); + _axtls_sk = std::shared_ptr(new PrivateKey(key, keyLen)); + setRSACert(_axtls_chain.get(), _axtls_sk.get()); } void WiFiServerSecure::setServerKeyAndCert_P(const uint8_t *key, int keyLen, const uint8_t *cert, int certLen) { diff --git a/libraries/ESP8266WiFi/src/WiFiServerSecureBearSSL.h b/libraries/ESP8266WiFi/src/WiFiServerSecureBearSSL.h index 3695e5f012..d104749f24 100644 --- a/libraries/ESP8266WiFi/src/WiFiServerSecureBearSSL.h +++ b/libraries/ESP8266WiFi/src/WiFiServerSecureBearSSL.h @@ -33,6 +33,7 @@ class WiFiServerSecure : public WiFiServer { public: WiFiServerSecure(IPAddress addr, uint16_t port); WiFiServerSecure(uint16_t port); + WiFiServerSecure(const WiFiServerSecure &rhs); virtual ~WiFiServerSecure(); // Override the default buffer sizes, if you know what you're doing... @@ -68,7 +69,11 @@ class WiFiServerSecure : public WiFiServer { int _iobuf_in_size = BR_SSL_BUFSIZE_INPUT; int _iobuf_out_size = 837; const X509List *_client_CA_ta = nullptr; - bool _deleteChainAndKey = false; + + // axTLS compat + std::shared_ptr _axtls_chain; + std::shared_ptr _axtls_sk; + }; };