From a5f2bd4e8165d51a856c73eacf301883b1d2819d Mon Sep 17 00:00:00 2001 From: Federico Francescon Date: Mon, 5 Feb 2024 14:35:00 +0100 Subject: [PATCH 1/8] feat: added semaphore around `TLSSocket_performHandshake` --- hal/tls/mbedtls/tls_mbedtls.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/hal/tls/mbedtls/tls_mbedtls.c b/hal/tls/mbedtls/tls_mbedtls.c index e2f152d08..e9e5eb3c0 100644 --- a/hal/tls/mbedtls/tls_mbedtls.c +++ b/hal/tls/mbedtls/tls_mbedtls.c @@ -99,6 +99,7 @@ struct sTLSSocket { /* time of last session renegotiation (used to calculate next renegotiation time) */ uint64_t lastRenegotiationTime; + Semaphore renegotiationLock; /* time of the last CRL update */ uint64_t crlUpdated; @@ -708,6 +709,7 @@ TLSSocket_create(Socket socket, TLSConfiguration configuration, bool storeClient self->storePeerCert = storeClientCert; self->peerCert = NULL; self->peerCertLength = 0; + self->renegotiationLock = Semaphore_create(1); TLSConfiguration_setupComplete(configuration); @@ -850,6 +852,7 @@ TLSSocket_getPeerCertificate(TLSSocket self, int* certSize) bool TLSSocket_performHandshake(TLSSocket self) { + Semaphore_wait(self->renegotiationLock); int ret = mbedtls_ssl_renegotiate(&(self->ssl)); if (ret == 0) { @@ -857,6 +860,9 @@ TLSSocket_performHandshake(TLSSocket self) raiseSecurityEvent(self->tlsConfig, TLS_SEC_EVT_WARNING, TLS_EVENT_CODE_WRN_INSECURE_TLS_VERSION, "Warning: Insecure TLS version", self); } + Semaphore_post(self->renegotiationLock); + + DEBUG_PRINT("TLS", "TLSSocket_performHandshake Success -> ret=%i\n", ret); return true; } else { @@ -868,6 +874,7 @@ TLSSocket_performHandshake(TLSSocket self) createSecurityEvents(self->tlsConfig, ret, flags, self); } + Semaphore_post(self->renegotiationLock); return false; } } @@ -1008,6 +1015,8 @@ TLSSocket_close(TLSSocket self) if (self->peerCert) GLOBAL_FREEMEM(self->peerCert); + Semaphore_destroy(self->renegotiationLock); + GLOBAL_FREEMEM(self); } From 83ad8f9ca9bad71aab563e5c1ed2fbfee2db4a89 Mon Sep 17 00:00:00 2001 From: Federico Francescon Date: Mon, 5 Feb 2024 14:35:31 +0100 Subject: [PATCH 2/8] fix: improved error checking in TLS read and write --- hal/tls/mbedtls/tls_mbedtls.c | 71 +++++++++++++++++++++-------------- 1 file changed, 42 insertions(+), 29 deletions(-) diff --git a/hal/tls/mbedtls/tls_mbedtls.c b/hal/tls/mbedtls/tls_mbedtls.c index e9e5eb3c0..d5c482f07 100644 --- a/hal/tls/mbedtls/tls_mbedtls.c +++ b/hal/tls/mbedtls/tls_mbedtls.c @@ -266,7 +266,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); */ @@ -855,7 +855,8 @@ TLSSocket_performHandshake(TLSSocket self) Semaphore_wait(self->renegotiationLock); 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); } @@ -868,10 +869,12 @@ TLSSocket_performHandshake(TLSSocket self) 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_INFO, TLS_EVENT_CODE_INF_SESSION_RENEGOTIATION, "Alarm: 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); } Semaphore_post(self->renegotiationLock); @@ -927,22 +930,33 @@ 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; - - if (ret < 0) { + int len = 0; + while (len < size) { + int ret = mbedtls_ssl_read(&(self->ssl), (buf + len), (size - len)); + if (ret == 0) { + break; + } else if (ret > 0) { + len += ret; + continue; + } + // Negative values means errors switch (ret) { + case MBEDTLS_ERR_SSL_WANT_READ: // Falling through + case MBEDTLS_ERR_SSL_WANT_WRITE: + case MBEDTLS_ERR_SSL_ASYNC_IN_PROGRESS: + case MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS: + continue; + break; + 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); @@ -952,19 +966,18 @@ TLSSocket_read(TLSSocket self, uint8_t* buf, int size) createSecurityEvents(self->tlsConfig, ret, flags, self); } - - return -1; } - } - return ret; + mbedtls_ssl_session_reset(&(self->ssl)); + return ret; + } + return len; } int TLSSocket_write(TLSSocket self, uint8_t* buf, int size) { - int ret; - int len = size; + int len = 0; checkForCRLUpdate(self); @@ -972,22 +985,22 @@ TLSSocket_write(TLSSocket self, uint8_t* buf, int size) 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) { + mbedtls_ssl_session_reset(&(self->ssl)); + DEBUG_PRINT("TLS", "mbedtls_ssl_write returned -0x%X\n", -ret); return -1; } - } - len = ret; + len += ret; + } return len; } From 091ea970ebfa44b23e4665423e0069438863f7c8 Mon Sep 17 00:00:00 2001 From: Federico Francescon Date: Wed, 7 Feb 2024 10:33:22 +0100 Subject: [PATCH 3/8] removed useless semaphore for renegotiation lock --- hal/tls/mbedtls/tls_mbedtls.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/hal/tls/mbedtls/tls_mbedtls.c b/hal/tls/mbedtls/tls_mbedtls.c index d5c482f07..a209b3ea4 100644 --- a/hal/tls/mbedtls/tls_mbedtls.c +++ b/hal/tls/mbedtls/tls_mbedtls.c @@ -99,7 +99,6 @@ struct sTLSSocket { /* time of last session renegotiation (used to calculate next renegotiation time) */ uint64_t lastRenegotiationTime; - Semaphore renegotiationLock; /* time of the last CRL update */ uint64_t crlUpdated; @@ -709,7 +708,6 @@ TLSSocket_create(Socket socket, TLSConfiguration configuration, bool storeClient self->storePeerCert = storeClientCert; self->peerCert = NULL; self->peerCertLength = 0; - self->renegotiationLock = Semaphore_create(1); TLSConfiguration_setupComplete(configuration); @@ -852,7 +850,6 @@ TLSSocket_getPeerCertificate(TLSSocket self, int* certSize) bool TLSSocket_performHandshake(TLSSocket self) { - Semaphore_wait(self->renegotiationLock); int ret = mbedtls_ssl_renegotiate(&(self->ssl)); if (ret == 0 || ret == MBEDTLS_ERR_SSL_WANT_READ || ret == MBEDTLS_ERR_SSL_WANT_WRITE || @@ -861,7 +858,6 @@ TLSSocket_performHandshake(TLSSocket self) raiseSecurityEvent(self->tlsConfig, TLS_SEC_EVT_WARNING, TLS_EVENT_CODE_WRN_INSECURE_TLS_VERSION, "Warning: Insecure TLS version", self); } - Semaphore_post(self->renegotiationLock); DEBUG_PRINT("TLS", "TLSSocket_performHandshake Success -> ret=%i\n", ret); return true; @@ -877,7 +873,6 @@ TLSSocket_performHandshake(TLSSocket self) DEBUG_PRINT("TLS", "mbedtls_ssl_session_reset failed -> ret: -0x%X\n", -ret); } - Semaphore_post(self->renegotiationLock); return false; } } @@ -1028,7 +1023,6 @@ TLSSocket_close(TLSSocket self) if (self->peerCert) GLOBAL_FREEMEM(self->peerCert); - Semaphore_destroy(self->renegotiationLock); GLOBAL_FREEMEM(self); } From 074b7454168e1f94c1e9543337afa5cc0b6b953f Mon Sep 17 00:00:00 2001 From: Federico Francescon Date: Wed, 7 Feb 2024 10:35:33 +0100 Subject: [PATCH 4/8] added some tls debug and cleared the session renegotiation events --- hal/tls/mbedtls/tls_mbedtls.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/hal/tls/mbedtls/tls_mbedtls.c b/hal/tls/mbedtls/tls_mbedtls.c index a209b3ea4..671ee1176 100644 --- a/hal/tls/mbedtls/tls_mbedtls.c +++ b/hal/tls/mbedtls/tls_mbedtls.c @@ -860,12 +860,13 @@ TLSSocket_performHandshake(TLSSocket 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); - raiseSecurityEvent(self->tlsConfig, TLS_SEC_EVT_INFO, TLS_EVENT_CODE_INF_SESSION_RENEGOTIATION, "Alarm: Renegotiation failed", self); + raiseSecurityEvent(self->tlsConfig, TLS_SEC_EVT_WARNING, TLS_EVENT_CODE_INF_SESSION_RENEGOTIATION, "Alarm: TLS session renegotiation failed", self); /* mbedtls_ssl_renegotiate mandates to reset the ssl session in case of errors */ ret = mbedtls_ssl_session_reset(&(self->ssl)); @@ -903,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; } @@ -963,9 +958,14 @@ TLSSocket_read(TLSSocket self, uint8_t* buf, int size) } } - mbedtls_ssl_session_reset(&(self->ssl)); + 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 len; } @@ -989,8 +989,12 @@ TLSSocket_write(TLSSocket self, uint8_t* buf, int size) } if (ret < 0) { - mbedtls_ssl_session_reset(&(self->ssl)); 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; } From 21a5d4fdb339af13a13e167dd1def458547adfc4 Mon Sep 17 00:00:00 2001 From: Federico Francescon Date: Wed, 7 Feb 2024 10:35:55 +0100 Subject: [PATCH 5/8] using mbedtls API instead of using internals --- hal/tls/mbedtls/tls_mbedtls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hal/tls/mbedtls/tls_mbedtls.c b/hal/tls/mbedtls/tls_mbedtls.c index 671ee1176..e7641aa41 100644 --- a/hal/tls/mbedtls/tls_mbedtls.c +++ b/hal/tls/mbedtls/tls_mbedtls.c @@ -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 From 3f6198b54b40982097b66f84b7c26b2b4455445d Mon Sep 17 00:00:00 2001 From: Federico Francescon Date: Wed, 7 Feb 2024 10:36:25 +0100 Subject: [PATCH 6/8] fixed deadlock situation with TLSSocket_read --- hal/tls/mbedtls/tls_mbedtls.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/hal/tls/mbedtls/tls_mbedtls.c b/hal/tls/mbedtls/tls_mbedtls.c index e7641aa41..068a2cc9a 100644 --- a/hal/tls/mbedtls/tls_mbedtls.c +++ b/hal/tls/mbedtls/tls_mbedtls.c @@ -923,22 +923,19 @@ TLSSocket_read(TLSSocket self, uint8_t* buf, int size) int len = 0; while (len < size) { int ret = mbedtls_ssl_read(&(self->ssl), (buf + len), (size - len)); - if (ret == 0) { - break; - } else if (ret > 0) { + if (ret > 0) { len += ret; continue; } - // Negative values means errors - switch (ret) - { - case MBEDTLS_ERR_SSL_WANT_READ: // Falling through + 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: - continue; - break; + // Known "good" cases indicating the read is done + return len; case MBEDTLS_ERR_SSL_PEER_CLOSE_NOTIFY: DEBUG_PRINT("TLS", " connection was closed gracefully\n"); From 15f508798e280be8f50fb0b3837808d9486c251b Mon Sep 17 00:00:00 2001 From: Federico Francescon Date: Wed, 7 Feb 2024 14:28:48 +0100 Subject: [PATCH 7/8] test fix sonarcloud minor notice --- hal/tls/mbedtls/tls_mbedtls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hal/tls/mbedtls/tls_mbedtls.c b/hal/tls/mbedtls/tls_mbedtls.c index 068a2cc9a..8da0a7887 100644 --- a/hal/tls/mbedtls/tls_mbedtls.c +++ b/hal/tls/mbedtls/tls_mbedtls.c @@ -37,7 +37,7 @@ #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);} #endif From 2890e3f179242e90617a991557cb0b4b62f47b08 Mon Sep 17 00:00:00 2001 From: Federico Francescon Date: Wed, 7 Feb 2024 14:52:01 +0100 Subject: [PATCH 8/8] still some sonarcloud minor things --- hal/tls/mbedtls/tls_mbedtls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hal/tls/mbedtls/tls_mbedtls.c b/hal/tls/mbedtls/tls_mbedtls.c index 8da0a7887..06b982344 100644 --- a/hal/tls/mbedtls/tls_mbedtls.c +++ b/hal/tls/mbedtls/tls_mbedtls.c @@ -39,7 +39,7 @@ #if (CONFIG_DEBUG_TLS == 1) #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