Skip to content

Commit

Permalink
fix: ssl renegotiation causing handshake failure (#494)
Browse files Browse the repository at this point in the history 
* feat: added semaphore around `TLSSocket_performHandshake`

* fix: improved error checking in TLS read and write

* removed useless semaphore for renegotiation lock

* added some tls debug and cleared the session renegotiation events

* using mbedtls API instead of using internals

* fixed deadlock situation with TLSSocket_read

* test fix sonarcloud minor notice

* still some sonarcloud minor things

---------

Co-authored-by: Federico Francescon <[email protected]>
  • Loading branch information
@fedefrancescon
fedefrancescon and Federico Francescon authored May 9, 2024
1 parent 0b57f14 commit 790e3e6
Showing 1 changed file with 55 additions and 38 deletions.
93 changes: 55 additions & 38 deletions hal/tls/mbedtls/tls_mbedtls.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@
#endif

#if (CONFIG_DEBUG_TLS == 1)
#define DEBUG_PRINT(appId, fmt, ...) fprintf(stderr, "%s: " fmt, appId, ## __VA_ARGS__);
#define DEBUG_PRINT(appId, fmt, ...) fprintf(stderr, "%s: " fmt, appId, ## __VA_ARGS__)
#else
#define DEBUG_PRINT(fmt, ...) {do {} while(0);}
#define DEBUG_PRINT(fmt, ...) do {} while(0)
#endif


Expand Down Expand Up @@ -265,7 +265,7 @@ TLSConfiguration_create()

mbedtls_ssl_conf_authmode(&(self->conf), MBEDTLS_SSL_VERIFY_REQUIRED);

mbedtls_ssl_conf_renegotiation(&(self->conf), MBEDTLS_SSL_LEGACY_NO_RENEGOTIATION);
mbedtls_ssl_conf_renegotiation(&(self->conf), MBEDTLS_SSL_RENEGOTIATION_ENABLED);

/* static int hashes[] = {3,4,5,6,7,8,0}; */
/* mbedtls_ssl_conf_sig_hashes(&(self->conf), hashes); */
Expand Down Expand Up @@ -297,7 +297,7 @@ TLSConfiguration_create()
void
TLSConfiguration_setClientMode(TLSConfiguration self)
{
self->conf.endpoint = MBEDTLS_SSL_IS_CLIENT;
mbedtls_ssl_conf_endpoint(&(self->conf), MBEDTLS_SSL_IS_CLIENT);
}

void
Expand Down Expand Up @@ -852,20 +852,26 @@ TLSSocket_performHandshake(TLSSocket self)
{
int ret = mbedtls_ssl_renegotiate(&(self->ssl));

if (ret == 0) {
if (ret == 0 || ret == MBEDTLS_ERR_SSL_WANT_READ || ret == MBEDTLS_ERR_SSL_WANT_WRITE ||
ret == MBEDTLS_ERR_SSL_ASYNC_IN_PROGRESS || ret == MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS) {
if (getTLSVersion(self->ssl.major_ver, self->ssl.minor_ver) < TLS_VERSION_TLS_1_2) {
raiseSecurityEvent(self->tlsConfig, TLS_SEC_EVT_WARNING, TLS_EVENT_CODE_WRN_INSECURE_TLS_VERSION, "Warning: Insecure TLS version", self);
}


DEBUG_PRINT("TLS", "TLSSocket_performHandshake Success -> ret=%i\n", ret);
raiseSecurityEvent(self->tlsConfig, TLS_SEC_EVT_INFO, TLS_EVENT_CODE_INF_SESSION_RENEGOTIATION, "TLS session renegotiation completed", self);
return true;
}
else {
DEBUG_PRINT("TLS", "TLSSocket_performHandshake failed -> ret=%i\n", ret);

if (self->tlsConfig->eventHandler) {
uint32_t flags = mbedtls_ssl_get_verify_result(&(self->ssl));
raiseSecurityEvent(self->tlsConfig, TLS_SEC_EVT_WARNING, TLS_EVENT_CODE_INF_SESSION_RENEGOTIATION, "Alarm: TLS session renegotiation failed", self);

createSecurityEvents(self->tlsConfig, ret, flags, self);
/* mbedtls_ssl_renegotiate mandates to reset the ssl session in case of errors */
ret = mbedtls_ssl_session_reset(&(self->ssl));
if (ret != 0) {
DEBUG_PRINT("TLS", "mbedtls_ssl_session_reset failed -> ret: -0x%X\n", -ret);
}

return false;
Expand Down Expand Up @@ -898,16 +904,10 @@ startRenegotiationIfRequired(TLSSocket self)
if (Hal_getTimeInMs() <= self->lastRenegotiationTime + self->tlsConfig->renegotiationTimeInMs)
return true;

raiseSecurityEvent(self->tlsConfig, TLS_SEC_EVT_INFO, TLS_EVENT_CODE_INF_SESSION_RENEGOTIATION, "Info: session renegotiation started", self);

if (TLSSocket_performHandshake(self) == false) {
DEBUG_PRINT("TLS", " renegotiation failed\n");
if (TLSSocket_performHandshake(self) == false)
return false;
}

DEBUG_PRINT("TLS", " started renegotiation\n");
self->lastRenegotiationTime = Hal_getTimeInMs();

return true;
}

Expand All @@ -920,22 +920,30 @@ TLSSocket_read(TLSSocket self, uint8_t* buf, int size)
return -1;
}

int ret = mbedtls_ssl_read(&(self->ssl), buf, size);

if ((ret == MBEDTLS_ERR_SSL_WANT_READ) || (ret == MBEDTLS_ERR_SSL_WANT_WRITE))
return 0;
int len = 0;
while (len < size) {
int ret = mbedtls_ssl_read(&(self->ssl), (buf + len), (size - len));
if (ret > 0) {
len += ret;
continue;
}

if (ret < 0) {
switch (ret) {
case 0: // falling through
case MBEDTLS_ERR_SSL_WANT_READ:
case MBEDTLS_ERR_SSL_WANT_WRITE:
case MBEDTLS_ERR_SSL_ASYNC_IN_PROGRESS:
case MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS:
// Known "good" cases indicating the read is done
return len;

switch (ret)
{
case MBEDTLS_ERR_SSL_PEER_CLOSE_NOTIFY:
DEBUG_PRINT("TLS", " connection was closed gracefully\n");
return -1;
break;

case MBEDTLS_ERR_NET_CONN_RESET:
DEBUG_PRINT("TLS", " connection was reset by peer\n");
return -1;
break;

default:
DEBUG_PRINT("TLS", " mbedtls_ssl_read returned -0x%x\n", -ret);
Expand All @@ -945,42 +953,50 @@ TLSSocket_read(TLSSocket self, uint8_t* buf, int size)

createSecurityEvents(self->tlsConfig, ret, flags, self);
}
}

return -1;
int reset_err = mbedtls_ssl_session_reset(&(self->ssl));
if (0 != reset_err) {
DEBUG_PRINT("TLS", "mbedtls_ssl_session_reset failed -0x%X\n", -reset_err);
}

return ret;
}

return ret;
return len;
}

int
TLSSocket_write(TLSSocket self, uint8_t* buf, int size)
{
int ret;
int len = size;
int len = 0;

checkForCRLUpdate(self);

if (startRenegotiationIfRequired(self) == false) {
return -1;
}

while ((ret = mbedtls_ssl_write(&(self->ssl), buf, len)) <= 0)
while (len < size)
{
if (ret == MBEDTLS_ERR_NET_CONN_RESET)
{
DEBUG_PRINT("TLS", "peer closed the connection\n");
return -1;
int ret = mbedtls_ssl_write(&(self->ssl), (buf + len), (size -len));
if ((ret == MBEDTLS_ERR_SSL_WANT_READ) || (ret == MBEDTLS_ERR_SSL_WANT_WRITE) ||
(ret == MBEDTLS_ERR_SSL_ASYNC_IN_PROGRESS) || (ret == MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS)) {
continue;
}

if ((ret != MBEDTLS_ERR_SSL_WANT_READ) && (ret != MBEDTLS_ERR_SSL_WANT_WRITE))
{
DEBUG_PRINT("TLS", "mbedtls_ssl_write returned %d\n", ret);
if (ret < 0) {
DEBUG_PRINT("TLS", "mbedtls_ssl_write returned -0x%X\n", -ret);

if (0 != (ret = mbedtls_ssl_session_reset(&(self->ssl)))) {
DEBUG_PRINT("TLS", "mbedtls_ssl_session_reset failed -0x%X\n", -ret);
}

return -1;
}
}

len = ret;
len += ret;
}

return len;
}
Expand Down Expand Up @@ -1008,6 +1024,7 @@ TLSSocket_close(TLSSocket self)
if (self->peerCert)
GLOBAL_FREEMEM(self->peerCert);


GLOBAL_FREEMEM(self);
}

Expand Down

0 comments on commit 790e3e6

Please sign in to comment.