8000 Fix memory related issues w/BearSSL server/client · esp8266/Arduino@a1ee21e · GitHub
[go: up one dir, main page]
Skip to content

Commit a1ee21e

Browse files
earlephilhowerearlephilhower
committed
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.
1 parent 1cacf92 commit a1ee21e

File tree

4 files changed

+63
-60
lines changed

4 files changed

+63
-60
lines changed

‎libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp

Lines changed: 29 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -79,9 +79,8 @@ void WiFiClientSecure::_clear() {
7979
_recvapp_buf = nullptr;
8080
_recvapp_len = 0;
8181
_oom_err = false;
82-
_deleteChainKeyTA = false;
8382
_session = nullptr;
84-
_cipher_list = NULL;
83+
_cipher_list = nullptr;
8584
_cipher_cnt = 0;
8685
}
8786

@@ -92,6 +91,9 @@ void WiFiClientSecure::_clearAuthenticationSettings() {
9291
_knownkey = nullptr;
9392
_sk = nullptr;
9493
_ta = nullptr;
94+
_axtls_ta = nullptr;
95+
_axtls_chain = nullptr;
96+
_axtls_sk = nullptr;
9597
}
9698

9799

@@ -102,20 +104,23 @@ WiFiClientSecure::WiFiClientSecure() : WiFiClient() {
102104
stack_thunk_add_ref();
103105
}
104106

107+
WiFiClientSecure::WiFiClientSecure(const WiFiClientSecure &rhs) : WiFiClient(rhs) {
108+
*this = rhs;
109+
stack_thunk_add_ref();
110+
}
111+
105112
WiFiClientSecure::~WiFiClientSecure() {
106113
if (_client) {
107114
_client->unref();
108115
_client = nullptr;
109116
}
110-
free(_cipher_list);
117+
_cipher_list = nullptr; // std::shared will free if last reference
111118
_freeSSL();
112-
// Serial.printf("Max stack usage: %d bytes\n", br_thunk_get_max_usage());
113119
stack_thunk_del_ref();
114-
if (_deleteChainKeyTA) {
115-
delete _ta;
116-
delete _chain;
117-
delete _sk;
118-
}
120+
// Clean up any dangling axtls compat structures, if needed
121+
_axtls_ta = nullptr;
122+
_axtls_chain = nullptr;
123+
_axtls_sk = nullptr;
119124
}
120125

121126
WiFiClientSecure::WiFiClientSecure(ClientContext* client,
@@ -808,12 +813,12 @@ extern "C" {
808813

809814
// Set custom list of ciphers
810815
bool WiFiClientSecure::setCiphers(const uint16_t *cipherAry, int cipherCount) {
811-
free(_cipher_list);
812-
_cipher_list = (uint16_t *)malloc(cipherCount * sizeof(uint16_t));
813-
if (!_cipher_list) {
816+
_cipher_list = nullptr;
817+
_cipher_list = std::shared_ptr<uint16_t>(new uint16_t[cipherCount], std::default_delete<uint16_t[]>());
818+
if (!_cipher_list.get()) {
814819
return false;
815820
}
816-
memcpy_P(_cipher_list, cipherAry, cipherCount * sizeof(uint16_t));
821+
memcpy_P(_cipher_list.get(), cipherAry, cipherCount * sizeof(uint16_t));
817822
_cipher_cnt = cipherCount;
818823
return true;
819824
}
@@ -895,10 +900,10 @@ bool WiFiClientSecure::_connectSSL(const char* hostName) {
895900
}
896901

897902
// If no cipher list yet set, use defaults
898-
if (_cipher_list == NULL) {
903+
if (_cipher_list.get() == nullptr) {
899904
br_ssl_client_base_init(_sc.get(), suites_P, sizeof(suites_P) / sizeof(suites_P[0]));
900905
} else {
901-
br_ssl_client_base_init(_sc.get(), _cipher_list, _cipher_cnt);
906+
br_ssl_client_base_init(_sc.get(), _cipher_list.get(), _cipher_cnt);
902907
}
903908
// Only failure possible in the installation is OOM
904909
if (!_installClientX509Validator()) {
@@ -1300,32 +1305,23 @@ bool WiFiClientSecure::probeMaxFragmentLength(IPAddress ip, uint16_t port, uint1
13001305

13011306
// AXTLS compatibility interfaces
13021307
bool WiFiClientSecure::setCACert(const uint8_t* pk, size_t size) {
1303-
if (_ta && _deleteChainKeyTA) {
1304-
delete _ta;
1305-
_ta = nullptr;
1306-
}
1307-
_ta = new X509List(pk, size);
1308-
_deleteChainKeyTA = true;
1308+
_axtls_ta = nullptr;
1309+
_axtls_ta = std::shared_ptr<X509List>(new X509List(pk, size));
1310+
_ta = _axtls_ta.get();
13091311
return _ta ? true : false;
13101312
}
13111313

13121314
bool WiFiClientSecure::setCertificate(const uint8_t* pk, size_t size) {
1313-
if (_chain && _deleteChainKeyTA) {
1314-
delete _chain;
1315-
_chain = nullptr;
1316-
}
1317-
_chain = new X509List(pk, size);
1318-
_deleteChainKeyTA = true;
1315 F438 +
_axtls_chain = nullptr;
1316+
_axtls_chain = std::shared_ptr<X509List>(new X509List(pk, size));
1317+
_chain = _axtls_chain.get();
13191318
return _chain ? true : false;
13201319
}
13211320

13221321
bool WiFiClientSecure::setPrivateKey(const uint8_t* pk, size_t size) {
1323-
if (_sk && _deleteChainKeyTA) {
1324-
delete _sk;
1325-
_sk = nullptr;
1326-
}
1327-
_sk = new PrivateKey(pk, size);
1328-
_deleteChainKeyTA = true;
1322+
_axtls_sk = nullptr;
1323+
_axtls_sk = std::shared_ptr<PrivateKey>(new PrivateKey(pk, size));
1324+
_sk = _axtls_sk.get();
13291325
return _sk ? true : false;
13301326

13311327
}

‎libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ namespace BearSSL {
3434
class WiFiClientSecure : public WiFiClient {
3535
public:
3636
WiFiClientSecure();
37+
WiFiClientSecure(const WiFiClientSecure &rhs);
3738
~WiFiClientSecure() override;
3839

3940
int connect(CONST IPAddress& ip, uint16_t port) override;
@@ -206,6 +207,14 @@ class WiFiClientSecure : public WiFiClient {
206207
bool _handshake_done;
207208
bool _oom_err;
208209

210+
// AXTLS compatibility shim elements:
211+
// AXTLS managed memory for certs and keys, while BearSSL assumes
212+
// the app manages these. Use this local storage for holding the
213+
// BearSSL created objects in a shared form.
214+
std::shared_ptr<X509List> _axtls_ta;
215+
std::shared_ptr<X509List> _axtls_chain;
216+
std::shared_ptr<PrivateKey> _axtls_sk;
217+
209218
// Optional storage space pointer for session parameters
210219
// Will be used on connect and updated on close
211220
Session *_session;
@@ -218,7 +227,7 @@ class WiFiClientSecure : public WiFiClient {
218227
unsigned _knownkey_usages;
219228

220229
// Custom cipher list pointer or NULL if default
221-
uint16_t *_cipher_list;
230+
std::shared_ptr<uint16_t> _cipher_list;
222231
uint8_t _cipher_cnt;
223232

224233
unsigned char *_recvapp_buf;
@@ -255,9 +264,6 @@ class WiFiClientSecure : public WiFiClient {
255264
bool _installServerX509Validator(const X509List *client_CA_ta); // Setup X509 client cert validation, if supplied
256265

257266
uint8_t *_streamLoad(Stream& stream, size_t size);
258-
259-
// AXTLS compatible mode needs to delete the stored certs and keys on destruction
260-
bool _deleteChainKeyTA;
261267
};
262268

263269
};

‎libraries/ESP8266WiFi/src/WiFiServerSecureBearSSL.cpp

Lines changed: 18 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -49,13 +49,15 @@ WiFiServerSecure::WiFiServerSecure(uint16_t port) : WiFiServer(port) {
4949
stack_thunk_add_ref();
5050
}
5151

52-
// Destructor only checks if we need to delete compatibilty cert/key
52+
WiFiServerSecure::WiFiServerSecure(const WiFiServerSecure &rhs) : WiFiServer(rhs) {
53+
*this = rhs;
54+
stack_thunk_add_ref();
55+
}
56+
5357
WiFiServerSecure::~WiFiServerSecure() {
5458
stack_thunk_del_ref();
55-
if (_deleteChainAndKey) {
56-
delete _chain;
57-
delete _sk;
58-
}
59+
_axtls_chain = nullptr;
60+
_axtls_sk = nullptr;
5961
}
6062

6163
// 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
7678
// Return a client if there's an available connection waiting. If one is returned,
7779
// then any validation (i.e. client cert checking) will have succeeded.
7880
WiFiClientSecure WiFiServerSecure::available(uint8_t* status) {
79-
WiFiClientSecure client;
80-
8181
(void) status; // Unused
8282
if (_unclaimed) {
8383
if (_sk && _sk->isRSA()) {
8484
WiFiClientSecure result(_unclaimed, _chain, _sk, _iobuf_in_size, _iobuf_out_size, _client_CA_ta);
8585
_unclaimed = _unclaimed->next();
8686
result.setNoDelay(_noDelay);
8787
DEBUGV("WS:av\r\n");
88-
client = result;
88+
return result;
8989
} else if (_sk && _sk->isEC()) {
9090
WiFiClientSecure result(_unclaimed, _chain, _cert_issuer_key_type, _sk, _iobuf_in_size, _iobuf_out_size, _client_CA_ta);
9191
_unclaimed = _unclaimed->next();
9292
result.setNoDelay(_noDelay);
9393
DEBUGV("WS:av\r\n");
94-
client = result;
94+
return result;
9595
} else {
9696
// No key was defined, so we can't actually accept and attempt accept() and SSL handshake.
9797
DEBUGV("WS:nokey\r\n");
9898
}
99-
} else {
100-
optimistic_yield(1000);
10199
}
102-
return client;
100+
101+
// Something weird, return a no-op object
102+
optimistic_yield(1000);
103+
return WiFiClientSecure();
103104
}
104105

105106

106107
void WiFiServerSecure::setServerKeyAndCert(const uint8_t *key, int keyLen, const uint8_t *cert, int certLen) {
107-
X509List *chain = new X509List(cert, certLen);
108-
PrivateKey *sk = new PrivateKey(key, keyLen);
109-
if (!chain || !key) {
110-
// OOM, fail gracefully
111-
delete chain;
112-
delete sk;
113-
return;
114-
}
115-
_deleteChainAndKey = true;
116-
setRSACert(chain, sk);
108+
_axtls_chain = nullptr;
109+
_axtls_sk = nullptr;
110+
_axtls_chain = std::shared_ptr<X509List>(new X509List(cert, certLen));
111+
_axtls_sk = std::shared_ptr<PrivateKey>(new PrivateKey(key, keyLen));
112+
setRSACert(_axtls_chain.get(), _axtls_sk.get());
117113
}
118114

119115
void WiFiServerSecure::setServerKeyAndCert_P(const uint8_t *key, int keyLen, const uint8_t *cert, int certLen) {

‎libraries/ESP8266WiFi/src/WiFiServerSecureBearSSL.h

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ class WiFiServerSecure : public WiFiServer {
3333
public:
3434
WiFiServerSecure(IPAddress addr, uint16_t port);
3535
WiFiServerSecure(uint16_t port);
36+
WiFiServerSecure(const WiFiServerSecure &rhs);
3637
virtual ~WiFiServerSecure();
3738

3839
// Override the default buffer sizes, if you know what you're doing...
@@ -68,7 +69,11 @@ class WiFiServerSecure : public WiFiServer {
6869
int _iobuf_in_size = BR_SSL_BUFSIZE_INPUT;
6970
int _iobuf_out_size = 837;
7071
const X509List *_client_CA_ta = nullptr;
71-
bool _deleteChainAndKey = false;
72+
73+
// axTLS compat
74+
std::shared_ptr<X509List> _axtls_chain;
75+
std::shared_ptr<PrivateKey> _axtls_sk;
76+
7277
};
7378

7479
};

0 commit comments

Comments
 (0)
0