From 67473bac28a459f72b0a04a67669559c104b9a23 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Thu, 15 Sep 2022 11:34:30 +0200 Subject: [PATCH] Code review fixes - Mark old epochs as invalid so we don't attempt to decrypt with them - Return a non-zero value if possible in unit tests - Move Dtls13CheckAEADFailLimit to dtls13.c - Reset state in processreply --- src/dtls13.c | 90 +++++++++++++++++++++++++++- src/internal.c | 143 ++++++++++++++++----------------------------- tests/api.c | 38 +++++++----- wolfssl/internal.h | 7 ++- 4 files changed, 166 insertions(+), 112 deletions(-) diff --git a/src/dtls13.c b/src/dtls13.c index bc25179c8..18b392cc7 100644 --- a/src/dtls13.c +++ b/src/dtls13.c @@ -1846,6 +1846,20 @@ struct Dtls13Epoch* Dtls13GetEpoch(WOLFSSL* ssl, w64wrapper epochNumber) return NULL; } +void Dtls13SetOlderEpochSide(WOLFSSL* ssl, w64wrapper epochNumber, + int side) +{ + Dtls13Epoch* e; + int i; + + for (i = 0; i < DTLS13_EPOCH_SIZE; ++i) { + e = &ssl->dtls13Epochs[i]; + if (e->isValid && w64LT(e->epochNumber, epochNumber)) { + e->side = (byte)side; + } + } +} + static void Dtls13EpochCopyKeys(WOLFSSL* ssl, Dtls13Epoch* e, Keys* k, int side) { byte clientWrite, serverWrite; @@ -2000,8 +2014,10 @@ int Dtls13NewEpoch(WOLFSSL* ssl, w64wrapper epochNumber, int side) #ifndef WOLFSSL_TLS13_IGNORE_AEAD_LIMITS /* We are updating the receiving keys for this connection. We can restart - * the failed decryption counter. */ - if (side == ENCRYPT_AND_DECRYPT_SIDE || side == DECRYPT_SIDE_ONLY) + * the failed decryption counter if we haven't reached the key update + * limit. */ + if ((side == ENCRYPT_AND_DECRYPT_SIDE || side == DECRYPT_SIDE_ONLY) && + w64IsZero(ssl->dtls13InvalidateBefore)) w64Zero(&ssl->macDropCount); #endif @@ -2018,6 +2034,14 @@ int Dtls13NewEpoch(WOLFSSL* ssl, w64wrapper epochNumber, int side) e->side = ENCRYPT_AND_DECRYPT_SIDE; } + /* Once handshake is done. Mark epochs older than the last one as encrypt + * only so that they can't be used for decryption. */ + if (ssl->options.handShakeDone && (e->side == ENCRYPT_AND_DECRYPT_SIDE || + e->side == DECRYPT_SIDE_ONLY)) { + w64Decrement(&epochNumber); + Dtls13SetOlderEpochSide(ssl, epochNumber, ENCRYPT_SIDE_ONLY); + } + return 0; } @@ -2624,4 +2648,66 @@ int wolfSSL_dtls13_has_pending_msg(WOLFSSL* ssl) return ssl->dtls13Rtx.rtxRecords != NULL; } +#ifndef WOLFSSL_TLS13_IGNORE_AEAD_LIMITS +/* Limits specified by + * https://www.rfc-editor.org/rfc/rfc9147.html#name-aead-limits + * We specify the limit by which we need to do a key update as the halfway point + * to the hard decryption fail limit. */ +int Dtls13CheckAEADFailLimit(WOLFSSL* ssl) +{ + w64wrapper keyUpdateLimit; + w64wrapper hardLimit; + switch (ssl->specs.bulk_cipher_algorithm) { +#if defined(BUILD_AESGCM) || (defined(HAVE_CHACHA) && defined(HAVE_POLY1305)) + case wolfssl_aes_gcm: + case wolfssl_chacha: + hardLimit = DTLS_AEAD_AES_GCM_CHACHA_FAIL_LIMIT; + keyUpdateLimit = DTLS_AEAD_AES_GCM_CHACHA_FAIL_KU_LIMIT; + break; +#endif +#ifdef HAVE_AESCCM + case wolfssl_aes_ccm: + if (ssl->specs.aead_mac_size == AES_CCM_8_AUTH_SZ) { + /* Limit is 2^7. The RFC recommends that + * "TLS_AES_128_CCM_8_SHA256 is not suitable for general use". + * We still should enforce the limit. */ + hardLimit = DTLS_AEAD_AES_CCM_8_FAIL_LIMIT; + keyUpdateLimit = DTLS_AEAD_AES_CCM_8_FAIL_KU_LIMIT; + } + else { + /* Limit is 2^23.5. + * Without the fraction is 11863283 (0x00B504F3) + * Half of this value is 5931641 (0x005A8279) */ + hardLimit = DTLS_AEAD_AES_CCM_FAIL_LIMIT; + keyUpdateLimit = DTLS_AEAD_AES_CCM_FAIL_KU_LIMIT; + } + break; +#endif + case wolfssl_cipher_null: + /* No encryption being done. The MAC check must have failed. */ + return 0; + default: + WOLFSSL_MSG("Unrecognized ciphersuite for AEAD limit check"); + WOLFSSL_ERROR_VERBOSE(DECRYPT_ERROR); + return DECRYPT_ERROR; + } + if (w64GT(ssl->macDropCount, hardLimit)) { + /* We have reached the hard limit for failed decryptions. */ + WOLFSSL_MSG("Connection exceeded hard AEAD limit"); + WOLFSSL_ERROR_VERBOSE(DECRYPT_ERROR); + return DECRYPT_ERROR; + } + else if (w64GT(ssl->macDropCount, keyUpdateLimit)) { + WOLFSSL_MSG("Connection exceeded key update limit. Issuing key update"); + /* If not waiting for a response then request a key update. */ + if (!ssl->keys.updateResponseReq) { + ssl->dtls13DoKeyUpdate = 1; + ssl->dtls13InvalidateBefore = ssl->dtls13PeerEpoch; + w64Increment(&ssl->dtls13InvalidateBefore); + } + } + return 0; +} +#endif + #endif /* WOLFSSL_DTLS13 */ diff --git a/src/internal.c b/src/internal.c index 4a90c7ea4..635a3b917 100644 --- a/src/internal.c +++ b/src/internal.c @@ -82,7 +82,8 @@ * possible attack. * WOLFSSL_TLS13_IGNORE_AEAD_LIMITS * Ignore the AEAD limits for messages specified in the RFC. After - * reaching the limit, we initiate a key update. + * reaching the limit, we initiate a key update. We enforce the AEAD limits + * by default. * https://www.rfc-editor.org/rfc/rfc8446#section-5.5 * https://www.rfc-editor.org/rfc/rfc9147.html#name-aead-limits */ @@ -18192,6 +18193,24 @@ int DoApplicationData(WOLFSSL* ssl, byte* input, word32* inOutIdx, int sniff) return OUT_OF_ORDER_E; } + +#if defined(WOLFSSL_DTLS13) && !defined(WOLFSSL_TLS13_IGNORE_AEAD_LIMITS) + /* Check if we want to invalidate old epochs. If + * ssl->dtls13InvalidateBefore is set then we want to mark all old + * epochs as encrypt only. This is done when we detect too many failed + * decryptions. We do this here to confirm that the peer has updated its + * keys and we can stop using the old keys. */ + if (ssl->options.dtls && IsAtLeastTLSv1_3(ssl->version)) { + if (!w64IsZero(ssl->dtls13InvalidateBefore) && + w64Equal(ssl->keys.curEpoch64, ssl->dtls13InvalidateBefore)) { + Dtls13SetOlderEpochSide(ssl, ssl->dtls13InvalidateBefore, + ENCRYPT_SIDE_ONLY); + w64Zero(&ssl->dtls13InvalidateBefore); + w64Zero(&ssl->macDropCount); + } + } +#endif + #ifndef WOLFSSL_AEAD_ONLY if (ssl->specs.cipher_type == block) { if (ssl->options.tls1_1) @@ -18776,98 +18795,24 @@ static WC_INLINE int VerifyMac(WOLFSSL* ssl, const byte* input, word32 msgSz, return 0; } -enum { - AEAD_LIMIT_OK, - AEAD_LIMIT_KEY_UPDATE, - AEAD_LIMIT_FAIL, -}; - -#if defined(WOLFSSL_DTLS13) && !defined(WOLFSSL_TLS13_IGNORE_AEAD_LIMITS) -/* Limits specified by - * https://www.rfc-editor.org/rfc/rfc9147.html#name-aead-limits - * We specify the limit by which we need to do a key update as the halfway point - * to the hard decryption fail limit. */ -static int checkDTLS13AEADFailLimit(byte bulk_cipher_algorithm, - word16 aead_mac_size, w64wrapper dropped) -{ - w64wrapper keyUpdateLimit; - w64wrapper hardLimit; - switch (bulk_cipher_algorithm) { -#if defined(BUILD_AESGCM) || (defined(HAVE_CHACHA) && defined(HAVE_POLY1305)) - case wolfssl_aes_gcm: - case wolfssl_chacha: - hardLimit = DTLS_AEAD_AES_GCM_CHACHA_FAIL_LIMIT; - keyUpdateLimit = DTLS_AEAD_AES_GCM_CHACHA_FAIL_KU_LIMIT; - break; -#endif -#ifdef HAVE_AESCCM - case wolfssl_aes_ccm: - if (aead_mac_size == AES_CCM_8_AUTH_SZ) { - /* Limit is 2^7. The RFC recommends that - * "TLS_AES_128_CCM_8_SHA256 is not suitable for general use". - * We still should enforce the limit. */ - hardLimit = DTLS_AEAD_AES_CCM_8_FAIL_LIMIT; - keyUpdateLimit = DTLS_AEAD_AES_CCM_8_FAIL_KU_LIMIT; - } - else { - /* Limit is 2^23.5. - * Without the fraction is 11863283 (0x00B504F3) - * Half of this value is 5931641 (0x005A8279) */ - hardLimit = DTLS_AEAD_AES_CCM_FAIL_LIMIT; - keyUpdateLimit = DTLS_AEAD_AES_CCM_FAIL_KU_LIMIT; - } - break; -#endif - case wolfssl_cipher_null: - /* No encryption being done. The MAC failed must have failed. */ - return 0; - default: - WOLFSSL_MSG("Unrecognized ciphersuite for AEAD limit check"); - return AEAD_LIMIT_FAIL; - } - if (w64GT(dropped, hardLimit)) { - WOLFSSL_MSG("Connection exceeded hard AEAD limit"); - return AEAD_LIMIT_FAIL; - } - else if (w64GT(dropped, keyUpdateLimit)) { - WOLFSSL_MSG("Connection exceeded key update limit. Issuing key update"); - return AEAD_LIMIT_KEY_UPDATE; - } - return AEAD_LIMIT_OK; -} -#endif - #ifdef WOLFSSL_DTLS static int HandleDTLSDecryptFailed(WOLFSSL* ssl) { - ssl->options.processReply = doProcessInit; - ssl->buffers.inputBuffer.idx = ssl->buffers.inputBuffer.length; - + int ret = 0; #if defined(WOLFSSL_DTLS_DROP_STATS) || \ (defined(WOLFSSL_DTLS13) && !defined(WOLFSSL_TLS13_IGNORE_AEAD_LIMITS)) w64Increment(&ssl->macDropCount); #if defined(WOLFSSL_DTLS13) && !defined(WOLFSSL_TLS13_IGNORE_AEAD_LIMITS) /* Handle AEAD limits specified by the RFC for failed decryption */ - if (IsAtLeastTLSv1_3(ssl->version)) { - int ret = checkDTLS13AEADFailLimit(ssl->specs.bulk_cipher_algorithm, - ssl->specs.aead_mac_size, ssl->macDropCount); - if (ret == AEAD_LIMIT_KEY_UPDATE) { - /* If not waiting for a response then request a key update. */ - if (!ssl->keys.updateResponseReq) - ssl->dtls13DoKeyUpdate = 1; - } - else if (ret == AEAD_LIMIT_FAIL) { - /* We have reached the hard limit for failed decryptions. */ - WOLFSSL_ERROR_VERBOSE(DECRYPT_ERROR); - return DECRYPT_ERROR; - } - } + if (IsAtLeastTLSv1_3(ssl->version)) + ret = Dtls13CheckAEADFailLimit(ssl); #endif #endif + (void)ssl; WOLFSSL_MSG("DTLS: Ignoring failed decryption"); - return 0; + return ret; } #endif @@ -19180,8 +19125,12 @@ int ProcessReplyEx(WOLFSSL* ssl, int allowSocketErr) #ifdef WOLFSSL_DTLS /* If in DTLS mode, if the decrypt fails for any * reason, pretend the datagram never happened. */ - if (ssl->options.dtls) + if (ssl->options.dtls) { + ssl->options.processReply = doProcessInit; + ssl->buffers.inputBuffer.idx = + ssl->buffers.inputBuffer.length; return HandleDTLSDecryptFailed(ssl); + } #endif /* WOLFSSL_DTLS */ #ifdef WOLFSSL_EXTRA_ALERTS if (!ssl->options.dtls) @@ -19336,8 +19285,12 @@ int ProcessReplyEx(WOLFSSL* ssl, int allowSocketErr) #ifdef WOLFSSL_DTLS /* If in DTLS mode, if the decrypt fails for any * reason, pretend the datagram never happened. */ - if (ssl->options.dtls) + if (ssl->options.dtls) { + ssl->options.processReply = doProcessInit; + ssl->buffers.inputBuffer.idx = + ssl->buffers.inputBuffer.length; return HandleDTLSDecryptFailed(ssl); + } #endif /* WOLFSSL_DTLS */ #ifdef WOLFSSL_EARLY_DATA if (ssl->options.tls1_3) { @@ -19402,8 +19355,12 @@ int ProcessReplyEx(WOLFSSL* ssl, int allowSocketErr) #ifdef WOLFSSL_DTLS /* If in DTLS mode, if the decrypt fails for any * reason, pretend the datagram never happened. */ - if (ssl->options.dtls) + if (ssl->options.dtls) { + ssl->options.processReply = doProcessInit; + ssl->buffers.inputBuffer.idx = + ssl->buffers.inputBuffer.length; return HandleDTLSDecryptFailed(ssl); + } #endif /* WOLFSSL_DTLS */ #ifdef WOLFSSL_EXTRA_ALERTS if (!ssl->options.dtls) @@ -22196,6 +22153,16 @@ int SendData(WOLFSSL* ssl, const void* data, int sz) byte comp[MAX_RECORD_SIZE + MAX_COMP_EXTRA]; #endif +#if defined(WOLFSSL_TLS13) && !defined(WOLFSSL_TLS13_IGNORE_AEAD_LIMITS) + if (IsAtLeastTLSv1_3(ssl->version)) { + ret = CheckTLS13AEADSendLimit(ssl); + if (ret != 0) { + ssl->error = ret; + return WOLFSSL_FATAL_ERROR; + } + } +#endif + #ifdef WOLFSSL_DTLS13 if (ssl->options.dtls && ssl->options.tls1_3) { byte isEarlyData = 0; @@ -22232,16 +22199,6 @@ int SendData(WOLFSSL* ssl, const void* data, int sz) } #endif /* WOLFSSL_DTLS13 */ -#if defined(WOLFSSL_TLS13) && !defined(WOLFSSL_TLS13_IGNORE_AEAD_LIMITS) - if (IsAtLeastTLSv1_3(ssl->version)) { - ret = CheckTLS13AEADSendLimit(ssl); - if (ret != 0) { - ssl->error = ret; - return WOLFSSL_FATAL_ERROR; - } - } -#endif - #ifdef WOLFSSL_DTLS if (ssl->options.dtls) { buffSz = wolfSSL_GetMaxFragSize(ssl, sz - sent); diff --git a/tests/api.c b/tests/api.c index 8f1776460..e320e9ad6 100644 --- a/tests/api.c +++ b/tests/api.c @@ -55129,8 +55129,10 @@ static int test_wolfSSL_dtls_plaintext(void) test_wolfSSL_client_server_nofail(&func_cb_client, &func_cb_server); - AssertTrue(func_cb_client.return_code); - AssertTrue(func_cb_server.return_code); + if (!func_cb_client.return_code) + return -1; + if (!func_cb_server.return_code) + return -2; } printf(resultFmt, passed); @@ -55397,42 +55399,43 @@ static void test_AEAD_limit_client(WOLFSSL* ssl) ssl->macDropCount = keyUpdateLimit; w64Increment(&ssl->macDropCount); /* 100 read calls should be enough to complete the key update */ + w64Zero(&counter); for (i = 0; i < 100; i++) { /* Key update should be sent and negotiated */ ret = wolfSSL_read(ssl, msgBuf, sizeof(msgBuf)); AssertIntGT(ret, 0); /* Epoch after one key update is 4 */ - if (w64Equal(ssl->dtls13Epoch, w64From32(0, 4))) { + if (w64Equal(ssl->dtls13PeerEpoch, w64From32(0, 4)) && + w64Equal(ssl->macDropCount, counter)) { didReKey = 1; break; } } AssertTrue(didReKey); - w64Zero(&counter); - AssertTrue(w64Equal(ssl->macDropCount, counter)); if (!w64IsZero(sendLimit)) { /* Test the sending limit for AEAD ciphers */ - didReKey = 0; - AssertNotNull(ssl->dtls13EncryptEpoch); - ssl->dtls13EncryptEpoch->nextSeqNumber = sendLimit; + Dtls13Epoch* e = Dtls13GetEpoch(ssl, ssl->dtls13Epoch); + AssertNotNull(e); + e->nextSeqNumber = sendLimit; test_AEAD_seq_num = 1; ret = wolfSSL_write(ssl, msgBuf, sizeof(msgBuf)); AssertIntGT(ret, 0); + didReKey = 0; + w64Zero(&counter); /* 100 read calls should be enough to complete the key update */ for (i = 0; i < 100; i++) { /* Key update should be sent and negotiated */ ret = wolfSSL_read(ssl, msgBuf, sizeof(msgBuf)); AssertIntGT(ret, 0); /* Epoch after another key update is 5 */ - if (w64Equal(ssl->dtls13Epoch, w64From32(0, 5))) { + if (w64Equal(ssl->dtls13Epoch, w64From32(0, 5)) && + w64Equal(ssl->macDropCount, counter)) { didReKey = 1; break; } } AssertTrue(didReKey); - w64Zero(&counter); - AssertTrue(w64Equal(ssl->macDropCount, counter)); } test_AEAD_fail_decryption = 2; @@ -55463,9 +55466,10 @@ static void test_AEAD_limit_server(WOLFSSL* ssl) if (test_AEAD_seq_num) { /* We need to update the seq number so that we can understand the * peer. Otherwise we will incorrectly interpret the seq number. */ - AssertNotNull(ssl->dtls13DecryptEpoch); - ssl->dtls13DecryptEpoch->nextPeerSeqNumber = sendLimit; - test_AEAD_seq_num = 1; + Dtls13Epoch* e = Dtls13GetEpoch(ssl, ssl->dtls13PeerEpoch); + AssertNotNull(e); + e->nextPeerSeqNumber = sendLimit; + test_AEAD_seq_num = 0; } (void)wolfSSL_read(ssl, msgBuf, sizeof(msgBuf)); ret = wolfSSL_write(ssl, msgBuf, sizeof(msgBuf)); @@ -55490,8 +55494,10 @@ static int test_wolfSSL_dtls_AEAD_limit(void) test_wolfSSL_client_server_nofail(&func_cb_client, &func_cb_server); - AssertTrue(func_cb_client.return_code); - AssertTrue(func_cb_server.return_code); + if (!func_cb_client.return_code) + return -1; + if (!func_cb_server.return_code) + return -2; printf(resultFmt, passed); diff --git a/wolfssl/internal.h b/wolfssl/internal.h index 8c77b9286..54db08cf5 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -4931,6 +4931,7 @@ struct WOLFSSL { Dtls13Epoch *dtls13DecryptEpoch; w64wrapper dtls13Epoch; w64wrapper dtls13PeerEpoch; + w64wrapper dtls13InvalidateBefore; byte dtls13CurRL[DTLS_RECVD_RL_HEADER_MAX_SZ]; word16 dtls13CurRlLength; @@ -5749,8 +5750,11 @@ WOLFSSL_API int wolfSSL_DtlsUpdateWindow(word16 cur_hi, word32 cur_lo, #ifdef WOLFSSL_DTLS13 -WOLFSSL_LOCAL struct Dtls13Epoch* Dtls13GetEpoch(WOLFSSL* ssl, +/* Use WOLFSSL_API to use this function in tests/api.c */ +WOLFSSL_API struct Dtls13Epoch* Dtls13GetEpoch(WOLFSSL* ssl, w64wrapper epochNumber); +WOLFSSL_LOCAL void Dtls13SetOlderEpochSide(WOLFSSL* ssl, w64wrapper epochNumber, + int side); WOLFSSL_LOCAL int Dtls13NewEpoch(WOLFSSL* ssl, w64wrapper epochNumber, int side); WOLFSSL_LOCAL int Dtls13SetEpochKeys(WOLFSSL* ssl, w64wrapper epochNumber, @@ -5802,6 +5806,7 @@ WOLFSSL_LOCAL int Dtls13HashHandshake(WOLFSSL* ssl, const byte* output, WOLFSSL_LOCAL void Dtls13FreeFsmResources(WOLFSSL* ssl); WOLFSSL_LOCAL int Dtls13RtxTimeout(WOLFSSL* ssl); WOLFSSL_LOCAL int Dtls13ProcessBufferedMessages(WOLFSSL* ssl); +WOLFSSL_LOCAL int Dtls13CheckAEADFailLimit(WOLFSSL* ssl); #endif /* WOLFSSL_DTLS13 */ #ifdef WOLFSSL_STATIC_EPHEMERAL