WiFiSecureClient "connect" corrupts [original] client cert [buffer]

[bkgoodman]

After a call to connect - WiFiSecureClient corrupts/truncates/overwrites the client cert (buffer passed into it). (This can be seen by adding a print of the certificate before and after the connect call).

This obviously renders subsequent calls to "connect" inoperable, as the cert is no longer valid, and possibly points to wider memory corruption.

I have traced the source corruption to the rmbedtls_pk_parse_key call within start_ssl_client.

I have no mbedtls source, so I don't know if it's a problem with the function, or if ssl_client->client_cert is of insufficient size to hold the parsed cert...

[me-no-dev]

here is mbedtls's source used to build the lib
https://github.com/espressif/esp-idf/tree/master/components/mbedtls

[copercini]

I think it's a good idea change the certificates to const char* instead of just char* it will solve issues like this...

[me-no-dev]

if the code underneath is trying to write to the buffer and you declare it const, you will get exception. Before making it const, it should be made not to modify it ;)

[copercini]

@bkgoodman Can you clone the WiFiClientSecure library from my fork and test if this issue was solved? https://github.com/copercini/arduino-esp32/tree/master/libraries/WiFiClientSecure/src

To do this you need to change your arduino code:
char* rootCABuff ="..." to const char* rootCABuff ="..."
char* certificateBuff ="..." to const char* certificateBuff ="..."
char* privateKeyBuff ="..." to const char* privateKeyBuff ="..."

and

setCACert((unsigned char*)rootCABuff); to setCACert(rootCABuff);
setCertificate((unsigned char*)certificateBuff); to setCertificate(certificateBuff);
setPrivateKey((unsigned char*)privateKeyBuff); to setPrivateKey(privateKeyBuff);

[bkgoodman]

I'll give it a shot - though just changing the buffers to "const" just tells the compiler to only allow "const" data types - but won't prevent a memory leak. Also - it could theoretically mask the problem by leaving the leak there, but putting my certs in memory which is physically read-only, thus masking a memory leak.

[bkgoodman]

So:

1. It did work a few times
2. It did blow up at one point when ret = mbedtls_ssl_handshake(&ssl_client->ssl_ctx)) != 0) failed and it kept going (hit a null pointer in subsequent printf - needs to abort the function on failure, as discussed before. (don't know exactly why it failed once)

I did see another failure at point point:

/Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/freertos/./queue.c:1445 (xQueueGenericReceive)- assert failed!
abort() was called at PC 0x400837e1

Backtrace was:

/Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/freertos/./queue.c:1998
/Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/freertos/./tasks.c:4399
/Users/bkg/Documents/Arduino/hardware/espressif/esp32/cores/esp32/esp32-hal-uart.c:277 
/Users/bkg/Documents/Arduino/hardware/espressif/esp32/cores/esp32/HardwareSerial.cpp:93
/Users/bkg/Documents/Arduino/hardware/espressif/esp32/cores/esp32/Print.cpp:227
cuments/Arduino/PumpHouseESP32/PumpHouseESP32.ino:318
/Users/bkg/Documents/Arduino/hardware/espressif/esp32/cores/esp32/esp32-hal-timer.c:179

The upper stuff (towards bottom) was my stuff - but I mention it here because this is the second time I have seen an assert in .queue.c. The first time was a couple of days ago, but then it was called via LWIP. So I am suspecting that it is indicitive of a deeper memory leak. (i'm not using any for the freeros stuff) - I am ONLY doing printfs and WiFiSecureClient. All my code does is it in a loop making the same connection over again and again.

I also often get a lot of the following after several runs:

Free heap before TLS 58340
Seeding the random number generator
Loading CA cert
Loading CRT cert
Loading private key
Setting up the SSL/TLS structure...
mbedtls_ssl_setup returned -0x7f00

(Things go to hell after this. because the error doesn't get propogoated down and we try to send/receive on the connection which causes a panic)

[copercini]

It did work a few times

Nice!

mbedtls_pk_parse_key needs a const unsigned char * certificate. In old implementation the certificate was declarated as char * so first it converts to unsigned and after to const.

When we convert signed to unsigned it may result in a +1 larger output.

So now it was declarated as const char* and just convert to unsigned when call mbedtls_pk_parse_key with the + 1 to length: https://github.com/espressif/arduino-esp32/blob/master/libraries/WiFiClientSecure/src/ssl_client.cpp#L170

It did blow up at one point when ret = mbedtls_ssl_handshake(&ssl_client->ssl_ctx)) != 0) failed and it kept going (hit a null pointer in subsequent printf - needs to abort the function on failure, as discussed before. (don't know exactly why it failed once)

This is related with your open issue: #211
In a nutshell it need to break the process and return a error instead of continue.

I also often get a lot of the following after several runs:
Free heap before TLS 58340

I think it's a bit low heap for complete the handshake, so it crash (need to return an error and break when the other issue is resolved).

[copercini]

Solved!