Fix memory related issues w/BearSSL server/client (#5706)

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.

Author: earlephilhower

Diff for: 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<uint16_t>(new uint16_t[cipherCount], std::default_delete<uint16_t[]>());
+  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<X509List>(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<X509List>(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<PrivateKey>(new PrivateKey(pk, size));
+  _sk = _axtls_sk.get();
   return _sk ? true : false;
 
 }

Diff for: 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<X509List>   _axtls_ta;
+    std::shared_ptr<X509List>   _axtls_chain;
+    std::shared_ptr<PrivateKey> _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<uint16_t> _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 for: 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,44 +78,38 @@ 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()) {
       WiFiClientSecure result(_unclaimed, _chain, _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 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<X509List>(new X509List(cert, certLen));
+  _axtls_sk = std::shared_ptr<PrivateKey>(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 for: 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<X509List>   _axtls_chain;
+    std::shared_ptr<PrivateKey> _axtls_sk;
+
 };
 
 };