Crash/Panic after SSL Handshake fails

[bkgoodman]

This only happens under a kind of odd edge-case - I specified my own Root CA, Client Cert and Key, and under "normal" conditions (when I access my server) this works fine.

In the case of the crash - this occurred when I inadvertently connected to the www.howsmyssl.com server when using my own certs. When I tried this only specifying my Root CA, I got the appropriate error that the certificate verify mismatched/failed, and everything aborted normally. But when I had my client cert and key in as well, I got this guru meditation.

Crash, I assume at the line:

DEBUG_PRINT("Protocol is %s \nCiphersuite is %s\n", mbedtls_ssl_get_version(&ssl_client->ssl_ctx), mbedtls_ssl_get_ciphersuite(&ssl_client->ssl_ctx));

So, in other words - I believe once the handshake failed (as it should have) - maybe it should just have aborted, rather than to continue - which might have (as a result of the handshake failure) gotten back bad pointers from the mbedtls_ssl_get_ciphersuite and/or mbedtls_ssl_get_version calls?

(I could try a fix - if I new how to build it? First time with Arduino here...)

Starting connection to server...
Free heap before TLS 106160
Seeding the random number generator
Loading CA cert
Loading CRT cert
Loading private key
Setting up the SSL/TLS structure...
Performing the SSL/TLS handshake...
mbedtls_ssl_handshake returned -0x2700
Guru Meditation Error of type LoadProhibited occurred on core  1. Exception was unhandled.
Register dump:
PC      : 0x400014fd  PS      : 0x00060430  A0      : 0x800d861c  A1      : 0x3ffde0f0  
A2      : 0x00000000  A3      : 0xfffffffc  A4      : 0x000000ff  A5      : 0x0000ff00  
A6      : 0x00ff0000  A7      : 0xff000000  A8      : 0x00000000  A9      : 0x3ffde0a0  
A10     : 0x00000000  A11     : 0x3f4042df  A12     : 0x3ffde334  A13     : 0x3ffc8ff0  
A14     : 0x00000000  A15     : 0x00000001  SAR     : 0x00000016  EXCCAUSE: 0x0000001c  
EXCVADDR: 0x00000000  LBEG    : 0x400014fd  LEND    : 0x4000150d  LCOUNT  : 0xffffffff  

Backtrace: 0x400014fd:0x3ffde0f0 0x400d861c:0x3ffde100 0x400d3008:0x3ffde410 0x400d1972:0x3ffde460 0x400d15aa:0x3ffde6b0 0x400d1608:0x3ffde6d0 0x400d13f3:0x3ffde700 0x4012934e:0x3ffde720

CPU halted.

0x400014fd: ?? ??:0
0x400d861c: _vfprintf_r at vfprintf.c:1529
0x400d3008: printf at printf.c:61
0x400d1972: start_ssl_client(sslclient_context*, unsigned int, unsigned int, unsigned char*, unsigned char*, unsigned char*) at ssl_client.cpp:230
0x400d15aa: WiFiClientSecure::connect(IPAddress, unsigned short, unsigned char*, unsigned char*, unsigned char*) at WiFiClientSecure.cpp:188
0x400d1608: WiFiClientSecure::connect(char const*, unsigned short, unsigned char*, unsigned char*, unsigned char*) at WiFiClientSecure.cpp:188
0x400d13f3: setup at BKGSecureWifi.ino:124
0x4012934e: loopTask(void*) at main.cpp:11 (discriminator 1)

[bkgoodman]

I did verify that mbedtls_ssl_get_ciphersuite was returning NULL.

I believe the best way to handle this might be to (close the socket and) abort if mbedtls_ssl_handshake returns NULL, returning 0. (Higher level code should regognize this as a failure)

That said - I am not sure if a lot of the other break; error cases should do the same. (Don't want to submit a patch without knowing - and would defer judgement to someone with more knowledge)

[copercini]

oh, it's a really odd edge-case hahaha
but it should not crash in any case. Thanks for report =)

I believe the best way to handle this might be to (close the socket and) abort if mbedtls_ssl_handshake returns NULL, returning 0. (Higher level code should regognize this as a failure)

It's a good solution. But returning -1 or the MbedTLS error, because higher level code just interprets as error if (ret < 0).

I want to release some fixes and heap optimizations for this lib soon and include your fix to this too.

[bkgoodman]

But the Adruino example does:

if (client.connect(server, 443))

So this would interpret "0" as an error, and nonzero as valid. This works if zero is not a valid "socket".

I was going to come up with a patch, but there seem to be some inconsistencies in how errors are returned. Some places return 0 for error, others return -1. The published client example codes (above) assumes zero is an error.

I don't want to break any "existing" applications - but I'm inclined to think a "clean break" at this point might be good.

If you think >=0 should be "valid" and <0 should mean error - and you're okay with "changing" the existing demo and API - I can submit a fix that cleans up some of this...

[copercini]

But it's called first here: https://github.com/espressif/arduino-esp32/blob/master/libraries/WiFiClientSecure/src/WiFiClientSecure.cpp#L96 just after that it returns to arduino.

[bkgoodman]

It's only in one of the 4 cases of "connect". In the first two, no explicit "return" is given (will c++ return the last value on the stack by default)? Even in the one you site, it always returns "1" - even on error. In the last case (below that) it returns "0" on name lookup failure, otherwise the value from the "third" case (one you sited) - which always returns "1".

Then there's start_ssl_client which returns the socket number (initialized to -1) - but does not clean up the connection or set the socket back to -1 in the case of error.

Seems like all the connect functions should really return the socket on success, or <0 on error.

[copercini]

@bkgoodman Could you please check if it is fixed on https://github.com/copercini/arduino-esp32/tree/master/libraries/WiFiClientSecure/src ?

[bkgoodman]

Getting warmer :-) ... It runs a few iterations and then died - but this time, before it did - it got:

Setting up the SSL/TLS structure...
SSL - Memory allocation failed
MbedTLS error: -32512

(The following panic was in the same .queue.c location, trying to read after the connection failed to come up, etc. I still seem to not be getting the return code that the connect failed).

So clearly a case of something not getting freed correctly, even in the happy case.

I am just sitting in a loop doing (comments, printfs and cruft removed for legibility):

  WiFiClientSecure *client = new WiFiClientSecure;
  if (client->connect(server, 443, test_ca_cert, test_client_cert, test_client_key)>=0)
  {
    client->printf("GET https://host.domain.com/cgi-bin/time.cgi?i1=%d&i2=%di3=%d&i4=%d&reason=0x%x HTTP/1.0",
          digitalRead(36),digitalRead(37),0,
          2,REG(RTC_CNTL_RESET_STATE_REG)
          );
    client->println("Host: host.domain.com");
    client->println("Connection: close");
    client->println();
  }

  while (!client->available()){ }  

  while (client->available()) {
    char c = client->read();
    Serial.write(c);
  }

  client->stop();
  delete client;

[copercini]

just for check, please put
WiFiClientSecure *client = new WiFiClientSecure;
just as global and remove it from loop().
remove too: delete client;

If it works, I know the problem hahahaha

[me-no-dev]

where do you see setup and loop?
Why do you new WiFiClientSecure if you gonna use the client only locally?
Try putting new line (\r\n) in the first printf. there isn't new line added so the server does not even get the host header. instead it gets invalid string.

[me-no-dev]

void testClient(){
  WiFiClientSecure client;
  if(!client.connect(server, 443, test_ca_cert, test_client_cert, test_client_key)){
    return;//connect fail
  }
  client.printf("GET https://host.domain.com/cgi-bin/time.cgi?i1=%d&i2=%di3=%d&i4=%d&reason=0x%x HTTP/1.0\r\n",
          digitalRead(36),digitalRead(37),0,
          2,REG(RTC_CNTL_RESET_STATE_REG)
          );
  client.print("Host: host.domain.com\r\n");
  client.print("Connection: close\r\n\r\n");

  while (!client.available()){ }  

  while (client.available()) {
    char c = client.read();
    Serial.write(c);
  }
}

[bkgoodman]

That worked for several iterations. - then I got the same error in queue.c:

Verifying peer X.509 certificate...
Certificate verified.
Free heap after TLS 21348
Connected to server!
UNKNOWN ERROR CODE (004E)
MbedTLS error: -78

Which appear to come from one of the client->printfs which come after the client->connect. (HTTP request, GET, etc).

Another time (after a reset and may loops) I did see:

Performing the SSL/TLS handshake...
BIGNUM - Memory allocation failed
MbedTLS error: -16

...and one other time I saw...

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

[me-no-dev]

you are running the latest esp32-arduino right?

[bkgoodman]

Not sure what you mean about the newlines. I have it in the first (and subsequent) printfs. Like I said - it works the first several times through the loop. My Apache log sees the request properly - no issues.

As for why I new/delete all the time - the idea is that this is going to sit in a 7/24 lights-out facility. It needs to survive WiFi outages - so the idea being that I should be completely tearing down and restarting the network connection every time through. What I'm REALLY trying to do is to get a good watchdog to come in and do a hard reset intermittently, but that is a whole other issue...

I am running the very latest:

[remote "origin"]
  url = https://github.com/espressif/arduino-esp32.git
  fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
  remote = origin
  merge = refs/heads/master

(With the exception of the WifiSecureClient/src files you wanted me to try in the comment above)

I just did a pull and while I haven't seen the queue.c error yet - I still see the UNKNOWN ERROR CODE 04E MbedTLS error: -78 (in the printf)?

I will keep trying, though...