From 67d518544b13a482013e21a149ea3b895804a74b Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Mon, 1 Aug 2022 15:51:40 +0200 Subject: [PATCH 1/5] EmbedReceiveFrom: fix when using a TCP socket - recvfrom() returns 0 on a closed TCP socket - TCP sockets set WOLFSSL_CBIO_ERR_ISR on a timeout --- src/wolfio.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/wolfio.c b/src/wolfio.c index 7a958d0ab..20a231ab9 100644 --- a/src/wolfio.c +++ b/src/wolfio.c @@ -481,6 +481,20 @@ int EmbedReceiveFrom(WOLFSSL *ssl, char *buf, int sz, void *ctx) } return recvd; } + else if (recvd == 0) { + int type; + XSOCKLENT length = sizeof(int); /* optvalue 'type' is of size int */ + + if (getsockopt(sd, SOL_SOCKET, SO_TYPE, &type, &length) == 0 && + type != SOCK_DGRAM) { + /* Closed TCP connection */ + recvd = WOLFSSL_CBIO_ERR_CONN_CLOSE; + } + else { + WOLFSSL_MSG("Ignoring 0-length datagram"); + } + return recvd; + } else if (dtlsCtx->connected) { /* Nothing to do */ } From fd1e8c49ebe47f5075d8a9837d15150e0dd868fe Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Mon, 1 Aug 2022 14:30:35 +0200 Subject: [PATCH 2/5] Reset timeout when reading a valid DTLS message - Increment the DTLS 1.3 timeout on a long timeout --- src/dtls13.c | 4 ++++ src/internal.c | 11 ++++++----- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/dtls13.c b/src/dtls13.c index 824bcc622..cf45b8ae4 100644 --- a/src/dtls13.c +++ b/src/dtls13.c @@ -2296,6 +2296,10 @@ int Dtls13RtxTimeout(WOLFSSL* ssl) return 0; } + /* Increase timeout on long timeout */ + if (DtlsMsgPoolTimeout(ssl) != 0) + return -1; + return Dtls13RtxSendBuffered(ssl); } diff --git a/src/internal.c b/src/internal.c index 088161390..09c2c41ed 100644 --- a/src/internal.c +++ b/src/internal.c @@ -7962,8 +7962,6 @@ void DtlsTxMsgListClean(WOLFSSL* ssl) * verify */ break; ssl->dtls_tx_msg_list_sz--; - /* Reset timer as deleting a node means that state has progressed */ - ssl->dtls_timeout = ssl->dtls_timeout_init; head = next; } ssl->dtls_tx_msg_list = head; @@ -8263,8 +8261,7 @@ int DtlsMsgPoolTimeout(WOLFSSL* ssl) } -/* DtlsMsgPoolReset() deletes the stored transmit list and resets the timeout - * value. */ +/* DtlsMsgPoolReset() deletes the stored transmit list. */ void DtlsMsgPoolReset(WOLFSSL* ssl) { WOLFSSL_ENTER("DtlsMsgPoolReset()"); @@ -8274,7 +8271,6 @@ void DtlsMsgPoolReset(WOLFSSL* ssl) ssl->dtls_tx_msg = NULL; ssl->dtls_tx_msg_list_sz = 0; } - ssl->dtls_timeout = ssl->dtls_timeout_init; } @@ -18745,6 +18741,11 @@ int ProcessReplyEx(WOLFSSL* ssl, int allowSocketErr) if (IsDtlsNotSctpMode(ssl) && !IsAtLeastTLSv1_3(ssl->version)) { _DtlsUpdateWindow(ssl); } + + if (ssl->options.dtls) { + /* Reset timeout as we have received a valid DTLS message */ + ssl->dtls_timeout = ssl->dtls_timeout_init; + } #endif /* WOLFSSL_DTLS */ WOLFSSL_MSG("received record layer msg"); From 3278210e1ccdfa79a8c7fa9003dcfb030d155555 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Mon, 1 Aug 2022 15:07:21 +0200 Subject: [PATCH 3/5] Silently discard DTLS msgs that fail decryption Don't send alerts when decryption fails inside a DTLS connection. TLS should always send a bad_record_mac when decryption fails. --- src/internal.c | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/src/internal.c b/src/internal.c index 09c2c41ed..cac28ecb8 100644 --- a/src/internal.c +++ b/src/internal.c @@ -18497,12 +18497,13 @@ int ProcessReplyEx(WOLFSSL* ssl, int allowSocketErr) aad_size = ssl->dtls13CurRlLength; } #endif /* WOLFSSL_DTLS13 */ - + /* Don't send an alert for DTLS. We will just drop it + * silently later. */ ret = DecryptTls13(ssl, in->buffer + in->idx, in->buffer + in->idx, ssl->curSize, - aad, aad_size, 1); + aad, aad_size, !ssl->options.dtls); #else ret = DECRYPT_ERROR; #endif /* WOLFSSL_TLS13 */ @@ -18529,16 +18530,20 @@ int ProcessReplyEx(WOLFSSL* ssl, int allowSocketErr) } else { WOLFSSL_MSG("Decrypt failed"); - WOLFSSL_ERROR(ret); -#ifdef WOLFSSL_DTLS13 - if (ssl->options.tls1_3 && ssl->options.dtls) { + #ifdef WOLFSSL_DTLS + /* If in DTLS mode, if the decrypt fails for any + * reason, pretend the datagram never happened. */ + if (ssl->options.dtls) { WOLFSSL_MSG("DTLS: Ignoring decrypted failed record"); ssl->options.processReply = doProcessInit; ssl->buffers.inputBuffer.idx = - ssl->buffers.inputBuffer.length; + ssl->buffers.inputBuffer.length; + #ifdef WOLFSSL_DTLS_DROP_STATS + ssl->macDropCount++; + #endif /* WOLFSSL_DTLS_DROP_STATS */ return 0; } -#endif /* WOLFSSL_DTLS13 */ + #endif /* WOLFSSL_DTLS */ #ifdef WOLFSSL_EARLY_DATA if (ssl->options.tls1_3) { if (ssl->options.side == WOLFSSL_SERVER_END && @@ -18554,28 +18559,20 @@ int ProcessReplyEx(WOLFSSL* ssl, int allowSocketErr) ssl->options.processReply = doProcessInit; ssl->buffers.inputBuffer.idx += ssl->curSize; if (ssl->buffers.inputBuffer.idx > - ssl->buffers.inputBuffer.length) + ssl->buffers.inputBuffer.length) { + WOLFSSL_ERROR(BUFFER_E); return BUFFER_E; + } return 0; } WOLFSSL_MSG("Too much EarlyData!"); } - SendAlert(ssl, alert_fatal, bad_record_mac); } #endif - #ifdef WOLFSSL_DTLS - /* If in DTLS mode, if the decrypt fails for any - * reason, pretend the datagram never happened. */ - if (ssl->options.dtls) { - ssl->options.processReply = doProcessInit; - ssl->buffers.inputBuffer.idx = - ssl->buffers.inputBuffer.length; - #ifdef WOLFSSL_DTLS_DROP_STATS - ssl->macDropCount++; - #endif /* WOLFSSL_DTLS_DROP_STATS */ - } - #endif /* WOLFSSL_DTLS */ + SendAlert(ssl, alert_fatal, bad_record_mac); + /* Push error once we know that we will error out here */ + WOLFSSL_ERROR(ret); return DECRYPT_ERROR; } } From ebcfa3199308da8e45de5d637e43c5e87a09917d Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Thu, 4 Aug 2022 11:35:27 +0200 Subject: [PATCH 4/5] Refactor checking socket type into a function --- src/wolfio.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/wolfio.c b/src/wolfio.c index 20a231ab9..3e2ca6dd1 100644 --- a/src/wolfio.c +++ b/src/wolfio.c @@ -372,6 +372,20 @@ static int sockAddrEqual( return 0; } +static int isDGramSock(int sfd) +{ + int type = 0; + XSOCKLENT length = sizeof(int); /* optvalue 'type' is of size int */ + + if (getsockopt(sfd, SOL_SOCKET, SO_TYPE, &type, &length) == 0 && + type != SOCK_DGRAM) { + return 0; + } + else { + return 1; + } +} + /* The receive embedded callback * return : nb bytes read, or error */ @@ -482,11 +496,7 @@ int EmbedReceiveFrom(WOLFSSL *ssl, char *buf, int sz, void *ctx) return recvd; } else if (recvd == 0) { - int type; - XSOCKLENT length = sizeof(int); /* optvalue 'type' is of size int */ - - if (getsockopt(sd, SOL_SOCKET, SO_TYPE, &type, &length) == 0 && - type != SOCK_DGRAM) { + if (!isDGramSock(sd)) { /* Closed TCP connection */ recvd = WOLFSSL_CBIO_ERR_CONN_CLOSE; } @@ -530,13 +540,10 @@ int EmbedSendTo(WOLFSSL* ssl, char *buf, int sz, void *ctx) int sent; const SOCKADDR_S* peer = NULL; XSOCKLENT peerSz = 0; - int type; - XSOCKLENT length = sizeof(int); /* optvalue 'type' is of size int */ WOLFSSL_ENTER("EmbedSendTo()"); - if (getsockopt(sd, SOL_SOCKET, SO_TYPE, &type, &length) == 0 && - type != SOCK_DGRAM) { + if (!isDGramSock(sd)) { /* Probably a TCP socket. peer and peerSz MUST be NULL and 0 */ } else if (!dtlsCtx->connected) { From 6d4f0146ca84033bc2dc1c4ddf500065cabeabf9 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Thu, 4 Aug 2022 12:06:26 +0200 Subject: [PATCH 5/5] Refactor sending alert on decryption failure Take sending of the alert outside of DecryptTls() and DecryptTls13(). The alert is now sent in ProcessReplyEx(). --- src/internal.c | 24 ++++++++---------------- src/sniffer.c | 2 +- src/tls13.c | 14 +------------- wolfssl/internal.h | 3 +-- 4 files changed, 11 insertions(+), 32 deletions(-) diff --git a/src/internal.c b/src/internal.c index cac28ecb8..5b7bc0f68 100644 --- a/src/internal.c +++ b/src/internal.c @@ -16969,17 +16969,6 @@ static int DecryptTls(WOLFSSL* ssl, byte* plain, const byte* input, word16 sz) /* Reset state */ ssl->decrypt.state = CIPHER_STATE_BEGIN; - /* handle mac error case */ - if (ret == VERIFY_MAC_ERROR) { - if (!ssl->options.dtls) { - SendAlert(ssl, alert_fatal, bad_record_mac); - } - #ifdef WOLFSSL_DTLS_DROP_STATS - if (ssl->options.dtls) - ssl->macDropCount++; - #endif /* WOLFSSL_DTLS_DROP_STATS */ - } - return ret; } @@ -18490,20 +18479,20 @@ int ProcessReplyEx(WOLFSSL* ssl, int allowSocketErr) #ifdef WOLFSSL_TLS13 byte *aad = (byte*)&ssl->curRL; word16 aad_size = RECORD_HEADER_SZ; -#ifdef WOLFSSL_DTLS13 + #ifdef WOLFSSL_DTLS13 if (ssl->options.dtls) { /* aad now points to the record header */ aad = ssl->dtls13CurRL; aad_size = ssl->dtls13CurRlLength; } -#endif /* WOLFSSL_DTLS13 */ + #endif /* WOLFSSL_DTLS13 */ /* Don't send an alert for DTLS. We will just drop it * silently later. */ ret = DecryptTls13(ssl, in->buffer + in->idx, in->buffer + in->idx, ssl->curSize, - aad, aad_size, !ssl->options.dtls); + aad, aad_size); #else ret = DECRYPT_ERROR; #endif /* WOLFSSL_TLS13 */ @@ -18534,7 +18523,7 @@ int ProcessReplyEx(WOLFSSL* ssl, int allowSocketErr) /* If in DTLS mode, if the decrypt fails for any * reason, pretend the datagram never happened. */ if (ssl->options.dtls) { - WOLFSSL_MSG("DTLS: Ignoring decrypted failed record"); + WOLFSSL_MSG("DTLS: Ignoring failed decryption"); ssl->options.processReply = doProcessInit; ssl->buffers.inputBuffer.idx = ssl->buffers.inputBuffer.length; @@ -18567,13 +18556,16 @@ int ProcessReplyEx(WOLFSSL* ssl, int allowSocketErr) return 0; } WOLFSSL_MSG("Too much EarlyData!"); + SendAlert(ssl, alert_fatal, unexpected_message); + WOLFSSL_ERROR(TOO_MUCH_EARLY_DATA); + return TOO_MUCH_EARLY_DATA; } } #endif SendAlert(ssl, alert_fatal, bad_record_mac); /* Push error once we know that we will error out here */ WOLFSSL_ERROR(ret); - return DECRYPT_ERROR; + return ret; } } diff --git a/src/sniffer.c b/src/sniffer.c index bc68559d0..72a99901a 100644 --- a/src/sniffer.c +++ b/src/sniffer.c @@ -4790,7 +4790,7 @@ static const byte* DecryptMessage(WOLFSSL* ssl, const byte* input, word32 sz, #ifdef WOLFSSL_TLS13 if (IsAtLeastTLSv1_3(ssl->version)) { - ret = DecryptTls13(ssl, output, input, sz, (byte*)rh, RECORD_HEADER_SZ, 0); + ret = DecryptTls13(ssl, output, input, sz, (byte*)rh, RECORD_HEADER_SZ); } else #endif diff --git a/src/tls13.c b/src/tls13.c index 0632b0103..ed520847c 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -2278,11 +2278,10 @@ static int Tls13IntegrityOnly_Decrypt(WOLFSSL* ssl, byte* output, * sz The length of the encrypted data plus authentication tag. * aad The additional authentication data. * aadSz The size of the addition authentication data. - * doAlert Generate alert on error (set to 0 for sniffer use cases) * returns 0 on success, otherwise failure. */ int DecryptTls13(WOLFSSL* ssl, byte* output, const byte* input, word16 sz, - const byte* aad, word16 aadSz, int doAlert) + const byte* aad, word16 aadSz) { int ret = 0; word16 dataSz = sz - ssl->specs.aead_mac_size; @@ -2477,17 +2476,6 @@ int DecryptTls13(WOLFSSL* ssl, byte* output, const byte* input, word16 sz, break; } -#ifndef WOLFSSL_EARLY_DATA - if (ret < 0) { - if (doAlert) { - SendAlert(ssl, alert_fatal, bad_record_mac); - } - ret = VERIFY_MAC_ERROR; - } -#else - (void)doAlert; -#endif - return ret; } diff --git a/wolfssl/internal.h b/wolfssl/internal.h index b526dc074..24393d8ca 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -1867,8 +1867,7 @@ WOLFSSL_LOCAL int ChachaAEADEncrypt(WOLFSSL* ssl, byte* out, const byte* input, #ifdef WOLFSSL_TLS13 WOLFSSL_LOCAL int DecryptTls13(WOLFSSL* ssl, byte* output, const byte* input, - word16 sz, const byte* aad, word16 aadSz, - int doAlert); + word16 sz, const byte* aad, word16 aadSz); WOLFSSL_LOCAL int DoTls13HandShakeMsgType(WOLFSSL* ssl, byte* input, word32* inOutIdx, byte type, word32 size, word32 totalSz);