Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ssl renegotiation causing handshake failure #494

Merged
merged 8 commits into from
May 9, 2024
Merged

fix: ssl renegotiation causing handshake failure #494

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a5f2bd4
feat: added semaphore around `TLSSocket_performHandshake`
Feb 5, 2024
83ad8f9
fix: improved error checking in TLS read and write
Feb 5, 2024
091ea97
removed useless semaphore for renegotiation lock
Feb 7, 2024
074b745
added some tls debug and cleared the session renegotiation events
Feb 7, 2024
21a5d4f
using mbedtls API instead of using internals
Feb 7, 2024
3f6198b
fixed deadlock situation with TLSSocket_read
Feb 7, 2024
15f5087
test fix sonarcloud minor notice
Feb 7, 2024
2890e3f
still some sonarcloud minor things
Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 55 additions & 38 deletions hal/tls/mbedtls/tls_mbedtls.c
View file
Open in desktop
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 */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it says:

*                  ................. you must stop using
 *                 the SSL context for reading or writing, and either free it
 *                 or call \c mbedtls_ssl_session_reset() on it before
 *                 re-using it for a new connection; the current connection
 *                 must be closed.

But by returning false here I believe it will flow down to INT_STATE_CLOSE_ON_ERROR and close it (via releaseSocket which then calls TCPSocket_close and then mbedtls_ssl_free), so it should not be reused.

So maybe after all this is not needed (as far as a more complex schema that we reset it instead of freeing it is done, but that would be much more complex I think).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I think the problem of relying on the releaseSocket leave the window open to potential issues that arises when testing continuous reads/writes.

If I get it correctly the releaseSocket calls TLSSocket_close which in turns call mbedtls_ssl_close_notify which then calls mbedtls_ssl_write_record which should not be done!
AS per mbedtls only mbedtls_ssl_session_reset or mbedtls_ssl_free should be done.

Also, by doing so we rely on higher level function to properly handle an mbed specific behaviour, which is not easy to get right. For example, during tests I've found that IedConnection_close in turns calls readSocket and this read CAN lead to a race condition with other reads in some cases, due to the read/write operations being executed on a separated thread. (that's also why I experimented with semaphores, with terrible results and multiple deadlocks 🙈)
Please note that I think that this PR is not a full formal fix, I suspect that there are still some possible "broken" corner cases, which are now just harder to get.
I consider this as good enough to be considered working as expected under normal conditions.

I thought a bit of using the mbedtls_ssl_free directly but, looking at the code this seems a rather "heavy" change that would require adding quite some error handling.

TLDR:
I think that the safest approach is mbedtls_ssl_session_reset as soon as possible to avoid "broken" connections. Also this approach let us keep the current code flow without much changes

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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed removing that or at least fix ot to "completed renegotiation"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yess, moved both completed/failed in TLSSocket_performHandshake right after the related mbedtls call

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why you put the loop here. I get the idea: you will loop reconstructing the packet until the desired size is reached.

But this as far as I understand is already done at a higher level in CotpConnection_readToTpktBuffer where a read is done (call to this function), then a check if we have enough needed data and if not it will return TPKT_WAITING which in IsoClientConnection_handleConnection will signal that the packet is not complete and will then retry later (using MmsConnection_tick) which also manages a kind of thread relaxation (via Thread_sleep calls).

So I'm not sure this change of logic is a good idea (you made the read waitiling loop at a lower level compared to before which was at state machine level), I would just in case augment the ret comparison with the two new MBEDTS_ERR_SSL_ and that's all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm, you're right and I had your same thoughts and doubts.
Here are my supposition, so take it with a big grain of salt:

Handling at a higher level, with sleeps and so on, left out the proper handling of MBEDTLS_ERR_SSL_ASYNC_IN_PROGRESS and/or MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS which I think is happening during a renegotiation.
This not properly read buffer (or delayed read) left the door open to an incoplete data read which resulted renegotiations always ending with Handshake failed for unknown reason.

Also I suspect that, in a similar way of the session reset, the reads may end up being "interleaved" with other operations which then are using a "dirty" read/write buffer.

Again, please take this as is: just ideas! As I found it to very hard for me to follow the flow logic of the events, also because this is happening with INTENSE reads/writes.

I can eventually give it some more tests in a later moment.

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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above that this will be closed, so maybe not needed.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the feeling that this is similar to the above: you moved the logic to complete the buffer at a lower level (so you loop chunk by chunk here until whole packet is done) which IMO is already done in CotpConnection_sendDataMessage which will chunk at appropriate size and loop. (see around here:

libiec61850/src/mms/iso_cotp/cotp.c

Lines 182 to 195 in 2890e3f

int sentBytes = writeToSocket(self, self->socketExtensionBuffer, self->socketExtensionBufferFill);
if (sentBytes > 0) {
if (sentBytes != self->socketExtensionBufferFill) {
int target = 0;
int i;
uint8_t* buf = self->socketExtensionBuffer;
for (i = sentBytes; i < self->socketExtensionBufferFill; i++) {
buf[target++] = buf[i];
}
self->socketExtensionBufferFill = self->socketExtensionBufferFill - sentBytes;
and

libiec61850/src/mms/iso_cotp/cotp.c

Lines 223 to 245 in 2890e3f

if (self->socketExtensionBufferFill == 0) {
sentBytes = writeToSocket(self, buffer, remainingSize);
}
if (sentBytes == -1)
goto exit_function;
if (sentBytes != remainingSize) {
/* write additional data to extension buffer */
if (self->socketExtensionBuffer) {
uint8_t* extBuf = self->socketExtensionBuffer;
int extCurrentPos = self->socketExtensionBufferFill;
int bytesNotSent = remainingSize - sentBytes;
int i;
for (i = 0; i < bytesNotSent; i++) {
extBuf[i + extCurrentPos] = buffer[sentBytes + i];
}
self->socketExtensionBufferFill = extCurrentPos + bytesNotSent;
}
else {
and then in CotpConnection_sendDataMessage)

So also here I would just keep possibly the two new return codes check without modifying the state machine too much.

{
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